chore(db): reword stats-migration comment - #3395
ValentaTomas wants to merge 2 commits into
Conversation
…#3393) ## Summary `InvalidateUnstartedTemplateBuilds` runs 2–60s instead of milliseconds because of a statistics blind spot, not scan cost: `status_group='pending'` is ~0.006% of env_builds (measured 19,906 of ~333M rows), so the default ~30k-row statistics sample never catches it — the value is absent from the column's MCV list and the planner estimates ~11 rows. That drives the update's join from the pending-scan side (~20k probes into env_build_assignments per execution, fresh-session EXPLAIN attached below) instead of one selective `(env_id, tag)` lookup. Every template-build registration then queues behind it on the envs row lock — the current upsert convoy (20–70s waits). Raising the per-column statistics target to 2000 (~600k-row sample) makes rare-but-hot status groups visible and flips the join order; the migration ANALYZEs immediately so the fix takes effect at promote rather than at the next autoanalyze. Same per-table-settings precedent as the autovacuum migrations. Fresh-session plan today (store-resident columnar unit present and unused — the misestimate, not columnar, is the bug): `Index Scan using idx_env_builds_status_group (rows=11 estimated, rows=19905 actual)` → nested-loop probing env_build_assignments ×19,905. ## Verification (post-draft) - **Scratch replay**: full migration chain via `scripts/migrator.go` on Postgres 16 applies cleanly; `goose down` (SET STATISTICS -1 + ANALYZE) and re-up also clean. - **Live read-only plan capture** (`PREPARE` + `EXPLAIN EXECUTE` ×7): custom plans (executions 1–5) are the pending-driven nested loop — est 11 rows, cost 662, the convoy plan; the generic plan (execution 6+) already flips to the assignments `(env_id, tag)` drive at cost 223. Production's pooled prepared statements re-run custom plans on every fresh connection, so busy templates keep landing on the bad side — this migration corrects the one wrong number in that comparison. - **Worst case bounded**: env_build_assignments is 323M rows; the heaviest template has ~438k assignment rows and the largest single `(env_id, tag)` group is ~219k. With truthful stats on both sides the planner picks the cheaper of the two known join orders — ≈ status-quo cost for that one template, ~1000× improvement for typical ones. - **Lock envelope**: ALTER…SET STATISTICS and ANALYZE both take SHARE UPDATE EXCLUSIVE (no read/DML blocking; queues behind a concurrent autovacuum on the same table at worst), bounded by the 1h timeout and rerunnable. Down is an instant revert. ## Postscript: the mechanism was observed live the same day At 16:09Z a routine autoanalyze happened to catch 'pending' into the MCV (frequency 0.0001) and the convoy vanished within the half-hour — rows returned collapsed from 14–17M/s (steady since 08:00Z) to ~0.5M/s, with no deploy, no columnar change, and the pending count unchanged. That is the dice roll this migration removes: at 0.0001 the value is one bad sample away from disappearing again, and every autoanalyze re-flips the coin. Target 2000 makes the catch deterministic (~36 expected sample hits).
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit 8682f56. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — comment-only rewording, no SQL or behavior changes.
What was reviewed: confirmed the diff only touches the explanatory comment above the ALTER TABLE ... SET STATISTICS statement; the SQL statements themselves (statistics target, ANALYZE, timeout bounds, down-migration) are byte-identical to the previous version. Since goose tracks migrations by version number rather than file checksum, this is safe for already-applied databases.
Extended reasoning...
Overview
The PR touches a single file, packages/db/migrations/20260725100500_env_builds_status_group_statistics.sql, and only rewords the prose comment explaining why the migration raises statistics target on env_builds.status_group. Every SQL statement (SET statement_timeout, ALTER TABLE ... SET STATISTICS 2000, ANALYZE, the down migration) is unchanged character-for-character.
Security risks
None. No SQL logic changed, no new privileges, no data exposure. Comment wording changes have zero runtime effect.
Level of scrutiny
This warrants minimal scrutiny — it's a pure documentation/comment change inside a migration file. The main thing to verify is that no SQL statements were altered, which I confirmed by diffing the file content directly.
Other factors
No outstanding review comments to address (only an automated Cursor bugbot summary, which independently reached the same low-risk conclusion). Migration is already merged (present in main at commit 8682f56 per the git log), so this is retroactively confirming safety of an already-landed trivial change.
|
Superseded: the comment rewording was folded into the tip commit directly (history rewritten in place). |
1 participant
Comment-only rewording of the 20260725100500 migration; no SQL statements change. Safe for already-migrated databases: goose tracks applied versions by number and does not checksum file content, so this neither re-runs nor invalidates the migration anywhere.