Repository navigation
Darling builds a background index so get_query_store_top no longer reads every entry a server holds (#4952) - #4969
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| [Fact] | ||
| public async Task TheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn() |
There was a problem hiding this comment.
REFUTED claim — the existing catalog HOT census never sees the new btree.
The body's test plan says: "The existing catalog check that no non-BRIN index covers any SET column also runs against the new indexes in the live test."
It does not. That check is QueryStoreIntervalWideBrinIndexLiveTests.NoNonBrinIndex_CoversAColumnTheUpsertSets_SoTheUpsertStaysHotForItsWholeSetList (QueryStoreIntervalWideBrinIndexLiveTests.cs:216), and at line 233 it calls the file's own EnsureAsync helper — which is QueryStoreIntervalWideBrinIndex.EnsureAsync (:60-61), the BRIN alone. ix_query_store_interval_wide_server_first_exec is never built in that database, so the NonBrinIndexedColumnsSql scan at :237 cannot see it. That test's body is untouched by this PR (the only change to that file is four private → internal modifiers).
So the repo's strongest HOT guard on this table — the data-independent, catalog-driven one, the one that fires on any future index whoever adds it — is blind to the index this PR adds to the hot upsert target. What this test (line 277) does instead is an empirical pg_stat_xact counter check on one seeded database: valuable, but it is not the census, and it is the census that would catch the next index.
This is trap #79 in its own words: "A roster-based census does not see the file it does not roster … An anti-pattern census sweeps the class (every tool body); rosters are for the positive contract only." The roster here is "whatever QueryStoreIntervalWideBrinIndex.EnsureAsync built".
One-line fix, and it passes as-is (neither server_id nor first_execution_time is in the SET list, verified at QueryStoreIntervalWide.cs:344-394 against :77-78): have NoNonBrinIndex_CoversAColumnTheUpsertSets_… ensure QueryStoreBackgroundIndexes.All instead of the BRIN spec alone. Then the claim becomes true and the census guards all three.
| private static async Task<List<string>> ReadAllAsync(NpgsqlConnection connection, CancellationToken ct) | ||
| { | ||
| var results = new List<string>(); | ||
| var end = DateTime.UtcNow; |
There was a problem hiding this comment.
The headline pin reads the clock twice, so the two "identical" reads are not over the same window.
end is re-read from DateTime.UtcNow on every call to ReadAllAsync, and ReadAllAsync is called twice — at :165 (absent) and :180 (present) — with Assert.Equal(absent, present) at :182. Between those two calls the test runs EnsureAllAsync (a CREATE INDEX CONCURRENTLY on the wide table plus a per-chunk build over three seeded day-chunks) and two ANALYZEs. Every second that takes moves start = end.AddHours(-hours) forward.
The table read bounds collection_time >= $2 (DarlingDataReader.cs:1953), and the wide seed puts collection_time on an hourly grid (LEAST(now, h + interval '1 hour') over generate_series(…, interval '1 hour'), :91-94). So if the elapsed gap straddles an hour grid point, the 24 h (or 60 h) floor crosses it and present loses the 60 rows that grid point holds for each server — and Assert.Equal(absent, present) fails with a diff that has nothing to do with the indexes. Probability is roughly gap / 3600 per boundary, which is exactly the size of flake that shows up on unrelated PRs and gets re-run rather than diagnosed.
This is trap #80 in its own words: "A test that reads the clock twice measures the CI scheduler: the delta-seeder tests failed at 123 and 243 on unrelated PRs (#3676). One anchor owns both ends of an interval."
Fix: hoist the anchor to the caller so both reads share it —
| var end = DateTime.UtcNow; | |
| private static async Task<List<string>> ReadAllAsync(NpgsqlConnection connection, DateTime end, CancellationToken ct) | |
| { | |
| var results = new List<string>(); |
…and pass one var end = DateTime.UtcNow; taken in the test body into both calls. (ProbePlanSql() at :189-194 takes a third reading, which is harmless — it only shapes an EXPLAIN — but the same anchor would do there too.)
|
|
||
| await using var scratch = await ScratchPostgres.CreateAsync(baseCs!, ct); | ||
| await using var connection = await OpenStoreAsync(scratch, ct); | ||
| await EnsureAllAsync(connection, ct); |
There was a problem hiding this comment.
The one test that pins the per-chunk drop-and-rebuild runs on a hypertable with zero chunks.
This test does not call SeedAsync, so at :254 and :267 collect.query_store_stats is a freshly converted hypertable holding no rows and therefore no chunks. The per-chunk arm (HypertableCreateSql, QueryStoreBackgroundIndexes.cs:133-135) and the per-chunk drop (:332 → HypertableDropSql) are exercised over an empty chunk set — the loop body never runs.
That matters because the hazard being defended against is not "the root flag is false", it is the state PgTableTuning.cs:241-243 actually measured: "a cancel MID-build … commits the chunks built so far, leaves the parent index indisvalid = false and the live chunk unindexed, after which this very idempotent statement reports 'already exists, skipping' on every start forever." The class doc at QueryStoreBackgroundIndexes.cs:39-40 names the read-then-drop as "what makes the per-chunk form safe here" — so this is the central safety claim of the whole LegacyProbe design, and the pin for it:
- fakes the state with
UPDATE pg_index SET indisvalid = falseon the root only (:263), leaving every chunk index present and valid — the opposite of the measured state's second half; and - then asserts only that the root is valid again and the OID changed (
:271-272), never that each chunk came back with the index.
Ensure_BuildsBothIndexesValid_PerChunkIncludingACompressedChunk_… does have the per-chunk census (:221-236: every chunk has a valid partial index, compressed shells included) — it is just not applied after a rebuild.
Fix: await SeedAsync(connection, ct); before :254, and re-run that same timescaledb_information.chunks census after the second EnsureAllAsync at :267. Then the rebuild is pinned over a real chunk set, which is the only version of this test that would have caught the failure mode PgTableTuning recorded.
| /// hypertable per-chunk build too: a cancel mid-build commits the chunks built so far and leaves the root index | ||
| /// <c>indisvalid = false</c>). <c>IF NOT EXISTS</c> would silently keep it, so <see cref="EnsureAsync"/> reads | ||
| /// validity first and drops an INVALID leftover before building. That read-then-drop is what makes the per-chunk | ||
| /// form safe here, where <c>PgTableTuning</c> rejects it for its start-time build.</para> |
There was a problem hiding this comment.
The tree now carries two contradictory measured readings of transaction_per_chunk on this same table, and the one that settles it lives only in the PR body.
This paragraph says the read-then-drop is "what makes the per-chunk form safe here, where PgTableTuning rejects it for its start-time build." But PgTableTuning.cs:239-243 rejects it for two reasons, and this file answers only the second:
"The per-chunk form, WITH (timescaledb.transaction_per_chunk), was measured and rejected: it takes the same root ShareLock first, buying no write concurrency here, and a cancel MID-build … leaves the parent index indisvalid = false and the live chunk unindexed …"
The first reason is the one that matters more on this path, because PgTableTuning's own list runs before collectors start (PgTableTuning.cs:234-236: "every INSERT into any chunk queues behind it — which is why this list runs BEFORE collectors start"), whereas this build fires 20 minutes into the collection loop (DarlingWorker.cs:2320, launched ahead of the loop) against the raw Query Store ingest table.
The PR body's lock sample answers it — "ShareLock on the older uncompressed chunk for about 93 ms, then on the newest chunk for about 64 ms, never held together, and none on the hypertable root" — which, if it holds, refutes PgTableTuning.cs:240. That is a genuinely useful measurement and it is nowhere in the tree. After merge the body is gone and a reader comparing the two files finds one saying the form takes a root ShareLock and the other saying the form is safe, with no way to tell which was measured when.
Two things belong in this paragraph (or in LegacyProbe's own remarks at :121-126), not just in the PR description:
- The lock finding and its method (sampled every 2 ms, AccessShare/Exclusive not sampled, a sub-2 ms lock could be missed) — and a correction or version-scope on
PgTableTuning.cs:240, since this PR's evidence contradicts it. - The cost the body lists under Limits and this file omits entirely: "The build blocks writes to the chunk it is on, for that chunk's scan time … about 64 ms on the newest 85 MB chunk … a 5-9 GB chunk on a volume capped at 250 MB/s would hold its lock for tens of seconds." A reader of this class today is told
CONCURRENTLY"blocks no writes" (:42-43) and is told nothing at all about what the hypertable arm costs the writer.
Not a code change — but on a file whose neighbours carry their measurements inline, leaving the decisive one in a PR body is the part that will not survive.
| } | ||
| catch (OperationCanceledException) when (!delayFinished) | ||
| { | ||
| logger.LogDebug("Query Store index ensure was cancelled before it started."); |
There was a problem hiding this comment.
Fanning one index out to three dropped the {Index} placeholder from exactly the lines that now need it.
The single-index version these replaced carried the name on every arm (QueryStoreIntervalWideBrinIndex.cs, removed lines: "Query Store interval index ensure ({Index}) was cancelled before it started.", "… ({Index}) was cancelled at shutdown; the next start retries, and drops any half-built leftover first."). With one index the name was redundant. With three it is the only fact the operator needs, and it is gone:
:248and:252-254— a shutdown mid-build now says some Query Store index was interrupted. The next start's:329-331warning will name which one it is dropping and rebuilding, but the two lines no longer join up, and an operator reading only the shutdown log cannot tell whether the interrupted build was the ~2 s wide btree or the per-chunk build on the raw ingest hypertable.:258— the outer generic catch logs the literal string"(shutdown)"into the{Index}slot, so a non-OperationCanceledExceptionthrown while cancellation is pending (which the sibling pin atQueryStoreIntervalWideBrinIndexTests.cs:117-121exists precisely because Npgsql does do this) producesQuery Store index ensure ((shutdown)) failed and is retried at the next start: …. That reads as a bug in the log line rather than as a shutdown.
Both are cheap: thread the current spec's name through (a IndexSpec? current set at the top of each loop iteration) and use it in all three places, falling back to a count/"(before any index)" when the delay itself was cancelled.
Related, on the body's claim that "QueryStoreIntervalWideBrinIndex keeps its public surface and delegates to it, so its behavior is unchanged" — the delegation is clean and Decide's reason strings come out byte-identical (:201 + :209 reproduce the removed literals exactly, and QueryStoreIntervalWideBrinIndexTests.cs:34's (150000, true) → SkipServerVersionBelowSixteen still pins version-before-hypertable ordering). But four public members were deleted, not kept: StartDelay, BuildTimeoutSeconds, CatalogReadTimeoutSeconds and RunDelayedAsync. No caller breaks — I grepped the service, viewer, storage and both test projects, and the only references left are the new ones — so this is a claim correction, not a defect. The log text above is the part of "behavior is unchanged" that genuinely isn't.
| CreateSql, | ||
| null, | ||
| DropSql, | ||
| DropSql, |
There was a problem hiding this comment.
Non-blocking (unreachable today): this spec's HypertableDropSql is DROP INDEX CONCURRENTLY, which the parameter's own contract forbids.
DropSql (:89) is "DROP INDEX CONCURRENTLY IF EXISTS collect.ix_query_store_interval_wide_collection_time_brin;" and it is passed for both the PlainDropSql and HypertableDropSql positions. The record documents that slot as "The drop of an INVALID leftover on a hypertable (never CONCURRENTLY)" (QueryStoreBackgroundIndexes.cs:96), for the reason the class doc gives at :44-45 — TimescaleDB refuses DROP INDEX CONCURRENTLY on a hypertable index.
It cannot fire now: HypertableCreateSql is null here, so Decide returns SkipHypertable (QueryStoreBackgroundIndexes.cs:206-209) and EnsureAsync returns at :315-319 before reaching the drop at :332. But it is a loaded gun — the day anyone gives this spec a per-chunk form (the obvious next step if query_store_interval_wide ever becomes a hypertable, which QueryStoreIntervalWide.PlainTableFloorSql's "today, always" already anticipates), the drop arm silently becomes the statement the design exists to avoid.
Nothing would catch it either: the census at QueryStoreBackgroundIndexesTests.cs:131-145 checks only Assert.Contains("IF EXISTS", spec.HypertableDropSql), and TheWideBtreeDdl_… (:76-85) checks Wide.PlainDropSql but not its hypertable twin. Cheapest fix is in the census rather than here — one line that holds for all three specs:
Assert.DoesNotContain("CONCURRENTLY", spec.HypertableDropSql, StringComparison.OrdinalIgnoreCase);
which fails on this spec today and makes the contract real.
| Assert.Contains("s.collection_time <= $3", probe); | ||
| Assert.Contains("s.interval_start_time_utc IS NULL", probe); | ||
| Assert.Contains("(server_id, collection_time)", Probe.PlainCreateSql); | ||
| Assert.Contains("WHERE interval_start_time_utc IS NULL", Probe.PlainCreateSql); |
There was a problem hiding this comment.
SoTheTwoCannotDrift is what this test is named, not what it does — the two sides are each pinned to a third literal, never to each other.
The index predicate and the probe predicate are two hand-written copies of one fact. This test checks copy A against the string "s.interval_start_time_utc IS NULL" (:95) and copy B against the string "WHERE interval_start_time_utc IS NULL" (:97), and the key the same way (:96 / :92). Nothing compares A to B. So the drift this test is named for passes it:
- add a column to the probe —
AND s.database_name = $4, say, or narrow it tointerval_start_time_utc IS NULL AND interval_end_time_utc IS NULL— and all six assertions stay green while the partial index no longer carries the read's predicate and theEXISTSquietly goes back to the 4 s heap sweep this PR exists to remove; - reorder the index key to
(collection_time, server_id)andAssert.Contains("(server_id, collection_time)", …)fails, so that one happens to be covered — but only because the substring is the whole key list.
That is trap #70 one step weaker than the trap's own case: "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." Here they are not even pinned equal to each other. And trap #78's test applies — "Ask what a pin would fail ON" — the answer is "only deleting the predicate outright".
The repo already has the shape that fixes this, two files over: ForcePlanFailuresAccessPathTests.cs:98-110 builds the expected CREATE INDEX text from the same column list the read uses and asserts Equal, so the statement and the read cannot diverge without the test failing. The same move works here — derive the index's predicate and key from QueryStoreIntervalWide.HasLegacyRowSql (or export the predicate as one shared const that both the probe and LegacyProbe's DDL interpolate) and assert equality rather than containment.
The live pin at QueryStoreBackgroundIndexesLiveTests.cs:174-185 (no partial index in the plan before, present after) does catch real drift — but only when DARLING_TEST_PG is set, so the store-less build job that CONTRIBUTING names as the parity/regression gate (CONTRIBUTING.md:541-542) keeps passing on a drifted pair.
Review notes (full tier) — context for the verdictWhere I landed. The mechanism is right and the structural argument holds against the tree. The probe's predicate really is what the partial index carries ( Six inline comments. Two are test defects, one is a claim correction, one is latent and unreachable, two are documentation. None of them ask for a design change. The hazard I raised blind, and how the body answered it. Before reading the body I had this down as the blocker:
The body does answer it, and well: the lock sample reports per-chunk What remains is that the evidence lives only in the PR body. After merge the tree carries two contradictory measured readings of the same DDL form on the same table, and the new class tells its reader only that On the limits the body volunteers. The compressed-chunk limit is documented accurately and is consistent with Callers re-read, per the callers rule. Every reference to the four deleted public members ( Not raised, deliberately. No missing-index DMV suggestions. No T-SQL style findings — the new SQL is PostgreSQL, and it keeps the house conventions anyway ( |
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Design is right; two test defects and one false claim. Specifics in the six inline comments and the top-level review notes.
Claims
- VERIFIED — Two indexes, background, no migration, no
StorageVersionchange — no migration among the eight changed files;QueryStoreIntervalWide.cs's hunks are comment-only - VERIFIED — Partial btree on
query_store_stats (server_id, collection_time) WHERE interval_start_time_utc IS NULL, per-chunk on a hypertable,CONCURRENTLYon a plain table —QueryStoreBackgroundIndexes.cs:131-135, chosen at:204-211 - VERIFIED — Btree on
query_store_interval_wide (server_id, first_execution_time),CONCURRENTLY, hypertable skipped with a warning —:155-162,:206-209,:315-319 - VERIFIED —
first_execution_timeis the seventh key so its bound cannot narrow the range —QueryStoreIntervalWide.cs:77-78; bound atDarlingDataReader.cs:1952-1955 - VERIFIED — One delayed task, 20 min, three indexes in order, own connection each, failure-isolated, drained at shutdown —
DarlingWorker.cs:2320-2321,:3813;QueryStoreBackgroundIndexes.cs:53,:233-244 - VERIFIED — Pins moved with the same assertions —
QueryStoreIntervalWideBrinIndexTests.cs:70,:120; onlyBrinAction.toIndexAction.changed - VERIFIED — No read's SQL changed; comments only in
QueryStoreIntervalWide.cs— both hunks sit inside comment blocks - VERIFIED —
TheWritersDoUpdateSetList_...asserts neitherserver_idnorfirst_execution_timeis set, both identity —QueryStoreBackgroundIndexesTests.cs:117-126againstQueryStoreIntervalWide.cs:344-394,:77-78 - VERIFIED — Lite parity: no probe to fix —
Lite/has noquery_store_interval_wide;LocalDataService.QueryStore.cs:995is aUNION ALLarm in one statement - VERIFIED — Targeted test counts — 4+4 theory cases + 7 facts = 15; live classes 4 and 5 facts
- UNVERIFIED — All field and rig numbers (5-10 s/call, probe 4.1 s / 6.8 M blocks, table read 10.2 s / 22.4 M, the EXPLAIN pairs, 2.35 s and 0.52 s builds) — live measurement on the seeded store
- UNVERIFIED — The lock sample (per-chunk ShareLock ~93 ms then ~64 ms, never together, none on the root) — live
pg_lockssample; see Divergence - UNVERIFIED — Writer during the rebuild: 1,500 inserts, slowest 68.3 ms, none failed — live run
- UNVERIFIED — HOT 61 of 61, control 0 of 61 — live run; the test asserts
updated >= 20andEqual(updated, hot)(QueryStoreBackgroundIndexesLiveTests.cs:310-312), so 61 is a run artifact, not a pin - REFUTED — "The existing catalog check that no non-BRIN index covers any SET column also runs against the new indexes in the live test" —
NoNonBrinIndex_CoversAColumnTheUpsertSets_...(QueryStoreIntervalWideBrinIndexLiveTests.cs:216) calls that file's ownEnsureAsyncat:233, the BRIN alone (:60-61); the new btree is never built there, so the scan at:237cannot see it, and that test's body is untouched - REFUTED — "
QueryStoreIntervalWideBrinIndexkeeps its public surface ... behavior is unchanged" —StartDelay,BuildTimeoutSeconds,CatalogReadTimeoutSecondsandRunDelayedAsyncwere deleted, and the log text changed: the cancel lines lost{Index}(QueryStoreBackgroundIndexes.cs:248,:252-254) and the outer catch logs the literal"(shutdown)"(:258). No caller breaks — I re-read every reference
Riskiest lines
Darling/Darling.Tests/QueryStoreBackgroundIndexesLiveTests.cs:111—DateTime.UtcNowre-read per call ofReadAllAsync, called twice (:165,:180) and asserted equal (:182); thecollection_time >= $2floor (DarlingDataReader.cs:1953) moves by the build duration across an hourly seed grid (:91-94) — UNPINNED: it is the pin, and the flake is in itDarling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:332— the per-chunkHypertableDropSqlarm, the load-bearing half of "what makes the per-chunk form safe here" (:39-40) — pinned byAnInvalidLeftover_IsDroppedAndRebuilt_ForBothIndexes, which never seeds, so the hypertable has no chunks and the rebuild crosses noneDarling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:158-159— the new btree on the hot upsert target — pinned empirically byTheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_...and structurally byTheWritersDoUpdateSetList_..., UNPINNED by the catalog census, the guard that would catch the next index
Traps near this diff
- "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies —
QueryStoreBackgroundIndexesTests.cs:90-97pins each predicate against its own literal, never against the other, so a column added toHasLegacyRowSql:501-511leaves all six assertions green - "A test that reads the clock twice measures the CI scheduler ... One anchor owns both ends of an interval (#3676)." — applies —
QueryStoreBackgroundIndexesLiveTests.cs:111re-anchors across both halves ofAssert.Equalat:182 - "A roster-based census does not see the file it does not roster (#3699)." — applies — the HOT census's roster is "whatever
QueryStoreIntervalWideBrinIndex.EnsureAsyncbuilt" (QueryStoreIntervalWideBrinIndexLiveTests.cs:233), missing the other two specs inAll - "A pin that encodes the lie ... Ask what a pin would fail ON (#3699)." — applies —
AnInvalidLeftover_IsDroppedAndRebuilt_ForBothIndexes:246-274fakes INVALID on the root only (:263) over zero chunks, so it fails only if the validity read is deleted, not on the statePgTableTuning.cs:241-243measured - "Lite and Darling ship in parity (#3594)." — does not apply — Lite is DuckDB with no
collect.query_store_interval_wideand no index-ensure machinery;LocalDataService.QueryStore.cs:995is aUNION ALLarm, not a probe - "A delta family that stores no interval (#3595)" / "
ELSE 0for unknowable (#3707)" / "truncation from a full page (#3699)" / "hours_back/limitclamped (#3699)" — do not apply — no collector, delta, rate, page or MCP parameter touched; no read's SQL changed - "
timescaledb_information.hypertablesnever lists a continuous aggregate's materialization (#3610)." — does not apply —HypertableSql:183-189asks only whether one named real table is a hypertable - "
chunk_compression_statsreturns ZERO rows when compression is off; count chunks fromtimescaledb_information.chunks(#3610)." — does not apply, and was heeded — the census atQueryStoreBackgroundIndexesLiveTests.cs:221-236counts fromchunkswithFILTER (WHERE c.is_compressed)
Divergence (blind read vs PR body)
Three; two in the body's favour.
Against it: the body says the existing catalog HOT census runs against the new indexes. It does not (REFUTED above). My blind read had flagged that census as the strongest guard on this table precisely because it is data-independent and fires on any future index; the body's sentence would stop a reader from checking.
For it: I had the per-chunk build on the raw ingest hypertable down as the blocker, on PgTableTuning.cs:240's measured "it takes the same root ShareLock first" plus the fact that this build runs with collectors live while PgTableTuning's list runs before they start (:234-236). The body answers with a lock sample showing per-chunk locks only and none on the root, a writer run where nothing failed, and the residual cost under Limits. Better evidence than I had, so not a blocker. But it refutes PgTableTuning.cs:240, and neither the refutation nor the per-chunk write cost is in the tree — QueryStoreBackgroundIndexes.cs:42-43 still tells its reader only that CONCURRENTLY "blocks no writes".
Unmentioned: the cancellation and generic-failure log lines lost their index name in the fan-out from one index to three — the opposite of the direction three indexes want.
Everything else matched what I had written down before reading the body, including the limits it volunteers and the items it marks unrun.
Reviewer checklist
- Run the full
Darling.Testssuite twice — storeless, then against a fresh live database — which the body marks[ ]and defers; the storeless run is CONTRIBUTING's named parity gate (CONTRIBUTING.md:541-542) - Re-run
TheReads_AreIdenticalWithTheIndexesAbsentAndPresent_...ten or more times, before and after fixing the clock anchor; the flake window is roughly (seconds between the two reads) / 3600, so one green run proves nothing - Reproduce a genuinely interrupted per-chunk build on a seeded hypertable (cancel mid-build, not
UPDATE pg_index) and confirm the root ends invalid, the next ensure rebuilds, and every chunk has the partial index afterwards - Settle
PgTableTuning.cs:240against this PR's lock sample on a live TimescaleDB: does the per-chunk form take a root ShareLock, and for how long? One of the two files is wrong; note the sample's blind spot (2 ms interval, AccessShare/Exclusive not sampled) - Measure the per-chunk build's write stall on a field-sized chunk (5-9 GB) with collectors live — what production hits 20 minutes after every restart, versus the rig's 85 MB
- Confirm both
CONCURRENTLYbuilds behave under the realNpgsqlDataSource: neither sets alock_timeout, while the house convention for blocking index DDL isBEGIN; SET LOCAL lock_timeout = '5s'; ...; COMMIT;(PgTableTuningTests.cs:34-38), andBuildTimeoutSecondsis 7200
| /// deadline, where a 5-9 GB chunk would hold it for tens of seconds. The newest chunk is the one the collector | ||
| /// writes: the live cycle and the backfill both stamp the current time, so older chunks, which take the same lock | ||
| /// for their own longer scans, are not written to.</para> |
There was a problem hiding this comment.
The backfill does not stamp the current time, so the 256 MB guard protects the one chunk that is never the exposed one.
This sentence — and the matching Limits bullet in the PR body ("Nothing writes into the older chunks (the live cycle and the backfill both stamp the current time), so their locks stall no write") — is contradicted by the backfill's own design:
QueryStoreBackfill.cs:48— "Slices carry a BACKDATED collection_time (the slice ceiling), so the rows land in time buckets adjacent to their own activity"QueryStoreBackfill.cs:101— "A slice lands rows at a BACKDATEDcollection_time"- How deep:
RollupStoreHorizon=min(RetentionTierRouter.RawMaxAge, HourlyRefreshStartSpan − HourlyRefreshScheduleSpan)=min(~3 d, 24 h − 1 h)= 23 hours below now (QueryStoreBackfill.cs:123-126,TimescaleSupport.cs:2912/2922).TimescaleSupport.ChunkIntervalDays = 1, so 23 h reaches straight into the chunk before the newest. - Those slices go through the same COPY chokepoint with the same 10 s deadline the guard is derived from —
DarlingCollectorRunner.cs:4128-4131says so in its own words ("the re-attempt on a fresh connection and the backfill are all covered here and nowhere else"), withStoreCopyStartDeadlineat:4154andimporter.Timeout = ServiceCommandDeadlines.CollectionSweepSecondsat:4196.
The timing makes it worse rather than incidental. The guard admits the build only while the newest chunk is ≤ 256 MB — at the field's 5-9 GB/chunk that is roughly the first hour after a chunk boundary, as Limits says. During exactly that hour the previous chunk is a full 5-9 GB and still uncompressed: with CompressAfterDays = 1 plus the query_store_stats offset of 1 h (TimescaleSupport.cs:80, :387), a chunk with range_end = D 00:00 is not even compression-eligible until D+1 01:00. So the permit window is precisely the window in which the previous chunk is largest, uncompressed, and inside the backfill's reach.
The PR's own lock sampling already observed the mechanism: "ShareLock on the older uncompressed chunk for about 93 ms, then on the newest chunk for about 64 ms". On the seeded store that chunk was 101 MB; at 5-9 GB and the doc's own 1 GB/s warm scan rate that is the "tens of seconds" NewestChunkMaxBytes exists to prevent — against a 10 s deadline, taken twice.
Consequences per the PR body's own accounting: a backfill slice times out in its start phase, StoreWriteReattempt re-runs it once (a second 10 s wait), and the slice's rows are not stored this tick. No data loss, but the stated invariant that justifies limiting only the newest chunk does not hold.
Options, in rough order of cost: bound the build on the largest uncompressed chunk rather than the newest one (the timescaledb_information.chunks read already in NewestChunkSql can filter NOT is_compressed and take MAX(pg_relation_size(...))); or gate the build on the backfill being idle / the write fence; or at minimum restate the limit's derivation so it does not rest on a premise QueryStoreBackfill contradicts.
| /// <summary> | ||
| /// How long a deferred attempt (<see cref="IndexAction.SkipNewestChunkLarge"/>) waits before the next one. The | ||
| /// newest chunk is small only for a while after each chunk boundary, so the ensure keeps trying until the index | ||
| /// is built or the service stops. | ||
| /// </summary> | ||
| public static readonly TimeSpan RetryInterval = TimeSpan.FromHours(1); |
There was a problem hiding this comment.
The hourly retry never samples a different phase of the chunk cycle, so "a poll can miss it on a given day" is really "this process never builds it".
RetryInterval is one hour, and every supported raw chunk interval is a whole number of hours that 1 h divides exactly — RawChunkIntervalPlanner.RungHours = { ChunkIntervalDays * 24, 12, 6 } = { 24, 12, 6 } (RawChunkIntervalPlanner.cs:74). TimescaleDB aligns chunk ranges to epoch multiples of the interval, so chunk boundaries land at fixed wall-clock times.
The first attempt fires at StartDelay (20 min) after service start, so every attempt thereafter lands at a fixed minute-of-the-hour M = (start minute + 20) mod 60, forever. Relative to each chunk boundary the sampled offsets are therefore always M, 60+M, 120+M, … — the same set on every chunk, every day. Only the hour-0 attempt (offset M) can ever be inside the ~40-75 minute window where the newest chunk is still under 256 MB, and whether it is depends only on M, which is fixed for the process's lifetime.
So the behaviour is not the per-day coin flip Limits describes ("the retry is hourly, so a poll can miss it on a given day"). It is decided once, at startup: if M exceeds the chunk's 256 MB fill time, no retry in that process will ever succeed, and the loop at :354-380 keeps paying a catalog read an hour indefinitely. LogDeferred drops to Debug after the first line (:406-414) by design, so an operator gets one Information line at startup and nothing afterwards — the permanently-stuck case and the will-build-tonight case look identical in the log.
The code comment here ("a chunk is small only for a while after each chunk boundary, so the ensure keeps trying until the index is built or the service stops", and the same sentence at :170-173) reads as if the retries wander across phases. They do not.
Cheapest fixes: add jitter to RetryInterval (or make it a non-divisor like 37-53 minutes) so successive passes walk the chunk cycle; or derive the next attempt from the newest chunk's own range_end and schedule just past the next boundary rather than polling. Either would also let the Debug/Information split stay as it is.
| if (decision.Action == IndexAction.SkipNewestChunkLarge) | ||
| { | ||
| LogDeferred(logger, spec, decision.Reason, isRetry); | ||
| return EnsureOutcome.RetryLater; | ||
| } |
There was a problem hiding this comment.
Non-blocking: the deferral arm sits above the INVALID-leftover drop at :503-511, so a half-built index plus a too-big newest chunk means the leftover is left in place for as long as the deferral lasts — which, per the retry-phase comment above, can be the whole process lifetime. An INVALID btree is unusable by the planner but is still maintained by every write, and dropping it is a catalog operation that does not scan anything, so it is not what the 256 MB limit is guarding against. Moving the indexValid == false drop ahead of the SkipNewestChunkLarge return would clear the leftover on the first deferred attempt and still defer the build. (For the partial probe index the write cost is near zero because the index is empty, which is why this is a note rather than a finding — but the shape generalises to any future spec that carries MaxNewestChunkBytes.)
| public void TheWideBtree_LeadsWithTheServerAndCarriesTheFirstExecutionBoundTheTableReadsFilterOn() | ||
| { | ||
| foreach (var sql in new[] { DarlingDataReader.QueryStoreTopTableSql }) | ||
| { | ||
| Assert.Contains("WHERE server_id = $1", sql); | ||
| Assert.Contains("first_execution_time >= $2 - ", sql); | ||
| } | ||
|
|
||
| Assert.Contains("(server_id, first_execution_time)", Wide.PlainCreateSql); | ||
| } |
There was a problem hiding this comment.
Non-blocking: the foreach iterates a one-element array, which reads like two or three statements were meant to be in it. The test's name says "the bound the table reads filter on" (plural), and the PR body names three such reads — DarlingDataReader.QueryStoreTopTableSql, ViewerDataService.QueryStoreTopTableSql and ViewerDataService.QueryStoreDurationTrendTableSql. Only the MCP one is pinned here, so the viewer twin's first_execution_time >= $2 - … bound (ViewerDataService.QueryStore.cs:209) could be dropped or re-shaped without this test noticing — which matters more than usual given it is the read the EXPLAIN table shows not taking the new index at 168 h. Either add the other two constants to the array or collapse it to a single assertion so the name matches the coverage.
| public static readonly QueryStoreBackgroundIndexes.IndexSpec Spec = new( | ||
| IndexName, | ||
| "collect.query_store_interval_wide", | ||
| CreateSql, | ||
| null, | ||
| DropSql, | ||
| DropSql, | ||
| MinimumServerVersionNum, | ||
| "a BRIN index on collection_time would make the upsert non-HOT below PG 16"); |
There was a problem hiding this comment.
The PR body says "QueryStoreIntervalWideBrinIndex keeps its public surface and delegates to it, so its behavior is unchanged." The behaviour claim holds, but the surface claim does not: the extraction deletes four members that were public on this class —
public static readonly TimeSpan StartDelaypublic const int BuildTimeoutSecondspublic const int CatalogReadTimeoutSecondspublic static Task RunDelayedAsync(NpgsqlDataSource, ILogger, TimeSpan, CancellationToken)
plus the internal StateSql / HypertableSql. Nothing outside this file still references them (I grepped both SKUs and both test projects — the only survivors are QueryStoreIntervalWideBrinIndexTests.cs:96, which was updated to QueryStoreBackgroundIndexes.StartDelay, and QueryStoreIntervalWide.cs:495, which references the new home), so this breaks nothing today. Worth correcting in the body so a later reader does not take "keeps its public surface" at face value.
While here: Decide, EnsureAsync, BrinAction and BrinDecision are now called only from this class's own tests — QueryStoreBackgroundIndexes.All takes Spec directly and the worker goes through QueryStoreBackgroundIndexes.RunDelayedAsync. Keeping them as a stable wrapper is a defensible choice; just noting that the production path no longer exercises the BrinAction mapping at :129-134, so those tests now pin a translation nothing reads.
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Claims
- VERIFIED — "
first_execution_timeis the seventh key of the unique index, so its bound cannot narrow the range" —QueryStoreIntervalWide.cs:77-78. - VERIFIED — the partial index carries the probe's own predicate — DDL at
QueryStoreBackgroundIndexes.cs:185-189againstHasLegacyRowSqlatQueryStoreIntervalWide.cs:502-511. - VERIFIED — "no migration, no
StorageVersionchange" — the 8-file diff touches noPgMigrations.csrung and noViewerDataService.cssentinel arm (still ends at V157,:1041). - VERIFIED — hypertable gets
WITH (timescaledb.transaction_per_chunk), plain getsCONCURRENTLY, hypertable drop neverCONCURRENTLY—:185-192, pinned byTheLegacyProbeDdl_IsAPartialBtree_WithThePerChunkOptionBeforeTheWhere_AndNeverConcurrentOnAHypertable. - VERIFIED — wide btree is
CONCURRENTLY, no hypertable form, hypertable skipped with a warning —:210-217(HypertableCreateSqlnull) +:485-489. - VERIFIED — "reads the newest chunk's heap size from the catalog ... no table is scanned" —
NewestChunkSql:251-262, called at:470-476only when a per-chunk build is coming. - VERIFIED — "At or below 256 MB it builds. Above it, skipped, logged once at Information, tried again one hour later" —
:287-293,:406-420,:352-379; pinned byThePartialIndex_BuildsPerChunkWhileTheNewestChunkIsAtOrBelowTheLimit_AndDefersAboveItandADeferral_IsLoggedOnceAtInformation_AndQuietlyOnEveryRetryAfterIt. - VERIFIED — "Only the partial index is limited this way" —
TheNewestChunkLimit_IsAboutTwoHundredFiftySixMegabytes_AndOnlyThePartialIndexCarriesItasserts nullMaxNewestChunkByteson the wide btree and the BRIN spec. - VERIFIED — the HOT premise: the upsert sets neither key of the new btree —
QueryStoreIntervalWide.cs:344-394has noserver_id/first_execution_timeinDO UPDATE SET, and both are inIdentityColumns:77-78; pinned byTheWritersDoUpdateSetList_NeverSetsTheServerOrTheFirstExecutionTime_SoTheWideBtreeKeepsUpdatesHotand the liveTheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn. - VERIFIED — "No read's SQL changed ... only comments changed in
QueryStoreIntervalWide.cs" — both hunks there are doc-comment text only. - VERIFIED — the 10 s COPY deadline the limit derives from —
ServiceCommandDeadlines.cs:95, applied atDarlingCollectorRunner.cs:4154and:4196, prepares at:4119/:4125. - VERIFIED — "the hourly wait takes the stopping token" —
:376, pinned byTheRetry_WaitsTheIntervalAndEndsQuietlyWhenTheServiceStops; and "a failed one still waits for the next start" —:359-372, pinned byAFailedAttempt_IsNotRetriedInTheSameRun_AndDoesNotStopTheNextIndex. - VERIFIED — "Lite parity: no probe to fix", at the exact cited lines —
Lite/Services/LocalDataService.QueryStore.cs:163,:181,:363,:938,:978,:995,:1027; Lite issues noCREATE INDEXanywhere (Lite/Database/DuckDbInitializer.cs, zero). - UNVERIFIED — every rig/field measurement: the 4.1 s probe, the 10.2 s table read, the whole EXPLAIN (ANALYZE, BUFFERS) table, before/after figures, the 2.35 s and 0.52 s build times, the 2 ms lock sampling. Needs a seeded TimescaleDB store at
random_page_cost = 1.1. - UNVERIFIED — the RED/GREEN counts (5 of 27 then 27 of 27; 4 of 5 live; 153 + 19 targeted) and "0 warnings". Needs
dotnet build/dotnet testwithDARLING_TEST_PG. - UNVERIFIED — that TimescaleDB accepts the per-chunk partial build with compressed chunks present, and that an interrupted per-chunk build leaves the root index
indisvalid = false— the premise the read-then-drop design rests on. Both are encoded in the live tests; neither is checkable without the extension. - REFUTED — "Nothing writes into the older chunks (the live cycle and the backfill both stamp the current time), so their locks stall no write" (Limits; same text as code doc
QueryStoreBackgroundIndexes.cs:78-80).QueryStoreBackfill.cs:48-49and:101say slices carry a BACKDATEDcollection_time;RollupStoreHorizon(:123-126) =min(RawMaxAge ~3 d, 24 h - 1 h)= 23 h below now, andTimescaleSupport.ChunkIntervalDays = 1, so slices land in the chunk below the newest — through the same COPY chokepoint and the same 10 s deadline (DarlingCollectorRunner.cs:4128-4131names the backfill explicitly). - REFUTED — "the retry is hourly, so a poll can miss it on a given day".
RetryIntervalis 1 h andRawChunkIntervalPlanner.RungHours = { 24, 12, 6 }(RawChunkIntervalPlanner.cs:74), so 1 h divides every supported chunk interval exactly: attempts land at one fixed minute-of-hour for the process's life, and only the hour-0 attempt can fall in the ~40-75 min permit window. Whether the index ever builds is decided once by the start minute, not re-rolled daily. - REFUTED — "
QueryStoreIntervalWideBrinIndexkeeps its public surface". The extraction deletes publicStartDelay,BuildTimeoutSeconds,CatalogReadTimeoutSecondsandRunDelayedAsync, plus internalStateSql/HypertableSql. Harmless (grepped both SKUs and both test projects; nothing outside the file names them) but false as written.
Riskiest lines
Darling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:287— the gate reads only the newest chunk, while the build ShareLocks every uncompressed chunk and the backfill writes backdated rows into the previous one under a 10 s deadline — pinned for the newest chunk byThePartialIndex_BuildsPerChunkWhileTheNewestChunkIsAtOrBelowTheLimit_AndDefersAboveItandThePartialIndex_IsDeferredWhileTheNewestChunkIsAboveTheLimit_AndBuiltOnceItIsAtIt; UNPINNED for any older chunk — no test writes into a non-newest chunk during a build.Darling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:89—RetryInterval = 1 hagainst 24/12/6 h chunks: a stuck deferral is permanent and logs only at Debug after the first line — the value is pinned byTheNewestChunkLimit_IsAboutTwoHundredFiftySixMegabytes_AndOnlyThePartialIndexCarriesIt; convergence is UNPINNED (ADeferredAttempt_IsRetriedUntilItSettles_AndASettledIndexIsNeverAttemptedAgaininjects a fake that settles on call three, pinning the loop's bookkeeping rather than that a real store's third call can succeed).Darling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:213-214— a new btree on the hottest-upsert table, where any indexed SET column re-breaks the #4250 HOT regression — pinned byTheWritersDoUpdateSetList_NeverSetsTheServerOrTheFirstExecutionTime_SoTheWideBtreeKeepsUpdatesHotand the liveTheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn, which parses the product's own SET list and carries a non-HOT control. Well covered.
Traps near this diff
- "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — this is PostgreSQL index DDL for Darling's store; Lite is DuckDB and issues no
CREATE INDEXat all (Lite/Database/DuckDbInitializer.cs), and has no separate legacy-row probe: itsinterval_start_time_utc IS NULLis arm 2 of oneUNION ALL(Lite/Services/LocalDataService.QueryStore.cs:978/:995). - "
pg_relation_sizeis the heap alone; TOAST and indexes live elsewhere ... Size a relation withpg_total_relation_size" — does not apply — the heap alone is the right measure andQueryStoreBackgroundIndexes.cs:246-250says why: the bounded quantity is how long a per-chunk heap scan holds a ShareLock, and the indexed columns are all fixed-width so nothing is de-TOASTed. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the live tests bind the product's constants (
HasLegacyRowSql,DarlingDataReader.QueryStoreTopTableSql,ViewerDataService.QueryStoreTopTableSql,QueryStoreDurationTrendTableSql, atQueryStoreBackgroundIndexesLiveTests.cs:150-211) and derive the HOT SET list by parsingQueryStoreIntervalWide.UpsertSql. - "
chunk_compression_statsreturns ZERO rows for a hypertable whose compression is off ... Count chunks fromtimescaledb_information.chunks" — does not apply — bothNewestChunkSql(:251-262) and the live per-chunk assertion usetimescaledb_information.chunks. - "A pin that encodes the lie ... Ask what a pin would fail ON." — does not apply, and the author pre-empted it —
TheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumnpassed with the index absent, and now assertsindisvalidfirst so it fails42P01without the ensure. - "
.gitattributessaystext=auto eol=crlf... A diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply — all four added/touched C# files are CRLF, and the two edited files show localised hunks, not whole-file rewrites. - "A census fires on any NEW string constant ... An anti-pattern census sweeps the class" — does not apply —
StorageCommandTimeoutTests,StartupCommandTimeoutTestsandCollectionSweepCommandTimeoutTestsname neither the old BRIN file nor a roster this new file must join; the only source pins that moved areQueryStoreIntervalWideBrinIndexTests.cs:69/:106, both repointed in this diff.
Divergence (blind read vs PR body)
Three places, all in one direction — the body's mechanism is narrower than the store's behaviour.
- The limit's justifying premise is false. Limits says older chunks take no writes because "the live cycle and the backfill both stamp the current time". The backfill does the opposite by design (
QueryStoreBackfill.cs:48,:101) and reaches 23 h back — the chunk below the newest, with 1-day chunks. Worse, the guard admits the build only while the newest chunk is at most 256 MB, i.e. the first hour after a boundary, and that is exactly when the previous chunk is a full 5-9 GB and still uncompressed: withCompressAfterDays = 1plus the 1 hquery_store_statsoffset, a chunk closing atD 00:00is not compression-eligible untilD+1 01:00. The PR's own lock sampling already saw it ("ShareLockon the older uncompressed chunk for about 93 ms", at 101 MB); at 5-9 GB and the doc's own 1 GB/s that is the tens of seconds the 256 MB number exists to prevent, against a 10 s deadline taken twice. Details inline. - "Can miss it on a given day" is really "will never build in this process". 1 h divides 24/12/6 h, so the hourly retry samples one fixed phase of the chunk cycle, fixed by the service's start minute. The body reads it as a per-day probability and the comments at
:86-87and:170-173read as if retries wander across phases. The Handoff item points at the right place but mis-states the shape, and Debug-after-first-line logging makes the permanently-stuck store look identical to the builds-tonight one. Details inline. - A surface claim the tree contradicts — "keeps its public surface" deleted four public members. No caller breaks; a body correction rather than a code change. Details inline.
Everything else the tree can reach matched, including the two gaps the body self-reports (the vacuous HOT pin, the shared window end), which the diff really does close.
Reviewer checklist
- On a seeded TimescaleDB store, run a backfill slice targeting a window 2-20 h old (so it lands in the chunk below the newest) while a per-chunk build of
ix_query_store_stats_legacy_server_timescans that chunk, and see whether the slice's COPY breachesStoreCopyStartDeadline(10 s). That decides whether the guard must bound the largest uncompressed chunk instead of the newest. - Confirm on a real store that the per-chunk partial build succeeds against
collect.query_store_statswith compressed chunks present on the shipped TimescaleDB version, and that cancelling it mid-build leaves the root indexindisvalid = falserather than valid-with-chunks-missing — the latter is the stateEnsureAsync's early return at:497-501would read as "already valid" forever. - Run the full
Darling.Testssuite with and withoutDARLING_TEST_PG(both unchecked in the body) and confirm a Debug build with 0 warnings. - Re-measure
get_query_store_topon the 43-server store after the 20-minute build, and read the service log: one Information deferral line with nothing after it is the signature of the permanently-deferred case above. - Decide whether these two indexes want a line in whatever surface reports index bytes to operators — they are deliberately not migration rungs, so the viewer's schema gate (
ViewerDataService.cs:1036-1041) correctly does not probe them and nothing else reports their size.
…s, so it cannot skip the small-chunk window after a chunk boundary (#4952) The newest chunk of collect.query_store_stats is under the 256 MB build limit only for a while after each chunk boundary: at 5-9 GB per chunk, roughly the first 40-75 minutes. A retry every hour keeps a fixed phase from the service start, so on such a store it could miss that window every day and never build the index. RetryInterval is now the Query Store collection cycle (the query_store default in CollectorScheduleDefaults, 5 minutes); a try is one catalog read, so the extra tries cost almost nothing. The pin asserts the interval is at most 5 minutes and not zero, and is named for that reason. Comments that described the retry as hourly now say why the interval is short.
| /// deadline, where a 5-9 GB chunk would hold it for tens of seconds. The newest chunk is the one the collector | ||
| /// writes: the live cycle and the backfill both stamp the current time, so older chunks, which take the same lock | ||
| /// for their own longer scans, are not written to.</para> |
There was a problem hiding this comment.
The premise this limit rests on is contradicted by the backfill path: older chunks are written to, under the same 10 s deadline.
The newest chunk is the one the collector writes: the live cycle and the backfill both stamp the current time, so older chunks, which take the same lock for their own longer scans, are not written to.
The backfill does not stamp the current time. It stamps the slice ceiling, deliberately backdated:
QueryStoreBackfill.cs:369-370— "COPY through the SAME writer with the slice ceiling as the backdated collection_time"QueryStoreBackfill.cs:499-500—/* Backdated so the rows land beside their own activity … */ await _runner.WriteBackfillBatchAsync(definition, rows, server, ceilingUtc, context, …)QueryStoreIntervalWide.cs:556says the same thing from the reader's side: "Backfill slices span at mostQueryStoreBackfillState.MaxSliceSpan… and stamp the slice ceiling ascollection_time."
collection_time is the partitioning column (DarlingCollectorRunner.cs:6295-6296: "collectedSince bounds the read on collection_time — the PARTITIONING column"), and TimescaleSupport.ChunkIntervalDays = 1, so a backdated slice lands in an older chunk, not the newest one. The reach is QueryStoreBackfill.HorizonFor: RollupStoreHorizon (≈ a day minus a refresh interval) on a TimescaleDB store, RetentionTierRouter.RawMaxAge (3 days) otherwise — and on exactly the large stores this gate targets, RawChunkIntervalPlanner narrows chunks to 12 or 6 hours, so the horizon spans two to four chunks back.
That write is the same COPY with the same deadline: WriteBackfillBatchAsync → WriteBatchAsync (DarlingCollectorRunner.cs:6274 → :3669), whose importer takes ServiceCommandDeadlines.CollectionSweepSeconds = 10 s at :4196. So it gets the same two 10 s waits through StoreWriteReattempt (:3749) and then throws, and QueryStoreBackfill.cs:350-360 records a slice failure: adaptive span shrink, and at SkipAfterConsecutiveSliceFailures a Warning plus that database deprioritized.
The gate as written bounds only ORDER BY c.range_end DESC LIMIT 1 (NewestChunkSql, line 264). By this file's own arithmetic a 5-9 GB older chunk holds its ShareLock for "tens of seconds" — well past the 10 s the backfill's COPY into that same chunk can wait. Either gate on the largest chunk the build will lock within QueryStoreBackfill.HorizonFor rather than the newest alone, or state the backfill's exposure as an accepted cost — but the sentence as written says there is no exposure, and the tree says there is.
The PR body carries the same claim verbatim under Limits ("Nothing writes into the older chunks (the live cycle and the backfill both stamp the current time), so their locks stall no write"), so it needs the same correction.
| /// hypertable per-chunk build too: a cancel mid-build commits the chunks built so far and leaves the root index | ||
| /// <c>indisvalid = false</c>). <c>IF NOT EXISTS</c> would silently keep it, so <see cref="EnsureAsync"/> reads | ||
| /// validity first and drops an INVALID leftover before building. That read-then-drop is what makes the per-chunk | ||
| /// form safe here, where <c>PgTableTuning</c> rejects it for its start-time build.</para> | ||
| /// |
There was a problem hiding this comment.
Non-blocking, but it leaves a measured claim in the tree that one of two files now has wrong. This cites PgTableTuning's rejection of the per-chunk form and answers one half of it. PgTableTuning.cs:239-244 gives two reasons, and the half not answered here is the one the 256 MB gate depends on:
The per-chunk form, WITH (timescaledb.transaction_per_chunk), was measured and rejected: it takes the same root ShareLock first, buying no write concurrency here, and a cancel MID-build — exactly what the command timeout is — commits the chunks built so far, leaves the parent index indisvalid = false and the live chunk unindexed …
The read-then-drop answers the second clause. The first clause — a root ShareLock held across the whole build — would make the newest-chunk limit meaningless, because the COPY would queue behind the root for the sum of every chunk's scan, not one chunk's.
Your own lock sampling in the PR body contradicts PgTableTuning: "ShareLock on the older uncompressed chunk for about 93 ms, then on the newest chunk for about 64 ms, never held together, and none on the hypertable root." I can't adjudicate a live measurement from a read-only tree, and I'll take the newer one — but then PgTableTuning.cs:239 is now a wrong measured claim sitting next to a rejection it half-justifies, and the next reader of either file inherits whichever one they open. Worth a sentence in one of the two (and ideally in this paragraph, since it is the one that cites the other) saying which reading holds on which TimescaleDB version.
| /// The newest chunk is small only for a while after each chunk boundary (at 5-9 GB per chunk, roughly the first | ||
| /// 40-75 minutes), and a retry on a long interval keeps a fixed phase from the service start, so on such a store | ||
| /// it can miss that window every day and the index is never built. A try is one catalog read | ||
| /// (<c>NewestChunkSql</c>), so the extra tries cost almost nothing. The ensure keeps trying until the index is | ||
| /// built or the service stops. |
There was a problem hiding this comment.
Non-blocking observation the Limits section does not carry: on the stores this gate targets, the retry pins the build to the minutes just after UTC midnight.
The reasoning here is sound — a long retry interval keeps a fixed phase and misses the window. But the window it waits for is not arbitrary in wall-clock terms. TimescaleSupport.ChunkIntervalDays = 1 and create_hypertable(…, by_range('collection_time', INTERVAL '1 days'), …) (TimescaleSupport.cs:273) means chunk boundaries are epoch-aligned, i.e. UTC midnight — TimescaleSupport.cs:7653 says so explicitly ("UTC midnight (ChunkIntervalDays)"). So on a 5-9 GB/chunk store the only time the newest chunk is under 256 MB is the first ~40 min after UTC midnight, and a 5-minute retry means the build starts within ~5 minutes of it, every time, for the life of the service.
That is in tension with StartDelay's own rationale three paragraphs up: builds "that each read a whole table or chunk set must not stack on top of" the maintenance/IO convoy. #4510's note at TimescaleSupport.cs:380-407 is about precisely that convoy in the post-midnight hours and went to some trouble to keep the heavy compression offsets "disjoint from each other and off hour 0" — and this schedules a full uncompressed-chunk-set scan into hour 0.
It may well be fine (the build measured at 0.52 s over five chunks, and the partial index is empty so each chunk's index is 8 kB). But the PR's Limits section says only "the build starts within about 5 minutes of the first chunk boundary after the service starts" without saying which minutes of the day those are, and that is the fact an operator reading the Limits list would want.
| [Fact] | ||
| public void TheWideBtree_LeadsWithTheServerAndCarriesTheFirstExecutionBoundTheTableReadsFilterOn() | ||
| { | ||
| foreach (var sql in new[] { DarlingDataReader.QueryStoreTopTableSql }) | ||
| { | ||
| Assert.Contains("WHERE server_id = $1", sql); | ||
| Assert.Contains("first_execution_time >= $2 - ", sql); | ||
| } | ||
|
|
||
| Assert.Contains("(server_id, first_execution_time)", Wide.PlainCreateSql); |
There was a problem hiding this comment.
Non-blocking: a one-element roster behind a plural name. The test is named …CarriesTheFirstExecutionBoundTheTableRead**s**FilterOn and the foreach reads as a census, but the array holds one statement. The repo's own count is three — QueryStoreIntervalWideFirstExecFloorTests.cs:22-27:
The three per-server reads of the table (the MCP Query Store top, the Queries grid and the Trends chart, all
WHERE server_id = $1) … Each now also carriesfirst_execution_time >= <window start> - PurgeEdgeMarginSql.
.github/REVIEW-TRAPS.md: "A roster-based census does not see the file it does not roster: EveryPagedTool_ObservesTruncation_… walked ten tools and missed three in another file (#3699)." Here the two it does not roster are ViewerDataService.QueryStoreTopTableSql and ViewerDataService.QueryStoreDurationTrendTableSql — both of which the new live test does bind, so the index's claim is about all three.
No actual coverage hole: QueryStoreIntervalWideFirstExecFloorTests already pins the floor text in all three. So either add the other two to the array (two lines, and then the loop earns its shape) or drop the foreach and name the one statement it asserts.
| /// This index as <see cref="QueryStoreBackgroundIndexes"/> builds it: <c>CONCURRENTLY</c> on the plain table, no | ||
| /// hypertable form (a hypertable is skipped with a warning), and not below PostgreSQL 16. | ||
| /// </summary> | ||
| public static readonly QueryStoreBackgroundIndexes.IndexSpec Spec = new( |
There was a problem hiding this comment.
Non-blocking correction to a PR-body claim. The body says:
QueryStoreIntervalWideBrinIndexkeeps its public surface and delegates to it, so its behavior is unchanged.
The delegation is right, but the surface is not kept — six members were removed from this class: StartDelay, BuildTimeoutSeconds, CatalogReadTimeoutSeconds (all public), StateSql, HypertableSql (internal) and the RunDelayedAsync overload (public). Nothing in the tree breaks — I grepped every reference to the type, and outside this file the only callers are QueryStoreBackgroundIndexes.cs:228 (.Spec) and the two test classes, both updated in this PR. So this is a wording fix, not a code fix.
Behavior is also not quite unchanged in one respect worth a line somewhere: every message this ensure logs moved from "Query Store interval index {Index} …" to "Query Store index {Index} …", and the RunDelayedAsync lines no longer carry {Index} at all (QueryStoreBackgroundIndexes.cs:380-395 vs. the deleted :638-647 here). Harmless for the product, but a log grep or an alert on the old text loses it silently.
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
One blocking finding: the premise the 256 MB newest-chunk gate rests on is refuted by the tree. The backfill backdates collection_time — the partitioning column — so it writes into the older chunks the gate says nothing writes to, under the same 10 s COPY deadline. Five inline comments; only the first is blocking. The shared-builder extraction, the retry state machine, the invalid-leftover drop and the absent-vs-present row-identity pin all hold up to reading.
Claims
- VERIFIED — the partial btree carries the probe's own predicate; per-chunk form on a hypertable,
CONCURRENTLYon a plain table,WITHbeforeWHERE—QueryStoreBackgroundIndexes.cs:188-198vsQueryStoreIntervalWide.cs:501-512; pinned byTheLegacyProbeIndex_HasTheProbesOwnPredicateAndKey_SoTheTwoCannotDrift+TheLegacyProbeDdl_IsAPartialBtree_WithThePerChunkOptionBeforeTheWhere_AndNeverConcurrentOnAHypertable. - VERIFIED — the wide btree is
CONCURRENTLY, warns on a hypertable, carries no size limit —:216-223,:293-299,:443; pinned byTheWideBtree_BuildsConcurrentlyOnTheHeap_AndSkipsAHypertable+TheNewestChunkLimit_IsAboutTwoHundredFiftySixMegabytes_AndOnlyThePartialIndexCarriesIt. - VERIFIED — at/below 256 MB builds, above defers with one Information line and a 5-minute retry —
Decide:302-309,LogDeferred:400-414, loop:358-376; pinned byThePartialIndex_BuildsPerChunkWhileTheNewestChunkIsAtOrBelowTheLimit_AndDefersAboveIt+ADeferral_IsLoggedOnceAtInformation_AndQuietlyOnEveryRetryAfterIt.RetryIntervalreadsCollectorScheduleDefaults.All["query_store"](:94-95vsCollectorScheduleDefaults.cs:44), so the initializer cannot throw. - VERIFIED — "no table is scanned" by the size read, read only when it can matter —
NewestChunkSql:259-267, guard:465-471. One delayed task after migrations, drained once at shutdown —DarlingWorker.cs:2322/:3818. Validity read first, INVALID leftover dropped, hypertable arm neverCONCURRENTLY—:234-241,:473-501,:496. - VERIFIED — no migration rung; both
QueryStoreIntervalWide.cshunks are comment text only. - VERIFIED — the HOT premise:
DO UPDATE SET(QueryStoreIntervalWide.cs:344-396) names neitherserver_idnorfirst_execution_time;IdentityColumns(:77-78) names both; the live HOT test now asserts the index valid first (QueryStoreBackgroundIndexesLiveTests.cs:408) with a control that can fail (:437-440). - REFUTED — "Nothing writes into the older chunks (the live cycle and the backfill both stamp the current time), so their locks stall no write" (=
QueryStoreBackgroundIndexes.cs:79-81).QueryStoreBackfill.cs:369-370("the slice ceiling as the backdated collection_time") and:499-500, corroborated byQueryStoreIntervalWide.cs:556.collection_timeis the partitioning column (DarlingCollectorRunner.cs:6295-6296),ChunkIntervalDays = 1, same 10 s COPY deadline (:3669,:4196). Inline comment. - REFUTED — "
QueryStoreIntervalWideBrinIndexkeeps its public surface":StartDelay,BuildTimeoutSeconds,CatalogReadTimeoutSeconds,StateSql,HypertableSql,RunDelayedAsyncall deleted. No caller breaks, so wording not code; log text changed with it. Inline comment. - UNVERIFIED — every measurement (field figures, EXPLAIN table, build times, the 2 ms lock sampling "none on the hypertable root", writer run, RED/GREEN counts): a read-only tree yields none of them.
- UNVERIFIED — that
DROP INDEX CONCURRENTLYis refused on a hypertable index, and that a partial index plustransaction_per_chunkis accepted on a compressed hypertable. Only a live server settles both.
Riskiest lines
QueryStoreBackgroundIndexes.cs:79-81— the sentence the gate rests on, refuted above;NewestChunkSqlreads onlyORDER BY c.range_end DESC LIMIT 1(:264), so the backfill's COPY into an older 5-9 GB chunk is unprotected. UNPINNED — the live tests seed 101/85 MB chunks, where a 10 s wait is unreachable.QueryStoreBackgroundIndexes.cs:194— per-chunkCREATE INDEXon a compressed hypertable while collectors run, the formPgTableTuning.cs:239measured and rejected. Pinned byEnsure_BuildsBothIndexesValid_PerChunkIncludingACompressedChunk_AndASecondEnsureChangesNothing+AnInvalidLeftover_IsDroppedAndRebuilt_ForBothIndexes, which do run (build.yml:1183setsDARLING_TEST_PG).QueryStoreBackgroundIndexes.cs:360—while (pending.Count > 0), unbounded for the service's life. Loop and shutdown pinned byADeferredAttempt_IsRetriedUntilItSettles_AndASettledIndexIsNeverAttemptedAgain+TheRetry_WaitsTheIntervalAndEndsQuietlyWhenTheServiceStops; the hour-0-UTC phase lock is UNPINNED.
Traps near this diff
- "
pg_relation_sizeis the heap alone; TOAST and indexes live elsewhere … Size a relation withpg_total_relation_size." — does not apply:NewestChunkSql(:259-267) wants exactly the main fork, because that is what an index build scans (:253-254). - "
chunk_compression_statsreturns ZERO rows for a hypertable whose compression is off … Count chunks fromtimescaledb_information.chunks." — does not apply: product read:262and live censusQueryStoreBackgroundIndexesLiveTests.cs:289both usechunks. - "
timescaledb_information.hypertablesnever lists a continuous aggregate's materialization … Enumeratecontinuous_aggregates." — does not apply:HypertableSql(:245-252) asks about one named raw table; it enumerates nothing. - "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply, and the diff goes the right way:
QueryStoreIntervalWideBrinIndex.cs:95-104is anIndexSpecand:124-137/:156-157delegate. - "A roster-based census does not see the file it does not roster … rosters are for the positive contract only." — applies, non-blocking:
QueryStoreBackgroundIndexesTests.cs:108loops a one-element array under a plural name, whileQueryStoreIntervalWideFirstExecFloorTests.cs:22-27counts three reads. No hole; that class pins all three. - "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — does not apply:
query_store_interval_wideis absent from Lite (Lite/Database/DuckDbInitializer.cs), and DuckDB has noCONCURRENTLY, hypertables or BRIN. CONTRIBUTING's T-SQL section also does not apply (no T-SQL); the PG keepsASaliases anyway (:239,:250,:263).
Divergence (blind read vs PR body)
- Blocking. The body's third Limits bullet asserts the absence of the exposure found above, in the code's own words. It is load-bearing, not stray: the body is explicit and right that "the limit protects the collector's write; it does not shorten the build … every uncompressed chunk, older ones included" — which is why the older chunks' locks need accounting for, not dismissing. The mechanism is the body's own write-timeout analysis applied to a writer it did not consider.
- The body claims a surface it removed; six members are gone. Wording, nothing breaks in-tree.
- Other direction: the body's lock sampling ("none on the hypertable root") contradicts
PgTableTuning.cs:239's measured "it takes the same root ShareLock first". I take the newer measurement, but one of those files is now wrong about a measured fact and this PR cites the other. - Unmentioned: chunk boundaries are epoch-aligned at
ChunkIntervalDays = 1(TimescaleSupport.cs:273,:7653), so the 5-minute retry pins the build to the first minutes after UTC midnight on a 5-9 GB/chunk store.
Reviewer checklist
- Settle the blocking item live: run the per-chunk build while
QueryStoreBackfilldigs a slice on a hypertable whose older chunks are field-sized, and see whether its COPY breaches the two 10 s waits — that decides whether the gate should read the largest chunk insideQueryStoreBackfill.HorizonFor. - Re-run the lock sampling, record the TimescaleDB version, reconcile with
PgTableTuning.cs:239. - Confirm on the
darling-pgruntime that the partial +transaction_per_chunkbuild is accepted on a compressed hypertable andDROP INDEX CONCURRENTLYis refused. - Build and run the full
Darling.Testssuite with and without a store (the body's two unchecked boxes); I could not build here. - Watch one field service across a UTC-midnight boundary and confirm the deferred index builds on the first retry after it.
…ly: the partial index on the raw hypertable, its newest-chunk size check and its retry are removed (#4952) The partial index was built per chunk on collect.query_store_stats, a hypertable. A per-chunk build takes a ShareLock on each chunk it builds, and the Query Store backfill writes backdated rows into older chunks, not only the newest, under the collector's 10 s COPY deadline, so no chunk is safe to lock while the service runs. The background step is back to one attempt per service start. A hypertable is skipped with a warning, as before. IndexSpec loses its hypertable build and drop forms, the ensure no longer returns an outcome, and the live tests build the BRIN and the btree.
… registry test; the BRIN decision tests already cover them (#4952)
| public static readonly IReadOnlyList<IndexSpec> All = new[] | ||
| { | ||
| QueryStoreIntervalWideBrinIndex.Spec, | ||
| WideServerFirstExec, | ||
| }; |
There was a problem hiding this comment.
The fourth reader of this table — the Custom Views / Compose route — is not in the body, not in Limits, and not in any test here, and it is the one route the tree already records a measured 4× plan regression on from an extra index on first_execution_time.
ComposeCompiler.cs:202-205, in its own words:
This route deliberately carries no first_execution_time floor (#4605). With a BRIN index on collection_time and random_page_cost 1.1, the floor made the planner read the window plus 26 h of rows through
idx_query_store_interval_wide_first_execinstead of that index, about 4x slower. Fleet-wide windows of 12 h or more rely on that index and that setting; without them this read scans the table.
The relation that route compiles (ComposeCompiler.cs:208-210) carries no server_id predicate and no first_execution_time floor:
(SELECT w.*, s.server_name FROM collect.query_store_interval_wide AS w
JOIN collect.servers AS s ON s.server_id = w.server_id
WHERE w.collection_time >= $a AND w.collection_time <= $b)and the panel's server scope is applied outside it as f.server_name = ANY($n) (:356, :796; the class remark at :121-123 says Postgres is expected to push that qual through). For a server-scoped panel the planner can therefore push server_name = ANY(...) to the small servers scan and nested-loop into w on server_id. Before this PR the only index leading with server_id was the 8-column ux_query_store_interval_wide (PgMigrations.cs:2475-2487), which is expensive to walk per server. This PR adds a 2-column index leading with server_id — strictly more attractive for exactly that inner index scan, at random_page_cost = 1.1, which is the setting the managed store ships and the setting the 4× figure was measured under.
That is the same plan flip ComposeCompiler recorded, from a narrower index than the one that caused it. I cannot confirm it from a read-only tree — hence PLAUSIBLE, not CONFIRMED — but nothing in the diff measures or pins it:
ReadAllAsync(QueryStoreBackgroundIndexesLiveTests.cs:134-185) exercises the MCP top, the viewer top and the duration trend. Not the Compose route.Limitsenumerates those same three reads and defers the rest to The Viewer's Query Store table read at 168 h and the duration-trend read do not use the per-interval btree #4971. The Compose route is named nowhere in the body.- The body's "Every read returns the same rows with or without the indexes" is a claim about rows, which any index satisfies. The risk here is plan shape, and the one route whose plan the repo has already measured as index-choice-fragile on this table is the unexamined one.
Please either EXPLAIN a server-scoped Custom Views Query Store panel at 12 h and 168 h with both indexes present on the seeded store and record it beside the existing table, or say in Limits that the Compose route was not re-measured so a reader of ComposeCompiler.cs:202 knows its measurement predates a second server_id-leading index.
| } | ||
|
|
||
| private static async Task<object?> ScalarAsync(NpgsqlConnection connection, string sql, CancellationToken ct) | ||
| internal static async Task<object?> ScalarAsync(NpgsqlConnection connection, string sql, CancellationToken ct) |
There was a problem hiding this comment.
(Anchored here because it is the nearest changed line; the finding is about :60-61 and :233 in this same file.)
The one data-independent guard on this table's HOT safety still builds the BRIN alone, so it cannot see QueryStoreBackgroundIndexes.All — the trap from .github/REVIEW-TRAPS.md applies here.
A roster-based census does not see the file it does not roster:
EveryPagedTool_ObservesTruncation_…walked ten tools and missed three in another file (#3699). An anti-pattern census sweeps the class (every tool body); rosters are for the positive contract only.
NoNonBrinIndex_CoversAColumnTheUpsertSets_SoTheUpsertStaysHotForItsWholeSetList (:216) is the strongest guard this table has: it is a catalog scan, not a seeded update, so it fires on any index covering any DO UPDATE SET column — including a column only an INCLUDE list or a partial predicate mentions (:247-252). Its roster is one line: EnsureAsync(connection, ct) at :233, which resolves to this file's local helper at :60-61 — the BRIN's ensure, and nothing else.
Before this PR that roster was complete: the BRIN was the only index built outside a migration rung. After it, All (QueryStoreBackgroundIndexes.cs:122-126) has two members and is built to grow, and this census sees one of them.
Today's btree happens to be safe — server_id and first_execution_time are both in IdentityColumns (QueryStoreIntervalWide.cs:77-78) and neither appears in the DO UPDATE SET list (:344-394), and TheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn proves it behaviourally with a control that can fail. But the guard that would catch the next member is now blind to it, and nothing else in the tree records that.
One-line fix: point :60-61 at a loop over QueryStoreBackgroundIndexes.All — the same EnsureAllAsync the new live file already has (QueryStoreBackgroundIndexesLiveTests.cs:64-70). That restores the census to "every index this product builds on this table" and weakens nothing it asserts today.
| } | ||
|
|
||
| [Fact] | ||
| public void EveryBackgroundIndex_IsRegisteredOnce_BuildsConcurrentlyAndIdempotently_AndHasAConcurrentDrop() |
There was a problem hiding this comment.
REFUTED — the body's RED/GREEN evidence names this test and describes an assertion it does not make.
The Test plan says:
RED with the btree taken out of the ensure.
EveryBackgroundIndex_IsRegisteredOnce_Idempotent_AndHasADropfailed: it found only the BRIN's name.
Two problems:
- There is no test called
EveryBackgroundIndex_IsRegisteredOnce_Idempotent_AndHasADrop. The nearest name is this one,EveryBackgroundIndex_IsRegisteredOnce_BuildsConcurrentlyAndIdempotently_AndHasAConcurrentDrop. - This test would not have failed that way. It takes
Allas given and asserts per-spec properties — distinct names,collect.prefixes,CREATE INDEX CONCURRENTLY IF NOT EXISTS, notransaction_per_chunk, a concurrent drop. It holds no expected-name list, so with the btree removed fromAllit iterates one spec and goes green. "It found only the BRIN's name" is not a thing this test can say.
The test that does assert the exact two names, in order, is TheBackgroundIndexes_AreTheBrinThenTheWideBtree_AndNoneTargetsTheRawHypertable at :87-104 (Assert.Equal(new[] { QueryStoreIntervalWideBrinIndex.IndexName, QueryStoreBackgroundIndexes.WideServerFirstExecIndexName }, All.Select(...))). That one goes red with the btree pulled, with exactly the symptom the body describes.
The second half of the same bullet checks out: the HOT live test did pass vacuously without the index and now asserts indisvalid first (QueryStoreBackgroundIndexesLiveTests.cs:344), so a missing ensure fails 42P01. Only the attribution is wrong — but it is the bullet a reader would follow to confirm the red, and it points at a test that cannot produce it. Please correct the name and the mechanism.
| logger.LogDebug("Query Store index ensure was cancelled before it started."); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| logger.LogInformation( | ||
| "Query Store index ensure was cancelled at shutdown; the next start retries, " | ||
| + "and drops any half-built leftover first."); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| LogEnsureFailure(logger, "(shutdown)", ex); |
There was a problem hiding this comment.
REFUTED, narrowly — "The BRIN's behavior is unchanged" is true of the DDL and the decision, not of the lines an operator reads.
The DDL half holds: CreateSql/DropSql are byte-identical, MinimumServerVersionNum survives, and QueryStoreIntervalWideBrinIndex.Decide maps the shared IndexAction back to BrinAction so the existing theory cases (including (150000, true) → SkipServerVersionBelowSixteen) still hold.
Three operator-facing changes rode along with the extraction:
:222—"Query Store interval index ensure ({Index}) was cancelled before it started."→"Query Store index ensure was cancelled before it started.":226-228— the shutdown line likewise lost{Index}:232— the generic-failure catch now logs the literal string"(shutdown)"where the index name used to go, so the line readsQuery Store index ensure ((shutdown)) failed and is retried at the next start: …
Dropping {Index} is the wrong direction for a fan-out from one index to two-and-growing: these three lines are now the only ones in the family that do not say which index they are about, and :216 and :240 (the per-spec failure) do. "(shutdown)" in a slot the message template calls {Index} is also a category error — it is the reason, not a name, and it rides a LogWarning rather than the LogInformation the sibling shutdown path uses.
Nothing in the tree greps for the old text (I checked: zero hits for "Query Store interval index" outside the deleted code), so this breaks no code — it is a body correction plus, ideally, restoring {Index} on :222/:226-228 and separating the shutdown reason from the name slot on :232.
The same bullet's other claims do check out: the six moved members are exactly StartDelay, BuildTimeoutSeconds, CatalogReadTimeoutSeconds, StateSql, HypertableSql, RunDelayedAsync, and nothing outside the two classes and their tests referenced any of them.
| public void TheWideBtree_LeadsWithTheServerAndCarriesTheFirstExecutionBoundTheTableReadsFilterOn() | ||
| { | ||
| foreach (var sql in new[] { DarlingDataReader.QueryStoreTopTableSql }) | ||
| { | ||
| Assert.Contains("WHERE server_id = $1", sql); | ||
| Assert.Contains("first_execution_time >= $2 - ", sql); | ||
| } | ||
|
|
||
| Assert.Contains("(server_id, first_execution_time)", Wide.PlainCreateSql); | ||
| } |
There was a problem hiding this comment.
Non-blocking. foreach (var sql in new[] { DarlingDataReader.QueryStoreTopTableSql }) is a one-element loop — the shape of a roster that was meant to hold more than one entry.
The test's own name says "the table reads", the spec's doc at QueryStoreBackgroundIndexes.cs:104-110 says "the per-server table reads", and the live file in this PR reads all three (QueryStoreBackgroundIndexesLiveTests.cs:143, :159, :170). ViewerDataService.QueryStoreTopTableSql (ViewerDataService.QueryStore.cs:206-209) and both arms of QueryStoreDurationTrendTableSql (ViewerDataService.QueryTrends.cs:358-362, :375-380) carry the same WHERE server_id = $1 + first_execution_time >= $2 - … shape this is asserting.
Not a coverage hole in practice — QueryStoreIntervalWideFirstExecFloorTests already pins all three individually (:62, :81, :98), each asserting WHERE server_id = $1 and the floor built from the margin constant. So this test is redundant with those for the one read it covers. Either add the other two to the array, or drop the loop and say the per-read pins live in QueryStoreIntervalWideFirstExecFloorTests — a vestigial one-element foreach reads like an incomplete census to the next person.
| There is no per-server probe on this table: its only secondary index leads with first_execution_time. The | ||
| per-server reads' bound on first_execution_time is served by the background btree on (server_id, | ||
| first_execution_time) (#4952, QueryStoreBackgroundIndexes.WideServerFirstExec). */ |
There was a problem hiding this comment.
Non-blocking, doc only. The new sentence is appended to a clause the PR makes false: "its only secondary index leads with first_execution_time". Once the background btree exists the table has two secondary indexes, and the new one leads with server_id — which is precisely what the appended sentence says. As written the two halves contradict each other inside one comment.
Two sibling comments describing the same index set were not touched and are now incomplete the same way:
:542-544— "Among the table's indexes are the unique key … andidx_query_store_interval_wide_first_exec." (thePurgeEdgeMarginSqlsummary, which is where a reader following the floor's argument lands)QueryStoreIntervalWideFirstExecFloorTests.cs:20-22— the same list, in the class remark of the test file that pins the floor
Since these indexes are deliberately not migration rungs, these comments are the only record of what is on this table, so it is worth keeping them in step.
| internal const string HypertableSql = @" | ||
| SELECT EXISTS | ||
| ( | ||
| SELECT 1 | ||
| FROM timescaledb_information.hypertables AS h | ||
| WHERE h.hypertable_schema || '.' || h.hypertable_name = $1 | ||
| );"; | ||
|
|
There was a problem hiding this comment.
Non-blocking. This arm also changed in the extraction, which the body's "The BRIN's behavior is unchanged" covers over. The BRIN's probe used to be two literal equality tests:
WHERE h.hypertable_schema = 'collect'
AND h.hypertable_name = 'query_store_interval_wide'It is now h.hypertable_schema || '.' || h.hypertable_name = $1. Equivalent for every name in All today, and the trap about timescaledb_information.hypertables missing a continuous aggregate's materialization does not bite here (this asks only whether one named real table is a hypertable, and both specs name a real collector table).
Worth noting anyway, because the concatenation form makes the match sensitive to how the spec's TableName is spelled in a way the two-column form was not: any TableName carrying a quoted or mixed-case identifier, or a schema name with a dot, silently answers "not a hypertable" and the build then fails at CREATE INDEX CONCURRENTLY instead of being skipped. Assert.StartsWith("collect.", spec.TableName, …) (QueryStoreBackgroundIndexesTests.cs:112) is the closest thing to a guard and it does not reach that. Splitting TableName on the last . and comparing the two halves, as the old form did, keeps the spec's name format from being load-bearing.
Top-level notes (re-review of the current tree, after 193d48f and 00e7f50)The design that drew the three earlier changes-requested verdicts is gone: there is no partial index on Two earlier items that survived into this tree:
Things I checked that hold
One thing the body's own evidence says, which the title does notThe EXPLAIN table shows the new index is chosen in three of six read × window cells. The viewer's table read at 168 h and both arms of the duration trend at 168 h stay on Related: the BRIN shipped with a plan pin ( CHANGELOG
|
There was a problem hiding this comment.
Verdict: CHANGES REQUESTED
Design is now right — the partial index on the raw hypertable is gone and All is two specs on one plain heap, which retires the blocking finding from the three earlier verdicts. What remains: one unexamined reader of the changed table whose plan the tree already records as fragile against this exact index shape, one applied trap, two refuted claims. Six inline comments and a top-level note carry the detail.
Claims
- VERIFIED —
first_execution_timeis the seventh key of the unique index, so its bound cannot narrow the range —PgMigrations.cs:2475-2487; the bound atDarlingDataReader.cs:1955. - VERIFIED — a btree on
(server_id, first_execution_time), no migration, noStorageVersionchange —QueryStoreBackgroundIndexes.cs:114-119; no rung in the diff, viewer gate still ends at V157 (ViewerDataService.cs:1058). - VERIFIED — six members moved, nothing outside the two classes and their tests used them — grep over both SKUs and both test projects returns only
QueryStoreBackgroundIndexes.cs:124and the two test files. - VERIFIED — one delayed task, BRIN then btree, connection per spec, failure-isolated, one attempt per start, delay takes the stopping token —
DarlingWorker.cs:2320-2321,:3816;QueryStoreBackgroundIndexes.cs:192-197,:208,:213-218. - VERIFIED — validity read first, INVALID leftover dropped —
StateSql:129-137, drop at:297-305; live-pinned byAnInvalidLeftover_IsDroppedAndRebuilt_ForTheBtreeAndTheBrin. - VERIFIED — a hypertable spec is skipped with a warning and no spec targets the raw table —
Decide:152-158,EnsureAsync:285-290, two-name list pinned byTheBackgroundIndexes_AreTheBrinThenTheWideBtree_AndNoneTargetsTheRawHypertable(QueryStoreBackgroundIndexesTests.cs:87). - VERIFIED — no read's SQL changed; comments only in
QueryStoreIntervalWide.cs— both hunks are doc text (:414-416,:489-495). - VERIFIED — the NULL-start row is asserted in the trend's output and both runs share one window end —
QueryStoreBackgroundIndexesLiveTests.cs:92-96,:212-213, oneendat:200feeding:201and:217. This closes the clock-anchor flake the last review raised. - VERIFIED —
TheWritersDoUpdateSetList_…parses the writer's SQL and asserts it sets neither key —QueryStoreBackgroundIndexesTests.cs:452-465against the 50-columnDO UPDATE SETatQueryStoreIntervalWide.cs:344-394andIdentityColumns:77-78. - VERIFIED — Lite parity: nothing to change —
query_store_interval_wideappears nowhere underLite/; DuckDB has noCONCURRENTLY, hypertable or BRIN. - UNVERIFIED — every field and rig number (5-10 s/call, 10.2 s / 22.4 M blocks, 4.1 s / 6.8 M blocks, the whole EXPLAIN table, the before/after pairs, the 2.35 s build): a read-only tree yields no plan and no buffer count.
- UNVERIFIED — the 110 / 137 / 10-of-10 counts and "0 warnings": needs
dotnet buildanddotnet testwithDARLING_TEST_PG; fullDarling.Testsis left unchecked in the body. - UNVERIFIED — "61 of 61 heap-only": the test pins
updated >= 20andEqual(updated, hot)(QueryStoreBackgroundIndexesLiveTests.cs:369-370), so 61 is a run artifact. - REFUTED — the RED bullet's
EveryBackgroundIndex_IsRegisteredOnce_Idempotent_AndHasADrop"found only the BRIN's name". No test has that name, andEveryBackgroundIndex_IsRegisteredOnce_BuildsConcurrentlyAndIdempotently_AndHasAConcurrentDrop(:106-121) holds no name list, so it goes green on a one-elementAll. The red belongs toTheBackgroundIndexes_AreTheBrinThenTheWideBtree_…(:87). Inline. - REFUTED — "The BRIN's behavior is unchanged". True of the DDL and the decision, false of the lines it emits:
:222and:226-228lost{Index}, and:232prints the literal"(shutdown)"into the{Index}slot. No caller breaks — I re-read every reference. Inline.
Riskiest lines
Darling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:122-126— a second, narrowerserver_id-leading btree on a table whose Compose routeComposeCompiler.cs:202-205records as having regressed 4x when the planner preferred afirst_execution_timebtree over the BRIN; that route carries noserver_idpredicate and no floor (:208-210) and scopes outside the flattenable subquery asf.server_name = ANY($n)(:356,:796) — UNPINNED.Darling/Darling.Tests/QueryStoreIntervalWideBrinIndexLiveTests.cs:233— the catalog HOT census (:216) still calls this file's BRIN-onlyEnsureAsync(:60-61) — pinned for today's set byTheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn, UNPINNED for the next member ofAll.Darling/PerformanceMonitor.Darling.Storage/QueryStoreBackgroundIndexes.cs:144—h.hypertable_schema || '.' || h.hypertable_name = $1replaces two literal equality tests, making a spec'sTableNamespelling load-bearing — pinned byASpecAimedAtTheRawHypertable_IsSkippedWithAWarning_AndBuildsNothingfor the one name it tries.
Traps near this diff
- "A roster-based census does not see the file it does not roster … rosters are for the positive contract only." — applies — the HOT census's roster is "whatever
QueryStoreIntervalWideBrinIndex.EnsureAsyncbuilds" (QueryStoreIntervalWideBrinIndexLiveTests.cs:233→:60-61), one of two members ofAll. - "A pin that encodes the lie … Ask what a pin would fail ON." — does not apply, pre-empted — the HOT live test now asserts
indisvalidfirst (:344) with a control at:373-376. - "A test that reads the clock twice measures the CI scheduler … One anchor owns both ends." — does not apply (fixed) — one
endat:200. - "Two copies of one definition pinned EQUAL … Alias, do not copy." — does not apply — the tests bind the product's own SQL constants and parse
UpsertSql. - "
timescaledb_information.hypertablesnever lists a continuous aggregate's materialization." — does not apply —HypertableSql:139-146asks about one named real table. - "
chunk_compression_statsreturns ZERO rows when compression is off; count chunks fromtimescaledb_information.chunks." — does not apply, heeded — the raw-index census at:286-292useschunks. - "Truncation inferred from a full page (
rows.Count >= limit)" and "hours_back/limit/toprefused, never clamped" — do not apply — no MCP page or parameter is touched. - "A delta family that stores no interval" and "
ELSE 0spells unknowable as 0" — do not apply — no collector, delta or rate column is touched. - "Lite and Darling ship in parity … a fix to one SKU without the counterpart's is a finding, not a nit." — does not apply — PostgreSQL index DDL for Darling's store; the table does not exist under
Lite/.
Divergence (blind read vs PR body)
Three. The body is ahead of me everywhere else — it volunteers the unfixed probe, the three-of-six index selection and the rig's small warm scale before a reviewer can ask.
- A reader the body never names.
Limitsand the EXPLAIN table treat "the table reads" as three. There is a fourth:ComposeCompiler.BuildFactRelation(:208-210), the Custom Views route. I flagged it before reading the body because the tree records that this route's plan already flipped toidx_query_store_interval_wide_first_execonce and cost 4x, and that fleet-wide windows of 12 h or more depend on the BRIN being chosen. This PR adds an index strictly cheaper than the one that caused that flip for the server-scoped nested loop the route compiles. "Every read returns the same rows with or without the indexes" is a claim about rows; the exposure is plan shape. - A test cited for a red it cannot produce — the bullet names a test that does not exist and describes an assertion the nearest real one does not make.
- "Behavior unchanged" at the log surface — three operator-facing lines changed, one printing
"(shutdown)"where a name goes.
Everything else I wrote down before reading the body matched it, including the removal of the partial-index design the earlier reviews blocked on.
Reviewer checklist
-
EXPLAIN (ANALYZE, BUFFERS)a server-scoped Custom Views Query Store panel at 12 h and 168 h on the seeded store, with both indexes present and then withix_query_store_interval_wide_server_first_execdropped — the one question a read-only tree cannot answer, and the oneComposeCompiler.cs:202has already been wrong about once. - Run full
Darling.Teststwice, storeless and against a freshDARLING_TEST_PGdatabase (both[ ]in the body), and confirm a warning-free Debug build. - On PostgreSQL 14 or 15, where
Decideskips the BRIN, confirm the wide btree builds and the upsert still reports 100% HOT:MinimumServerVersionNum = 0makes it the first background index built below PG 16, and no test covers that combination. - After the 20-minute build on the 43-server store, re-measure
get_query_store_topand read the log for the two "built in Ns (valid)" lines; two sequential builds atBuildTimeoutSeconds = 7200make the worst-case shutdown drain four hours of cancellable work, not two. - Decide whether indexes built outside the migration ladder want a line on whatever surface reports index bytes to an operator; nothing reports their size today.
… name their index, the HOT census builds every background index, the table-read roster covers every per-server read, the hypertable probe splits the table name, and the per-server table floor read is answered by the wide btree; signature only for the split, behavior unchanged (#4952)
… the plan under test is the wide btree's index-only scan; on a table whose pages are not yet all-visible the planner takes the older first_execution_time index instead (#4952)
…n line, and its hypertable probe compares the schema and the name as two parameters; the comments that listed the table's indexes name both secondary btrees and say the wide one is built in the background (#4952)
erikdarlingdata
changed the title
Every finding in this review is resolved on the current head; the PR body records each change.
1 participant
Part of #4952.
Why
On a 43-server store,
get_query_store_topcost 5-10 s and 0.5-1.1 GB of disk reads per call at its default 24 h window. Its main read took about 28 s at 168 h, against a 60 s statement timeout. Two statements did most of the work, and neither cost came from the rows the window needs:QueryStoreTopTableSql): mean 10.2 s and 22.4 M blocks per call.first_execution_timeis the seventh key of the unique index, so its bound cannot narrow the range, and the scan reads every entry the server has. This PR fixes this read.QueryStoreIntervalWide.HasLegacyRowSql): mean 4.1 s and 6.8 M blocks per call. It is anEXISTSthat finds nothing on a normal store, so it fetches every raw heap page in the server's window to answer no. This PR does not fix it. Darling: the Query Store read's check for rows with no start time reads every raw row in the window #4972 tracks it (see Limits).What changes
collect.query_store_interval_wide (server_id, first_execution_time), built in the background. There is no migration and noStorageVersionchange. The table is a plain heap, so the index is built withCREATE INDEX CONCURRENTLY, which blocks no writes.QueryStoreBackgroundIndexes, for the BRIN and the new btree. It holds what was the BRIN class's own machinery. That is the state read, the build-or-skip decision, the leftover drop, the build and the delayed start.QueryStoreIntervalWideBrinIndexkeeps its spec, its decision andEnsureAsync, and it delegates to the shared class. Six of its members moved there:StartDelay,BuildTimeoutSeconds,CatalogReadTimeoutSeconds,StateSql,HypertableSqlandRunDelayedAsync. Nothing outside the two classes and their tests used them. The BRIN's DDL and its build-or-skip decision are unchanged. Two things around it changed:CONCURRENTLYbuild leaves an INVALID index, whichIF NOT EXISTSkeeps for good. The ensure reads validity first and drops the leftover before it builds.CONCURRENTLYon a hypertable. The per-chunk form is not offered. It takes a ShareLock on each chunk it builds, and the collector's write into that chunk waits behind it under a 10 s deadline. The Query Store backfill writes backdated rows into older chunks too, so no chunk ofcollect.query_store_statsis safe to lock while the service runs. No spec targets that table.QueryStoreIntervalWide.ResolveReadAsync, called fromDarlingWebEndpoints.RunComposedPanelAsync. For a window of 12 h or more, each check reads the server's earliestfirst_execution_time(PlainTableFloorSql). Without the wide btree, PostgreSQL walks the olderfirst_execution_timeindex until it meets one of that server's rows. A server whose rows are all recent pays for every older entry. With the wide btree on a vacuumed table, the read is one index-only lookup, pinned below. The legacy-row probe in the same check still costs what it does today (Darling: the Query Store read's check for rows with no start time reads every raw row in the window #4972), and the checks still run one server at a time.QueryStoreIntervalWide.cs. The probe's remarks now state its cost instead of saying it "costs nothing extra". The comments that list the table's indexes now name both background indexes, here and inQueryStoreIntervalWideFirstExecFloorTests.Limits
collect.query_store_stats, a compressed hypertable. Nothing builds one there safely while the service runs (see above). Darling: the Query Store read's check for rows with no start time reads every raw row in the window #4972 lists the options and asks for a measurement first.first_execution_timeindex, as before. The Viewer's Query Store table read at 168 h and the duration-trend read do not use the per-interval btree #4971 tracks measuring them on a large store. Custom Views panels scoped to servers use it too, and got faster (below).Test plan
The seeded store for the measurements:
16 servers, with 1.81 M raw rows over five daily chunks (three compressed). The heaviest server has 557,600 of them.
1.30 M interval rows (678 MB heap) over nine days.
random_page_cost = 1.1, otherwise defaults.The product's own statements, prepared and bound. Warm cache.
EXPLAIN (ANALYZE, BUFFERS) of each table read with the BRIN and the wide btree present. Heaviest server, read-only, rolled back, one run each:
idx_query_store_interval_wide_first_exec, 1,369.1 msBefore and after, MCP table read. At 24 h: 497.9 ms to 46.4 ms, and shared buffers 303,360 to 2,809. Before, it was a Parallel Index Scan on the single-column
first_execution_timeindex, with 88,433 rows removed by the collection-time filter. At 168 h: 1,394.6 ms to 545.9 ms. Before, the same scan ran with three workers, and each spilled a 7-8 MB sort to disk. After, it is an Index Scan on the new index withIndex Cond: server_id = 1 AND first_execution_time >= ...and no spill.Custom Views (Compose) route. Its fact relation has no
server_idpredicate and nofirst_execution_timefloor.ComposeCompilerrecords an earlier 4x plan flip on this table. Measured with the product's compiled SQL for a panel grouped by query hash, on the server with the most rows (325,500 of 1.30 M). Times are medians of five runs:serversinto a Bitmap Heap Scan over a BitmapAnd of the BRIN and the wide btreeWithout the wide btree, the one-server plans keep the same shape, with the unique index as the BitmapAnd's second leg. The wide btree's leg reads 298 index pages for that server against the unique index's 5,423. Heap blocks are the same either way. The wide btree is never scanned on its own here, so the earlier flip does not repeat.
Build time: the wide btree,
CREATE INDEX CONCURRENTLY, 2.35 s on 1.30 M rows.Same results (
QueryStoreBackgroundIndexesLiveTests). Three reads run with the indexes absent and then present, and every row must match, timestamps to the tick:DarlingDataReader.QueryStoreTopTableSql)ViewerDataService.QueryStoreTopTableSql), bound with a closed window end and with the open end a preset window binds as NULLQueryStoreDurationTrendTableSql).The seed holds a NULL-start row in the wide table. The test asserts that row is in the trend's output, so the trend's legacy arm is compared, not empty. Both runs share one window end.
The index set is pinned.
Allis exactly the BRIN and then the wide btree, and no spec targetscollect.query_store_stats. A live test aims a spec at the raw hypertable. The spec is skipped with a warning and builds nothing, and the real set adds no index there.Every table read fits the wide btree. A unit test checks the MCP and viewer reads and both arms of the duration trend. Each filters on the server and bounds
first_execution_timeat the window start.RED with the btree taken out of
All.TheBackgroundIndexes_AreTheBrinThenTheWideBtree_AndNoneTargetsTheRawHypertablefailed: it asserts the two names in order and found only the BRIN's. The HOT check passed with the index absent, which proved nothing. It now asserts first that the wide btree exists and is valid. With the ensure taken out, it fails with42P01: relation "collect.ix_query_store_interval_wide_server_first_exec" does not exist.HOT check, rolled back (
TheUpsertsWholeSetList_StaysHotWithTheWideBtreePresent_AndNotWithABtreeOnASetColumn). With the wide btree present, one transaction gives a changed value to every column that the writer'sON CONFLICT ... DO UPDATE SETnames. The test asserts at least 20 updated rows, all heap-only, from the transaction'spg_stat_xactcounters. The recorded run updated 61, all heap-only. The control, a btree on a SET column (execution_count), shows none heap-only, so the counters can fail.TheWritersDoUpdateSetList_NeverSetsTheServerOrTheFirstExecutionTime_SoTheWideBtreeKeepsUpdatesHotalso parses the writer's SQL and asserts it sets neither column.The catalog HOT census covers every background index.
NoNonBrinIndex_CoversAColumnTheUpsertSets_SoTheUpsertStaysHotForItsWholeSetListnow builds every spec inAll, not just the BRIN. The next member is checked too.The per-server floor read uses the wide btree. A live test seeds two servers and builds every index in
All. It vacuums and analyzes the table, then reads the plan ofPlainTableFloorSql. The plan must be a Limit over an index-only scan ofix_query_store_interval_wide_server_first_execwithIndex Cond: (server_id = 1), and no Seq Scan. The test asserts the index name, because the older index's walk is also a Limit over an index scan. With the wide btree dropped, the test failed: it expected the wide btree and gotidx_query_store_interval_wide_first_exec.The log lines are pinned. Unit tests drive
RunDelayedAsyncwith an injected ensure. A run cancelled before it started names every index. The shutdown line names the index in progress. An error raised during shutdown logs at Information with that index, never "(shutdown)" in its place. Each test failed before the fix. For example, the shutdown-error test expected no Warning and got one. A failed build names the server error's SQLSTATE in its warning, and an error that carries none names none. With the SQLSTATE part stubbed out to empty text, that test failed.The hypertable check's split is pinned.
SplitTableNamesplits a spec's table name at its last dot and refuses a name with no schema. Unit tests cover both, and the hypertable check binds the two parts as$1and$2.Lite parity: nothing to change. Lite's Query Store top read (
Lite/Services/LocalDataService.QueryStore.cs) is one statement with no separate probe. DuckDB has noCONCURRENTLY, hypertables or BRIN.Targeted classes, Debug build with 0 warnings. The unit, census and pin classes, including
QueryStoreBackgroundIndexesTests,QueryStoreIntervalWideBrinIndexTestsandQueryStoreIntervalWideFirstExecFloorTests: 249 of 249. With a live store,QueryStoreBackgroundIndexesLiveTestsandQueryStoreIntervalWideBrinIndexLiveTests: 11 of 11. After the last change,TsqlConventionGuardTestsand the index, census and pin classes: 171 of 171.Full suites in CI at the head (
56ebce33f).Darling.Testswithout a store: 20,079 tests, 0 failed, 1,330 skipped, 1 not run. With a store: 20,079 tests, 0 failed, 7 skipped, 1 not run.Lite.Tests: 7,264 tests, 0 failed, 0 skipped.CHANGELOG
No entry in this PR; the release owner adds it under [Unreleased].
Proposed entry: