[19.0][FIX] base_tier_validation: systray recount cost grows with the reviewer's backlog - #59
Open
bosd wants to merge 1 commit into
Open
Conversation
``res.users.review_user_count`` is what the systray badge asks for, and it costs a handful of SQL queries *per pending review* -- so the reviewer with the largest backlog pays the most, every time the badge refreshes. On a production database a single call was measured at ~300 queries and 5-12 seconds, and the count fell by exactly three for each review that got approved. Two independent causes: - ``tier.review._can_review_value`` browses its own document to find the lowest pending sequence. Called from ``_compute_can_review``'s per-record loop, that browse has a prefetch set of exactly one id, so each review pays a fresh read of its document's ``review_ids`` instead of sharing one. Read the relation once per model up front; the per-record browse then finds the values in the environment cache and issues no query at all. This is the ~3 queries per review. - ``review_user_count`` put ``can_review`` in the *document* domain. Its search method re-searches the reviewer's entire backlog on that model and then evaluates the field in Python over all of it -- work that is thrown away, because the candidate set was already narrowed to the pending reviews read just above. Evaluate the field on those candidates instead: same answer, one prefetched batch instead of a second search. While there, the cancelled-record filter moves into the domain whenever the state field is a real column, so PostgreSQL discards those rows rather than Python discarding them after they have been read. The new test pins the property rather than a magic number: growing a reviewer's backlog from 2 to 12 pending documents may not grow the query count by more than a constant. The "model was uninstalled" guard the comment already promised is also made real: ``self.env[model]`` was evaluated before the check, so an orphaned review raised a KeyError instead of being skipped. A third per-record read hid behind the same pattern on the document side: ``_get_sequences_to_approve`` filters ``review_ids`` and then reads ``reviewer_ids`` on what is left, and ``filtered`` returns a recordset whose prefetch set is narrowed to the records it kept -- so the many2many was read one document at a time. Reading it once for the batch, before the loop narrows anything, is what makes the endpoint genuinely flat.
Contributor
|
Hi @LoisRForgeFlow, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
res.users.review_user_countis what the systray badge asks for, and its costgrows with the reviewer's backlog: a handful of SQL queries per pending
review, on every call. The reviewer with the most to approve pays the most,
every time the badge refreshes.
On a production database (Odoo 19, ~56 users) a single call was measured at
~300 queries and 5–12 seconds, and the query count fell by exactly three for
each review that got approved.
Measured here
The new test grows one reviewer's backlog from 2 to 22 pending documents and
counts the queries
review_user_countissues:Three per-record read patterns
1.
tier.review._can_review_valuebrowses its own document.self.env[self.model].browse(self.res_id)has a prefetch set of exactly oneid, so every review pays a fresh read of its document's
review_ids. Calledfrom
_compute_can_review's per-record loop, that is the ~3 queries perreview. Reading the relation once per model up front puts the values in the
environment cache, where the per-record browse then finds them for free.
2.
review_user_countputcan_reviewin the document domain._search_can_reviewre-searches the reviewer's entire backlog on that modeland evaluates the field in Python over all of it — work that is thrown away,
because the candidate set was already narrowed to the pending reviews read
just above. Evaluating the field on those candidates gives the same answer
from one prefetched batch instead of a second search.
3.
_get_sequences_to_approvereadsreviewer_idsafter afiltered().filteredreturnsself.browse(kept_ids), whose prefetch set is narrowed tothe records it kept — so the many2many is read one document at a time. This
one is invisible until you dump the SQL: 22 of 29 remaining queries were the
same m2m read repeated per document. Reading it once for the batch, before the
loop narrows anything, is what makes the endpoint genuinely flat.
While there, the cancelled-record filter moves into the domain whenever the
state field is a real column, so PostgreSQL discards those rows instead of
Python discarding them after they have been read; and the "model was
uninstalled" guard the comment already promised is made real (
self.env[model]was evaluated before the check, so an orphaned review raised
KeyError).Test
test_16c_review_user_count_cost_flat_in_backlogpins the property rather thana magic number: twenty more pending documents may not cost twenty more round
trips. It fails on
19.0(52 vs a bound of 29) and passes here.Note for reviewers of #53
19.0-fix-sequential-premature-promotionadds a per-document browse of its ownin
_update_review_status:review_user_countcalls that method on every invocation, so merging it as-isreintroduces the same cost.
_prefetch_resource_reviewsadded here batches itin one line.
Relation to #52
Independent. #52 bounds how often the systray recounts; this bounds what a
recount costs. Either can merge first.