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:

  1. Missing preloads in merge_requests_for_approval_reset: Only :source_project was preloaded, but downstream code heavily accesses approvals and approval_rules on each MR, causing per-MR queries.

  2. overall_approver_ids repeated pluck per rule: In ApprovalWrappedRule#overall_approver_ids, when merge_request.approvals is not loaded, each rule executes an identical SELECT DISTINCT user_id FROM approvals query. After reset_approval_cache! clears the loaded state, this fires again across subsequent evaluation passes.

  3. :group_users not preloaded on approval rules: ApprovalRuleLike#with_role_approvers checks users.loaded? && group_users.loaded? — only :users and :groups were preloaded, but NOT :group_users (a has_many :through with disable_joins: true). This caused the User.from_union SQL path to fire per rule per evaluation pass, producing per-rule group_id lookups, user SELECT queries, and COUNT(*) checks.

  4. approvals.load wiped by reset_approval_cache!: The initial .load call helped the first evaluation pass, but reset_approval_cache! calls approvals.reset before the second and third passes (all_approval_rules_approved? checks), causing DISTINCT pluck queries to fire again per rule.

Solution

  1. Add preloading: Preload :approvals and approval_rules: [:users, :group_users] in merge_requests_for_approval_reset to eliminate per-MR association queries.

  2. Eagerly load approvals: Call merge_request.approvals.load before iterating approval rules in delete_code_owner_approvals. This makes overall_approver_ids take the in-memory map(&:user_id).to_set path.

  3. Preload :group_users on approval rules: Add :group_users to the preload in applicable_to_branch (on MergeRequest) and in merge_requests_for_approval_reset. This flips with_role_approvers from the User.from_union SQL path to the in-memory users | group_users | ... path, eliminating per-rule group_id lookups, user queries, and COUNT checks.

  4. Re-load approvals after reset_approval_cache!: Add merge_request.approvals.load after each reset_approval_cache! call in perform_code_owner_approval_deletion and trigger_code_owner_webhook_events, so overall_approver_ids stays 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_users are 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): Uses service.execute (full service path) with 2 code owner rules as control, then adds 2 more rules. Asserts zero additional queries with not_to exceed_query_limit(control) (no threshold needed).

  • Eager load assertion (reset_approvals_service_spec.rb): Directly verifies merge_request.approvals.loaded? is true after delete_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_id pluck (Passes 2 and 3)
  • 3x group_id lookups from approval_merge_request_rules_groups (all 3 passes)
  • 3x User.from_union SELECT (all 3 passes)
  • 2x COUNT(*) approved-count check (Passes 2 and 3)

Performance data source

#579591 (comment 3111867536)

Edited by Marc Shaw

Merge request reports

Loading