base_exception: side-effect in @api.constrains lead to no retry #1642
Labels
stale
PR/Issue without recent activity, it'll be soon closed automatically.
Comments
|
Hi @guewen If I understand correctly, there are 2 issues. I wonder, with the PR made to limit the effect of concurrency, do we still have concurrency issues in this case? How is it possible? Also, wouldn't it be possible to avoid the modification of the exception.rule, by ignoring the log_access field? It would avoid all the concurrent update issues. (which is better than lock and wait or retry) Anyway, even if this concurrent update issue is resolved, I think it would still be better to call the |
Yes
PR #1638 make it very unlikely, if not impossible, to happen indeed.
I remember @gurneyalex considered it but discarded it, I don't remember the reason, maybe to limit the change as much as possible.
Yep, plus it breaks the principle of least astonishment. |
|
Well, I guess we agree the Independently, I'd like to have @gurneyalex opinion about the log_access removal in this specific case, as it seems to be the best solution for me, for this particular case.
I did not understand this part. @hparfr FYI |
+1 on this. |
|
My test of #1638 is not positive.
I tried this, and even tried to put @florian-dacosta were you the author of this part ?Do you remember why the write happens on the rule with a |
|
@guewen
So, if I remember correctly, the module was sometimes writing on the target recordset (python code rule) and sometimes writing on the rule (domain rule), and the code started to become really complex. This maybe could be changed by making 2 writes on the target recordset instead like But, from a performance point of view in case of larges recordset, this may be really worse. Anyway it is strange that the What could be done is to extact this part : On a separate method (like |
guewen
added a commit
to guewen/sale-workflow
that referenced
this issue
Aug 13, 2019
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Aug 13, 2019
In the documentation. The method called by '_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and the related model (such as sale.order). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA#1642
|
Thanks for the explanation! The pity thing is that we should be only adding or removing entries in the relation table, so writing on one side or the other should change nothing at all. I opened 2 pull requests which do not solve the concurrency issue but remove the |
Found why. Using _log_access removes the In my scenario, I have sale orders already linked with the configured exception rules and I create a new sales order (id 693334 in the logs). The logs (filtered on this query) show this: |
I'm surprised, I don't really see what would take more time since the things happening are: case 1 when we write on
case 2 when we write on
EDIT: got it, when |
|
So, I did not personally made the test on performance, so it is hard to be categoric on this.
Detect exceptions on large recordset seems quite usual to me (cron that check all records with exceptions for instance). But for sure, if main_exception_id is recomputed for all linked recordset, that's not good. (I am surprised it is, if we only add or remove 1 record from the rule, it should be recomputed only for this record...) The thing is, if we proceed rule by rule (like today, for each rule, we check a recordset) it seems really more logical to write on the rule, once by rule. The domain on the rule make us kind of forced to proceed rule by rule (else it make no sense to have a domain...) About what to do, like I said earlier, I think we could make the write (on rule or linked recordset) in a separate method, so if for a specific usecase, it make a lot of different in term of performance, we still can change the behavior easily. If we continue writing on the rule, we also may manage the main_exception_id as a "normal" (not computed) field, since the standard behavior is not the best. We could recompute the main_exception_id, only on the I personally don't have a strong opinion about the way to do it. Maybe doing more performance test, on small and very large recordset would be a good idea. It could be nice to have other points of view also, now that the problems are quite clear... |
|
@florian-dacosta it's easy to streamline all the records to add/write for a rule while writing on the comodel (sale.order, ...) instead of the rule, working on it. |
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Aug 14, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
|
@florian-dacosta here it is #1647, thanks for your help! |
|
@florian-dacosta IMO the issue with concurrence on the write is more troublesome that performances. @gurneyalex could reduce the number of UPDATE queries done on rules plus add a lock on row level, but the lock is not enough with simultaneous exceptions as we don't only write on the rules but also on more sales than expected. So it's less likely to fail but still the python exceptions are not of the right type. And as I understand, the write have other a side effects in sale_exception that could lead to more concurrence errors with data written were we don't need to, either on The fix that moves all possible write out of api.constraint is the way to go to get the proper python exception with a queuejob context. For the issue with the field I understand it was made that way to be able to order/search on it in the view list. But it has a major drawback on concurrence. For search, you probably can use filters on the many2many toward rules. |
|
@yvaucher our messages crossed but my PR fixes the concurrent errors without using any locks, and I claim it does not have performance penalties. |
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Aug 14, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
guewen
added a commit
to guewen/sale-workflow
that referenced
this issue
Aug 14, 2019
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Aug 14, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Aug 15, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
guewen
added a commit
to guewen/sale-workflow
that referenced
this issue
Sep 13, 2019
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
guewen
added a commit
to guewen/server-tools
that referenced
this issue
Oct 14, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
Tardo
pushed a commit
to Tecnativa/server-tools
that referenced
this issue
Dec 5, 2019
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
jaredkipe
pushed a commit
to hibou-io/oca-sale-workflow
that referenced
this issue
Dec 6, 2021
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
nicomacr
pushed a commit
to adhoc-dev/server-tools
that referenced
this issue
Feb 4, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
augusto-weiss
pushed a commit
to adhoc-dev/server-tools
that referenced
this issue
Feb 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
augusto-weiss
pushed a commit
to adhoc-dev/server-tools
that referenced
this issue
Feb 16, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
augusto-weiss
pushed a commit
to adhoc-dev/server-tools
that referenced
this issue
Feb 16, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
damdam-s
pushed a commit
to damdam-s/server-tools
that referenced
this issue
Mar 23, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
damdam-s
pushed a commit
to damdam-s/sale-workflow
that referenced
this issue
Mar 23, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
gfcapalbo
pushed a commit
to gfcapalbo/server-tools
that referenced
this issue
Apr 13, 2022
In the documentation. The method called by '_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and the related model (such as sale.order). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA#1642
JuaniFreedoo
pushed a commit
to JuaniFreedoo/sale-workflow
that referenced
this issue
May 10, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
gorkaegui
pushed a commit
to gorkaegui/server-tools
that referenced
this issue
May 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
TxemaSaezUna
pushed a commit
to TxemaSaezUna/server-tools
that referenced
this issue
May 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
DsolanoRuiz
pushed a commit
to DsolanoRuiz/server-tools
that referenced
this issue
May 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
OSKMCC
pushed a commit
to OSKMCC/sale-workflow
that referenced
this issue
May 10, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
KKKARLOS
pushed a commit
to KKKARLOS/server-tools
that referenced
this issue
May 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
TxemaSaezUna
pushed a commit
to TxemaSaezUna/sale-workflow
that referenced
this issue
May 10, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
DsolanoRuiz
pushed a commit
to DsolanoRuiz/sale-workflow
that referenced
this issue
May 10, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
OSKMCC
pushed a commit
to OSKMCC/server-tools
that referenced
this issue
May 10, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
cesar-tecnativa
pushed a commit
to Tecnativa/server-tools
that referenced
this issue
Jun 6, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
cesar-tecnativa
pushed a commit
to Tecnativa/server-tools
that referenced
this issue
Jun 28, 2022
The goal of the modified method is to create or remove the relationship (in the M2m relation tabel) between the tested model (such as sale_order) and the exception rules. When the ORM writes on ExceptionRule.sale_ids (using the example of sale_exception), it will first proceeds with these updates: * an UPDATE on exception_rule to set the write_date * INSERT or DELETE on the relation table * but then, as "write" is called on the exception rule, the ORM will trigger the api.depends to recompute all the "main_exception_ids" of the records (sales, ...) related to it, leading to an UPDATE for each sale order We end up with RowExclusiveLock on such records: * All the records of the relation table added / deleted for the current sale order * All the records of exception_rule matching the current sale order * All the records of sale_order related to the exception rules matching the current sale order The first one is expected, the next 2 are not. We can remove the lock on the exception_rule table by removing `_log_access`, however in any case, the main_exception_ids computed field will continue to lock many sale orders, effectively preventing 2 sales orders with the same exception to be confirmed at the same time. Reversing the write by writing on SaleOrder instead of ExceptionRule fixes the 2 unexpected locks. It should not result in more queries: the "to remove" part generates a DELETE on the relation table for the rule to remove and the "to add" part generates an INSERT for the rule to add, both will be exactly the same in both cases. Related to OCA#1642 Replaces OCA#1638
nikul-serpentcs
pushed a commit
to nikul-serpentcs/sale-workflow
that referenced
this issue
Oct 10, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
matiasperalta1
pushed a commit
to adhoc-dev/sale-workflow
that referenced
this issue
Nov 17, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
sbejaoui
pushed a commit
to acsone/sale-workflow
that referenced
this issue
Dec 28, 2022
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
sonhd91
pushed a commit
to sonhd91/sale-workflow
that referenced
this issue
Mar 30, 2023
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
nguyenminhchien
pushed a commit
to nguyenminhchien/sale-workflow
that referenced
this issue
Dec 15, 2023
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
nguyenminhchien
pushed a commit
to nguyenminhchien/sale-workflow
that referenced
this issue
Feb 2, 2024
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
|
There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
nguyenminhchien
pushed a commit
to nguyenminhchien/sale-workflow
that referenced
this issue
Apr 11, 2024
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
Deriman-Alonso
pushed a commit
to Deriman-Alonso/sale-workflow
that referenced
this issue
Aug 5, 2024
The method called by 'sale_check_exception' has a side effect, it writes on 'exception.rule' + on the Many2many relation between it and sale.order(.line). When decorated by @api.constrains, any error during the method will be caught and re-raised as "ValidationError". This part of code is very prone to concurrent updates as 2 sales having the same exception will both write on the same 'exception.rule'. A concurrent update (OperationalError) is re-raised as ValidationError, and then is not retried properly. Calling the same method in create/write has the same effect than @api.constrains without shadowing the exception type. Full explanation: OCA/server-tools#1642
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Introduction
The
base_exceptionmodule states that we have to call_check_exceptionin an@api.constrainsmethod.server-tools/base_exception/models/base_exception.py
Lines 221 to 236 in 5157720
And it's what's been done in the
sale_exceptionmodule:https:/OCA/sale-workflow/blob/8428446c79981aee281eb1773ff84c5a32d02067/sale_exception/models/sale.py#L49-L53
So what?
One of the method called in
_check_exception, the methoddetect_exceptions, has side-effects on the database; it does a write onexception.ruleon a field which must be aMany2manywith the record being checked. For instance, sale_ids for the sales. the write will issue an UPDATE onexception.rule(for thewrite_date) and an INSERT in theMany2manyrelation.A PR has already been created to limit the effect of concurrency here, as 2+ orders with an exception won't be able to call the method at the same time due to the
write_datechange.The subject of this issue is about the usage of
@api.constrainswith side-effects, so what's the problem?@api.constrainsmethod will be caught and re-raised asValidationError(https:/odoo/odoo/blob/af3f6e596df6a558de8b465019028ff8cc2d7439/odoo/models.py#L1056)psycopg2.OperationalErrorOperationalErrorare normally retried (https:/odoo/odoo/blob/af3f6e596df6a558de8b465019028ff8cc2d7439/odoo/service/model.py#L110)Summarizing:
Any create or write done in a method decorated by
@api.constrainswill never be retried when anOperationalErrorthat could be retried (such as "could not serialize access due to concurrent update") occurs as theOperationalErroris shadowed by aValidationError.Solutions
We should either ensure there is no write in the check method, or change the documentation and implementation of sale_exceptions to call
_check_exceptionincreateandwritemethods instead of an@api.constrains. Considering the current code, the second one seems better to me.cc @florian-dacosta
The text was updated successfully, but these errors were encountered: