Repository navigation
db: drop indexes superseded by tests_run_id_idx and runs_package_enqueued_at_idx (MARSOHS-1642) - #14
Merged
Merged
Conversation
ElanHasson
marked this pull request as ready for review
September 21, 2026 22:31
…eued_at_idx (MARSOHS-1479) Co-authored-by: Cursor <cursoragent@cursor.com>
ElanHasson
force-pushed
the
drop-redundant-indexes
branch
from
September 21, 2026 22:38
ebf4d8d to
28a4152
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of MARSOHS-1479 (e2e canary runner DB tuning). Follow-up to #11, which declared
tests_run_id_idxandruns_package_enqueued_at_idx.Appends a migration that drops three indexes that the new ones supersede. The migration is
DROP INDEX IF EXISTSso that it is a no-op in production, where the indexes are dropped by hand first withDROP INDEX CONCURRENTLY(see below); on fresh databases it removes the indexes thatinitialcreates.downrecreates them.Do not merge until
The three hand-run
DROP INDEX CONCURRENTLYstatements below have been executed on the production cluster. If this merges first, tern runs a plainDROP INDEXinside a transaction at app startup, which takes anACCESS EXCLUSIVElock onruns/tests— brief, but it blocks inserts ontests(and the concurrent web/worker replicas) while it runs.Merge order
add tests run_id index and runs package covering index.autovacuum-reloptions) if it lands first with a migration entry — rebase this branch so thepgMigrationsslice order stays stable (tern tracks migrations by position/name; entries must never be reordered once applied).Evidence (production, PG 16,
pg_stat_user_indexes.idx_scan)Observation window 2026-09-21 19:09:21Z → 19:33:18Z (24 min) while the app was active:
tests_run_id_idx+127,683,runs_finished_at_idx+693,runs_package_enqueued_at_idx+13,324 over the same window.EXPLAIN ANALYZEruns_package_idxruns (package)ScheduleRun/ListRunsForPackage(WHERE package = $1) before the covering index existed. It is a strict prefix ofruns_package_enqueued_at_idx, so nothing can need it.runs_enqueued_at_started_at_idxruns (enqueued_at, started_at)ListRunsForPackageplan (backward scan + package filter).ClaimRunusesruns_finished_at_idx.tests_package_idxtests (package)ListTestsForPackageInRange— but only as a mis-plan, see below. No production caller filterstestsby package alone (ListTestsForPackageis unused outside tests).Total reclaimed: ~1,160 MB (737 MB from the two
runsindexes alone).Plan checks (
EXPLAIN (ANALYZE, BUFFERS), all 15 packages, after today'sVACUUM (ANALYZE) runs)ScheduleRun(SELECT count(*) FILTER (WHERE finished_at IS NULL), max(enqueued_at) FROM runs WHERE package = $1): 15/15 packages →Index Only Scan using runs_package_enqueued_at_idx, 1.7–40 ms.ListRunsForPackage(… WHERE package = $1 ORDER BY enqueued_at DESC LIMIT 5): 15/15 →Index Scan using runs_package_enqueued_at_idx, <0.1 ms.ClaimRun(WHERE started_at IS NULL AND finished_at IS NULL AND package = ANY($1) AND NOT package = ANY($2) ORDER BY enqueued_at LIMIT 1, several include/exclude shapes): all →Index Scan using runs_finished_at_idx+ top-N sort, ~0.1 ms. Never touches a package index.ListTestsForPackageInRange(WHERE package = $1 AND result->'started_at' BETWEEN $2 AND $3 ORDER BY result->'started_at', 7-day window):Index Scan using tests_expr_idx+ filter → ~10 ms.BitmapAnd(tests_expr_idx, tests_package_idx)→ 80–1,020 ms. Thetests_package_idxbitmap side reads 100k–590k TIDs per query (e.g. dbaas: 575,430 rows, 971 ms) to intersect with the 9,257 rows the 7-daytests_expr_idxrange produces. The planner over-estimates thetests_expr_idxrange (63k est vs 9k actual) and under-estimates the package bitmap cost.SET LOCAL enable_bitmapscan = offin a rolled-back transaction) for dbaas, spaces, account, functions:Index Scan using tests_expr_idx, 9.5–10.9 ms — i.e. the same plan the fast 6 packages already get.So
tests_package_idxis not "unused" in the strict sense — the 3 non-me scans in the window are package-page views on one of the 9 affected packages — but every one of those scans made the request 8–100× slower than it would be without the index. Dropping it removes a mis-plan rather than a needed access path. (A compositetests (package, (result->'started_at'))would be the optimal index for that page and could be a follow-up; it is not needed for this PR to be safe.)Hand-run production steps (before merging)
Run each statement on its own, outside a transaction (
CONCURRENTLYcannot run inside one). Each waits for in-flight transactions on the table to finish, does not block reads/writes, and is safe to re-run if interrupted (an interruptedDROP INDEX CONCURRENTLYleaves the index markedINVALID; just run it again).Verify afterwards:
Expected: none of the three names present;
tests_run_id_idx,runs_package_enqueued_at_idx,runs_finished_at_idx,runs_started_at_finished_at_idx,tests_expr_idx,runs_pkey,tests_pkeyremain.Rollback if needed (also outside a transaction):
Testing
TestPG_Init_Indexesnow also asserts the three indexes are absent afterIniton a fresh database (they are created byinitial, so this exercises theDROP). Fails without the migration, passes with it.gofmt -l .clean,go vet ./...clean,PG_DSN=… go test -race ./db/...green against a throwawaypostgres:16.