Backport of 'Skip rewriting unchanged vulnerability identifiers' to 19.4

Kate Grechishkinarequested to merge
backport-256535-19-4 into 19-4-stable-ee

What does this MR do and why?

Backport of !256535 (merged)

Makes Security::Ingestion::Tasks::IngestIdentifiers upsert only the identifiers that are new or genuinely different, instead of rewriting every identifier on every scan.

Measured over one hour on patroni-sec:

vulnerability_identifiers
Inserts per hour 39,166
Updates per hour 903,257
Updates per insert 23.1
HOT fraction 32.0%
Index entries written per hour by the non-HOT share 4,299,400 across 7 indexes

96% of row-touches on this table are updates. Identifiers are stable by nature — a CVE's name and URL do not change between scans — so most of those rewrites change nothing, while each one takes an exclusive row lock held until the slice transaction commits. Those are the rows the statement timeouts in #629530 land on.

Behind skip_unchanged_vulnerability_identifiers (gitlab_com_derisk, default off).

Three things worth reviewing closely

Ids now come from two places. after_ingest previously built its fingerprint map purely from RETURNING. Skipped rows return nothing, so the map is seeded from the pre-loaded rows and then overlaid with RETURNING. Without that, finding_map.identifier_ids would go nil for every skipped identifier — the failure mode this MR most needs to avoid.

updated_at is excluded from the comparison, deliberately. It is rewritten on every scan, so including it would mark every row changed and make the filter a no-op. COMPARED_ATTRIBUTES is external_id, external_type, name, url.

The lookup is grouped by project rather than by_projects(...).with_fingerprint(...), which would form a cross product. That is irrelevant for report ingestion, where a slice belongs to one pipeline and therefore one project, but not for CVS ingestion via IngestCvsSliceService, where one batch spans many projects.

One trade-off to name: this adds a SELECT inside a transaction that the RCA separately wants to shorten. It should still be a clear net win — one indexed read to avoid up to 50 exclusive row locks held to commit — but it is a trade, not free.

This change also puts Vulnerabilities::Identifier.with_fingerprint to use for the first time, so its entry is removed from scripts/lint/keela_baseline.yml.

Query plan for the new lookup

IngestIdentifiers groups report identifiers by project and runs, once per project in the batch:

SELECT * FROM vulnerability_identifiers WHERE project_id = $1 AND fingerprint IN ($2, ...)

A slice of 50 findings can carry up to 1000 unique identifiers, so 1000 fingerprints in one IN list is the worst case for a single project.

Plan for that worst case, taken against a synthetic clone of vulnerability_identifiers (2,000,000 rows, 2,000 projects, 1,000 identifiers per project, 424 MB total relation size) in a local GDK sec database — no Database Lab access here:

Index Scan using vi_clone_project_id_fingerprint on vi_clone
  (cost=0.43..1408.51 rows=1 width=123)
  (actual time=0.035..1.859 rows=1000 loops=1)
  Buffers: shared hit=1010
Planning Time: 2.654 ms
Execution Time: 1.909 ms

It uses the existing unique index (index_vulnerability_identifiers_on_project_id_and_fingerprint); no new index is needed.

The per-project grouping avoids a single query with project_id IN (...) spanning multiple projects for the same 1000 fingerprints. All three variants below still pick the same index scan, but the number of index probes scales with projects × fingerprints:

Projects in IN list Execution Time Buffers (shared hit)
1 (grouped, shipping form) 1.909 ms 1010
20 6.949 ms 1441
200 28.667 ms 5504
1000 115.404 ms 23651

Caveats: the data is synthetic and uniformly distributed, the table was freshly analysed, and every block was already in shared buffers, so the absolute timings are optimistic compared with production. What the numbers establish is the plan shape (index scan on the existing unique index, no sequential scan) and how the single-query form degrades as a batch spans more projects — relevant because this query runs inside the ingestion transaction that the linked RCA is trying to shorten.

References

MR acceptance checklist

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

  • This MR is backporting a bug fix, documentation update, or spec fix, previously merged in the default branch.
  • The MR that fixed the bug on the default branch has been deployed to GitLab.com (not applicable for documentation or spec changes).
  • The MR title is descriptive (e.g. "Backport of 'title of default branch MR'"). This is important, since the title will be copied to the patch blog post.
  • Required labels have been applied to this merge request
  • This MR has been approved by a maintainer (only one approval is required).
  • Ensure the E2E Omnibus GitLab EE pipeline (triggered by thee2e:test-on-omnibus-ee job) has succeeded. If it has failed (or shows warnings), investigate the failures. If you determine the failures are unrelated, you may proceed. If you need assistance investigating, request help in the #s_developer_experience Slack channel to confirm the failures are unrelated to the merge request.

Note to the merge request author and maintainer

If you have questions about the patch release process, please:

Merge request reports