Look up only referenced SHAs in commit reference filter

Hordur Freyr Yngvasonrequested to merge
fix/commit-reference-filter-mr-sha-lookup into master

What does this MR do and why?

Banzai::Filter::References::CommitReferenceFilter decides whether a commit reference links to the commit inside its merge request (/-/merge_requests/:iid/diffs?commit_id=...) or to the standalone commit page.

To decide, it called MergeRequest#all_commit_shas, which loaded every commit SHA of the merge request — up to 10,000 rows across its 100 most recent diff versions — and then kept the ones prefix-matching a reference in the text. The referenced SHAs, of which a note normally has a handful, were only ever used to filter that large set back down.

This MR inverts the lookup. The reference cache has already resolved every reference in the text to a full SHA, so we ask the merge request which of those SHAs it contains. The cost now scales with the number of references in the text rather than with the size of the merge request.

MergeRequest#all_commit_shas had no other callers, so it and the query it wrapped (#all_commit_shas_from_metadata) are removed.

Why the old shape hurts

The removed query is unbounded on the merge request side, and it runs in an unforgiving place:

  1. It is issued while rendering a system note's markdown, which happens in a before_save callback (CacheMarkdownField#refresh_markdown_cache) — so inside the note's write transaction.
  2. By that point in MergeRequests::RefreshService the load balancer session is already sticky, so it goes to the primary, not a replica.
  3. Every reference filter is wrapped in Gitlab::RenderTimeout, which allows 30 seconds under Sidekiq. When the query exceeds that, the timeout fires inside PG::Connection#exec_params and UpdateMergeRequestsWorker fails with Timeout::ExitException: execution expired, losing the system note.

Long-lived, force-pushed merge requests are the ones that reach the 10,000-row cap, and they are also the ones whose "added N commits" system notes carry the most commit references.

Behaviour

No user-visible change is intended. Exact SHA matching replaces the prefix matching that the old code needed only because it was scanning the merge request's SHAs — abbreviated references are resolved to full SHAs by the reference cache before the lookup, which the new spec covers.

References

Screenshots or screen recordings

No visual change is intended. These validate on GDK that commit references resolve to the same links with the new lookup, on both data paths:

Scenario Screenshot
Comment referencing an in-MR commit (full and abbreviated) and a commit outside the merge request shot1-note
The in-MR references link to the commit inside the merge request (/-/merge_requests/2/diffs?commit_id=f15b3227…) shot2-diffs
The outside reference links to the standalone commit page shot3-commit
The same comment posted with mr_diff_commits_read_new_table disabled renders identical links via the fallback shot4-note-ff

How to set up and validate locally

  1. Use a project with a Git repository and a merge request that has a few commits (in GDK seed data, any flightjs/Flight merge request works), or create one.

  2. From the merge request's Commits tab, copy the full SHA of one of its commits. From the target branch history, copy the full SHA of a commit that is not part of the merge request.

  3. Comment on the merge request, referencing both, plus an abbreviated form:

    In-MR commit <sha-in-mr>, outside commit <sha-outside>, abbreviated <first 11 chars of sha-in-mr>

  4. Verify the rendered links: both references to the merge request's commit (full and abbreviated) must link to /-/merge_requests/<iid>/diffs?commit_id=<sha>, while the outside commit must link to the standalone commit page (/-/commit/<sha>).

  5. Disable the new-table read path in a Rails console and repeat the comment; the links must be identical:

    Feature.disable(:mr_diff_commits_read_new_table)

Automated coverage:

  • spec/lib/banzai/filter/references/commit_reference_filter_spec.rb — 53 examples, 0 failures. Adds coverage for abbreviated references in merge request context, and asserts that only the referenced SHAs are looked up, and only those resolved within the merge request's own project.
  • spec/models/merge_request_spec.rb — #existing_commit_shas and #commit_exists?, 18 examples, 0 failures. The #all_commit_shas coverage for the mid-backfill fallback and project_id partition pruning is ported to #existing_commit_shas, plus assertions that the query count does not grow with the number of SHAs, that batching kicks in per MAX_PLUCK slice, and that a SHA appearing in multiple diff versions is returned once and cannot crowd out other SHAs.
  • spec/services/system_notes/commit_service_spec.rb — 18 examples, 0 failures.

Database review

The new lookup is the query MergeRequest#commit_exists? already runs in production, generalised to accept many SHAs — so the plan shape is not new. It resolves against the unique (project_id, sha) index on merge_request_commits_metadata, and keeps the fallback to merge_request_diff_commits that is still required until mr_diff_commits_read_new_table is enabled everywhere. Input is sliced by ApplicationRecord::MAX_PLUCK, so the number of queries does not grow with the number of references.

Performance summary

All plans below ran on Database Lab against !139219, a merge request with 227 diff versions — the pathological shape this MR targets. In short: the removed lookup has an estimated plan cost of ~58,000 and an EXPLAIN (ANALYZE, BUFFERS) of it did not return a result within two hours there, while the replacement metadata lookup is costed at ~700 and executes in ≲0.5 s cold / ~1 ms warm. The merge_request_diff_commits fallback stays under ~285 ms cold and ~1 ms warm even in its worst case, a full scan of the 100-version window that finds nothing.

Removed query

Loads up to 10,000 SHAs; the outer table is restricted only by project_id, so nothing pins the planner to per-row primary key lookups on merge_request_commits_metadata:

SELECT "merge_request_commits_metadata"."sha"
FROM "merge_request_commits_metadata"
INNER JOIN (
  SELECT "merge_request_diff_commits"."merge_request_commits_metadata_id"
  FROM "merge_request_diff_commits"
  WHERE "merge_request_diff_commits"."merge_request_diff_id" IN (
    SELECT "merge_request_diffs"."id"
    FROM "merge_request_diffs"
    WHERE "merge_request_diffs"."merge_request_id" = $1
      AND "merge_request_diffs"."diff_type" = 1
    ORDER BY "merge_request_diffs"."id" DESC
    LIMIT 100)
    AND "merge_request_diff_commits"."project_id" = $2
  LIMIT 10000) AS diff_commits
  ON diff_commits.merge_request_commits_metadata_id = merge_request_commits_metadata.id
WHERE "merge_request_commits_metadata"."project_id" = $2

Estimated plan, total cost ≈58,000. An EXPLAIN (ANALYZE, BUFFERS) of this query against !139219 did not return a result within two hours on Database Lab, so only the estimate is linked; the production symptom it causes is the render timeout described above.

With mr_diff_commits_read_new_table disabled — its state on GitLab.com today — the project_id filters above are absent (project_id_pruning_enabled? requires both flags) and this second query also runs. merge_request_commits_metadata_id IS NULL is not indexed; the only index on that column is partial, WHERE merge_request_commits_metadata_id IS NOT NULL:

SELECT "merge_request_diff_commits"."sha"
FROM "merge_request_diff_commits"
WHERE "merge_request_diff_commits"."merge_request_diff_id" IN (
  SELECT "merge_request_diffs"."id"
  FROM "merge_request_diffs"
  WHERE "merge_request_diffs"."merge_request_id" = $1
    AND "merge_request_diffs"."diff_type" = 1
  ORDER BY "merge_request_diffs"."id" DESC
  LIMIT 100)
  AND "merge_request_diff_commits"."merge_request_commits_metadata_id" IS NULL
LIMIT 10000

Estimated plan: the LIMIT 10000 caps a nested loop otherwise costed at ~1.9M with 2.1M estimated rows.

New query

$2..$4 are the SHAs the references in the text resolved to, so the IN list and the LIMIT are both the number of references:

SELECT "merge_request_commits_metadata"."sha"
FROM "merge_request_commits_metadata"
WHERE "merge_request_commits_metadata"."project_id" = $1
  AND "merge_request_commits_metadata"."sha" IN ($2, $3, $4)
  AND (EXISTS (
    SELECT 1
    FROM "merge_request_diff_commits"
    WHERE (merge_request_diff_commits.merge_request_commits_metadata_id = merge_request_commits_metadata.id)
      AND (EXISTS (
        SELECT 1
        FROM "merge_request_diffs"
        WHERE "merge_request_diffs"."merge_request_id" = $5
          AND "merge_request_diffs"."diff_type" = 1
          AND (merge_request_diffs.id = merge_request_diff_commits.merge_request_diff_id)))
      AND "merge_request_diff_commits"."project_id" = $1))
LIMIT 3

Plans, against !139219 (227 diff versions, merge_request_id = 268857248), with three SHAs: the current head, a commit from diff version 41 of 227, and one nonexistent. All variants drive from the unique (project_id, sha) index with nested-loop EXISTS probes; the planner hazard that needed a LATERAL rewrite in the sibling query (!237028 (merged)) does not appear:

One observation for the pruning-flag rollout rather than for this MR: with the inner project_id filter, the version-41 SHA is not returned (probe with only that SHA), because merge_request_diff_commits.project_id is not yet backfilled for that row's era. #commit_exists? already had this behavior.

And, only for SHAs not found above and only while mr_diff_commits_read_new_table is disabled. DISTINCT is needed because the same SHA is stored once per diff version it appears in; without it, duplicates of one SHA could fill the LIMIT before the other SHAs are found:

SELECT DISTINCT "merge_request_diff_commits"."sha"
FROM "merge_request_diff_commits"
WHERE "merge_request_diff_commits"."merge_request_diff_id" IN (
  SELECT "merge_request_diffs"."id"
  FROM "merge_request_diffs"
  WHERE "merge_request_diffs"."merge_request_id" = $1
    AND "merge_request_diffs"."diff_type" = 1
  ORDER BY "merge_request_diffs"."id" DESC
  LIMIT 100)
  AND "merge_request_diff_commits"."sha" IN ($2, $3, $4)
LIMIT 3

Plans. merge_request_diff_commits has no index on sha, so this scans the primary-key ranges of the (at most 100) diff versions and filters by sha in the heap — the all-miss run is the upper bound, since matches only let the LIMIT stop sooner:

Getting a row-returning plan required an old SHA deliberately: rows written since the dual-write to merge_request_commits_metadata began have sha populated only in the metadata table, so on recently-pushed merge requests this fallback finds nothing — the metadata query has already answered for those rows.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Hordur Freyr Yngvason

Merge request reports