Fix N+1 queries in reset approvals worker
Summary
Fixes N+1 query patterns in the MergeRequestResetApprovalsWorker that cause redundant database queries when processing code owner approval resets.
Problem
The find_approved_code_owner_rules step is the number 3 bottleneck:
| Percentile | Duration |
|---|---|
| P90 | 0.387s |
| P95 | 0.846s |
| P99 | 2.741s |
Four N+1 patterns were identified, each firing per code owner rule:
-
Missing preloads in
merge_requests_for_approval_reset: Only:source_projectwas preloaded, but downstream code heavily accessesapprovalsandapproval_ruleson each MR, causing per-MR queries. -
overall_approver_idsrepeated pluck per rule: InApprovalWrappedRule#overall_approver_ids, whenmerge_request.approvalsis not loaded, each rule executes an identicalSELECT DISTINCT user_id FROM approvalsquery. Afterreset_approval_cache!clears the loaded state, this fires again across subsequent evaluation passes. -
:group_usersnot preloaded on approval rules:ApprovalRuleLike#with_role_approverschecksusers.loaded? && group_users.loaded?— only:usersand:groupswere preloaded, but NOT:group_users(ahas_many :throughwithdisable_joins: true). This caused theUser.from_unionSQL path to fire per rule per evaluation pass, producing per-rulegroup_idlookups, user SELECT queries, andCOUNT(*)checks. -
approvals.loadwiped byreset_approval_cache!: The initial.loadcall helped the first evaluation pass, butreset_approval_cache!callsapprovals.resetbefore the second and third passes (all_approval_rules_approved?checks), causing DISTINCT pluck queries to fire again per rule.
Solution
-
Add preloading: Preload
:approvalsandapproval_rules: [:users, :group_users]inmerge_requests_for_approval_resetto eliminate per-MR association queries. -
Eagerly load approvals: Call
merge_request.approvals.loadbefore iterating approval rules indelete_code_owner_approvals. This makesoverall_approver_idstake the in-memorymap(&:user_id).to_setpath. -
Preload
:group_userson approval rules: Add:group_usersto the preload inapplicable_to_branch(onMergeRequest) and inmerge_requests_for_approval_reset. This flipswith_role_approversfrom theUser.from_unionSQL path to the in-memoryusers | group_users | ...path, eliminating per-rule group_id lookups, user queries, and COUNT checks. -
Re-load approvals after
reset_approval_cache!: Addmerge_request.approvals.loadafter eachreset_approval_cache!call inperform_code_owner_approval_deletionandtrigger_code_owner_webhook_events, sooverall_approver_idsstays on the in-memory path across all evaluation passes.
N+1 regression tests
All fixes have dedicated regression tests verified to fail when the fix is removed:
-
Preload test (
base_service_spec.rb): Verifies:approvals,:approval_rules,:users, and:group_usersare all preloaded. Also adds additional MRs and accesses nested associations inside a query recorder block — without the preloads, each additional MR triggers per-MR queries. -
Full service N+1 test (
reset_approvals_service_spec.rb): Usesservice.execute(full service path) with 2 code owner rules as control, then adds 2 more rules. Asserts zero additional queries withnot_to exceed_query_limit(control)(no threshold needed). -
Eager load assertion (
reset_approvals_service_spec.rb): Directly verifiesmerge_request.approvals.loaded?is true afterdelete_code_owner_approvals.
Local benchmarks (GDK, MR with 5 approvals, 3 code owner rules)
Query reduction (full service.execute path, adding 2 extra code owner rules):
| Queries | |
|---|---|
| Before all fixes | 54 baseline, +20 for 2 extra rules |
| After all fixes | 36 baseline, +0 for 2 extra rules |
| Saved | 18 baseline queries + eliminated all per-rule scaling |
The 20 per-rule queries broke down as ~10 per additional rule:
- 2x DISTINCT
approvals.user_idpluck (Passes 2 and 3) - 3x
group_idlookups fromapproval_merge_request_rules_groups(all 3 passes) - 3x
User.from_unionSELECT (all 3 passes) - 2x
COUNT(*)approved-count check (Passes 2 and 3)