test(datastore): pin the ordered eviction walks against a bitmap plan

What this fixes

Three test:integration jobs on main failed in the last day on internal/datastore.TestNpmRemoteVersionStore_ListIDsForDeleteAll_UsesTheKeysetIndexBackward: job 16059475513 on PostgreSQL 16, and jobs 16058381758 and 16051770827 on PostgreSQL 18. Each reported the same plan:

Limit
  ->  Sort
        Sort Key: npm_remote_versions.created_at DESC, npm_remote_versions.id DESC
        ->  Bitmap Heap Scan on npm_remote_versions_p16
              Filter: (... ROW(created_at, id) < ROW(...))
              ->  Bitmap Index Scan on npm_remote_versions_p16_pkey

All three of the test's assertions rest on the ordering coming from the index: Scan Backward present, no Sort Key, and the row-value bound folded into an Index Cond. explainListPlan pins enable_seqscan alone, so a bitmap heap scan under a Sort stays available — and on a three-row fixture its cost sits close enough to the ordered index scan's that the choice turns on whatever else the shared database happened to hold when ANALYZE ran. The assertion was deciding on statistics the fixture does not control, which is also why two PostgreSQL versions produced it and the third did not.

What changed

  • explainKeysetPlan pins enable_seqscan and enable_bitmapscan, removing the alternative rather than out-costing it.
  • explainListPlan keeps its single setting and its 36 other callers, so no caller that rests on the cost estimate to rule a bitmap plan out loses that signal. Both delegate to a shared explainOnPinnedConn.
  • The three ordered npm remote eviction walks move onto the new helper. The versions walk is the one that failed; the packages and files walks carry the same Sort Key assertion on fixtures of three rows, so they carry the same exposure and had not yet fired.

The *testing.T parameters on the three helpers are renamed tt to t, which is internal to each function and changes no call site. docs/dev/go-testing.md reserves tt for a subtest's inner parameter; these are shared helpers, so thelper's naming check applies and now passes.

The trade this makes

internal/datastore/migrations/schema_helpers_test.go already made the same choice for its helper of this name and records the cost: with no bitmap plan left to produce, a regression whose symptom would have been a bitmap plan comes back as an index scan, and only the caller's own Sort and Index Cond assertions separate the two. All three walks moved here carry both, which is why they are the ones that move.

Verification

  • The three walks pass against a PostgreSQL matching the CI service's max_locks_per_transaction=1024.
  • The full internal/datastore integration suite runs with one failure, TestListNpmVersionIDsForDeleteAll_HostedKeepsTheAcceptanceBound, which reproduces identically on a clean origin/main and is unrelated to this change.
  • golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/datastore/ reports nothing on either changed region.

Scope

Test-only. No production file is touched, and the change is confined to which planner settings one EXPLAIN helper pins.

The two remaining main failures in the same sweep are a different root cause (the counter pipeline's dirty set is one globally-named key, so concurrently running suites on the shared CI Redis claim each other's scopes) and are addressed separately.

Related issue: #817 (closed) Related issue: #818 (closed)

Edited by David Fernandez

Merge request reports

Loading
Loading