fix(indexer): give the planner real distinct counts for provider snapshot join keys - #3836
Conversation
…shot join keys The gpu breakdown stops probing the indexes on "providerSnapshotNode" and "providerSnapshotNodeGPU" and hashes both tables whole once its window passes roughly 120 days, because the 30k row sample ANALYZE takes puts the distinct counts of the two join keys 10 to 20 times below the real ones. On a copy of the production database a 366 day window took 8.2 s warm before the overrides and 224 ms after, for byte identical results; 180 days goes from 4.8 s to 169 ms and the 30 day default is unchanged. A negative n_distinct records the ratio instead of a count, so the values stay right as the tables grow. The expression statistics do the same job for the case insensitive vendor and model filter, which was estimated at about four rows and made the planner scan the GPU table first.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 52 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a production database migration that manually overrides planner statistics (n_distinct) and adds version-gated DDL, and the author's own description flags "verification still outstanding" on the staging query performance, a human look would still be worthwhile.
What was reviewed: the n_distinct override values and their negative-fraction semantics, the version-guarded DO block for CREATE STATISTICS on Postgres >= 14, the trailing ANALYZE statements, and the _journal.json registration. Two candidate issues were also examined and ruled out: ANALYZE running inside drizzle-kit's per-migration transaction wrapper, and the CREATE STATISTICS ... ON lower(vendor), lower(name) expression-list syntax.
Extended reasoning...
Overview
The diff adds a single SQL migration for apps/indexer (0011_fix_provider_snapshot_join_key_statistics.sql) plus its _journal.json registration entry. The migration: (1) sets n_distinct storage parameters on two FK columns to correct the planner's cardinality estimate for a join used by /v1/gpu-breakdown; (2) conditionally (Postgres >= 14 only, via a DO $$ ... $$ block checking server_version_num) creates MCV extended statistics on lower(vendor)/lower(name); (3) runs ANALYZE on both affected tables so the new settings take effect. No application source or test files are touched.
Security risks
None identified. This is a DDL-only migration adjusting planner statistics metadata; it does not touch access control, input handling, or expose data. The EXECUTE inside the DO block is a static string with no user input, so no injection surface.
Level of scrutiny
This warrants a higher-than-default level of scrutiny despite being "just SQL": it runs against a 715 GB production table set, changes planner behavior for a live query path, and the author explicitly states in the PR description that post-deploy verification against staging Loki metrics is still outstanding — i.e., the author themselves hasn't fully closed the loop. Migrations are also inherently harder to walk back than application code (columns/indices already built on assumptions baked in), so correctness of syntax and transaction semantics matters more here than in typical PRs.
Other factors
Two candidate correctness concerns were raised and then ruled out by the automated investigation: whether ANALYZE can run inside the implicit transaction that drizzle-kit migrate wraps around each migration file, and whether CREATE STATISTICS ... ON lower(vendor), lower(name) is valid syntax (versus needing each expression independently parenthesized, as CREATE STATISTICS requires unlike CREATE INDEX). I was unable to independently execute Postgres or fetch drizzle-orm/drizzle-kit source in this sandbox (network and package-manager access are blocked here) to re-verify those two points myself, so I'm relying on the prior investigation's conclusion that they are not bugs. Given that reliance, plus the explicit "verification still outstanding" note from the author and the production-scale blast radius of a statistics-altering migration, I'm deferring rather than approving so a human can do a final sanity check before this merges.
1 participant
Why
#3811 bounded
/v1/gpu-breakdownto a date window but never measured the new query on production-size data. Measured now on a copy of the production database (715 GB, snapshots through 2026-03-12): the 30-day default is fast, but from about 120 days up the planner abandons the index probes and hashesproviderSnapshotNodeandproviderSnapshotNodeGPUwhole, which costs 30x more than the nested-loop plan it passed over.The reason is statistics, not the SQL.
ANALYZEsamples 30k rows, and on a high-cardinality FK column that sample lands 10 to 20 times below the truth:providerSnapshotNode.snapshotIdis recorded as 171,288 distinct values against 3.49M real, andproviderSnapshotNodeGPU.snapshotNodeIdas 405,255 against 4.37M. The planner therefore expects 27 and 63 rows per index probe where 1 to 6 turn up, and concludes that hashing 38M rows beats 14k probes. No change to the query shape can fix that, because the estimate is what drives the choice.Prod runs the same
default_statistics_target, so the same plans are expected there.What
n_distinctoverrides on the two join keys, plus extended statistics onlower(vendor)andlower(name)so the case-insensitive filter is no longer estimated at about four rows (which is what made the planner scan the GPU table first). The overrides are negative, which Postgres reads as a fraction of the table rather than a fixed count, so they stay correct as the tables grow: each snapshot has around 3.7 nodes and each GPU node around 5.8 GPUs.Measured on the production copy, same six windows before and after, two runs each, sessions pinned to UTC as Sequelize pins them:
No sequential scan of either table survives at any window, and every result set hashes identically before and after, which it must: statistics change the plan, not the rows.
The
CREATE STATISTICSis guarded onserver_version_num >= 140000because expression statistics need Postgres 14. Prod runs 14.22 and the CI database image is 14.18, so the branch executes in both; the guard only keeps the file applicable to an older sandbox, testnet or local database.ALTER COLUMN SET (...)andCREATE STATISTICStake onlySHARE UPDATE EXCLUSIVE, so the migration does not block writes. The twoANALYZEs are what make the overrides take effect; I left out anANALYZEofproviderSnapshotthat was in the investigation script, since autovacuum keeps that table current in prod and a one-shot analyze in a migration buys nothing.A note on what CI checked
validate (api)is skipped on this PR, because the path filter only triggers it onapps/api/**and the api's workspace dependencies, and the indexer's drizzle folder is neither. The api functional suite is the only thing in CI that applies these migrations to a real database, so nothing here exercised the migration. That gap predates this PR and affects every indexer migration; widening the api filter to coverapps/indexer/drizzle/**would be its own change.I ran it locally instead, twice:
apps/apifunctional gpu specs against a freshly migrated database (14 passing), anddrizzle-kit's migrator against a scratch database, reading backn_distinct=-0.27andn_distinct=-0.17on the two columns andstxkind {m,e}on the statistics object, which confirms both the MCV and the per-expression statistics were built.Verification still outstanding
Local numbers on the production copy are roughly 3 to 4x faster than prod for the same query, so the 366-day window should land near 1 s rather than the 15 to 40 s it would take today. That needs confirming on
console-api-mainnet-staging.akash.networkonce this deploys, against the Loki p50/p95 for the route. I will record it here.