Repository navigation
Collect the real Query Store interval identity and use it (#1841 tier 2) - #1853
Conversation
Query Store rows are cumulative per-interval snapshots and the collector re-fetches the OPEN interval every cycle, so every aggregate has to collapse an interval to its latest snapshot before summing. Tier 1 (#1845) did that on the first_execution_time PROXY because the schema exposed no interval key and no interval clock. Both now exist. The collector selects runtime_stats_interval_id and interval_start_time_utc (sys.query_store_runtime_stats_interval.start_time AT TIME ZONE 'UTC') from the one shared payload body, LEFT JOINed for the same reason the replica join is: an unresolved interval row must cost one column, not every runtime-stats row for that database. Neither column is version-gated - both are original Query Store surface, verified present on SQL Server 2016 SP3, the collector's own AppliesTo floor. Lite schema v49, Darling store V41, both additive, nullable, and appended so the positional writers stay aligned; the Darling viewer gains the matching probe sentinel and map arm. Every dedup keys on the real id, with the proxy kept beside it rather than COALESCEd: the id is NULL on exactly the pre-tier-2 generation, so the two can never collide and a legacy-only window degrades to precisely tier 1's key. Where the id exists it also fixes what the proxy could not - a row whose first_execution_time is NULL had no identity at all and collapsed with every other such interval of the same plan, which under-counts. The four analysis-layer dedups that predate tier 1 get the same key upgrade. The slicer bars and the duration trend now place work at the interval's start. The trend's tier-1 exclusion is deliberately reversed: its reasoning was sound but rested on a false premise, that the interval clock is the monitored server's LOCAL wall time. Query Store's interval start_time and first_execution_time are both datetimeoffset - verified live on a server reading +00:00 while sitting at UTC-4 - and the collector already normalized through DateTimeOffset.UtcDateTime before storing, so the interval clock always shared an axis with collection_time. Rows without the identity keep the pre-tier-2 treatment exactly; the arms split on IS NULL / IS NOT NULL, which partitions the rows with no overlap and no gap, so a mixed window counts every row once and resolves itself as those rows age out. The decision-pin tests from #1845 are re-pinned to the new behavior including the legacy boundary. Watched red first against real DuckDB and a real PostgreSQL 18.4 + TimescaleDB 2.28.1 store: the one-bucket lag (a bar at 16:00 for work that ran at 15:00), the un-deduped trend (4 points where 2 intervals ran), and the tier-1 under-count (5 executions where 11 ran). Filed rather than deferred silently: #1849 (the hourly/daily rollups still bake in the inflation - carries the corrected-rollup design and the live TimescaleDB probe results showing a CAGG cannot bucket on interval_start_time_utc) and #1850 (the analysis dedups still drop a replica's rows on an AG primary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review: Collect the real Query Store interval identity and use it (#1841 tier 2)What it does: Adds General assessmentThis is careful, well-reasoned work. Highlights that check out on inspection:
Correctness edge case (see inline comment)The slicer and duration-trend queries still filter their CTE on Minor / non-blockingSeven test files ( Nothing here blocks the PR from my read — the core correctness fix (interval identity dedup + placement) is sound and thoroughly tested; the window-edge behavior is a pre-existing class of issue this PR's own approach introduces alongside its fix, worth a follow-up rather than a blocker. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| -- so this is a placement fix and not a timezone trade. Legacy rows have no interval start and keep | ||
| -- collection_time placement, so a window spanning the upgrade renders correctly on both sides and the | ||
| -- lag simply disappears as the pre-upgrade rows age out. | ||
| date_trunc('hour', COALESCE(interval_start_time_utc, collection_time)) AS bucket, |
There was a problem hiding this comment.
Edge case: the CTE above filters on collection_time >= $2 AND collection_time <= $3 (the hoursBack/fromDate..toDate window), but this bucket now keys on interval_start_time_utc when it exists. Those aren't the same instant any more — interval_start_time_utc is the interval's start, while the row that survives the window filter is picked by its closing collection_time, which for a 60-minute interval lands up to ~1 interval-length later.
Concretely: an interval that started at startTime - 30min and closed with its final collection at startTime + 2min passes the collection_time >= $2 filter (it's inside the window), but its bucket is date_trunc('hour', startTime - 30min) — dated before the nominal window start. Pre-PR this couldn't happen because bucket == collection_time == the filtered column, so every bucket was guaranteed inside [startTime, endTime]. Now a "last N hours" chart can show one extra bar left of the requested range (bounded by one Query Store interval length, default 60 min).
Not sure this needs fixing before merge — it's a narrow, cosmetic edge case rather than a data-correctness bug (the totals in that bucket are still accurate) — but worth confirming it's intentional, since it wasn't covered by the new live-Postgres test (that test's seeded interval start (h0) is never earlier than the queried start (h0.AddMinutes(-1)), so it doesn't exercise this boundary). Darling's ViewerDataService.QueryStore.cs/QueryTrends.cs have the identical construction, so this would apply to both apps equally.
…in the CAGG chain (#1849) The hourly/daily Query Store rollups materialized sum(execution_count) from un-deduped raw rows, so one interval's work was baked in once per COLLECTION. Tier 2 (#1853) fixed every raw-tier read and could not reach these: the duplicates are gone once materialized and the rollup output carries no interval identity to key on. A Custom Views panel inside the raw tier read corrected numbers; past it, inflated ones, with a visible step at the routing boundary. Three NEW aggregates carry the corrected numbers, alongside the old pair rather than replacing it (a CAGG cannot be reshaped in place, and rebuilding would destroy 21 days of hourly and all daily history - #1759/#1793): query_store_stats_interval_hourly L1, interval grain, last(x, collection_time) query_store_stats_corrected_hourly composer grain, identity-width off L1 query_store_stats_corrected_daily composer grain, 1-day bucket off L1 Column names are identical to the old pair, so ComposeCaggValueMapper and every composed panel read them unchanged. Measured on a seeded store: one interval re-collected 496 times reports 506 where the old rollup reports 123,311. The shape is forced by what TimescaleDB accepts, live-probed on PG 18.4 / TimescaleDB 2.28.1. An identity-width hierarchical CAGG is a LEAF - nothing can be built on a child whose bucket equals its parent's width, though a plain 1h -> 1d -> 7d chain is fine - so the corrected daily is a SIBLING of the corrected hourly sourced from L1, where every other daily here is built on its hourly. That constraint is not in #1849; it was found building this. Arming: query_store_stats now has two rollup families, so RawTierCoverage's coverage became a LIST and the #1680 gate an AND over all of them. Raw stays paused until both reach back over everything it holds. Evaluated in the reader, not folded into GREATEST, which skips NULLs and would let an empty new rollup vanish from the comparison. Routing: ComposeCaggInfo gains the superseded pair. Corrected wins wherever its coverage reaches; beyond it reads fall back to the old pair, comparatively (only where it measurably reaches further back), so a backfilled store never reads inflated. Availability is settled before coverage - a store whose service predates this routes to the old pair exactly as before, which is why this needs no migration and no viewer version gate. Backfill: RollupViews now carries BucketWidth explicitly. "Hierarchical" and "daily" used to be the same fact and the backfill inferred grain from the source column; the corrected hourly is the first hierarchical rollup that is not a daily, and inferring would have given it a 24x-too-wide bucket - an under-estimate, the one direction the disk preflight exists to prevent. Capacity settled before shipping (#1581): the interval layer is near-raw cardinality, so it takes a short 7-day horizon rather than the 21-day one - 79 MB against 238 MB projected on a 600-query store at the default cadence. Verified against live PostgreSQL 18.4 + TimescaleDB 2.28.1, watched red first: removing L1 from the arming gate arms a purge over history nothing holds; last() -> sum() in L1 reports 123,256; dropping or inverting the legacy fallback each fails a named routing test. Darling 3848 passed / 0 failed, Lite 1728 / 0, full-solution rebuild 0 warnings. Filed rather than guessed: #1869, the daily tier's ~2x hour-straddle residual and the fourth near-raw-cardinality aggregate that would remove it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
From the review bot's inline comment on #1919, taking its first option rather than the comment-softening one: softening would have been a Lite/Darling parity scope-down. QueryStoreDedupReadTests.EveryQueryStoreAggregateInTheFile_CarriesADedupCte reads exactly one file, LocalDataService.QueryStore.cs, which is right for what it checks but left 2 of Lite's 7 dedup sites -- the ones in Analysis/DrillDownCollector.Queries.cs and Analysis/DuckDbFactCollector.QueryPerf.cs, both of which this same PR added tie-breaks to -- covered by no test at all. Dropping execution_count DESC from either would have gone uncaught, which is the recurrence mode #1841/#1845/#1853/#1907 keep demonstrating, and Darling was already guarded against it. Lite.Tests/QueryStoreSliceTieBreakSourceTests mirrors Darling's counterpart exactly: an enumerated DedupSites list (3 files, 7 sites) rather than a glob, so MOVING a read has to be re-declared instead of silently shrinking coverage, and a missing tie-break fails naming the file. It also carries a parity check that reads Darling's guard and asserts its declared total, so neither app can quietly fall behind the other. The source comments in both apps claimed "a source-containment test pins all of them" -- true only on the Darling side. Both now name the actual tests and their real counts. Watched red: dropping the tie-break from DuckDbFactCollector.QueryPerf.cs leaves the OLD guard green (proving the gap was real) while the new guard fails with "Lite\Analysis\DuckDbFactCollector.QueryPerf.cs @ char 9829". Lite 1903 passed / 0 failed, build 0 warnings / 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion sys.query_store_runtime_stats returns the flushed slice and the still-in-memory slice of one runtime_stats_interval_id as two separate rows, and they are ADDITIVE members of one interval rather than competing snapshots of it. The collector selected straight from the view, so both were stored -- and they then shared every column of the read-side dedup key (erikdarlingdata#1841/erikdarlingdata#1845/erikdarlingdata#1853) AND collection_time, so the viewer's ROW_NUMBER and the rollups' last(execution_count, collection_time) were both ordering by a value identical for both rows. The survivor was whichever the engine emitted first: 8 executions shown where 94 was true, differently on each run. Reproduces on box SQL Server 2022 (16.0.4255.1), not just Azure: 100 flushed + 25 in memory came back as two rows while dm_exec_procedure_stats reported 125 at the same instant. BuildPayloadBody -- the one body both execution shapes run -- now groups on the natural key of the view (plan_id, runtime_stats_interval_id, execution_type, replica group). execution_count SUMs; every avg_* takes the count-weighted mean, because Query Store stores an average and a count but never a total; min_*/max_* take the extreme; first/last execution time the interval's span. The emitted row shape is unchanged -- same 55 columns, same order, no migration -- only the row count per interval. The cutoff moves from a per-slice WHERE to HAVING MAX(...) at interval grain, which is load-bearing: the flushed slice is static, so a per-slice predicate stops matching it once the growing in-memory slice advances the watermark, and the sum degrades to the sliver alone. The IN (...) pre-filter is a prune only, and the fixed query is faster than the one it replaces (375ms vs 453ms on a real 212k-row Query Store) -- half the rows means half the plan XML to materialize. The deprecated Dashboard's collect.query_store_collector had the same defect against the same view and takes the same fix. Rows already collected cannot be repaired: all 19 read-side dedup sites in both apps gain a documented execution_count tie-break so a pre-fix tie resolves to the flushed slice deterministically instead of flapping. Closest-available, not correct -- residual tracked in erikdarlingdata#1912. Verified live on SQL Server 2022 (emitted SQL returns 125/70 against raw slices of {100,25}/{60,10}, matching dm_exec_procedure_stats) and on PostgreSQL 18 + TimescaleDB 2.28.1 (corrected rollups report the hand-computed 155 and the exact weighted mean; the same store fed split slices can never reach the interval's true 125). Lite 1854 passed / 0 failed, Darling 3997 passed / 0 failed, full solution rebuild 0 warnings / 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Tier 2 of #1841.
Fixes #1841deliberately NOT used — the CAGG half is spun out to #1849.What was missing
Query Store rows are CUMULATIVE per-interval snapshots and the collector re-fetches the OPEN interval every cycle, so every aggregate has to collapse an interval to its latest snapshot before summing. Tier 1 (#1845) did that on the
first_execution_timeproxy, because the schema exposed no interval key — and no interval clock, so bars were keyed on when an interval was last COLLECTED. Both now exist.Collecting the identity
BuildPayloadBody— the ONE body both execution shapes run — gains two columns:LEFT JOIN sys.query_store_runtime_stats_interval, notJOIN, for exactly the reason the replica join beside it is a LEFT JOIN: an interval row that fails to resolve must cost this collector one column, not every runtime-stats row for that database.Neither column is version-gated, unlike
plan_type_desc/ the log-bytes family /replica_rolearound them. Both are original Query Store surface — verified against SQL Server 2016 SP3 (13.0.6300.2), the collector's ownAppliesTofloor, along withAT TIME ZONE 'UTC'itself. A gate would be dead weight and would deny the identity to the oldest supported servers.Lite schema v49, Darling store V41. Both additive, nullable, appended (positional writers), not backfilled — nothing can reconstruct the identity for rows already stored. The Darling viewer gets the matching probe sentinel and map arm, which is the trap that would otherwise lock the viewer out of a fully-migrated store.
The premise tier 1 was blocked on was false
Tier 1 recorded that placing work at the interval's own clock would trade a magnitude bug for a timezone bug, "because
first_execution_timeis the monitored server's LOCAL wall clock while the axis is UTC."It is not. Both
sys.query_store_runtime_stats_interval.start_timeandfirst_execution_timearedatetimeoffset, andReadRowsAsyncalready normalized them throughDateTimeOffset.UtcDateTimebefore storing. Verified live on SQL 2022:Z, on a host sitting at UTC-4. The interval clock always shared an axis withcollection_time. The honest fix was available the moment the identity was collected.What changed in the reads
Dedup key —
runtime_stats_interval_idjoins the partition at all 13 tier-1 sites, plus the 4 analysis-layer dedups that predate tier 1. The proxy stays beside it rather than being COALESCEd into one expression: the id is NULL on exactly the pre-tier-2 generation and non-NULL on exactly the post-tier-2 one, so the two can never collide, and a legacy-only window degrades to precisely tier 1's key. Where the id exists it also fixes what the proxy could not — a row whosefirst_execution_timeis NULL had no identity at all under tier 1 and collapsed with every other such interval of the same plan, which UNDER-counts.Slicer placement — bars bucket on
COALESCE(interval_start_time_utc, collection_time). An interval is drawn in the hour it RAN, not the hour it was last fetched, which on the default 60-minute interval was reliably one bar late.The duration trend — tier 1's exclusion is deliberately reversed. Two arms, split on
interval_start_time_utc IS NULL:collection_time, byte-for-byte the pre-tier-2 behavior.The split partitions the rows with no overlap and no gap, so a mixed window counts every row exactly once. A store holding pre-upgrade history does not lie, and the mixture resolves itself as those rows age out of retention. The series resolution becomes Query Store's own interval length, which is the honest resolution of this source.
Both apps, everywhere: Lite's slicer / top queries / comparison / trend, the Viewer's equivalents plus regressions and the slicer overlay, the MCP + REST reader, and the Custom Views raw route.
CAGG verdict: assessed, designed, filed (#1849)
Not a blind rebuild, and not a hand-wave. Five candidate CAGG shapes were built against live PostgreSQL 18.4 + TimescaleDB 2.28.1:
time_bucket('1 hour', interval_start_time_utc)time bucket function must reference the primary hypertable dimension columnlast(execution_count, collection_time)grouped by interval identitylast(x, bucket)Probe 1 is the constraint everything hangs on: a CAGG on
query_store_statscannot bucket on the interval start; it must bucket oncollection_time. So a corrected rollup needs a NEW interval-grain L1 plus a hierarchical L2 — buildable, but it necessarily moves routing (ComposeCaggCatalog,RollupViews, the coverage probe) and retention arming (RawTierCoveragegates the raw purge on the OLD view'smin(bucket); move reads without moving the gate and raw can be purged over history the new rollup has not materialized). The existing CAGGs must also be kept, per the #1759/#1793 precedent and this repo's own comment that a rebuild "would permanently destroy the 21-day hourly and indefinite daily history."That is a staged migration, so it is filed with the complete design, the probe evidence, the capacity caveat, and four rejected alternatives with reasons — rather than half-built here. Deliverables 1-3 stand on their own: every row collected from now on is dedupable by real identity, so the data #1849 needs is already accumulating.
Also filed: #1850 — four analysis-layer dedups carry a narrower key than the read side and silently drop a replica's rows on an AG primary with Query Store for secondary replicas. Fixing it is not mechanical (drop / blend / split is a product call on the fact row shape), so it is written up with all three options rather than guessed at.
Testing
Watched red first, against both real stores:
first_execution_timeThe live PG test is new coverage: it makes collection time and interval time genuinely disagree, then adds a legacy row to the same window and asserts nothing already counted moves. Gating verified honestly — it SKIPS without
DARLING_TEST_PGand PASSES with it, so the store really was exercised.The #1845 decision-pin tests were read, then deliberately re-pinned to the new behavior including the legacy boundary — both arms present,
IS NULL/IS NOT NULLeach appearing exactly once so the split cannot silently stop being total.Suites, against a live PG 18.4 + TimescaleDB 2.28.1 cluster on a dedicated port:
-t:Rebuild: 0 warnings, 0 errorsInstaller.Tests deliberately not run.
🤖 Generated with Claude Code