Skip to content

[12.0] Port base_exception fixes (locks, memory) - #1648

Merged
OCA-git-bot merged 2 commits into
OCA:12.0from
guewen:12.0-base_exception-concurrent-errors
Oct 16, 2019
Merged

OCA-git-bot merged 2 commits into
OCA:12.0from
guewen:12.0-base_exception-concurrent-errors

Conversation

@guewen

@guewen guewen commented Aug 14, 2019

Copy link
Copy Markdown
Member

@guewen
guewen force-pushed the 12.0-base_exception-concurrent-errors branch from 382a792 to 8dc7b7f Compare August 14, 2019 13:45

@florian-dacosta florian-dacosta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually approved too fast.
iteritems does not exist anymore in python 3

Comment thread base_exception/models/base_exception.py Outdated
Comment thread base_exception/models/base_exception.py Outdated
Comment thread base_exception/models/base_exception.py Outdated
Comment thread base_exception/models/base_exception.py Outdated
hparfr and others added 2 commits October 14, 2019 14:52
This recently added feature is counter intuitive, error prone and is
already causing bugs in sale_workflow.
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
guewen force-pushed the 12.0-base_exception-concurrent-errors branch from 3c372a2 to 43def64 Compare October 14, 2019 12:52
@guewen

guewen commented Oct 14, 2019

Copy link
Copy Markdown
Member Author

Squashed and rebased

@sebastienbeau sebastienbeau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yvaucher

Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

On my way to merge this fine PR!
Prepared branch 12.0-ocabot-merge-pr-1648-by-yvaucher-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Oct 16, 2019
Signed-off-by yvaucher
@OCA-git-bot
OCA-git-bot merged commit 43def64 into OCA:12.0 Oct 16, 2019
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 62eec9e. Thanks a lot for contributing to OCA. ❤️

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (18.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants