db: drop the superseded env_build_assignments tip-lookup index - #3409
Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit ce2ba97. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
…ts statistics (#3403) The current-tip lookup orders by (created_at DESC, build_id DESC) within an (env_id, tag) equality, but the existing index stops at created_at, leaving a top-N sort whose plan choice depends on sampled statistics — an autoanalyze re-sample can temporarily flip it to a much slower plan. Completing the index to the full sort key makes the lookup a pure ordered index scan (LIMIT 1, no sort) that stays cheapest under any statistics roll. Also raises the statistics targets on the key columns and re-analyzes, same treatment as env_builds.status_group. This PR is deliberately ADDITIVE — it only creates the new index and hardens statistics. Dropping the superseded 3-column index (identical ordering prefix, redundant write maintenance) is split into a follow-up PR (#3409) to merge only after this one has soaked in production. Query-shape check across every first-party consumer of this table: each query falls into (a) the (env_id[, tag]) prefix family — served identically or better by the new index, since it shares the exact ordering prefix; (b) build_id lookups — served by idx_env_build_assignments_build, untouched; or (c) primary-key/uniqueness paths. The (created_at DESC, build_id DESC) tie-break matches the high-frequency tip check that motivated this change; this repository's own latest-build laterals order by created_at alone and ride the same prefix unchanged. Latest-default-build laterals that select build_id can now run index-only.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b914eff81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a production index-maintenance migration on a hot-insert table (env_build_assignments) with an explicit operational precondition — the PR description says not to merge until the prerequisite additive migration (#3403) has soaked in production. That sequencing/timing judgment isn't something CI or automated review can verify, so it's worth a human sign-off before merge.
What was reviewed: the up/down migration logic (CONCURRENTLY drop/recreate, statement_timeout bounds and restoration, INVALID-index cleanup on rollback), and the claimed index-coverage rationale (4-column index shares the 3-column index's ordering prefix). The concerns raised by the bug-hunting system about connection-pool statement_timeout scoping and the prerequisite index not yet existing in this checkout were investigated and ruled out — the latter is because #3403 is intentionally not yet merged, not a bug.
Extended reasoning...
Overview
Single-file migration that drops the old 3-column tip-lookup index (idx_env_build_assignments_env_tag_created) on env_build_assignments now that a 4-column index with the same ordering prefix supersedes it. Uses DROP INDEX CONCURRENTLY with a bounded 1h statement_timeout on the up path, and a down path that clears any leftover INVALID index before rebuilding.
Security risks
None — this is a pure schema/index change with no new query paths, auth logic, or user-controllable input.
Level of scrutiny
Migrations that run CONCURRENTLY against a hot-insert production table warrant more scrutiny than an average PR, even though the SQL itself is mechanically simple. The bigger risk here isn't correctness of the SQL (which looks sound: NO TRANSACTION mode, bounded timeout, proper restoration to the 3h baseline, IF EXISTS/IF NOT EXISTS guards, INVALID-index cleanup on rollback) but the operational precondition stated in the PR description: this must not be merged until the prerequisite additive migration (#3403) has soaked in production. That's a human judgment call about deployment timing that automated review can't validate, so I'm deferring rather than approving outright.
Other factors
The bug-hunting system found no bugs. Candidate concerns it raised and ruled out were: whether the replacement 4-column index actually exists (it doesn't in this checkout, but that's expected — #3403 hasn't merged yet, consistent with the PR's stated precondition), and whether SET statement_timeout reliably applies to the connection executing DROP INDEX CONCURRENTLY given the migrator uses a pgxpool with MaxConns=4 (packages/db/scripts/migrator.go) — plausible in theory but not something I can confirm is a real issue without knowing whether goose pins a single connection per migration, and even if the SET didn't apply, the connection would just fall back to the pool's 3h AfterConnect default rather than being unbounded, so worst case is a longer wait, not correctness risk.
6b914ef to
ce2ba97
Compare
2 participants
Follow-up to #3403 (the additive half): the 4-column index shares the 3-column index's exact ordering prefix, so every scan it served runs identically on the new one — keeping both only doubles write maintenance on a hot-insert table. DO NOT merge until #3403 has soaked in production; goose orders this migration after it by timestamp either way.
Schema proof, both definitions in this repo: the 3-column index (20251218160000 L42-L43,
(env_id, tag, created_at DESC)) is the exact leading prefix of the 4-column one (20260727032500 L24-L25,(env_id, tag, created_at DESC, build_id DESC)). Why a leading prefix makes the shorter index redundant: PostgreSQL docs — multicolumn indexes.