Combine Query Store's flushed and in-memory interval slices at collection (#1907)

[erikdarlingdata]

Fixes #1907.

The bug

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, not
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 (#1841/#1845/#1853) and collection_time. So the
viewer's ROW_NUMBER() ... ORDER BY collection_time DESC and the rollups' last(execution_count, collection_time) were both ordering by a value identical for both rows, and the survivor was whichever
the engine happened to emit first. Live: 8 executions reported where 94 was true, with a different
subset wrong on each run.

The dedup itself is correct and stays — it exists to collapse re-collections of one interval across
cycles. It just cannot also be asked to add two slices within one cycle, and no read-side rule can
express both. A TimescaleDB continuous aggregate cannot express it at all.

It is not an Azure peculiarity. #1907's evidence was Azure SQL Database only and flagged box SQL
Server as untested. It reproduces there — SQL Server 2022 (16.0.4255.1), DATA_FLUSH_INTERVAL_SECONDS = 60, 100 executions then a forced flush then 25 more:

rsi | plan_id | module               | count_executions
  1 |       3 | dbo.pm_order_summary |              100   <- flushed, static
  1 |       3 | dbo.pm_order_summary |               25   <- in memory, growing
  1 |       4 | dbo.pm_recent_orders |               60
  1 |       4 | dbo.pm_recent_orders |               10

sys.dm_exec_procedure_stats, a wholly separate source read at the same instant, said 125 and 70.
SUM matches; the larger slice alone does not. Both slices carry an identical first_execution_time, so
the tier-1 proxy key had exactly the same exposure as the real interval id — which is why pre-tier-2 rows
are affected too.

With Query Store's default 900s flush against its default 3600s interval, one interval can hold several
flushed slices, so the split is not bounded at two.

The fix

BuildPayloadBody — the ONE body both the on-prem and Azure execution shapes run (#1844's single-body
guarantee, drift guard still green) — groups on exactly the natural key of the view and combines:

Column family	Combined by
count_executions	SUM
avg_* (11 of them)	count-weighted mean, SUM(avg * count) / NULLIF(SUM(count), 0)
min_* / max_*	MIN / MAX (includes min_dop/max_dop, which have no avg)
first_execution_time / last_execution_time	MIN / MAX — the interval's own span

The weighted mean is the whole game: Query Store stores an average and a count but never a total, so
avg * count is what recovers a slice's total. A plain AVG() of the slice averages would weight a
25-execution sliver the same as a 100-execution flush. Verified live — slices of (1778.42 over 100) and
(2245.60 over 25) combine to 1871.856, which is (1778.42*100 + 2245.60*25) / 125, and is not the
2012.01 an unweighted average gives.

Every avg_* column goes through one WeightedAverage helper rather than being written out by hand,
because the wrong form is not a compile error and does not look wrong. A test discovers the avg_ columns
from the emitted SQL and requires the weighted shape on each, so a newly added one is covered the
moment it appears.

The cutoff had to move from WHERE to HAVING

This is load-bearing, not tidiness. The flushed slice is static, so once the growing in-memory slice
pushes the watermark past its last_execution_time the flushed slice stops qualifying — and a sum over
the survivors is the sliver alone, the original defect with an aggregate bolted on top. HAVING MAX(last_execution_time) > @cutoff_time asks the question at interval grain: did this interval see new
activity, and if so give me all of it. It is strictly more permissive than the predicate it replaces, so
nothing that used to be collected stops being.

It is faster than what it replaces

Measured on a real 212,000-row Query Store (SQL 2025), full 55-column payload, warm, three runs:

Shape	Elapsed	Rows
Pre-fix	453 / 485 / 516 ms	510
Post-fix	375 / 422 / 438 ms	262
Post-fix without the interval pre-filter	1203 / 1203 / 1235 ms	263

Half the rows means half the nvarchar(max) query text and plan XML to materialize and ship, which more
than pays for the aggregate. The IN (...) pre-filter is a prune, not a semantic — its interval list
is by construction a superset of what the HAVING keeps, so it can never subtract a row — and the third
row is why it is there.

Nothing about the stored row moved

Same 55 columns in the same order, same positional writers, GoldenCollectorSchema untouched, no
migration and no storage-version bump. Only the number of rows per interval (now at most one per interval
per collection). The TOP backstop now caps intervals rather than slices, and sits outside the
aggregate for cause: a cap falling mid-interval would emit a partial sum, which is worse than omitting the
interval.

replica_group_id joins the grouping key under the same 2022+/Azure gate the attribution column carries,
for the same bind-safety reason — naming a column that does not exist in a GROUP BY fails the whole
SELECT, and on Azure's per-database path that means every database. It has to be in the key where it does
exist: two replicas' rows for one interval are different work, and grouping without it would sum a
secondary's executions into the primary's — the exact bug replica attribution was added to prevent.

Deprecated Dashboard: same defect, same fix

install/09_collect_query_store.sql reads the same view the same way and had the same bug. Same
aggregation, same WHERE to HAVING move, same gated replica_group_id. It is a proc body change, so an
upgrade re-applies it with no schema step and no upgrades/ entry. Verified live against SQL 2022 by
executing the proc's own generated SQL in all three version shapes — it returns 125 and 70 where the raw
view holds the split slices.

Legacy rows: made deterministic, and the residual is filed

Rows already collected cannot be rewritten. All 19 read-side dedup sites across both apps now order by
collection_time DESC, execution_count DESC, so a pre-fix tie resolves to the flushed slice — the one
holding the bulk of the interval's work — deterministically instead of flapping. On new rows the clause
can never fire, because there is one row per partition per collection.

That is closest-available, not correct. The correct value is the sum, and the materialized CAGGs
cannot be tie-broken at all (last() has no tie-break, and the tied rows are gone once materialized).
Filed as #1912, including the part that does not age out: the indefinitely-kept daily tier keeps
understated counts for the pre-fix period.

Testing

Watched red per mutation, each reverted after:

Mutation	Test that went red
weighted mean to AVG(qsrs.avg_x)	Payload_EveryAverageColumn_IsTheCountWeightedMean
replica_group_id ungated in the GROUP BY	BuildPerItemQuery_ReplicaGroupIdEntersTheGroupingKey_OnlyWhereItBinds
HAVING back to a per-slice WHERE	3 tests, including both paths' cutoff pins
tie-break dropped from one Lite read	TiedSlicesOfOneInterval_... (returned the 25 sliver) + the source guard
tie-break dropped from one Darling read	the Darling source guard, naming the file
post-fix interval reseeded as split slices	the live-PG rollup test (125 became 100)

The behavioural DuckDB test seeds both insertion orders as two independent queries, so it cannot pass
by accident on an engine that happens to favour the earlier row — without the tie-break one arm returns
the sliver.

Live SQL Server 2022 — the emitted collector SQL (not a hand-composed approximation) executed in all
three version shapes; all 55 columns bind on the 2016 floor, on 2017, and on 2022. Against the repro
database it returns 125 and 70 where the raw view holds {100, 25} and {60, 10}, matching
dm_exec_procedure_stats exactly.

Live PostgreSQL 18 + TimescaleDB 2.28.1 — a new QueryStoreCorrectedRollupLiveTests case asserts both
halves in one store on one refresh: the corrected rollups report the hand-computed 155 for an hour of
two combined intervals plus the exact execution-weighted mean off the same rows, while the same store fed
the pre-fix split slices returns a single slice and can never reach the 125 they add up to. Gating
verified honestly — it SKIPS without DARLING_TEST_PG and PASSES with it.

Suites: Lite 1854 passed / 0 failed; Darling 3997 passed / 0 failed (10 skipped = gated tests
needing a live SQL Server or the bundled runtime). Full-solution -t:Rebuild: 0 warnings, 0 errors.
Installer.Tests deliberately not run.

Deferred, filed before this PR opened

• Query Store rows collected before #1907 keep their split slices: read-side dedup is now deterministic but still under-counts, and the materialized CAGGs cannot be tie-broken at all #1912 — the pre-Query Store: the in-memory and flushed slices of one runtime_stats_id collide in the read-side dedup, understating execution counts non-deterministically #1907 rows and the CAGG buckets materialized from them. Three options written up,
  with the reason none of them repairs the indefinitely-kept daily tier.

🤖 Generated with Claude Code

[claude]

Nit: this edit introduced a UTF-8 BOM (EF BB BF) before the leading /* comment (confirmed with od -c). No other file touched in this PR has one (e.g. the new QueryStoreSliceTieBreakSourceTests.cs starts with a plain /*), so this looks like an accidental artifact of whatever tool made the edit rather than an intentional change. Harmless to the build, but worth stripping for consistency.

[claude]

Review

What this does: sys.query_store_runtime_stats returns the flushed and still-in-memory slices of one runtime_stats_interval_id as separate additive rows. The collector previously stored both straight through, and since they share the entire read-side dedup key and collection_time, every reader/rollup picked an arbitrary one of the two instead of their sum — a real, previously-live undercount (8 reported vs 94 true). The fix groups BuildPayloadBody's single shared query on the view's natural key (plan_id, runtime_stats_interval_id, execution_type_desc, gated replica_group_id) and combines slices at collection: SUM for count_executions, execution-count-weighted mean for every avg_*, MIN/MAX for extremes and the interval span, with the incremental cutoff moved from a per-slice WHERE to an interval-grain HAVING (necessarily — a per-slice WHERE re-introduces the bug once the in-memory slice's growth pushes past the flushed slice's last_execution_time). The deprecated Dashboard proc (install/09_collect_query_store.sql) gets the identical fix since it reads the same view the same way. All 12 (Darling) + 7 (Lite) read-side dedup sites get a deterministic execution_count DESC tie-break for legacy rows that already collided, with the residual tracked as a separate issue (#1912) rather than silently claimed as "fixed."

Correctness: Traced through BuildPayloadBody in PerformanceMonitor.Collectors/QueryStoreCollector.cs end to end — every column the outer projection references from the derived table (qsrs.*) is either part of the GROUP BY key or one of the aggregate aliases, so nothing binds to an ungrouped column. The IN (...) interval pre-filter is correctly a superset of what the HAVING keeps (it's not correlated by plan/execution-type, only by interval id, which is the right grain since a runtime_stats_interval_id is a global per-database time bucket, not a per-plan one), so it can only prune, never drop a real row. The version gates for replica_group_id in the grouping key track hasReplicaAttribution exactly, so there's no scenario in this change where the replica join is on but the grouping key is missing the column (or vice versa) — summing across distinct replicas isn't a new risk introduced here.

Lite/Darling parity: Verified directly — QueryStoreCollector.cs lives in the shared PerformanceMonitor.Collectors project referenced by both PerformanceMonitor.Darling.Analysis/Service/Storage and Lite, so the one Lite-side unit test suite (QueryStoreCollectorDefinitionTests.cs) genuinely covers both apps' collection path rather than needing a duplicate in Darling.Tests. The 12 Darling + 7 Lite read-side tie-break sites match between the new QueryStoreSliceTieBreakSourceTests.cs source-guard (Darling) and the updated regex count in QueryStoreDedupReadTests.cs (Lite). I grep'd the whole repo for any PARTITION BY ... runtime_stats_interval_id ... ORDER BY collection_time DESC shape outside the files this PR touches and found none — no missed dedup site.

Testing: Notably thorough — live SQL Server 2022 verification of the actual emitted SQL (not a hand-written approximation) across all three version shapes, a live Postgres/TimescaleDB test that asserts the corrected rollup and the pre-fix split-slice behavior in the same store/refresh so it can't pass by accident, and a test that discovers avg_ columns from the emitted SQL text so a newly added one is covered automatically.

Minor nit (left inline): QueryStoreCorrectedRollupLiveTests.cs picked up a stray UTF-8 BOM at the top of the file, inconsistent with the rest of the codebase.

Observation, not a blocker: The Dashboard proc fix (install/09_collect_query_store.sql) has no automated regression test — coverage there is manual live verification only (per the PR description). Given it's the deprecated path this is probably an acceptable tradeoff, but worth knowing if that dynamic SQL ever regresses silently.

No SQL injection, secrets, or unsafe file/network/process concerns — all the new SQL fragments are built from internal boolean version-gate flags, not user input, consistent with the rest of the collector's existing pattern.

[claude]

Review: #1919 — combine Query Store's flushed/in-memory slices at collection

Went through the diff in detail (collector SQL, all 12 read-side dedup sites in both apps, the deprecated Dashboard proc, and the new test coverage). This is an unusually well-documented and well-tested fix — the reasoning in the source comments matches the code exactly everywhere I checked it.

What I verified:

• QueryStoreCollector.BuildPayloadBody's new slice-aggregating derived table: GROUP BY key matches the SELECT list exactly, WeightedAverage() is used consistently for all 11 avg_* columns (no hand-written AVG()/MAX() snuck in), min_*/max_* (including the no-avg_-sibling min_dop/max_dop) use MIN/MAX, and the version-gated fragments (numPhysIoReadsAgg/logBytesAgg/tempdbAgg, replicaGroupKey) vanish cleanly on pre-2017/pre-2022 targets without leaving a dangling comma.
• The WHERE→HAVING move is correct: the outer WHERE qsrs.runtime_stats_interval_id IN (...) pre-filter is a superset prune (grouped only by runtime_stats_interval_id, not the full key), so it can't drop a row the HAVING MAX(last_execution_time) > @cutoff_time would otherwise keep — confirmed this is the only thing keeping it from re-scanning the whole retained Query Store every cycle.
• Lite/Darling parity: grepped for every PARTITION BY ... runtime_stats_interval_id ... ORDER BY collection_time DESC across both apps — all 12 sites (4+2+1+1+1+1+1+1 as declared in QueryStoreSliceTieBreakSourceTests) picked up the execution_count DESC tie-break identically in both apps, and the QueryStoreDedupReadTests/QueryStoreSliceTieBreakSourceTests guards make a future omission fail loudly instead of silently. The handful of remaining bare ORDER BY collection_time DESC hits in Query Store files (the query_text lateral-join lookups in ViewerDataService.QueryStore.cs/LocalDataService.QueryStore.cs, and the unrelated v_query_stats/parameter-sensitivity reads) are correctly out of scope — they don't partition on runtime_stats_interval_id, so they can't tie on this defect.
• Deprecated Dashboard's install/09_collect_query_store.sql mirrors the same grouping/weighted-mean/HAVING shape, gated the same way, with its own source-guard test (QueryStoreSliceAggregationSqlTests) since it's dynamic SQL the compiler can't see into.
• No new user-controlled input, no injection surface change — this is entirely internal collector/query-shape SQL.
• The "faster despite the added aggregate" performance claim is plausible and consistent with what's measured (fewer, wider rows moved off the wire) — not flagging a regression here.

One pre-existing (not introduced by this PR) inconsistency worth a note: the shared collector's cutoff comparisons use > (WHERE f.last_execution_time > @cutoff_time / HAVING MAX(...) > @cutoff_time) while the deprecated Dashboard proc uses >= throughout. Both are internally consistent, and this PR correctly preserved each app's existing operator rather than introducing drift, so it isn't a bug in this PR — just flagging in case it's meant to be unified at some point.

Nothing else stood out as a correctness, security, or Lite/Darling parity issue. (Per review scope, not flagging missing-index recommendations.)

[claude]

This comment says "a source-containment test pins all of them," but that's only true on the Darling side. Lite.Tests/QueryStoreDedupReadTests.cs's EveryQueryStoreAggregateInTheFile_CarriesADedupCte guard only scans LocalDataService.QueryStore.cs (5 sites). It does not cover the tie-break added in this same PR to Lite/Analysis/DrillDownCollector.Queries.cs or Lite/Analysis/DuckDbFactCollector.QueryPerf.cs — I confirmed neither file is referenced anywhere under Lite.Tests/.

Darling's counterpart, Darling.Tests/QueryStoreSliceTieBreakSourceTests.cs, explicitly enumerates all 8 files / 12 dedup sites across the app and fails loudly if one is dropped or moved. Lite has no equivalent cross-file guard, so a future edit that silently drops the execution_count DESC tie-break from either of those two files (exactly the failure mode #1841/#1845/#1853/#1907 keep recurring on) would not be caught by any test today, unlike on the Darling side.

Worth either widening the Lite guard to a DedupSites-style list covering all 7 sites (mirroring the Darling test), or softening this comment so it doesn't claim coverage that doesn't exist.

[claude]

Review summary

Reviewed the full diff (collector SQL, all 12/7 read-side dedup sites in Darling/Lite, the deprecated Dashboard proc, and the new/updated tests).

What the PR does: sys.query_store_runtime_stats can return a flushed slice and a still-in-memory slice of the same runtime_stats_interval_id as two additive rows. The collector previously stored both, and they shared the entire read-side dedup key and collection_time, so one slice was silently dropped depending on emission order — a real undercount bug (well-documented and reproduced live on both Azure SQL DB and box SQL Server 2022). The fix combines the slices at collection time in QueryStoreCollector.BuildPayloadBody (grouping on the view's natural key, SUM for the additive counter, count-weighted mean for every avg_* column via a single WeightedAverage helper, MIN/MAX for extremes), moves the incremental cutoff from a per-slice WHERE to an interval-grain HAVING (necessary, not cosmetic — a per-slice WHERE would silently degrade back to the sliver once the growing in-memory slice pushes the watermark past the static flushed slice), and adds an execution_count DESC tie-break to the 19 pre-existing read-side dedup sites so ties in already-collected data resolve deterministically to the flushed (larger) slice.

Correctness: I traced the SQL carefully —

• The weighted-mean formula (SUM(avg * count) / NULLIF(SUM(count), 0)) is correct and matches the live-verified numbers in the PR description.
• The IN (...) pre-filter combined with the HAVING is a sound semantics-preserving optimization: the pre-filter subquery only checks runtime_stats_interval_id (not the full grouping key), so it can pass through groups that ultimately fail the HAVING — but it can never wrongly exclude one, since it's a superset by construction. Confirmed against the outer projection's column references — none of the 55 columns' aliases drifted.
• replica_group_id's 2022+/Azure gating in the new GROUP BY is consistent with the existing hasReplicaAttribution gate and the deprecated Dashboard script's mirrored @replica_group_available flag.
• The query_plan_text "only the newest row gets the XML" ROW_NUMBER() still works correctly against the new aggregated derived table.
• I did not find a functional bug in the SQL itself, and the test suite (including new live SQL Server/PG-backed tests) is unusually thorough for this defect class.

Lite/Darling parity: Left one inline comment — Darling has a comprehensive cross-file source-containment guard (QueryStoreSliceTieBreakSourceTests) enumerating all 12 dedup sites across 8 files, so a future accidental drop of the tie-break fails loudly. Lite's equivalent guard only covers the 5 sites in LocalDataService.QueryStore.cs; the 2 sites added in DrillDownCollector.Queries.cs and DuckDbFactCollector.QueryPerf.cs (both fixed correctly in this PR) have no test coverage at all today, contradicting a comment added by this same PR that says "a source-containment test pins all of them."

Minor/non-blocking observation (not flagged inline, no action needed): the deprecated Dashboard script already used >= for the cutoff comparison pre-PR while QueryStoreCollector.cs uses > — this operator difference predates this PR and is preserved as-is, so it's not a regression introduced here.

Security/perf: No injection surface introduced (identifiers are QUOTENAMEd as before, no new user input paths); the measured perf numbers in the description are credible given half the rows now need materializing. No missing-index recommendations given per instructions.

Overall this is a well-reasoned, thoroughly tested fix for a real correctness bug. The one actionable item is the Lite test-coverage gap noted inline.

[erikdarlingdata]

GUARD WIDENED — pushed a66e785f.

Lite.Tests/QueryStoreSliceTieBreakSourceTests now mirrors Darling's counterpart: an enumerated DedupSites list covering all 3 files / 7 sites (LocalDataService.QueryStore.cs 5, Analysis/DrillDownCollector.Queries.cs 1, Analysis/DuckDbFactCollector.QueryPerf.cs 1). Enumerated rather than globbed, so MOVING a read has to be re-declared instead of silently shrinking coverage; a missing tie-break fails naming the file. A third test reads Darling's guard and asserts its declared total of 12, so neither app can quietly fall behind the other.

The overclaiming comment is fixed in both apps — each now names the actual test and its real count instead of "a source-containment test pins all of them".

Watched red, and it demonstrates the gap was real: dropping the tie-break from DuckDbFactCollector.QueryPerf.cs leaves the OLD guard (EveryQueryStoreAggregateInTheFile_CarriesADedupCte) green, while the new guard fails with Lite\Analysis\DuckDbFactCollector.QueryPerf.cs @ char 9829. Restored, verified byte-identical to the committed file.

Lite 1903 passed / 0 failed; full-solution build 0 warnings / 0 errors.

[claude]

Reviewed. This is an unusually well-documented and well-tested fix — the live repros on both SQL Server 2022 and Azure SQL DB, the weighted-mean test that discovers avg_ columns from the emitted SQL rather than listing them, and the cross-file parity guard (QueryStoreSliceTieBreakSourceTests in both Lite.Tests and Darling.Tests, asserting each app's declared total against the other's) are exactly the kind of regression-proofing this defect class needs. The single-body guarantee (BuildPayloadBody feeding both BuildQuery and BuildPerItemQuery) is a good structural reason to trust that Lite and Darling can't drift apart on this fix specifically.

One gap, not touched by this PR: the deprecated Dashboard's own analysis reads against collect.query_store_data — the exact table install/09_collect_query_store.sql was just fixed to populate correctly — still have the pre-#1907 tie-break vulnerability:

• deprecated/Dashboard/Analysis/SqlServerFactCollector.QueryPerf.cs:238 (CollectPlanRegressionFactsAsync, feeds the PLAN_REGRESSION fact)
• deprecated/Dashboard/Analysis/SqlServerDrillDownCollector.Queries.cs:511 (CollectRegressedQueries, the drill-down behind that same finding)

Both dedup with ROW_NUMBER() OVER (PARTITION BY database_name, query_id, plan_id, server_first_execution_time ORDER BY collection_time DESC) — the tier-1 proxy-key shape the PR description calls out as sharing the same exposure as the real interval id, with no execution_count DESC tie-break added. For any collect.query_store_data rows collected before this fix ships (which, per the PR, cannot be rewritten), these two reads will still nondeterministically pick the flushed or in-memory slice on ties, the same symptom (arbitrary fraction of the true count) this PR closes everywhere else. The 19-site count and the QueryStoreSliceTieBreakSourceTests guards only cover Lite and Darling, so nothing catches this pair.

Given SqlServerFactCollector.cs still actively calls both of these (not dead code) and the Dashboard app is getting other fixes in this same release per the CHANGELOG, this seems worth either patching alongside the proc fix or filing next to #1912 as a known residual, so it doesn't get lost as "the same defect, same fix" claim implies broader coverage than these two sites got.

No other correctness, security, or Lite/Darling parity issues found in the diff itself. Not flagging missing-index recommendations per your standing note.

[claude]

Review: Combine Query Store's flushed and in-memory interval slices at collection (#1907)

Overview

This is a genuine correctness fix. sys.query_store_runtime_stats splits one interval into additive flushed/in-memory slices, and the collector previously stored both — which then collided on the entire read-side dedup key and collection_time, so ROW_NUMBER() and the rollups' last() picked whichever slice the engine happened to emit first instead of summing them. The fix centralizes slice-combination in the one shared T-SQL body (QueryStoreCollector.BuildPayloadBody) both Lite and Darling execute, using SUM/count-weighted-AVG/MIN/MAX grouped on the view's natural key, moves the incremental cutoff from a per-slice WHERE to an interval-grain HAVING, and adds a deterministic execution_count DESC tie-break to every read-side dedup site in both apps for legacy rows that can't be rewritten. The deprecated Dashboard's collector proc gets the identical write-side fix.

What's solid

• The core fix lives in one shared method used by both apps (PerformanceMonitor.Collectors/QueryStoreCollector.cs), which eliminates most Lite/Darling parity risk for the write-side half of this change.
• The weighted-average math (SUM(avg * count) / NULLIF(SUM(count), 0)) is correct — Query Store never stores a per-slice total, so this is the only way to recover one, and min_dop/max_dop (the columns with no avg_ sibling) correctly stay plain MIN/MAX rather than being swept into the weighted-average helper.
• Moving the incremental cutoff from WHERE to HAVING is the right call, and the IN (...) pre-filter is provably a superset-only prune (its interval list can never subtract a row the HAVING would otherwise keep), so it's a pure performance optimization, not a semantic change.
• The execution_count DESC read-side tie-break is applied consistently across all 12 Darling sites and 7 Lite sites, and both apps get a source-guard test (QueryStoreSliceTieBreakSourceTests) that enumerates every file + site count, so a future edit dropping the tie-break — or a new dedup site that omits it — fails loudly instead of silently regressing.
• Test coverage is unusually thorough: SQL-shape pins that discover avg_* columns from the emitted text (so a newly added column is covered automatically), a live-SQL-Server verification pass across all three version shapes, and a live-PG/TimescaleDB rollup test that seeds both the pre-fix and post-fix row shapes in one store to prove the rollup arithmetic actually changed.

Issue: the deprecated Dashboard's write-side fix isn't matched by its own read side

The PR explicitly applies "the same fix" to the deprecated Dashboard's collector (install/09_collect_query_store.sql), but two of the Dashboard's own Query Store dedup reads were not given the corresponding execution_count DESC tie-break that every Lite and Darling dedup site received in this PR:

• deprecated/Dashboard/Analysis/SqlServerFactCollector.QueryPerf.cs (~line 238) — ROW_NUMBER() OVER (PARTITION BY database_name, query_id, plan_id, server_first_execution_time ORDER BY collection_time DESC) over collect.query_store_data
• deprecated/Dashboard/Analysis/SqlServerDrillDownCollector.Queries.cs (~line 511) — the same partition/order shape, same table

These are the Dashboard's counterpart to the exact class of dedup site fixed everywhere else (note they even use server_first_execution_time as a tier-1-style proxy key — the same key shape called out in the PR description as sharing full exposure with collection_time). Since the collector fix only prevents new split-slice rows from being stored, any row the deprecated Dashboard already collected (or collects between now and whenever these two reads are fixed) will keep resolving non-deterministically through these two queries — the same "8 reported where 94 is true, a different wrong number next run" symptom this PR fixes everywhere else. The new deprecated/Installer.Tests/QueryStoreSliceAggregationSqlTests.cs only guards the write-side SQL in install/09_collect_query_store.sql; nothing in this PR guards these two C# read sites, so the gap won't be caught by CI.

Left inline comments on both spots. If the deprecated Dashboard is still receiving bug fixes for this defect class (the install-script change shows it is), it seems worth completing — either add the same tie-break there, or note explicitly why the read side is being left out of scope.

Minor / non-blocking

• install/09_collect_query_store.sql keeps its pre-existing >= cutoff comparison while the shared QueryStoreCollector body uses >. That predates this PR and isn't something it needs to reconcile — just noting it's not a new inconsistency.
• HAVING MAX(qsrs.last_execution_time) > @cutoff_time is logically implied by the IN (...) pre-filter (any interval reaching the GROUP BY already had at least one row pass that test, and MAX over a superset of that interval's rows can only be ≥ it) — the PR frames it as defensive/documentary rather than load-bearing on its own, which reads as intentional, just flagging for confirmation.

No SQL injection, secrets, or file/network/process concerns — this is pure T-SQL query construction over internal collector context, consistent with the existing QUOTENAME-guarded dynamic-SQL pattern already used elsewhere in these files. Per the review instructions, I did not flag missing-index recommendations.

[claude]

(Follow-up: GitHub won't let me attach inline comments to deprecated/Dashboard/Analysis/SqlServerFactCollector.QueryPerf.cs:238 and deprecated/Dashboard/Analysis/SqlServerDrillDownCollector.Queries.cs:511 since neither file is part of this PR's diff — that's expected; the finding above still applies at those two line numbers on main/dev.)