Repository navigation
Dedup Query Store intervals before aggregating (#1841 tier 1) - #1845
Conversation
query_store_stats rows are CUMULATIVE per-Query-Store-interval snapshots, and the collector is incremental on last_execution_time, so an OPEN interval is re-fetched every cycle and stored again with a growing execution_count. The schema carries no interval id, and every aggregate read summed the raw rows -- counting one interval's work once per collection. A live store held 496 collections of a single interval inside one hour bucket, each billing its full CPU and duration. Read-side fix only: collapse each interval to its LATEST snapshot before aggregating, keyed on first_execution_time, matching the ROW_NUMBER convention already used by the plan-regression analysis collectors. This corrects data already collected; no migration or re-collection needed. Fixed: Lite's slicer, top-queries grid and comparison; the Darling Viewer's equivalents plus regressions and the slicer overlay; the service's MCP/REST get_query_store_top; and the Custom Views raw route. Deliberately unchanged, each documented at the source: - Bar placement still keys on collection_time, so a bar lags by one bucket on Query Store's default 60-minute interval. Correcting it needs first_execution_time, which is server-LOCAL against a UTC axis. - The duration trend keeps its un-deduped (overstating) form. Deduping keeps one row per interval-close, and a 60-minute interval against a 5-minute cadence collapses twelve snapshots onto one timestamp -- a 1-hour window would render a single zero-valued point. - The hourly/daily CAGGs materialized their sums from un-deduped rows and cannot be repaired at read time; a continuous aggregate cannot contain a window function, so they need a rebuild. The compose partition carries server_name as well as server_id so the panel's server scope can still push through the window function. Tests: real-DuckDB and real-Postgres round-trips assert an interval is counted once, each watched failing against the old queries first; SQL pins on every affected constant; a source-containment guard for Lite, whose SQL is inline and had no drift guard; and the first test that executes the compose compiler's output against Postgres. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| WITH deduped_baseline AS ( | ||
| /* LOAD-BEARING (correctness, not just perf) — #1841. The rows are CUMULATIVE per-interval | ||
| snapshots and the collector re-fetches the OPEN interval every cycle, so the SAME interval | ||
| (same first_execution_time) is stored repeatedly with a growing execution_count. Keep the | ||
| LATEST snapshot per interval before aggregating. | ||
|
|
||
| This read is especially exposed: the baseline arm is UNBOUNDED (collection_time < $2, |
There was a problem hiding this comment.
Performance regression risk: unbounded baseline now sorted, not just aggregated.
Before this PR, baseline_performance did a single streaming/hash aggregate (SUM/AVG/COUNT DISTINCT) over every row with collection_time < $2. Now it first materializes a ROW_NUMBER() OVER (PARTITION BY ... ORDER BY collection_time DESC) across that same unbounded row set before aggregating — which for a long-lived server (the comment itself says "potentially months") means Postgres has to build partitions and sort within them (or hash+sort per partition) over the entire history on every regressions-tab load, not just scan-and-reduce it once.
This is a real cost trade for correctness (the comment explains why the dedup is necessary here), but it's worth flagging explicitly as a performance regression on busy/long-retention servers, since the recent-window dedup (bounded) doesn't have this problem. Might be worth confirming via EXPLAIN (ANALYZE, BUFFERS) against a store with a large history before/after, since the regressions tab is presumably used interactively.
| } | ||
|
|
||
| return "(SELECT * FROM (SELECT *, ROW_NUMBER() OVER (PARTITION BY server_id, server_name, database_name, " |
There was a problem hiding this comment.
The correctness of this dedup is well covered (string pins + the live-Postgres compose test), but its performance for a server-scoped panel depends entirely on Postgres pushing the outer f.server_name = ANY(...) qual down through this window function — which only happens because server_name is included in PARTITION BY here. That's a real, documented Postgres planner optimization (predicate push-down below WindowAgg is safe only when the predicate's columns are a subset of every window's PARTITION BY), so the design is sound, but there's no test that pins the query plan — only that the SQL text contains server_name in the partition and that the live query returns the right values. If a future refactor reorders the partition list or a Postgres version regresses this optimization, a fleet-scoped panel would silently start ranking every server's raw query_store_stats rows for the whole window before narrowing — exactly the regression the partition placement is meant to prevent. Worth an EXPLAIN-based assertion (or at least a comment note to re-verify with EXPLAIN against a multi-server seed) rather than relying solely on string/value pins for this specific guarantee.
Review summaryWhat this does: collapses Correctness. The dedup key choice ( Lite/Darling parity. No drift found. Every dedup added to a Darling read has a matching Lite read updated with the identical partition key and Performance. Left two inline notes: (1) Security. No concerns. All new/changed SQL is parameterized; no string-built predicates from user input; Nothing else stood out as a correctness or parity risk. The two performance notes above are suggestions, not blockers. |
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>
…gdata#1841 tier 2) 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 (erikdarlingdata#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 erikdarlingdata#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: erikdarlingdata#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 erikdarlingdata#1850 (the analysis dedups still drop a replica's rows on an AG primary). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…S dedups by replica (erikdarlingdata#1850) erikdarlingdata#1846 — alert history rendered "0.00" for alerts that have no number to report. The value columns are NOT NULL doubles, producers hand the store a display STRING, and the store's parser yields 0 for text carrying no digit, so a role ("PRIMARY"), a connection state ("DISCONNECTED"), a suspend reason or the literal "resolved" all landed as a sentinel that rendered as a measurement. Both grids now render an em dash for those metrics. Write side untouched: the column stays NOT NULL, 0 stays the sentinel, and rows already collected display correctly with no migration. Enumerated from the fire sites rather than the issue text, which found 19 metrics rather than 9: the seven AG metrics, Server Unreachable/Restored, and every resolution notice — Darling's BuildResolutionRecord hardcodes CurrentValueText "resolved" for both its own self-alert recoveries and the shared engine's resolution callback, and those reach history only in Darling. Resolutions match through the existing shared AlertMetricClassifier .IsResolution, so a future recovery metric is right without an edit, and the predicate lives in PerformanceMonitor.Common rather than being hand-mirrored into two formatters — the drift that class exists to stop. The dash is gated on a stored 0, never the name alone, and that gate is load-bearing: the parser scans to the first digit ANYWHERE, so AG Sync Fell Behind spells its lag into its prose and any of them can pick a digit out of an object name. Three Darling self-alerts are excluded for that reason (erikdarlingdata#1881), Store Disk Pressure most of all — its parsed value is percent-free, where a real 0 means a full volume. A measurement that happens to be zero is not a missing value; both suites pin that separately. erikdarlingdata#1850 — four analysis-layer Query Store dedups kept a key narrower than the read side's, with no replica_role. sys.query_store_runtime_stats is keyed by (plan_id, interval, execution_type, replica_group), and on a SQL Server 2022+ AG with Query Store for secondary replicas the primary holds ONE shared Query Store carrying every replica's rows — so the rn = 1 filter was not de-duplicating those rows, it was DROPPING one. An under-count, which is worse than the double-count the dedup exists to fix: a double-count is visible in the number, a dropped row is silent. erikdarlingdata#1845 used this reasoning for the read-side key; these four are the same defect one layer down. Split rather than blended: replica_role is carried through the grouping and the ranking, a regression is measured WITHIN a replica, and it is exposed in both drill-down row shapes. The joins use IS NOT DISTINCT FROM, never = — replica_role is NULL on every standalone/non-AG/pre-2022 server and NULL = NULL is UNKNOWN, so an equi-join would have silently disabled plan-regression detection for nearly every install. Verified live on real DuckDB and real PostgreSQL 18.4 + TimescaleDB, with a NULL-replica arm in both suites. Watched RED first on both stores: two replicas over the same two intervals, the old key reporting one offender at 3x against a true two offenders and 12x — dropping the primary reported the secondary's milder regression as the server's worst. One downstream consequence fixed mechanically: the force-plan recommendation reads fixed keys, so two rows for one query would have rendered two sp_query_store_force_plan calls naming the same query with different plan ids; it now keeps the worst per query, also watched red. Which replica should drive that recommendation is erikdarlingdata#1882. The Darling FROM/JOIN drift guard needed a real fix rather than an accommodation: it scanned comments and read the word "join" in English prose as a relation, and read the FROM inside IS NOT DISTINCT FROM as one too. 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>
…r action for it The erikdarlingdata#1845-style annotation the lead asked for, placed where that precedent put it: in ComposeCompiler's doc, beside the erikdarlingdata#1849 routing boundary it shares. It records that a SECOND, permanent understatement sits underneath the inflated-rollup one -- rollups materialized from pre-erikdarlingdata#1907 rows carry ONE SLICE of each split interval, the repair reaches only as far back as raw still does, and re-materializing past that DESTROYS the rollup rather than correcting it. Nothing about it is visible in a panel; the numbers are simply low, which is exactly why it is written down. CHANGELOG Important gets two entries. Lite's is the v39-style one-time note: first launch takes a few extra seconds, the archive is rewritten, and the file may get slightly BIGGER despite losing rows -- 28.0 MB to 31.6 MB while 22,760 rows were removed -- because parquet size follows row-group layout and per-column compression rather than row count. Disclosed rather than pretended away. Darling's is the operator action next to --backfill-rollups, and it leads with the timing because the timing is the unusual part: the verb gets LESS useful every day you wait. It repairs only what raw still holds, so run it right after upgrading and it fixes the recent history you are looking at; run it a week later and it correctly reports nothing to do while the numbers stay wrong. What it cannot reach is permanent. DARLING01 named explicitly. Also fixes a doc-hygiene displacement the guard caught -- my insertion had pushed QueryArchiveAsync's summary onto the wrong member -- and corrects that summary, which still said DuckDB's parquet cache is per-connection. It is per-PATH at INSTANCE scope; a fresh connection from the same store does not escape it, which is the entire trap. Darling 4033 passed / 0 failed against live PG 18.4 + TSDB 2.28.1, Lite 1927 passed / 0 failed, full -t:Rebuild 0 warnings / 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ata#1921, option 1) erikdarlingdata#1841 moved the slicer BARS to the hour an interval ran and left the OVERLAY on the hour it was collected, so a point sat up to one Query Store interval -- 60 minutes by default, 1440 at most -- to the RIGHT of the bar describing the very same work. The file's own comment stated the invariant this broke: its dedup half kept holding, its placement half silently stopped. Both apps now place the overlay on COALESCE(interval_start_time_utc, collection_time), and the WINDOW moves with the x-value -- filtering on collection time while plotting on the interval start disagrees at both edges AND disagrees with the bars about whether an interval is in the window at all. Same collection_time pruning bounds the slicer already ships (erikdarlingdata#1892/erikdarlingdata#1923); the issue's own "no ceiling" scope note predates that round. ORDER BY moves too, or the chart gets a non-monotonic series and draws a zig-zag rather than a curve. TWO THINGS THE INSTRUCTION DID NOT ANTICIPATE, both verified before acting: - The nominated watched-red pin does not move. Its rows are seeded WITHOUT an interval identity, so COALESCE correctly falls back to collection_time and the point stays at bucketStart+15m. Rather than force it, its assertion is unchanged and now pins the legacy FALLBACK, and the behavioral proof went into the tier-2 test whose rows can actually show the move -- where the interval starts in h0 and every collection lands in h1. Watched red there: reverting the x-value fails it with "Expected 06:00, Actual 07:10", the 70-minute offset, and the legacy pin keeps passing. Both generations are now covered by the test whose data can demonstrate each. - Lite has no dedicated overlay read. It fed the overlay from the history GRID's read, which is a deliberately raw per-collection projection (erikdarlingdata#1845) windowed on collection time -- right for a list of collection events, wrong for a series drawn over the bars. Moving that shared read would have silently changed the history grid too, so Lite gained a dedicated read instead, matching the structure Darling has had since erikdarlingdata#683. It picks up the DEDUP the overlay never had: an interval collected repeatedly used to draw as a rising staircase of restatements and now draws as one point at its final values, which is what the bar beside it always showed. That is the same invariant, not extra scope. Both source guards caught the new read and were updated deliberately (5 -> 6 dedup sites, 7 -> 8 rank filters, tie-break total 7 -> 8) -- the erikdarlingdata#1912 guard fired from the direction of a read being ADDED rather than edited, which is what it was written for. Darling 4033 passed / 0 failed against live PG 18.4 + TSDB 2.28.1, Lite 1937 passed / 0 failed, full -t:Rebuild 0 warnings / 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Read-side fix for #1841. Fixes #1841 is deliberately NOT used — tier 2 remains.
The bug
query_store_statsrows are CUMULATIVE per-Query-Store-interval snapshots, and the collector is incremental onlast_execution_time, so while a query keeps running its OPEN interval is re-fetched every cycle and stored again with a growingexecution_count. The schema has no interval-id column and every aggregate read summed the raw rows, so one interval's work was counted once per collection.Live evidence from a real store, same
(server_id, database_name, query_id, plan_id)inside ONE hour bucket: 496 rows withexecution_count = 1each (one interval, 496 collections, its CPU and duration billed 496 times), plus cases growing 1 -> 23 cumulatively where every re-collection also re-charged the accumulated total.Execution-count weighting did not rescue the averages: repeated snapshots of one interval carry DIFFERENT growing weights AND different
avg_*values, so an open interval was weighted by the triangular sum of its own growth.The fix
Collapse each interval to its LATEST snapshot before aggregating, keyed on
first_execution_time(the interval identity the schema does expose), via theROW_NUMBER() ... WHERE rn = 1shape already used byPgFactCollector.QueryPerf/PgDrillDownCollector.Queriesand their Lite twins. This corrects data already collected — no migration, no re-collection.The partition is
(database_name, query_id, plan_id, first_execution_time, execution_type_desc, replica_role), uniform across both apps.execution_type_descandreplica_roleare there becausesys.query_store_runtime_statsis keyed by(plan_id, interval, execution_type, replica_group)— a narrower key could DROP a legitimate row, turning a double-count into a silent under-count.get_query_store_topThree things this does NOT fix, all documented at the source
Bar placement lags one bucket. Bars key on
collection_time, so an interval is attributed to the hour it was last COLLECTED, not the hour it ran — on Query Store's default 60-minute interval, reliably one bucket late. Window totals are right; placement is not. Fixing it needsfirst_execution_time, which is the monitored server's LOCAL wall clock while the axis is UTC.The duration trend is deliberately left un-deduped and still overstates. Deduping is right for totals and wrong for that chart: it keeps one row per interval, at the collection where the interval closed, and a 60-minute interval against a 5-minute cadence collapses twelve snapshots onto one
collection_time. Measured: a 1-hour window renders a SINGLE point, valued 0 because the LAG has no predecessor; a 4-hour window drops from ~48 points to 4. An honestly-labelled inflated line beats an empty chart until tier 2 can place work at the time it ran. Pinned by test in both apps so the exclusion stays a decision.The CAGGs are not repaired.
query_store_stats_hourlymaterializedsum(execution_count)and both weighted sums from un-deduped rows, and_dailyinherits them hierarchically. No read-side change can undo that — the duplicates are gone once materialized, and the CAGG output carries nofirst_execution_timeto key on. A TimescaleDB continuous aggregate cannot contain a window function orDISTINCT ONat all, so this needs a rebuild (or write-side dedup), not an edit. Consequence to know: a Custom Views panel whose window reaches past the raw tier still reads the old inflated numbers, so numbers step at the routing boundary.Testing
Lite.Tests/QueryStoreDedupReadTests.cs) seeding both live shapes — an interval collected 4x with a FLAT count of 1 (the 496x shape) and one collected 3x while it grew 10 -> 25 -> 40. Watched RED first: 22 vs 13 ms CPU, 75 vs 40 executions, 4 vs 1, 15 vs 11.Independent review caught the trend regression and a Postgres qual-pushdown problem (the compose partition now carries
server_name, or the panel's server scope could not push through the window function and a fleet store would rank every server's rows first).🤖 Generated with Claude Code