Repository navigation
Darling builds a partial index at start so the Query Store legacy-row check reads an index instead of recent raw rows (#4972) - #5021
Conversation
erikdarlingdata
left a comment
There was a problem hiding this comment.
Claude posting for Erik Darling
Verdict: the change is sound and can merge after small fixes. The statement, its start-path-only build and its lock handling are correct. I found no Defects. The main gaps are in the tests and the wording. The plan pin can pass its "over compressed chunks" claim without compressing any chunk. The live tests build the index on an empty hypertable, never on one that already has compressed chunks, which is the case a production upgrade hits. And two comments claim more than the production numbers show.
Claims checked:
- VERIFIED, start path only:
PgTableTuning.cs:418-463. A CREATE statement first runs theindisvalidcatalog probe (:131-133). If the index is present, the statement counts and goes on. On the hourly pass a missing index only logs a warning andcontinues. Only the start path runsGuardedCreate(:111-112, 5 slock_timeout, 300 s CommandTimeout). - VERIFIED by reading the code, not run: "abandoned and retried". This is a plain one-transaction CREATE, so a timeout or cancel rolls it back and leaves no INVALID index.
ApplyAsync's catch issues a ROLLBACK. The sibling comment (:286-294) agrees: only the per-chunk form leavesindisvalid = false. An INVALID index of this name could only come from a hand build. The probe would then warn on every start and never rebuild it (:441-449). That is existing behaviour and not new here. - Nit on unchanged code: a start-path CREATE that hits its
lock_timeoutlogs "the next pass tries again" (PgTableTuning.cs:474-477). For an index, only the next start tries again. - VERIFIED, the second-pass proof:
QueryStoreLegacyRowIndexLiveTests.cs:128-147copies the sibling (TuningHourlyCreateLiveTests.cs:62-78). A CREATE that was issued would wait outGuardedCreate's 5 sSET LOCAL(which overrides the session's 2 s) and fail with 55P03. TheLockNotAvailablecatch does not count it, soapplied == Statements.Countproves that no CREATE was issued, not that one was skipped. The ALTER TABLE SET statements take a lock that ROW EXCLUSIVE does not block. The DROP IF EXISTS of an absent index takes no table lock. - VERIFIED, the plan pin is tight:
:157-213. I can't name a plan text that passes wrongly on a fresh scratch store:- Every chunk-level
Scannode must be a ColumnarScan, a read of the partial index, or a Bitmap Heap Scan whose next node is a Bitmap Index Scan on it. - A BitmapAnd with a second index fails, because the other Bitmap Index Scan is itself in
scans. - A
DecompressChunknode fails. - A heap
Filter:on the NULL test fails. - The fresh store's compressed relations are
_hyper_N_M_chunk_compressed, which:179excludes. - Gaps: an upgraded store's
compress_hyper_*child index would fail falsely.Index Scan Backwardis rejected. Both are false failures, not false passes.
- Every chunk-level
- UNVERIFIED (needs a database): which TimescaleDB version CI uses.
:205-208requires theColumnarScannode name. An older version that names the nodeDecompressChunkwould fail loudly, not pass silently.PlanChunkScansalready accepts both names. - REFUTED, partly: the comments claim more than was measured (see INLINE
QueryStoreIntervalWide.cs:495andPgTableTuning.cs:327). - VERIFIED, the PR title: "…reads an index instead of recent raw rows" holds. The compressed chunks are not "recent raw rows" in the sense the body uses, and the body states the 168 h remainder.
- VERIFIED, the CHANGELOG ENTRY: it says only the 24-hour window is answered from the index alone, gives "1.3 s of 1.7 s", and calls the minute "the build's read", not the build. That matches the numbers. The 24 h claim also holds by reasoning: with
CompressAfterDays = 1(TimescaleSupport.cs:80), a chunk is compressed only after its whole range is more than a day old. - VERIFIED, the PR body: "inferred, not a production build" for 30-70 s; 300/70 is about 4.3, so "over 4x" holds (
PgTableTuning.cs:351). - UNVERIFIED (needs a database): four TimescaleDB behaviours.
- A hypertable CREATE INDEX over existing compressed chunks gives each one an empty 8 KB index and reads only the uncompressed heaps.
- Recompression empties the partial index.
- A partly compressed chunk (backfill into an old chunk) plans its heap part on this index.
- Only the new-chunk inheritance is covered by a test (the index exists before the seeded chunks,
:63).
- VERIFIED, other checks:
- The own-store marker sits on the line directly above the column-0 class (
:29-30). There is no[Collection]attribute, the same as the sibling. - The source-text pins are one-line literals, so they do not depend on CRLF. The EXPLAIN parse calls
ReplaceLineEndings(:159). - No CA1068, CA1859 or xUnit2013 bait: CancellationToken is last everywhere, and
Assert.Equal(5, created.Count)is not a 0 or 1 count. - No host or store names appear.
- The own-store marker sits on the line directly above the column-0 class (
Counts: 0 Defects, 4 Notes, 3 Nits.
…rect the comments The plan test now follows the upgrade order (data and compressed chunks first, then the tuning pass), asserts compression happened, that compressed chunks carry an 8 KB copy and the compressed relations none, and that a new chunk inherits the index. A unit pin ties the read's IS NULL predicate to the partial index. The hourly test asserts the single warning; the duplicate lock block is dropped. Comments state the compressed-chunk read and the measured figures precisely.
erikdarlingdata
left a comment
There was a problem hiding this comment.
Claude posting for Erik Darling
Round 2 at 01a269f59 is clean. All seven round-1 items are answered, there are no new findings, and nothing blocks the merge. I read the one-commit delta (4 files, +94/-31) and did not run it. CI is green at this head.
-
The vacuous compressed half: FIXED. A one-day chunk interval is set before seeding. With TimescaleDB on, the test now asserts that at least one chunk was compressed and at least one was not, so neither half of the plan pin can pass empty.
-
The production order: FIXED. The index is built AFTER the store holds data with compressed chunks, the way a production upgrade meets it. The catalog checks pin three things:
- every compressed chunk's copy is the 8,192-byte empty index;
- no
*_compressedrelation carries it; - a chunk created afterwards inherits it.
The size query matches the index by its own definition (
… WHERE (interval_start_time_utc IS NULL)), so another index can't satisfy it. -
The
HasLegacyRowSqlremarks: FIXED. They now say the index serves the uncompressed chunks only, and that compressed chunks are still read by the columnar scan. -
The predicate pin: FIXED. A new fact pins
interval_start_time_utc IS NULLexactly, beside the server and time bounds. A rewrite the planner can't prove against the index's predicate now fails a plain build. -
"(a few buffers)": FIXED. The comment now gives the measured figures.
-
"4.1 s per call": FIXED. The comment now says it is a mean over a day.
-
The duplicated block: FIXED. The ROW EXCLUSIVE block is gone; the sibling
TuningHourlyCreateLiveTestsstill lists this index. The hourly pass now assertsStatements.Count - 1and exactly one warning that names the index and its table.
On the two long comment lines (~:309-311 and ~:327-328): leave them. A rewrap doesn't justify another push and CI run.
Counts: no Defects, Notes or Nits.
…ery_stats and procedure_stats read from a small index (#4605) (#5046) Darling builds two start-path partial indexes on (collection_time) WHERE sample_interval_seconds = 0, one on collect.query_stats and one on collect.procedure_stats, so the restart rows (a first sighting, an uncredited counter reset, or a gap past the policy, where no counter in the row was knowable) read from a small index. Two new readers in IntervalRollupRestartRows read those rows; no routing uses them yet, pending a production measurement after install. The readers carry presence and counts, not CPU. The large start-path indexes, these two and the legacy-row index from #5021, now share one build per start: once one CREATE INDEX has been issued in a start's tuning pass, the remaining missing ones are deferred to the next start with an Information line naming each. An index that already exists never counts. On a timeout, a dedicated Warning names the index, the table, the elapsed time and that it retries at the next start, pausing collection for up to SetupTimeoutSeconds. The builds run before collectors start and never on the hourly pass. On the largest store measured, each build takes about 0.5-1.5 minutes. Refs #4605
…rd, plan in the failure message (#5061) QueryStoreLegacyRowIndexLiveTests' plan check seeds to a fixed time of day, 23:50 UTC on a fixed past date, instead of the wall clock. Chunks align to UTC midnight and the newest chunk stays uncompressed, so a run just after midnight left that chunk holding a page or so of rows. There a Seq Scan costs within a fraction of a point of the partial index, and the planner could skip the index; the test failed three times on CI between 00:08 and 00:30 UTC. The newest chunk now always holds a full day, a guard requires at least 1,000 rows in it, and both plan assertions print the plan on failure. The test still asserts that the shipped check plans on the partial index; no plan is forced. Follow-up to #5021
Refs #4972. Part of #4952.
Why
Before a Query Store read uses the interval table,
QueryStoreIntervalWide.ReadsTableAsyncchecks that the server's window holds no raw row without an interval start (HasLegacyRowSql, clause 6). Four reads run that check:DarlingWebEndpoints.RunComposedPanelAsync);get_query_store_top;On a field store the answer is almost always no. To say no, the check fetched every raw heap row of the server's window, because the only index it could use is
(server_id, collection_time)and the NULL test was a row filter. On a large production store that cost a mean of 4.1 s and 6.8 M blocks per call (#4969). Measured again for this PR, one run each for the busiest server: 614 ms at 24 h and 1,723 ms at 168 h (Production measurement, below). The #4969 figure is a mean over a day of calls, cold and warm.What changes
CREATE INDEX IF NOT EXISTS idx_query_store_stats_server_time_null_start ON collect.query_store_stats (server_id, collection_time) WHERE interval_start_time_utc IS NULL, added toPgTableTuning.Statements, with its name asPgTableTuning.LegacyRowIndexName.query_store_statsindexes are built: at service start, after migrations and before the collectors, throughGuardedCreate(its own transaction, a 5 slock_timeout), and only when the catalog shows it absent. The hourly pass never builds it. It is not a migration, so there is noStorageVersionchange.CREATE INDEX CONCURRENTLYon a hypertable. A per-chunk or background build is unsafe while the service runs, because the Query Store backfill writes backdated rows into older chunks (Darling builds a background index so get_query_store_top no longer reads every entry a server holds (#4952) #4969,QueryStoreBackgroundIndexes).HasLegacyRowSql's remarks andQueryStoreBackgroundIndexesnow describe the index. The new statement's comment carries the measurements below.How the build behaves on a compressed hypertable
Checked on a local TimescaleDB 2.30.1 rig:
CREATE INDEXon the hypertable builds one index per chunk from that chunk's own heap. Each compressed chunk gets an empty 8 KB index, and the compressed relations get none. So the build reads only the uncompressed chunks' heaps, not the compressed data.server_idsegmentby index, as before (a few thousand buffers per chunk). It reads uncompressed chunks through the new index (1-2 buffers per chunk), where it used to walk the time index with a heap fetch per row.Production measurement
Read-only, on the largest production store measured, at 15:05-15:26 UTC on 2026-10-03.
The build's read set.
collect.query_store_statshad 17 chunks of 6 hours. The 12 compressed chunks had empty heaps. The 5 uncompressed chunks, about the last 27 hours, held 11.2 GB of heap and 19.7 M rows. One serial scan of every chunk's own heap with the index's predicate (FROM ONLY <chunk> WHERE interval_start_time_utc IS NULL,max_parallel_workers_per_gather = 0) is what the build reads:Applying the rig's build-to-scan ratio (0.5 with parallel workers to 1.2 with none and a cold cache) puts the first-start build at roughly 30-70 s. That is inferred, not a production build.
SetupTimeoutSecondsis 300 s, so the headroom is over 4×.The check today, before this change.
HasLegacyRowSqlfor the busiest server, with the product's default parallelism, one run each, run right after the scan above:Limits
SetupTimeoutSeconds(300 s) is rolled back and retried at the next start, and collection waits for it each time. The production scan above is the evidence that this store is well inside that budget.DarlingTrendReader~:1176,QueryStoreTrendRouting~:343,ViewerDataService.QueryTrends~:271) filterquery_store_statsthe same way, so the index is eligible for them. Not measured.ViewerDataService.QueryTrends~:378 reads the interval table, which this index does not touch.Test plan
Rig: a local TimescaleDB 2.30.1 (PostgreSQL 18) rig with the real schema. 4 servers × 4 daily chunks × 855,360 rows per server per day (the field rate), 585 B per row, the 2 oldest chunks compressed and the 2 newest uncompressed at 2.0 GB of heap each.
The check, before and after. One server, bound parameters, median of 3:
The build: 0.59-0.66 s warm for 4.0 GB of uncompressed heap with the default 2 parallel maintenance workers, and 1.33 s with none. A serial scan of the same heaps takes 1.21-1.33 s, so the build costs about half that scan. With the cache restarted (not the OS page cache), 1.15 s against a 1.43 s scan. NULL-start rows cost about 35 B each in the index.
Plan pin (
QueryStoreLegacyRowIndexLiveTests.TheShippedCheck_PlansOnThePartialIndex_AndAnswersExactly). It runs in the order of a first start after an upgrade: it seeds two servers' interval-stamped rows over three days in one-day chunks, compresses the older chunks where TimescaleDB is present (asserting at least one compressed and one uncompressed chunk, so the compressed half cannot pass vacuously), and only then runs the start-path tuning pass. From the catalog, each compressed chunk's copy of the index is exactly 8 KB, no compressed relation carries it, and a chunk created afterwards inherits it. It then EXPLAINs the shippedHasLegacyRowSqlwith its real parameter types. Every uncompressed chunk must be read through the partial index (an index-only, index or bitmap scan of it). No scan may carry the NULL test as a rowFilter:. Compressed chunks must use the columnar scan. The check answers false for both servers, then true for one server only once a NULL-start row lands for it.Start-path pin (
TheStartPath_BuildsTheIndexOnce_AndTheHourlyPassNeverBuildsIt). After the first start-path pass the index exists with the exact definition and is valid. That a second start issues noCREATEis held byTuningHourlyCreateLiveTests, whose converged-store test now lists this index. With the index dropped, the hourly pass leaves it absent, returnsStatements.Count - 1, and logs exactly one warning naming the index andcollect.query_store_stats.RED with the statement removed: both new tests failed,
PgTableTuningTestsfailed on its exact-text assert, andTuningIndexReaderPairingTestsfailed with "Expected 5, Actual 4". Restored, all green.The read's predicate is pinned: a
TuningIndexReaderPairingTestsfact holdsinterval_start_time_utc IS NULLexactly inHasLegacyRowSql, beside its server and time bounds, so the index's predicate stays provable from the read. RED withIS NULLmutated out of the read.Pins updated:
PgTableTuningTests(15 statements, 5CREATE INDEX, the exact statement text),TuningIndexReaderPairingTests(the index paired withHasLegacyRowSqlonserver_id),TuningHourlyCreateLiveTests(the index list).Targeted, census and guard classes with a store: 454 tests. The one failure,
TuningDropLockRecoveryLiveTests.BlockedDrop_GivesUpAtItsTimeout…, fails the same way ondevon this machine and passes in CI.Full
Darling.Testssuite with a store, on macOS at the head (01a269f59): 21,185 tests, 481 failed, 244 skipped. Every one of the 481 also fails ondevon the same machine and store (Viewer/WPF, install and managed-store classes that need Windows); none fails only on this branch. CI decides those.Builds:
Darling.TestsandLite.Testswith--no-incremental, 0 errors and no new warnings.CI at the head (
01a269f59): build, the three PostgreSQL test shards, the four Lite test shards, the whole-tree guards and the Linux build all pass.CHANGELOG
SECTION: Fixed
ENTRY: - Query Store reads no longer scan the last day's raw rows to rule out legacy rows ([#5021]) - Before a Query Store read uses the interval table (Custom Views panels,
get_query_store_top, and the viewer's Query Store grid and duration trend), Darling checks that the server's window holds no raw row without an interval start. That check read every recent raw row in the window: 0.6 s for one busy server at 24 h on a large store. Darling now builds a small partial index on those rows when the service starts, so a 24-hour window's check is answered from the index alone. Older, compressed data is still read as before, so a 168-hour window keeps that part of the cost (1.3 s of 1.7 s). Results are unchanged. The first start after the upgrade builds the index once, before collection begins; on a large store the build's read took about a minute.REF: [#5021]: #5021