Skip to content

The forced-plan-failures alert read gets the covering index the planner will actually take — the composite it needed was already there and priced out, so it walked the whole fleet's two-hour slice per server (#3573) - #3586

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/3573-lane
Sep 18, 2026

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

The lie

#3573 reopened #3496's deferred second site with the cleanest evidence the series could have produced: since the v3.8.0 install the largest store's alerting-read failures were exactly three, all on ForcePlanFailuresSql, newest at 10,322 ms against the 10 s deadline, while the fixed recent-runs site sat at zero. The live EXPLAIN (ANALYZE, BUFFERS) on the shipped statement named the mechanism:

Index Scan Backward using _hyper_.._chunk_query_store_stats_collection_time_idx
  Index Cond: (collection_time > now() - '02:00:00')
  Filter: (server_id = ...)
  Rows Removed by Filter: 691,058        actual rows: 37,878
  Buffers: shared hit=54,664 read=2,643

Every server's read walks the entire fleet's two-hour slice of query_store_stats through TimescaleDB's default time index and throws 95% of it away. Forty-three servers deep, every 30 s pass, that is the whole slice re-read forty-three times per cycle: 422 ms warm because this statement is what keeps the slice in cache, and a cold tail past 10 s whenever cache pressure evicts a slice ~4x the size it had when the deadline was derived (the table went from ~6 GB to 23 GB).

The issue's diagnosis was that query_store_stats "carries only a collection_time index" and the fix was the (server_id, collection_time) composite collection_log already has. That is where this PR had to stop and check, because V1 of the ladder generates exactly that index for every collector table.

The index was there. The planner would not take it.

A read of the production catalog (the same SSM/psql path the issue's EXPLAIN used; pg_indexes on the hypertable and on the very chunk in the plan) shows idx_query_store_stats_time (server_id, collection_time) present on both, beside the two PgTableTuning indexes and the Timescale default. So the fix as proposed — a second plain composite, DESC-ordered — would have been a duplicate the planner priced and ignored identically, built across a 23 GB hypertable for nothing.

Why it is ignored is in pg_stats: server_id's physical correlation on the live chunk is 0.022 (forty-three servers interleaved by collection pass), collection_time's is 0.99999. The cost model therefore charges the composite one random page per tuple — ~50K pages at random_page_cost = 4 — while the perfectly-correlated time index's 54,664-page stream costs 58,677 on paper and wins. The controlled toggle proves it is a pricing problem and not a catalog one: under random_page_cost = 1.1 the identical statement took the existing composite (Bitmap Index Scan, Index Cond on both columns) and touched 5,063 buffers instead of 57,307 — Heap Blocks: exact=5001 for 67K rows, because a server's rows land in one contiguous run per collection pass (~13 rows a page) that a single correlation statistic cannot see. The composite was right the whole time, by 11x.

The fix: make the choice stop depending on the cost model

A covering index: (server_id, collection_time DESC) INCLUDE (database_name, query_id, plan_id, force_failure_count, is_forced_plan, plan_forcing_type, last_force_failure_reason) — the read's predicate as the key and every other column it touches as INCLUDE. The plan becomes an Index Only Scan whose cost is the index pages for one server's two hours and nothing else: no heap term to misprice, at any random_page_cost, at any share of the fleet the busiest server grows into. Both uncompressed production chunks read relallvisible = 100% of relpages (the insert-autovacuum override PgTableTuning already applies to this table is what keeps them there), so the visibility-map precondition an Index Only Scan needs is already met in the field.

On a PG18 / TimescaleDB 2.28.1 rig seeded in the production write pattern (43 servers, per-pass contiguous batches, four compressed and two live day-chunks, the production's four indexes), the shipped statement reproduced the production plan exactly — time index, Filter: (server_id = 1), Rows Removed by Filter: 18900 — and with the covering index became Index Only Scan Backward … Heap Fetches: 0, 50 buffers against 1,514, at default costs.

This is the same cure PgTableTuning already applied to this same table in 2026-07 ("a PLAIN index still paid a Bitmap Heap Scan re-fetching the aggregate columns, so the three composer indexes are COVERING") — the discipline existed one class over and had not reached the alerting pass.

It lives in PgTableTuning, not the ladder — and that is a correction to the brief, not a preference

PgTableTuning's own contract is results-invariant perf indexes: "must NOT bump StorageVersion — that would gate the Viewer's connect-time schema check on perf indexes it does not need." Three more reasons this one belongs there, each measured:

  • The ladder applier wraps every rung in a transaction block, and CREATE INDEX … WITH (timescaledb.transaction_per_chunk) is refused inside one ("cannot run inside a transaction block"). PgTableTuning runs each statement autocommit.
  • Failure isolation. A rung that exceeds its 300 s silence budget fails the service start. A PgTableTuning statement that does warns, the store keeps working, and the next start retries — the right failure mode for a build on a 23 GB hypertable.
  • Sequencing. PgTableTuning runs after the TimescaleDB block (so the index is created on a hypertable and propagates to chunks) and before collectors start (so the build contends with no live inserts from this service).

So there is no V127, no StorageVersion bump, no viewer-probe sentinel, and no PgSchemaGenerator change: a fresh store and an upgraded store both get the index from the same startup statement.

What the rig measured about compressed chunks, per-chunk builds, and locks

  • Compressed chunks are free. CREATE INDEX on the hypertable with 4 of 6 chunks compressed succeeded in 273 ms with no notice. Every chunk got a copy; the compressed chunks' copies are 8,192 bytes each — their uncompressed relations are empty shells on 2.28.1 — while the two live chunks got 46 MB and 27 MB. The owner's ruling, "index only the uncompressed/new chunks," needs no mechanism: it is what the engine does. New chunks inherit the index at creation; decompress_chunk fills the shell (56 MB) and compress_chunk empties it again (8 KB). Both measured.
  • transaction_per_chunk was measured and rejected. It works autocommit and is idempotent with IF NOT EXISTS, but it takes the same ShareLock on the hypertable root first — no write-concurrency gain — and a cancel mid-build (which is exactly what a CommandTimeout is) commits the chunks built so far, leaves the parent index indisvalid = false and the live chunk unindexed, after which the idempotent CREATE INDEX IF NOT EXISTS reports "already exists, skipping" on every start forever. A one-timeout permanent trap. The plain form is one transaction: a cancel leaves nothing, the next start retries.
  • CREATE INDEX CONCURRENTLY is refused on hypertables ("hypertables do not support concurrent index creation").
  • Locks. The plain build holds ShareLock on the hypertable root for its duration: reads proceed (AccessShare is compatible), every INSERT into any chunk queues behind it. Pre-collection, on a store with one writing service, that is nothing; a collision with the compression job on yesterday's chunk makes one wait for the other, and if that puts this build past its 300 s budget the outcome is the cancel-and-retry above.

The cost, stated plainly — this is the part that wants the owner's eye

INCLUDE disables btree deduplication, so this index is ~86 bytes/row on the rig where V1's deduplicated composite is ~7. On the largest store that is roughly 0.7–0.9 GB per day-chunk (8–16 M rows/day against a 4.7–9.4 GB heap per chunk), one more btree maintained on every insert into the store's largest table, and a first build that reads both live chunks (~14 GB of heap, 24.6 M rows on the day of the catalog read) inside a 300 s budget with maintenance_work_mem at 1.6 GB and two parallel maintenance workers. The footprint is self-limiting — it is the one or two uncompressed chunks, and the compression policy reduces each to 8 KB a day later — but it is ~15x the plain composite the issue pictured. It is in the same class as idx_query_store_stats_query_hash, which the store already carries at 1,639 MB on today's chunk.

Two cheaper shapes were considered and are deliberately not in this PR. random_page_cost: it flipped the plan, but by 53,095 to 58,677 for a server holding ~8% of the fleet's rows, and that ratio scales with the share, so it flips back on the next-busiest store or this one a month on — and a store-wide planner setting is an owner's ruling for every read at once. A partial index WHERE is_forced_plan would be a few MB, but the read aggregates unforced rows on purpose (a plan's previous sighting may be its unforced one), so it needs the statement reshaped and its first-sighting semantics changed. That is the cheaper long-run shape and the one to weigh against the footprint above; it is deferred rather than smuggled in.

Census: the other reads of query_store_stats

32 sites (grep -rn "FROM query_store_stats" Darling/ | grep -v Tests): 7 are continuous-aggregate definitions with no server predicate, 3 are ladder view re-expansions, 1 is this read. Of the remaining 21:

  • Gains for free (columns ⊆ the INCLUDE set): QueryStoreBackfill's candidate-database scan, SELECT DISTINCT database_name … WHERE server_id = $1 AND collection_time > $2 — an Index Only Scan on the new index is available to it.
  • Same shape, NOT covered — same disease, unchanged by this PR: the Query Store regression readers (DarlingQueryStoreRegressionReader ×2, ViewerDataService.QueryStoreRegressions ×2), the top/trend readers (DarlingDataReader.GetQueryStoreTop, DarlingTrendReader ×2, ViewerDataService.QueryStore ×3, ViewerDataService.QueryTrends ×2, QueryStoreTrendRouting ×2). All are server_id = $1 AND collection_time <range> with wide column lists, so the planner faces the identical composite-versus-stream choice for their uncompressed tail. Their windows are hours to days and mostly fall on compressed chunks, where compress_segmentby = 'server_id' already supplies the server dimension, so their exposure is the same tax at a different dose. Named here, not fixed here.
  • Already served: the point lookups keyed on (server_id, database_name, query_id[, plan_id]) take idx_query_store_stats_server_db_query_plan_time; MIN(collection_time) and the SELECT 1 … LIMIT 1 probes are covered by the plain composite as it stands.

What this PR does not do

  • The 10 s deadline does not move. The cadence bound has not changed, and a deadline re-fitted to a read that now costs milliseconds would only mean the next drift is caught later. The 1,744.9 ms anchors in DarlingAlertReadAdapter.AlertPassCommandTimeoutSeconds and both ServiceCommandDeadlines sites keep their history and gain the arc (6 GB → 23 GB, the fleet-width tax, 10.3 s, this index).
  • No query reshape. ForcePlanFailuresSql is byte-identical; its remarks gain a paragraph naming its access path and the consequence of referencing a column the index does not carry.
  • No ladder rung, StorageVersion bump, viewer probe, or generator change (above). No random_page_cost change. No partial index. No CHANGELOG.md edit (coordinator's).

Tests

Darling.Tests targets net10.0-windows; this Mac has no Microsoft.WindowsDesktop.App, so CI is the first execution of the new tests. Both affected builds ran clean here — dotnet build Darling/Darling.Tests/Darling.Tests.csproj -c Release (0 warnings, 0 errors) and the Linux dotnet publish gate — and the pins were verified by construction against the committed source:

  • ForcePlanFailuresAccessPathTests.TheCoveringIndex_CarriesExactlyTheColumnsTheReadReferences — parses every qs.<column> out of ForcePlanFailuresSql and asserts the set equals PgTableTuning.ForcePlanFailuresIndexColumns both ways (a missing column silently degrades the plan; a surplus one is bytes on every insert), and that the key is (server_id, collection_time).
  • …TheStatement_IsBuiltFromTheColumnList_AndTakesNeitherMeasuredTrap — the literal statement equals the derivation from that list; not CONCURRENTLY, not transaction_per_chunk, not partial.
  • …TheShippedRead_PlansAsAnIndexOnlyScanOnTheCoveringIndex_AgainstDevPostgres (DARLING_TEST_PG) — builds the store the way the service does (ladder, hypertable conversion where TimescaleDB is present, PgTableTuning.ApplyAsync), checks pg_indexes for the definition, seeds six servers' rows in per-pass contiguous batches, VACUUM ANALYZE, and EXPLAINs the exact shipped SQL with its real bound parameters: asserts Index Only Scan on the new index, no collection_time_idx, no server_id applied as a post-scan Filter, and that the adapter still returns the one seeded failure delta through the new path. The production failure was a plan choice with the right index already in the catalog, so a pg_indexes-only check would have passed on the broken store; this one asks the planner. Its exact seed and EXPLAIN were run on the 2.28.1/PG18 rig and produced Index Only Scan Backward using _hyper_1_1_chunk_idx_query_store_stats_server_time_forcing with both predicates in the Index Cond.
  • PgTableTuningTests — the new statement's text, 8 CREATE INDEX IF NOT EXISTS, 13 statements.

Out of lane, observed while here

  • idx_query_store_stats_query_hash (the composer's covering index, 7 INCLUDE columns) is 1,639 MB on today's chunk with idx_scan = 0 since that chunk was created, against 58 MB for the plain composite — that is one day of a Custom Views panel not being opened, not evidence of anything, but it is the footprint class this PR joins and worth knowing when weighing the cost above.
  • The issue's "collection_time-only index" premise is corrected by this PR's catalog read; the mechanism it found (the fleet-width filter) and every number it measured stand.

…er will actually take, instead of walking the whole fleet's two-hour slice per server (#3573)

The read's (server_id, collection_time) composite was on the production
store all along — V1 generates it — and the planner priced it out:
server_id's physical correlation is 0.022, so the cost model charged one
random page per row and preferred streaming the entire fleet's two-hour
slice through the time index (Rows Removed by Filter: 691,058 to keep
37,878; 57,307 buffers; 422 ms warm, 10.3 s cold against a 10 s deadline).
Forced under random_page_cost = 1.1 the same statement took the existing
composite at 5,063 buffers, so a second plain composite would have been
priced and ignored identically.

Covering removes the heap term the model got wrong: (server_id,
collection_time DESC) INCLUDE every other column the read touches, so it
plans as an Index Only Scan over one server's rows (rig: 50 buffers vs
1,514, Heap Fetches: 0). It lives in PgTableTuning, not the ladder —
results-invariant perf, autocommit, failure-isolated — as a plain CREATE
INDEX: on TimescaleDB 2.28.1 compressed chunks get an 8 KB shell so only
the live chunks carry it; transaction_per_chunk was measured leaving an
invalid parent index after a mid-build cancel that IF NOT EXISTS then
skips forever; CONCURRENTLY is refused on hypertables.

Tests pin the index's column list against the read's own column
references, the statement text against that list, the two rejected
shapes, and (DARLING_TEST_PG) EXPLAIN the shipped statement on a seeded
store for the Index Only Scan. The 1,744.9 ms deadline anchors keep
their history and gain the arc; the 10 s deadline does not move.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — reviewed the covering-index addition (idx_query_store_stats_server_time_forcing) in PgTableTuning, the doc-only updates to DarlingAlertReadAdapter/ServiceCommandDeadlines, and the new ForcePlanFailuresAccessPathTests. Verified: the INCLUDE column list matches every qs.* column the shipped ForcePlanFailuresSql references (checked both directions), the CREATE INDEX statement is derived from the same shared column list the test pins against (no duplicated literal to drift), the live EXPLAIN test's seed/delta math checks out (pass 1→pass 0 gives failure_delta=1, total_failures=7 matching the assertions), the statement is applied through PgTableTuning's existing per-statement failure-isolated loop (not the migration ladder, consistent with the PR's stated rationale), and it doesn't bump StorageVersion. No Lite/Darling parity gap — Lite's DuckDB schema carries none of the Postgres composer covering indexes, so this is Darling/Postgres-only tuning with no counterpart needed. No SQL injection concerns (index name/columns are compile-time constants). No blocking issues found.

@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) September 18, 2026 15:17
@erikdarlingdata
erikdarlingdata merged commit 552a7bf into dev Sep 18, 2026
8 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/3573-lane branch September 18, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant