From f61c09afed6ca52b8f5ac4b4d660ee9250b3fecf Mon Sep 17 00:00:00 2001 From: bosd <5e2fd43-d292-4c90-9d1f-74ff3436329a@anonaddy.me> Date: Thu, 27 Aug 2026 13:27:19 +0200 Subject: [PATCH] [FIX] base_tier_validation: systray recount cost grows with the backlog ``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. --- base_tier_validation/models/res_users.py | 37 ++++++++--- base_tier_validation/models/tier_review.py | 31 +++++++++ .../models/tier_validation.py | 7 ++ .../tests/test_tier_validation.py | 64 +++++++++++++++++++ 4 files changed, 130 insertions(+), 9 deletions(-) diff --git a/base_tier_validation/models/res_users.py b/base_tier_validation/models/res_users.py index c26e04ea7..70748f985 100644 --- a/base_tier_validation/models/res_users.py +++ b/base_tier_validation/models/res_users.py @@ -33,14 +33,25 @@ def review_user_count(self): aggregates=["id:recordset"], ) for model, tier_review in review_groups: - Model = self.env[model] # Skip Models not having Tier Validation enabled (example: was unistalled) - if tier_review and hasattr(Model, "can_review"): - records_domain = ( - Domain("id", "in", tier_review.mapped("res_id")) - & Domain("validation_status", "!=", "rejected") - & Domain("can_review", "=", True) - ) + if not tier_review or model not in self.env: + continue + Model = self.env[model] + if hasattr(Model, "can_review"): + records_domain = Domain( + "id", "in", tier_review.mapped("res_id") + ) & Domain("validation_status", "!=", "rejected") + # Excludes any cancelled records depending on the structure of + # the model. Let PostgreSQL do it whenever the state field is a + # real column -- which it is on every model that stores its + # state -- and keep the Python pass only as the fallback for + # models whose state field is computed and not stored. + state_field = Model._fields.get(Model._state_field) + state_in_db = state_field is not None and state_field.store + if state_in_db: + records_domain &= Domain( + Model._state_field, "!=", Model._cancel_state + ) try: records = ( Model.with_user(user) @@ -60,8 +71,16 @@ def review_user_count(self): model, ) continue - # Excludes any cancelled records depending on the structure of the model - if Model._state_field in Model._fields: + # ``can_review`` is deliberately *not* part of ``records_domain``. + # Its search method re-searches the reviewer's whole backlog on + # this model and then evaluates the field in Python over all of + # it, so putting it in the domain made the systray cost grow + # with the backlog -- on every call, for every reviewer. The + # candidate set is already narrowed to the pending reviews found + # above, so evaluate the field on those records instead: same + # answer, one prefetched batch instead of a second search. + records = records.filtered("can_review") + if not state_in_db and Model._state_field in Model._fields: records = records.filtered( lambda x: x[x._state_field] != x._cancel_state ) diff --git a/base_tier_validation/models/tier_review.py b/base_tier_validation/models/tier_review.py index 36b0b68d1..2bfaec2e9 100644 --- a/base_tier_validation/models/tier_review.py +++ b/base_tier_validation/models/tier_review.py @@ -2,6 +2,7 @@ # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). import logging +from collections import defaultdict from odoo import api, fields, models from odoo.exceptions import ValidationError @@ -93,9 +94,39 @@ def _compute_reviewed_formated_date(self): @api.depends("status", "approve_sequence", "sequence", "model", "res_id") def _compute_can_review(self): + # Only sequential definitions reach the branch of ``_can_review_value`` + # that looks at the document, so only those are worth warming. + self.filtered("approve_sequence")._prefetch_resource_reviews() for record in self: record.can_review = record._can_review_value() + def _prefetch_resource_reviews(self): + """Warm the cache with the reviews of every resource in ``self``. + + ``_can_review_value`` browses its own ``res_id`` to find the lowest + pending sequence on that document. Record by record, that browse has a + prefetch set of exactly one id, so every review pays a fresh read of + its document's ``review_ids`` -- a handful of queries per review, on + every recompute of this (stored) field. The systray recount flushes + ``can_review``, so the cost of the counter grew with the reviewer's + backlog. + + Reading the same relation once per model puts the values in the + environment cache, where the per-record browse then finds them for + free. Callers pass the subset whose documents they are about to look + at. + """ + res_ids_per_model = defaultdict(list) + for record in self: + if record.model and record.res_id: + res_ids_per_model[record.model].append(record.res_id) + for model, res_ids in res_ids_per_model.items(): + # The model may have been uninstalled, or have dropped tier + # validation, while reviews pointing at it survive. + if model not in self.env or "review_ids" not in self.env[model]._fields: + continue + self.env[model].browse(res_ids).review_ids.fetch(["status", "sequence"]) + def _update_review_status(self): """Promote reviews that are currently available to pending.""" # To defer recompute, use context key diff --git a/base_tier_validation/models/tier_validation.py b/base_tier_validation/models/tier_validation.py index dd4266b91..07af55536 100644 --- a/base_tier_validation/models/tier_validation.py +++ b/base_tier_validation/models/tier_validation.py @@ -112,6 +112,13 @@ def _get_sequences_to_approve(self, user): "review_ids.status", ) def _compute_can_review(self): + # ``_get_sequences_to_approve`` filters ``review_ids`` and then reads + # ``reviewer_ids`` on what is left. ``filtered`` returns a recordset + # whose prefetch set is narrowed to the records it kept, so that read + # is issued one document at a time -- one query per pending review, + # every time the systray recounts. Read the relation once, for the + # whole batch, before the loop narrows anything. + self.review_ids.mapped("reviewer_ids") for rec in self: rec.can_review = rec._get_sequences_to_approve(self.env.user) diff --git a/base_tier_validation/tests/test_tier_validation.py b/base_tier_validation/tests/test_tier_validation.py index 69dcd5e8e..942582f2a 100644 --- a/base_tier_validation/tests/test_tier_validation.py +++ b/base_tier_validation/tests/test_tier_validation.py @@ -662,6 +662,70 @@ def test_16b_review_user_count_no_model_access(self): result = self.test_user_2.with_user(self.test_user_2).review_user_count() self.assertEqual(result, []) + def test_16c_review_user_count_cost_flat_in_backlog(self): + """The systray recount must not get dearer as the backlog grows. + + ``review_user_count`` used to put ``can_review`` in the *document* + domain, whose search method re-searches the reviewer's entire backlog + on that model and then evaluates the field in Python over all of it; + and every recompute of the stored ``tier.review.can_review`` browsed + its document one row at a time. Both made the endpoint cost a handful + of queries per pending review, paid by every reviewer on every + notification -- enough to occupy every HTTP worker on a batch approval. + """ + self.tier_def_obj.create( + { + "model_id": self.tester_model.id, + "review_type": "individual", + "reviewer_id": self.test_user_1.id, + "definition_domain": "[('test_field', '=', 2.0)]", + "approve_sequence": True, + "notify_on_pending": False, + "sequence": 5, + "name": "Definition for test 16c - backlog", + } + ) + + def add_backlog(count): + for _i in range(count): + record = self.test_model.create({"test_field": 2.0}) + record.with_user(self.test_user_2).request_validation() + + def measure(): + # A batch approval keeps moving review statuses, so the stored + # ``can_review`` is permanently dirty in production. Reproduce that, + # then drop the cache: marking the field to recompute must not go + # through a read of the reviews, or the cache it fills would hide + # exactly the queries this test is about. + self.env.flush_all() + self.env.add_to_compute( + self.env["tier.review"]._fields["can_review"], + self.test_user_1.review_ids, + ) + self.env.invalidate_all(flush=False) + before = self.env.cr.sql_log_count + result = self.test_user_1.with_user(self.test_user_1).review_user_count() + return self.env.cr.sql_log_count - before, result + + add_backlog(2) + small_queries, small = measure() + self.assertEqual(small[0]["pending_count"], 2) + + add_backlog(20) + large_queries, large = measure() + self.assertEqual(large[0]["pending_count"], 22) + + # Twenty more pending documents may not cost twenty more round trips. + # The tolerance is deliberately loose: the point is that the growth is + # bounded, not that the absolute query count never moves. + self.assertLessEqual( + large_queries, + small_queries + 3, + "review_user_count scales with the reviewer's backlog: " + f"{small_queries} queries for 2 pending documents, " + f"{large_queries} for 22.", + ) + def test_17_search_records_no_validation(self): """Search for records that have no validation process started""" records = self.env["tier.validation.tester"].search(