From eed534e9a974656d36aa336e0cb47132d8e9036a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:36:42 -0400 Subject: [PATCH 1/3] Combine Query Store's flushed and in-memory interval slices at collection 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 (#1841/#1845/#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 #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 --- CHANGELOG.md | 14 ++ Darling/Darling.Tests/DarlingComposeTests.cs | 7 +- .../QueryStoreCorrectedRollupLiveTests.cs | 151 +++++++++++++ .../QueryStoreSliceTieBreakSourceTests.cs | 129 ++++++++++++ .../PgDrillDownCollector.Queries.cs | 2 +- .../PgFactCollector.QueryPerf.cs | 2 +- .../Compose/ComposeCompiler.cs | 2 +- .../Mcp/DarlingDataReader.cs | 2 +- .../ViewerDataService.ItemTimeline.cs | 2 +- .../ViewerDataService.QueryStore.cs | 23 +- ...ViewerDataService.QueryStoreRegressions.cs | 4 +- .../ViewerDataService.QueryTrends.cs | 2 +- .../QueryStoreCollectorDefinitionTests.cs | 198 +++++++++++++++++- Lite.Tests/QueryStoreDedupReadTests.cs | 65 +++++- Lite/Analysis/DrillDownCollector.Queries.cs | 2 +- .../Analysis/DuckDbFactCollector.QueryPerf.cs | 2 +- Lite/Services/LocalDataService.QueryStore.cs | 22 +- .../QueryStoreCollector.cs | 176 +++++++++++++++- install/09_collect_query_store.sql | 149 ++++++++++++- 19 files changed, 922 insertions(+), 32 deletions(-) create mode 100644 Darling/Darling.Tests/QueryStoreSliceTieBreakSourceTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index a6a53a2d3..4e27b8dfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Query Store execution counts stopped reporting a sliver of an interval instead of the whole thing** ([#1907]) - `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 ([#1841]/[#1845]/[#1853]) **and** `collection_time`, which meant 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. The survivor was whichever the engine happened to emit first. Live on Azure SQL Database that showed as **8 executions reported where 94 was true**, on two of five rows, with a different two wrong on the next run - not a stale number but an arbitrary fraction of one. Query Store's default 900-second flush against its default 3600-second interval means one interval can hold several flushed slices, so the split is not bounded at two, and it is not an Azure peculiarity: it reproduces on box SQL Server 2022 (16.0.4255.1), where 100 executions flushed plus 25 in memory came back as two rows while `sys.dm_exec_procedure_stats` - a wholly separate source, read at the same instant - reported 125. + + **The slices are now combined at COLLECTION**, in the one payload body both the on-prem and Azure execution shapes run, grouped on exactly the natural key of the view (`plan_id`, `runtime_stats_interval_id`, `execution_type`, replica group) so one interval yields at most one row per cycle. Read-side aggregation was not an option and is not a matter of taste: it would mean teaching every consumer the difference between "same interval, later cycle" (keep the latest) and "same interval, same cycle" (add them), and a TimescaleDB continuous aggregate cannot express that at all. `execution_count` SUMs; every `avg_*` column takes the count-WEIGHTED mean, because Query Store stores an average and a count but never a total, so `avg * count` is what recovers a slice's total - a plain average of the slice averages would weight a 25-execution sliver the same as a 100-execution flush. `min_*` and `max_*` take the extreme, `first_execution_time`/`last_execution_time` the interval's own span. Verified live: the emitted collector SQL returns 125 and 70 where the raw view holds `{100, 25}` and `{60, 10}`, matching `dm_exec_procedure_stats` exactly, and the weighted mean lands on 1871.856 for slices of (1778.42 over 100) and (2245.60 over 25) - which is `(1778.42*100 + 2245.60*25) / 125` and is not the 2012.01 an unweighted average would have given. + + **The incremental cutoff had to move from a per-slice `WHERE` to an interval-grain `HAVING`, and that is load-bearing rather than 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 whether the INTERVAL saw new activity and then takes all of it, which is strictly more permissive than the predicate it replaces, so nothing that used to be collected stops being. **The fixed query is FASTER than the one it replaces**, despite the added aggregate: measured on a real 212,000-row Query Store with the full 55-column payload, 375/422/438 ms against 453/485/516 ms, because half the rows means half the `nvarchar(max)` query text and plan XML to materialize and ship. That depends on the interval pre-filter, which is a prune and not a semantic - its interval list is by construction a superset of what the `HAVING` keeps, so it cannot subtract a row - and without it the aggregate runs over the database's entire retained Query Store every cycle and costs 1203 ms instead. + + **No stored row shape changed**: the same 55 columns in the same order, the same positional writers, no migration and no storage-version bump - only the number of rows per interval. The `TOP` backstop now caps intervals rather than slices, which is also why it sits outside the aggregate: a cap falling mid-interval would emit a partial sum, which is worse than omitting the interval. The deprecated Dashboard's `collect.query_store_collector` had the identical defect against the identical view and takes the identical fix (it is a proc body change, so an upgrade re-applies it with no schema step); its grouping key carries `replica_group_id` under a 2022+/Azure gate for the same bind-safety reason the collector's does, since naming a column that does not exist in a `GROUP BY` fails the whole batch rather than yielding a NULL. + + **Rows already collected cannot be repaired, and are handled honestly rather than quietly.** 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 between runs. That is closest-available, not correct; the correct value is the sum, and the materialized rollups cannot be tie-broken at all because `last()` has no tie-break and the tied rows are gone once materialized. The residual, including the fact that the indefinitely-kept daily tier keeps understated counts for the pre-fix period, is tracked as [#1912]. Verified end to end against live PostgreSQL 18 + TimescaleDB 2.28.1: the corrected rollups report the hand-computed 155 for an hour of two combined intervals and the exact execution-weighted mean off the same rows, while the same store fed the pre-fix split slices reports a single slice and can never reach the 125 they add up to. + - **A cross-database lock now names the database the contended object actually lives in, from both blocking collectors** ([#1893]) - [#1876] made the two collectors' `contentious_object` labels agree at the incident identity, and corrected the `blocked_process_report` sentinel to name the lock RESOURCE's database rather than the blocked session's. The DMV snapshot side could not follow: its normalization runs in C# against a stored row whose only database name is `database_name`, which the collector writes as the blocked SESSION's database. A cross-database lock is held in one database by a session running in another, so for exactly that case the two sides still disagreed - the report row said `database: StackOverflow2013` where the snapshot row said `database: master` - and one contended object still raised two alerts. **The resource's database id comes from `sys.dm_os_waiting_tasks.resource_description`, not from the wait resource.** MS Learn documents every lock resource type's description as carrying a `dbid=` token - `keylock`, `pagelock`, `ridlock`, `objectlock`, `databaselock`, `filelock`, `extentlock`, `applicationlock`, `metadatalock`, `hobtlock` and `allocunitlock` - so a single parse covers every lock shape, where splitting `wait_resource` positionally would need a branch per resource type and would still miss the ones whose layout differs. `DB_NAME()` then runs in the same query, which is the whole reason this had to move server-side: the id was always sitting in the row, but nothing downstream could turn it into a name. Verified live on SQL Server 2022 against a genuine cross-database KEY lock, whose description reads `keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...`; the same blocking pair produced `Unresolved: key lock, database: pm1893_res` from both collectors, byte for byte, where before the fix the snapshot row carried the raw `KEY: 11:72057594045726720 (8194443284a0)` and could only ever have been named with the session's database. @@ -2175,7 +2185,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1899]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1899 [#1889]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1889 [#1893]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1893 +[#1845]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1845 +[#1853]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1853 [#1873]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1873 [#1896]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1896 [#1898]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1898 [#1902]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1902 +[#1907]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1907 +[#1912]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1912 diff --git a/Darling/Darling.Tests/DarlingComposeTests.cs b/Darling/Darling.Tests/DarlingComposeTests.cs index 1aa4efdaa..cb4ed405f 100644 --- a/Darling/Darling.Tests/DarlingComposeTests.cs +++ b/Darling/Darling.Tests/DarlingComposeTests.cs @@ -1172,9 +1172,14 @@ is the whole reason it has to be in the partition too. */ ValidPlan("{\"source\":\"query_store_stats\",\"measure\":\"qs_executions\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}"), servers: ["srv-a", "srv-b"]); + /* The execution_count tie-break is the #1907 residual and is pinned as part of the ORDER BY rather + than left to a looser Contains: collection_time alone was not a total order on rows collected + before that fix, where Query Store's flushed and in-memory slices of one interval were stored as + two rows sharing this whole partition AND collection_time. It cannot fire on rows collected since + — the collector combines the slices — so it exists for what is already stored (#1912). */ Assert.Contains( "PARTITION BY server_id, server_name, database_name, query_id, plan_id, runtime_stats_interval_id, " - + "first_execution_time, execution_type_desc, replica_role ORDER BY collection_time DESC", + + "first_execution_time, execution_type_desc, replica_role ORDER BY collection_time DESC, execution_count DESC", sql, StringComparison.Ordinal); Assert.Contains("AS qs_rn", sql, StringComparison.Ordinal); Assert.Contains("WHERE qs_rn = 1", sql, StringComparison.Ordinal); diff --git a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs index da89b0351..c9fcdff53 100644 --- a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs +++ b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs @@ -458,8 +458,159 @@ the removal lands. Shallow is the real precondition and it is what the gate itse "all three consumers now cover L1, so the held purge should have armed itself on this sweep."); } + /// + /// #1907 against the real TimescaleDB: what the corrected rollups compute when an interval arrives as ONE + /// row per collection (the post-fix collector) versus as the two tied slice rows every pre-fix build + /// stored, proving the defect reached the materialized rollup and that the fix resolves it exactly. + /// + /// Query Store returns the flushed and the still-in-memory slice of one runtime_stats_interval_id as + /// two ADDITIVE rows. Stored as-is they share the L1 GROUP BY key AND collection_time, so + /// last(execution_count, collection_time) — the aggregate #1853 probed and chose because a CAGG + /// cannot contain a window function — is ordering by a value identical for both rows. It cannot sum them + /// and it cannot even choose between them. Whatever it returns is a SLICE, so the rollup understates the + /// interval no matter which row wins; on the live Azure store that was 8 reported against 94 true. + /// + /// Both halves are asserted in ONE test on purpose. The post-fix number alone would pass against a + /// build that never had the bug, and the pre-fix number alone says nothing about whether the fix works — + /// it is the pair, in one store on one refresh, that shows the rollup arithmetic actually changed. + /// + [Fact] + public async Task CorrectedRollups_SeeTheWholeInterval_OnlyWhenTheSlicesAreCombinedAtCollection() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #1907 slice-aggregation rollup test (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(connection, null, ct), + "the dev fixture is expected to have TimescaleDB installed"); + await TimescaleSupport.ConvertToHypertablesAsync(connection, null, ct); + + var postFixHour = new DateTime(2026, 5, 6, 9, 0, 0, DateTimeKind.Unspecified); + var preFixHour = postFixHour.AddHours(1); + + /* ── POST-FIX: the collector combines the slices, so each collection contributes exactly one row and + execution_count is the interval's running TOTAL. 40 -> 90 -> 125 is the live SQL 2022 repro's + arithmetic (100 flushed + 25 in memory = 125, cross-checked against dm_exec_procedure_stats) + arriving over three cycles. Honest answer for the interval: its last snapshot, 125. ── */ + await SeedSnapshotsAsync(connection, intervalId: 7001, queryId: 61, planId: 91, + intervalStart: postFixHour, avgDurationUs: 100, avgCpuUs: 50, + snapshots: + [ + (postFixHour.AddMinutes(5), 40L), + (postFixHour.AddMinutes(10), 90L), + (postFixHour.AddMinutes(15), 125L), + ], ct: ct); + + /* A second interval in the same hour, so the hour total is a sum of two deduped intervals rather than + one value that could be right by accident. */ + await SeedSnapshotsAsync(connection, intervalId: 7002, queryId: 62, planId: 92, + intervalStart: postFixHour.AddMinutes(30), avgDurationUs: 200, avgCpuUs: 80, + snapshots: + [ + (postFixHour.AddMinutes(35), 10L), + (postFixHour.AddMinutes(40), 30L), + ], ct: ct); + + /* ── PRE-FIX: the SAME interval and the SAME true total of 125, but stored the way it used to be — + the flushed slice (100) and the in-memory slice (25) as two rows at ONE collection_time. ── */ + await SeedTiedSlicesAsync(connection, intervalId: 7003, queryId: 63, planId: 93, + intervalStart: preFixHour, collectionTime: preFixHour.AddMinutes(5), + sliceCounts: [100L, 25L], avgDurationUs: 100, avgCpuUs: 50, ct: ct); + + await EnsureAggregatesWithoutRefreshPoliciesAsync(connection, ct); + await RefreshAllAsync(connection, ct); + + /* ── 1. L1 collapses each interval to one row. Post-fix that row IS the interval's truth. ── */ + var l1 = await ReadIntervalRowsAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct); + + var combined = Assert.Single(l1, r => r.QueryId == 61); + Assert.Equal(125, combined.ExecutionCount); + Assert.Equal(7001L, combined.IntervalId); + + var second = Assert.Single(l1, r => r.QueryId == 62); + Assert.Equal(30, second.ExecutionCount); + + /* ── 2. THE HAND-COMPUTED HOUR. 125 + 30 = 155, and the weighted mean composes off the same two + deduped rows: (125 x 100 + 30 x 200) / 155. ── */ + var correctedHour = await ReadCompositeAsync(connection, TimescaleSupport.QueryStoreStatsCorrectedHourlyView, postFixHour, ct); + Assert.Equal(155, correctedHour.ExecutionCountSum); + Assert.Equal( + ((125d * 100d) + (30d * 200d)) / 155d, + correctedHour.DurationWeightedSum / correctedHour.ExecutionCountSum, + 6); + + /* ── 3. THE SPLIT SLICES, same store, same refresh. L1 still produces ONE row — the two slices share + its whole grouping key — but the value it carries is one SLICE, never the 125 they add up to. + Which slice is not asserted, because last() has no tie-break and picking one is not something + the engine promises; that it cannot reach 125 is the point, and it is what makes this an + under-count rather than a coin flip with a correct face. ── */ + var split = Assert.Single(l1, r => r.QueryId == 63); + Assert.Equal(7003L, split.IntervalId); + Assert.Contains(split.ExecutionCount, new long[] { 100L, 25L }); + Assert.NotEqual(125, split.ExecutionCount); + + var splitHour = await ReadCompositeAsync(connection, TimescaleSupport.QueryStoreStatsCorrectedHourlyView, preFixHour, ct); + Assert.Equal(split.ExecutionCount, splitHour.ExecutionCountSum); + Assert.True(splitHour.ExecutionCountSum < 125, + $"the split-slice hour must UNDER-report the interval's true 125, got {splitHour.ExecutionCountSum}"); + + /* ── 4. The daily tier inherits both, so the defect and its fix are visible where history is kept + indefinitely — which is exactly why the pre-fix residual needed its own issue (#1912) rather + than an assumption that retention would carry it away. ── */ + var correctedDay = await ReadCompositeAsync(connection, TimescaleSupport.QueryStoreStatsCorrectedDailyView, postFixHour.Date, ct); + Assert.Equal(155 + split.ExecutionCount, correctedDay.ExecutionCountSum); + } + /* ─────────────────────────── seeding + reading helpers ─────────────────────────── */ + /// + /// Plants ONE Query Store interval as the pre-#1907 shape: several slice rows sharing a single + /// collection_time and the entire dedup key, differing only in execution_count — the flushed + /// slice and the still-in-memory slice as sys.query_store_runtime_stats hands them back and as + /// every build before #1907 stored them. + /// + /// Separate from because that helper derives one collection_id per + /// collection_time, which is precisely what cannot be done here: these rows SHARE a collection time, so + /// the id has to come from the slice's position instead. + /// + private static async Task SeedTiedSlicesAsync( + NpgsqlConnection connection, long intervalId, long queryId, long planId, DateTime intervalStart, + DateTime collectionTime, long[] sliceCounts, long avgDurationUs, long avgCpuUs, CancellationToken ct) + { + const string sql = @" +INSERT INTO collect.query_store_stats + (collection_id, collection_time, server_id, server_name, database_name, module_name, query_hash, + query_id, plan_id, execution_type_desc, replica_role, + runtime_stats_interval_id, interval_start_time_utc, first_execution_time, + execution_count, avg_duration_us, avg_cpu_time_us, max_duration_us, max_cpu_time_us) +VALUES + ((extract(epoch FROM $1)::bigint * 100000) + ($2 * 10) + $10, $1, $3, 'SQL01', 'AdventureWorks', 'dbo.GetOrders', '0xABCD', + $4, $5, 'Regular', 'PRIMARY', $2, $6, $6, $7, $8, $9, 900, 400)"; + + for (var slice = 0; slice < sliceCounts.Length; slice++) + { + await using var command = new NpgsqlCommand(sql, connection); + command.Parameters.AddWithValue(collectionTime); + command.Parameters.AddWithValue(intervalId); + command.Parameters.AddWithValue(TestServerId); + command.Parameters.AddWithValue(queryId); + command.Parameters.AddWithValue(planId); + command.Parameters.AddWithValue(intervalStart); + command.Parameters.AddWithValue(sliceCounts[slice]); + command.Parameters.AddWithValue(avgDurationUs); + command.Parameters.AddWithValue(avgCpuUs); + command.Parameters.AddWithValue(slice); + await command.ExecuteNonQueryAsync(ct); + } + } + /// /// Plants one Query Store interval as CUMULATIVE re-collections of the same /// interval — execution_count running 1..n, which is what the collector actually stores when it diff --git a/Darling/Darling.Tests/QueryStoreSliceTieBreakSourceTests.cs b/Darling/Darling.Tests/QueryStoreSliceTieBreakSourceTests.cs new file mode 100644 index 000000000..2ff0620b4 --- /dev/null +++ b/Darling/Darling.Tests/QueryStoreSliceTieBreakSourceTests.cs @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// Source-containment guard for the #1907 tie-break across EVERY Darling Query Store dedup site. +/// +/// Query Store returns the flushed and the still-in-memory slice of one runtime-stats interval as two +/// ADDITIVE rows. Until #1907 the collector stored both, and they then shared the entire read-side dedup key +/// AND collection_time — so ROW_NUMBER() ... ORDER BY collection_time DESC was ordering by a +/// value identical for both rows and the survivor was whichever the engine emitted first. A grid could show +/// an in-memory sliver of 8 where the interval's total was 94, and a different number on the next run. +/// +/// The collector now combines the slices before storing, so rows collected since cannot tie at all. +/// The tie-break is for the rows ALREADY stored, which cannot be rewritten: it resolves them to the FLUSHED +/// slice — the one holding the bulk of the interval's work — deterministically instead of flapping. That is +/// closest-available, not correct; the correct value is the SUM, which no read-side rule can express, and +/// the residual is tracked in #1912. +/// +/// This is a SOURCE guard rather than a set of per-query assertions because the failure it prevents is +/// omission. The existing per-read pins all assert Contains("ORDER BY collection_time DESC"), which a +/// site missing the tie-break still satisfies — so a thirteenth dedup site, or a "simplified" ORDER BY, +/// would ship silently. Counting every partition against every tie-break is what makes that impossible. +/// Lite has the same guard in QueryStoreDedupReadTests; the two together cover both apps, which is +/// the parity this defect class keeps breaking. +/// +public sealed class QueryStoreSliceTieBreakSourceTests +{ + /// + /// Every Darling file holding a Query Store dedup window, with the number of dedup sites in each. + /// Written out rather than globbed so that DELETING a read — or moving one to a new file — fails here + /// and has to be re-declared, instead of quietly reducing the guard's coverage to whatever is left. + /// + private static readonly (string Path, int Sites)[] DedupSites = + [ + (Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.QueryStore.cs"), 4), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.QueryStoreRegressions.cs"), 2), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.QueryTrends.cs"), 1), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.ItemTimeline.cs"), 1), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingDataReader.cs"), 1), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "Compose", "ComposeCompiler.cs"), 1), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Analysis", "PgFactCollector.QueryPerf.cs"), 1), + (Path.Combine("Darling", "PerformanceMonitor.Darling.Analysis", "PgDrillDownCollector.Queries.cs"), 1), + ]; + + [Fact] + public void EveryQueryStoreDedup_OrdersByCollectionTimeThenExecutionCount() + { + /* The ORDER BY of a window whose PARTITION BY carries the interval identity. ComposeCompiler builds + its SQL by C# concatenation with an interpolated time column, so the time term is matched as + either the literal or the placeholder. */ + var dedupOrderBy = new Regex( + @"PARTITION BY[^()]*?runtime_stats_interval_id[^()]*?ORDER BY\s+(? public sealed class QueryStoreCollector : CollectorDefinitionBase { @@ -294,6 +301,12 @@ WHERE actual_state IN (1, 2, 4) /// TOP 200, procedure_stats TOP 150): those curate the "top N"; this only trims a pathological cycle. /// It bounds COUNT, not BYTES — is the primary memory bound — /// and is the same const the host warns on (). + /// + /// Since #1907 the TOP sits OUTSIDE the slice aggregation, so it caps INTERVALS rather + /// than the raw slices an interval decomposes into. That is the more useful unit — a cap that fell + /// mid-interval would have truncated one interval's slices and emitted a partial sum, which is worse + /// than not emitting the interval at all — and it is strictly more generous, since one interval is + /// one row where it used to be several. /// public const int MaxRowsPerDatabase = 50_000; @@ -508,7 +521,27 @@ not need to — both bind identically. /* Build version-conditional column fragments for the Query Store query. None of these contain a single quote, so they splice into the body identically whether the - body stays as written (Azure) or gets quote-doubled for sp_executesql nesting (on-prem). */ + body stays as written (Azure) or gets quote-doubled for sp_executesql nesting (on-prem). + + Each version-gated family now needs TWO fragments (#1907): one INSIDE the slice-aggregating + derived table, which must vanish entirely when the columns do not exist (referencing an + unbound column inside an aggregate fails the whole SELECT exactly as it would outside one), + and one in the OUTER projection, which keeps emitting the typed NULL placeholder at the same + ordinal so the 55-column reader contract never moves. The inner fragments carry a LEADING + comma and sit at the END of the inner select list precisely because they can be empty; the + outer ones keep their original trailing-comma form because they are never empty. */ + string numPhysIoReadsAgg = isNew + ? $",\n {WeightedAverage("avg_num_physical_io_reads")},\n min_num_physical_io_reads = MIN(qsrs.min_num_physical_io_reads),\n max_num_physical_io_reads = MAX(qsrs.max_num_physical_io_reads)" + : ""; + + string logBytesAgg = isNew + ? $",\n {WeightedAverage("avg_log_bytes_used")},\n min_log_bytes_used = MIN(qsrs.min_log_bytes_used),\n max_log_bytes_used = MAX(qsrs.max_log_bytes_used)" + : ""; + + string tempdbAgg = isNew + ? $",\n {WeightedAverage("avg_tempdb_space_used")},\n min_tempdb_space_used = MIN(qsrs.min_tempdb_space_used),\n max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)" + : ""; + string numPhysIoReadsCols = isNew ? "qsrs.avg_num_physical_io_reads, qsrs.min_num_physical_io_reads, qsrs.max_num_physical_io_reads," : "avg_num_physical_io_reads = NULL, min_num_physical_io_reads = NULL, max_num_physical_io_reads = NULL,"; @@ -560,6 +593,20 @@ the separator. */ ? "LEFT JOIN sys.query_store_replicas AS qsr\n ON qsr.replica_group_id = qsrs.replica_group_id" : ""; + /* replica_group_id is part of sys.query_store_runtime_stats' natural key, so it belongs in the + slice-aggregation grouping (#1907) — two replicas' rows for one interval are DIFFERENT work, + not slices of the same work, and summing them together would blend a secondary's executions + into the primary's, re-creating by hand the exact bug replica attribution exists to prevent. + It carries the SAME 2022+/Azure gate as the attribution column above, and for the same + bind-safety reason: the column does not exist on older servers, and naming it in a GROUP BY + fails the whole SELECT just as naming it in a select list would. When the gate is off there is + only ever one replica group to begin with, so dropping it from the key changes no grouping. + Leading comma: it splices into both the inner select list and the GROUP BY, and is empty on + targets without the column. */ + string replicaGroupKey = hasReplicaAttribution + ? ",\n qsrs.replica_group_id" + : ""; + /* There is deliberately NO self-exclusion predicate in this query (#1565, actual-plan evidence from a 103k-row burst). The old form (query_sql_text NOT LIKE N'%marker%') was 75% of the query's total elapsed time — a per-row substring scan over full nvarchar(max) text (11.2s of @@ -590,6 +637,65 @@ that trimmed an interval row out from under us would silently delete real collec AT TIME ZONE is SQL Server 2016+, matching the floor above, and the expression contains no single quote... except the timezone literal, which quote-doubles cleanly for the sp_executesql nesting exactly like the rest of the body. */ + + /* Slice aggregation (#1907) — the derived table below, and the reason this query has one. + + sys.query_store_runtime_stats returns the FLUSHED slice and the still-IN-MEMORY slice of one + runtime_stats_interval_id as SEPARATE ROWS, and they are ADDITIVE members of one interval, not + competing snapshots of it. Verified on box SQL Server 2022 (16.0.4255.1) as well as the Azure + SQL Database where it was found: 100 executions flushed + 25 executions in memory came back as + two rows, and sys.dm_exec_procedure_stats — an entirely separate source, same instant — + reported 125. SUM matches; the larger slice alone (100) does not. With a 900s default flush + against a 3600s default interval, ONE interval can hold several flushed slices, so the count + is not bounded at two. + + Selecting them straight through stored both, and they then shared every column of the + read-side dedup key (#1841/#1845/#1853) AND collection_time, so the ROW_NUMBER survivor and the + CAGGs' last() were decided by whichever row the engine happened to emit first — a grid could + show the in-memory sliver (8) where the interval's truth was 94. 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. + + So the slices are combined HERE, where the identity is unambiguous, keyed on exactly the + natural key of the view — (plan_id, runtime_stats_interval_id, execution_type, replica_group) — + and one interval now yields at most one row per cycle. The EMITTED ROW SHAPE is unchanged: the + same 55 columns in the same order, only fewer rows, so the positional writers and every + downstream reader are untouched. + + How each column combines: + - count_executions SUM — the additive counter itself. + - avg_* the count-WEIGHTED mean, SUM(avg * count) / SUM(count). Query Store + stores avg and count, never a total, so avg * count reconstructs + each slice's total exactly and the quotient is the interval's true + average. A plain AVG() of the slice averages would weight a 25- + execution sliver equally with a 100-execution flush. NULLIF guards + the divide-by-zero rather than letting a zero-execution row (which + should not exist, and would still not be worth failing a whole + database's collection over) raise 8134. + - min_* / max_* MIN / MAX — extremes over a union of slices are the extremes of the + slice extremes. Includes min_dop / max_dop, which have no avg. + - first_execution_time MIN, last_execution_time MAX — the interval's own span. Both slices + of a pair share first_execution_time in practice, which is exactly + why the tier-1 proxy key could not tell them apart either. + + The incremental filter moves from WHERE to HAVING, and that is load-bearing rather than + cosmetic. A per-slice WHERE would break the SUM within one cycle of the fix: the flushed slice + is STATIC, so once the growing in-memory slice pushes the watermark past the flushed slice's + last_execution_time, the flushed slice stops qualifying and the "sum" becomes the sliver alone + — the original bug with extra steps. HAVING MAX(last_execution_time) > @cutoff_time asks the + question at interval grain: has this interval seen new activity, and if so give me ALL of it. + It is strictly more permissive than the old per-slice predicate, so nothing that used to be + collected stops being collected. + + The IN (...) pre-filter is a performance prune, not a semantic one — the HAVING already gives + the exact answer, and the pre-filter's interval list is by construction a superset of the + intervals the HAVING can keep, so it can never subtract a row. It is here because without it + the aggregate has to run over the database's ENTIRE retained Query Store every cycle, which is + the one shape that made this materially slower. Measured on a real 212k-row Query Store + (SQL 2025), full 55-column payload, warm, three runs: pre-fix 453/485/516 ms for 510 rows; + post-fix 375/422/438 ms for 262 rows; post-fix WITHOUT the pre-filter 1203/1203/1235 ms. The + fixed query is FASTER than the one it replaces despite the added aggregate, because half the + rows means half the nvarchar(max) query text and plan XML to materialize and ship. */ return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; SELECT /* PerformanceMonitorLite */ TOP ({MaxRowsPerDatabase}) @@ -649,7 +755,56 @@ ELSE COALESCE( {replicaRoleCol}, runtime_stats_interval_id = qsrs.runtime_stats_interval_id, interval_start_time_utc = CONVERT(datetime2, qsrsi.start_time AT TIME ZONE 'UTC') -FROM sys.query_store_runtime_stats AS qsrs +FROM +( + SELECT + qsrs.plan_id, + qsrs.runtime_stats_interval_id, + qsrs.execution_type_desc{replicaGroupKey}, + first_execution_time = MIN(qsrs.first_execution_time), + last_execution_time = MAX(qsrs.last_execution_time), + count_executions = SUM(qsrs.count_executions), + {WeightedAverage("avg_duration")}, + min_duration = MIN(qsrs.min_duration), + max_duration = MAX(qsrs.max_duration), + {WeightedAverage("avg_cpu_time")}, + min_cpu_time = MIN(qsrs.min_cpu_time), + max_cpu_time = MAX(qsrs.max_cpu_time), + {WeightedAverage("avg_logical_io_reads")}, + min_logical_io_reads = MIN(qsrs.min_logical_io_reads), + max_logical_io_reads = MAX(qsrs.max_logical_io_reads), + {WeightedAverage("avg_logical_io_writes")}, + min_logical_io_writes = MIN(qsrs.min_logical_io_writes), + max_logical_io_writes = MAX(qsrs.max_logical_io_writes), + {WeightedAverage("avg_physical_io_reads")}, + min_physical_io_reads = MIN(qsrs.min_physical_io_reads), + max_physical_io_reads = MAX(qsrs.max_physical_io_reads), + {WeightedAverage("avg_clr_time")}, + min_clr_time = MIN(qsrs.min_clr_time), + max_clr_time = MAX(qsrs.max_clr_time), + min_dop = MIN(qsrs.min_dop), + max_dop = MAX(qsrs.max_dop), + {WeightedAverage("avg_query_max_used_memory")}, + min_query_max_used_memory = MIN(qsrs.min_query_max_used_memory), + max_query_max_used_memory = MAX(qsrs.max_query_max_used_memory), + {WeightedAverage("avg_rowcount")}, + min_rowcount = MIN(qsrs.min_rowcount), + max_rowcount = MAX(qsrs.max_rowcount){numPhysIoReadsAgg}{logBytesAgg}{tempdbAgg} + FROM sys.query_store_runtime_stats AS qsrs + WHERE qsrs.runtime_stats_interval_id IN + ( + SELECT + f.runtime_stats_interval_id + FROM sys.query_store_runtime_stats AS f + WHERE f.last_execution_time > @cutoff_time + ) + GROUP BY + qsrs.plan_id, + qsrs.runtime_stats_interval_id, + qsrs.execution_type_desc{replicaGroupKey} + HAVING + MAX(qsrs.last_execution_time) > @cutoff_time +) AS qsrs JOIN sys.query_store_plan AS qsp ON qsp.plan_id = qsrs.plan_id JOIN sys.query_store_query AS qsq @@ -659,11 +814,26 @@ JOIN sys.query_store_query_text AS qst LEFT JOIN sys.query_store_runtime_stats_interval AS qsrsi ON qsrsi.runtime_stats_interval_id = qsrs.runtime_stats_interval_id {replicaJoin} -WHERE qsrs.last_execution_time > @cutoff_time ORDER BY qsrs.last_execution_time DESC OPTION(RECOMPILE, LOOP JOIN);"; } + /// + /// One slice-combining avg_* column for the aggregation in + /// (#1907): the count-WEIGHTED mean of the slices, aliased back to the column's own name so the + /// outer projection's reference does not change. + /// + /// Every avg_* column in the payload goes through here rather than being written out + /// by hand, because the wrong form is not a compile error and not obviously wrong at a glance — + /// a bare AVG(qsrs.avg_x) reads fine and silently weights a 25-execution sliver the same as a + /// 100-execution flush. Query Store exposes an average and a count but never a total, so + /// avg * count is how a slice's total is recovered, and the quotient of the summed totals + /// over the summed counts is the interval's true average. Pinned by test: every avg_ column + /// the payload emits must match this shape. + /// + private static string WeightedAverage(string column) => + $"{column} = SUM(qsrs.{column} * qsrs.count_executions) / NULLIF(SUM(qsrs.count_executions), 0)"; + /// /// The incremental cutoff both paths bind as @cutoff_time: only runtime_stats intervals /// newer than what this database already has are fetched. Falls back to 60 minutes back when the diff --git a/install/09_collect_query_store.sql b/install/09_collect_query_store.sql index ae41400c0..7ce3de6d2 100644 --- a/install/09_collect_query_store.sql +++ b/install/09_collect_query_store.sql @@ -51,6 +51,7 @@ BEGIN @error_message nvarchar(4000), @new bit = 0, @plan_type_available bit = 0, /*plan_type_desc requires SQL Server 2022+*/ + @replica_group_available bit = 0, /*replica_group_id requires SQL Server 2022+ (#1907)*/ @engine integer = CONVERT ( @@ -97,6 +98,28 @@ BEGIN @plan_type_available = 1; END; + /* + @replica_group_available = 1 where sys.query_store_runtime_stats.replica_group_id exists: + SQL Server 2022+ (version 16+), and Azure SQL Database (engine 5), which under-reports + PRODUCTVERSION as 12 while running an evergreen engine. Managed Instance (engine 8) keeps the + pure version gate. Same gate, same reasoning, as QueryStoreCollector.hasReplicaAttribution. + + Controls only the slice-aggregation grouping key below (#1907) — this proc does not collect the + replica attribution itself. The column must be in that key where it exists: two replicas' rows + for one interval are DIFFERENT work under "Query Store for secondary replicas", and grouping + without it would SUM a secondary's executions into the primary's. Naming a column that does not + exist in a GROUP BY fails the whole batch, hence the gate. + */ + IF + ( + @product_version >= 16 + OR @engine = 5 + ) + BEGIN + SELECT + @replica_group_available = 1; + END; + BEGIN TRY /* @@ -622,8 +645,129 @@ BEGIN PATH(''compilation_metrics''), TYPE ), - query_plan_hash = p.query_plan_hash - FROM ' + QUOTENAME(@database_name) + N'.sys.query_store_runtime_stats AS rs + query_plan_hash = p.query_plan_hash'; + + /* + #1907: combine the slices of one runtime-stats interval BEFORE anything reads them. + + sys.query_store_runtime_stats returns the FLUSHED slice and the still-IN-MEMORY slice of one + runtime_stats_interval_id as SEPARATE rows, and they are ADDITIVE members of one interval, not + competing snapshots of it. Verified on SQL Server 2022 (16.0.4255.1): 100 executions flushed + plus 25 in memory came back as two rows, while sys.dm_exec_procedure_stats — a separate source + read at the same instant — reported 125. With a 900s default flush against a 3600s default + interval one interval can hold several flushed slices, so the count is not bounded at two. + + Selecting them straight through stored both, so the Dashboard grids showed a fraction of an + interval's work wherever the reads picked one slice. Grouping on the natural key of the view + makes one interval yield at most one row per collection. The emitted column list is unchanged. + + avg_* columns take the count-WEIGHTED mean: Query Store stores an average and a count but never + a total, so avg * count recovers each slice's total and the quotient of the sums is the + interval's true average. A plain AVG() of slice averages would weight a 25-execution sliver + equally with a 100-execution flush. min_* / max_* take the extreme, and first/last execution + time the interval's own span. + + The cutoff moves from WHERE to HAVING deliberately. A per-slice WHERE cannot survive the + aggregation: the flushed slice is STATIC, so once the growing in-memory slice pushes the + watermark past it the flushed slice stops qualifying and the sum degrades to the sliver alone. + The IN (...) pre-filter is a prune only — its interval list is a superset of what the HAVING + keeps, so it cannot subtract a row — and it is what stops the aggregate running over the whole + retained Query Store every cycle. + + Mirrors QueryStoreCollector.BuildPayloadBody, which is where this defect was fixed for Lite and + Darling. Kept in step with it deliberately: this is the deprecated Dashboard proc, but it reads + the same view and had the same bug. + */ + SET @sql += N' + FROM + ( + SELECT + rs.plan_id, + rs.runtime_stats_interval_id, + rs.execution_type_desc,'; + + IF @replica_group_available = 1 + BEGIN + SET @sql += N' + rs.replica_group_id,'; + END; + + SET @sql += N' + first_execution_time = MIN(rs.first_execution_time), + last_execution_time = MAX(rs.last_execution_time), + count_executions = SUM(rs.count_executions), + avg_duration = SUM(rs.avg_duration * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_duration = MIN(rs.min_duration), + max_duration = MAX(rs.max_duration), + avg_cpu_time = SUM(rs.avg_cpu_time * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_cpu_time = MIN(rs.min_cpu_time), + max_cpu_time = MAX(rs.max_cpu_time), + avg_logical_io_reads = SUM(rs.avg_logical_io_reads * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_logical_io_reads = MIN(rs.min_logical_io_reads), + max_logical_io_reads = MAX(rs.max_logical_io_reads), + avg_logical_io_writes = SUM(rs.avg_logical_io_writes * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_logical_io_writes = MIN(rs.min_logical_io_writes), + max_logical_io_writes = MAX(rs.max_logical_io_writes), + avg_physical_io_reads = SUM(rs.avg_physical_io_reads * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_physical_io_reads = MIN(rs.min_physical_io_reads), + max_physical_io_reads = MAX(rs.max_physical_io_reads), + avg_clr_time = SUM(rs.avg_clr_time * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_clr_time = MIN(rs.min_clr_time), + max_clr_time = MAX(rs.max_clr_time), + avg_query_max_used_memory = SUM(rs.avg_query_max_used_memory * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_query_max_used_memory = MIN(rs.min_query_max_used_memory), + max_query_max_used_memory = MAX(rs.max_query_max_used_memory), + avg_rowcount = SUM(rs.avg_rowcount * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_rowcount = MIN(rs.min_rowcount), + max_rowcount = MAX(rs.max_rowcount),'; + + /* + The 2017+ families, aggregated. These must VANISH rather than emit a NULL placeholder when the + columns do not exist: an unbound column inside an aggregate fails the batch exactly as it would + outside one. The outer projection above still emits its NULL placeholders, so the collected + column list does not move. min_dop / max_dop follow them, ungated, so this fragment can keep the + file''s trailing-comma style and still be omitted whole. + */ + IF @new = 1 + BEGIN + SET @sql += N' + avg_num_physical_io_reads = SUM(rs.avg_num_physical_io_reads * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_num_physical_io_reads = MIN(rs.min_num_physical_io_reads), + max_num_physical_io_reads = MAX(rs.max_num_physical_io_reads), + avg_log_bytes_used = SUM(rs.avg_log_bytes_used * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_log_bytes_used = MIN(rs.min_log_bytes_used), + max_log_bytes_used = MAX(rs.max_log_bytes_used), + avg_tempdb_space_used = SUM(rs.avg_tempdb_space_used * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0), + min_tempdb_space_used = MIN(rs.min_tempdb_space_used), + max_tempdb_space_used = MAX(rs.max_tempdb_space_used),'; + END; + + SET @sql += N' + min_dop = MIN(rs.min_dop), + max_dop = MAX(rs.max_dop) + FROM ' + QUOTENAME(@database_name) + N'.sys.query_store_runtime_stats AS rs + WHERE rs.runtime_stats_interval_id IN + ( + SELECT + f.runtime_stats_interval_id + FROM ' + QUOTENAME(@database_name) + N'.sys.query_store_runtime_stats AS f + WHERE f.last_execution_time >= @cutoff_time + ) + GROUP BY + rs.plan_id, + rs.runtime_stats_interval_id, + rs.execution_type_desc'; + + IF @replica_group_available = 1 + BEGIN + SET @sql += N', + rs.replica_group_id'; + END; + + SET @sql += N' + HAVING + MAX(rs.last_execution_time) >= @cutoff_time + ) AS rs JOIN ' + QUOTENAME(@database_name) + N'.sys.query_store_plan AS p ON p.plan_id = rs.plan_id JOIN ' + QUOTENAME(@database_name) + N'.sys.query_store_query AS q @@ -632,7 +776,6 @@ BEGIN ON qt.query_text_id = q.query_text_id LEFT JOIN #objects AS o ON q.object_id = o.object_id - WHERE rs.last_execution_time >= @cutoff_time ORDER BY rs.last_execution_time DESC;'; IF @debug = 1 From 640e35713a5996de9b97d3346eb24004dd051aad Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:57:47 -0400 Subject: [PATCH 2/3] Review follow-ups: strip a stray BOM, guard the Dashboard proc's aggregation Both from the PR review on #1919. - QueryStoreCorrectedRollupLiveTests.cs picked up a UTF-8 BOM when the merge resolution was scripted. Removed; the file matches the rest of the codebase again. - The Dashboard proc's half of the fix had no automated regression test, and nothing else could have caught one: the aggregation lives inside a dynamically assembled @sql string so the compiler sees nothing, the sql-validation workflow only proves the proc COMPILES, and the tests that would execute it are the DB-touching classes CI filters out. A source guard in Installer.Tests (which CI does run, and which needs no database) pins the grouping key, the weighted mean on every avg_ column discovered FROM THE FILE, the interval-grain HAVING, the absence of the per-slice WHERE that would reintroduce the bug, and the version gate on replica_group_id. Watched red: changing one weighted mean to AVG() fails EveryAveragedColumnUsesTheCountWeightedMean. Installer.Tests 194 passed / 0 failed under CI's own non-DB filter. Co-Authored-By: Claude Fable 5 --- .../QueryStoreCorrectedRollupLiveTests.cs | 2 +- .../QueryStoreSliceAggregationSqlTests.cs | 174 ++++++++++++++++++ 2 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 deprecated/Installer.Tests/QueryStoreSliceAggregationSqlTests.cs diff --git a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs index 155305fb3..758f997eb 100644 --- a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs +++ b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs @@ -1,4 +1,4 @@ -/* +/* * Copyright (c) 2026 Erik Darling, Darling Data LLC * * This file is part of the SQL Server Performance Monitor. diff --git a/deprecated/Installer.Tests/QueryStoreSliceAggregationSqlTests.cs b/deprecated/Installer.Tests/QueryStoreSliceAggregationSqlTests.cs new file mode 100644 index 000000000..669ddecb1 --- /dev/null +++ b/deprecated/Installer.Tests/QueryStoreSliceAggregationSqlTests.cs @@ -0,0 +1,174 @@ +using System.Text.RegularExpressions; + +namespace Installer.Tests; + +/// +/// Source guard for the #1907 slice aggregation in install/09_collect_query_store.sql. +/// +/// sys.query_store_runtime_stats returns the FLUSHED slice and the still-IN-MEMORY slice of one +/// runtime_stats_interval_id as two ADDITIVE rows. Verified on SQL Server 2022 (16.0.4255.1): 100 +/// executions flushed plus 25 in memory came back as two rows while sys.dm_exec_procedure_stats — a +/// separate source read at the same instant — reported 125. The proc used to select them straight through, +/// so the Dashboard grids showed a fraction of an interval's work. +/// +/// This is a SOURCE guard because there is nothing else that could catch a regression here. The +/// aggregation lives inside a dynamically assembled @sql string, so the compiler sees nothing; the +/// sql-validation workflow installs the proc on four SQL Server versions but only proves it COMPILES, +/// which it would with any arithmetic at all; and the tests that would execute it are the DB-touching classes +/// CI deliberately filters out. A guard that reads the file is the only thing standing between this fix and a +/// silent regression, which is exactly the situation the repo's other source-parsing pins exist for. +/// +/// The shared collector's half of the same fix (QueryStoreCollector.BuildPayloadBody, which +/// serves Lite and Darling) is pinned separately and far more thoroughly in +/// Lite.Tests/QueryStoreCollectorDefinitionTests.cs. This file exists so the deprecated Dashboard's +/// copy cannot drift away from it unnoticed. +/// +public class QueryStoreSliceAggregationSqlTests +{ + private static string CollectorSql() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "PerformanceMonitor.sln"))) + { + dir = dir.Parent; + } + + Assert.True(dir is not null, "could not locate the repository root from " + AppContext.BaseDirectory); + + var path = Path.Combine(dir!.FullName, "install", "09_collect_query_store.sql"); + Assert.True(File.Exists(path), "expected the Query Store collector script at " + path); + return File.ReadAllText(path); + } + + /// + /// The slices are combined on the natural key of the view, and the counters combine the way the source's + /// own semantics require: SUM for the additive counter, extremes for the extremes, the interval's span + /// from its slices' span. + /// + [Fact] + public void CollectorGroupsRuntimeStatsOnTheIntervalIdentity() + { + var sql = CollectorSql(); + + Assert.Contains("rs.runtime_stats_interval_id,", sql, StringComparison.Ordinal); + Assert.Contains("count_executions = SUM(rs.count_executions),", sql, StringComparison.Ordinal); + Assert.Contains("first_execution_time = MIN(rs.first_execution_time),", sql, StringComparison.Ordinal); + Assert.Contains("last_execution_time = MAX(rs.last_execution_time),", sql, StringComparison.Ordinal); + + /* min_dop / max_dop are the pair with no avg_ sibling, so they are the pair a mechanical edit is most + likely to hand the wrong aggregate. */ + Assert.Contains("min_dop = MIN(rs.min_dop),", sql, StringComparison.Ordinal); + Assert.Contains("max_dop = MAX(rs.max_dop)", sql, StringComparison.Ordinal); + + Assert.Contains("GROUP BY", sql, StringComparison.Ordinal); + Assert.Matches( + new Regex(@"GROUP BY\s+rs\.plan_id,\s+rs\.runtime_stats_interval_id,\s+rs\.execution_type_desc"), + sql); + } + + /// + /// EVERY avg_* column the aggregate produces must be the count-WEIGHTED mean. + /// + /// Query Store stores an average and a count but never a total, so avg * count is the only + /// way to recover a slice's total. The wrong forms are not errors and do not read as wrong — a bare + /// AVG(rs.avg_duration) looks perfectly natural and silently weights a 25-execution sliver the same + /// as a 100-execution flush. The columns are discovered FROM THE FILE rather than listed here, so a newly + /// added one is covered the moment it appears instead of when someone remembers this test. + /// + [Fact] + public void EveryAveragedColumnUsesTheCountWeightedMean() + { + var sql = CollectorSql(); + + /* Only the aggregating derived table. The outer projection references the same names as plain + columns, which is correct there and must not be read as an un-weighted aggregate. */ + var open = sql.IndexOf("FROM\n (", StringComparison.Ordinal); + if (open < 0) + { + open = sql.IndexOf("FROM\r\n (", StringComparison.Ordinal); + } + + Assert.True(open > 0, "could not locate the slice-aggregating derived table in the collector script"); + + var close = sql.IndexOf(") AS rs", open, StringComparison.Ordinal); + Assert.True(close > open, "the slice-aggregating derived table is not closed with ') AS rs'"); + + var aggregate = sql[open..close]; + + var averages = Regex.Matches(aggregate, @"(avg_[a-z0-9_]+) = ") + .Select(m => m.Groups[1].Value) + .Distinct() + .ToList(); + + /* duration, cpu_time, logical_io_reads, logical_io_writes, physical_io_reads, clr_time, + query_max_used_memory, rowcount, num_physical_io_reads, log_bytes_used, tempdb_space_used. */ + Assert.Equal(11, averages.Count); + + foreach (var column in averages) + { + Assert.Contains( + $"{column} = SUM(rs.{column} * rs.count_executions) / NULLIF(SUM(rs.count_executions), 0),", + aggregate, + StringComparison.Ordinal); + } + + Assert.DoesNotContain("AVG(rs.avg_", aggregate, StringComparison.Ordinal); + Assert.DoesNotContain("MAX(rs.avg_", aggregate, StringComparison.Ordinal); + Assert.DoesNotContain("MIN(rs.avg_", aggregate, StringComparison.Ordinal); + } + + /// + /// The incremental cutoff is asked at INTERVAL grain, never per slice. + /// + /// This is the assertion that would catch the most tempting wrong edit. A per-slice + /// WHERE rs.last_execution_time >= @cutoff_time reads like the obvious filter and reintroduces the + /// whole defect: the flushed slice is STATIC, so once the growing in-memory slice pushes the watermark past + /// it the flushed slice stops qualifying and the SUM silently becomes the sliver alone. + /// + [Fact] + public void TheCutoffIsAppliedToTheIntervalRatherThanTheSlice() + { + var sql = CollectorSql(); + + Assert.Matches(new Regex(@"HAVING\s+MAX\(rs\.last_execution_time\) >= @cutoff_time"), sql); + + /* The pre-filter is a prune, not a semantic: its interval list is a superset of what the HAVING keeps, + so it can never subtract a row, and without it the aggregate runs over the whole retained Query + Store every cycle. */ + Assert.Contains("WHERE rs.runtime_stats_interval_id IN", sql, StringComparison.Ordinal); + Assert.Contains("WHERE f.last_execution_time >= @cutoff_time", sql, StringComparison.Ordinal); + + Assert.DoesNotContain("WHERE rs.last_execution_time >= @cutoff_time", sql, StringComparison.Ordinal); + } + + /// + /// replica_group_id enters the grouping key only where the column exists. + /// + /// Naming a column that does not exist in a GROUP BY fails the whole batch rather than yielding a + /// NULL, so an ungated reference would break Query Store collection outright on every pre-2022 server. + /// 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. + /// + [Fact] + public void ReplicaGroupIdIsGatedBehindItsVersionFlag() + { + var sql = CollectorSql(); + + Assert.Contains("@replica_group_available bit = 0", sql, StringComparison.Ordinal); + + /* Every mention of the column sits inside an IF on that flag — asserted by requiring that the flag is + tested at least as often as the column is named. */ + var columnMentions = Regex.Matches(sql, @"rs\.replica_group_id").Count; + var gateTests = Regex.Matches(sql, @"IF @replica_group_available = 1").Count; + + Assert.True(columnMentions > 0, "the grouping key must carry replica_group_id where it exists"); + Assert.True( + gateTests >= columnMentions, + $"replica_group_id is named {columnMentions} time(s) but the version gate is tested only " + + $"{gateTests} time(s) — an ungated reference fails the whole batch on a pre-2022 server."); + + /* The gate itself must admit Azure SQL Database, which under-reports PRODUCTVERSION as 12 while + running an evergreen engine — the same rule QueryStoreCollector.hasReplicaAttribution uses. */ + Assert.Matches(new Regex(@"@product_version >= 16\s+OR @engine = 5"), sql); + } +} From a66e785faf4830d8d1441af4a30f3d2d2c398024 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 31 Jul 2026 02:36:43 -0400 Subject: [PATCH 3/3] Widen Lite's tie-break guard to every dedup site, not just one file 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 --- .../ViewerDataService.QueryStore.cs | 5 +- .../QueryStoreSliceTieBreakSourceTests.cs | 151 ++++++++++++++++++ Lite/Services/LocalDataService.QueryStore.cs | 5 +- 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs index 833cfefaa..2edbeca93 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryStore.cs @@ -138,7 +138,10 @@ replica_role and execution_type_desc are in the partition because the aggregate rewritten: it deterministically picks the FLUSHED slice, the one holding the bulk of the interval's work, instead of flapping. Closest-available, not correct — the correct value is the SUM of the slices, which no read-side rule can express (#1912). This applies to EVERY dedup - site in both apps; a source-containment test pins all of them, so deleting one fails loudly. */ + site in both apps: Darling.Tests/QueryStoreSliceTieBreakSourceTests pins all 12 of Darling's + across the 8 files that hold them, and Lite.Tests/QueryStoreSliceTieBreakSourceTests pins its + 7, so dropping one fails loudly and names the file. Both enumerate their files rather than + globbing, so MOVING a read has to be re-declared instead of silently shrinking the guard. */ SELECT *, ROW_NUMBER() OVER diff --git a/Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs b/Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs new file mode 100644 index 000000000..4d88013f7 --- /dev/null +++ b/Lite.Tests/QueryStoreSliceTieBreakSourceTests.cs @@ -0,0 +1,151 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace Lite.Tests; + +/// +/// Source-containment guard for the #1907 tie-break across EVERY Lite Query Store dedup site. The exact +/// counterpart of Darling.Tests/QueryStoreSliceTieBreakSourceTests, deliberately built to the same +/// shape so the two apps are guarded the same way rather than one app being guarded and the other trusted. +/// +/// Query Store returns the flushed and the still-in-memory slice of one runtime-stats interval as two +/// ADDITIVE rows. Until #1907 the collector stored both, and they then shared the entire read-side dedup key +/// AND collection_time — so ROW_NUMBER() ... ORDER BY collection_time DESC was ordering by a +/// value identical for both rows and the survivor was whichever the engine emitted first. A grid could show +/// an in-memory sliver of 8 where the interval's total was 94, and a different number on the next run. +/// +/// The collector now combines the slices before storing, so rows collected since cannot tie at all. +/// The tie-break is for the rows ALREADY stored, which cannot be rewritten: it resolves them to the FLUSHED +/// slice — the one holding the bulk of the interval's work — deterministically instead of flapping. That is +/// closest-available, not correct; the correct value is the SUM, and the residual is tracked in #1912. +/// +/// Why this exists ON TOP OF . That test's +/// EveryQueryStoreAggregateInTheFile_CarriesADedupCte reads exactly one file, +/// LocalDataService.QueryStore.cs, which is right for what it checks (the dedup CTEs, the rank +/// filters, the trend's two arms) but leaves 2 of Lite's 7 dedup sites — the ones in +/// Analysis/DrillDownCollector.Queries.cs and Analysis/DuckDbFactCollector.QueryPerf.cs — +/// covered by no test at all. A future edit dropping execution_count DESC from either would have gone +/// uncaught, which is precisely the recurrence mode #1841 / #1845 / #1853 / #1907 keep demonstrating. Caught +/// by review on #1919; the alternative on offer was to soften the source comment to stop claiming coverage +/// that did not exist, which would have been a Lite/Darling parity scope-down. +/// +public sealed class QueryStoreSliceTieBreakSourceTests +{ + /// + /// Every Lite file holding a Query Store dedup window, with the number of dedup sites in each. Written + /// out rather than globbed so that DELETING a read — or moving one to a new file — fails here and has to + /// be re-declared, instead of quietly reducing the guard's coverage to whatever is left. + /// + private static readonly (string Path, int Sites)[] DedupSites = + [ + (Path.Combine("Lite", "Services", "LocalDataService.QueryStore.cs"), 5), + (Path.Combine("Lite", "Analysis", "DrillDownCollector.Queries.cs"), 1), + (Path.Combine("Lite", "Analysis", "DuckDbFactCollector.QueryPerf.cs"), 1), + ]; + + [Fact] + public void EveryQueryStoreDedup_OrdersByCollectionTimeThenExecutionCount() + { + /* The ORDER BY of a window whose PARTITION BY carries the interval identity. Lite's Query Store SQL + is all inline string literals, so there is no constant to pin and the file itself is the surface. */ + var dedupOrderBy = new Regex( + @"PARTITION BY[^()]*?runtime_stats_interval_id[^()]*?ORDER BY\s+collection_time\s+DESC(?,\s*execution_count DESC)?", + RegexOptions.Singleline); + + var untied = new List(); + var total = 0; + + foreach (var (relative, expected) in DedupSites) + { + var source = File.ReadAllText(SourcePath(relative)); + var matches = dedupOrderBy.Matches(source); + + Assert.True( + matches.Count == expected, + $"{relative}: expected {expected} Query Store dedup site(s), found {matches.Count}. " + + "A read was added, removed, or moved — update this guard deliberately rather than letting " + + "its coverage drift."); + + foreach (Match m in matches) + { + total++; + if (!m.Groups["tie"].Success) + { + untied.Add($"{relative} @ char {m.Index}"); + } + } + } + + Assert.True( + untied.Count == 0, + "Query Store dedup sites missing the #1907 execution_count tie-break:\n " + string.Join("\n ", untied)); + + /* The count is asserted as a whole too: a file dropping to zero sites would otherwise be caught only + by its own per-file assertion, and this states the total the fix actually swept. */ + Assert.Equal(7, total); + } + + /// + /// The tie-break must FOLLOW collection_time, never replace it. "Latest" is decided by when a row was + /// collected — an interval's execution_count can sit still across a hundred re-collections (the 496x + /// shape from #1841), so ordering by the count first would keep the stalest snapshot's averages. + /// + [Fact] + public void TheTieBreakNeverBecomesThePrimarySort() + { + foreach (var (relative, _) in DedupSites) + { + var source = File.ReadAllText(SourcePath(relative)); + Assert.DoesNotContain("ORDER BY execution_count", source, StringComparison.Ordinal); + } + } + + /// + /// Lite and Darling must carry the SAME number of guarded dedup sites as their own guards declare, so + /// neither app can quietly fall behind the other. This reads Darling's guard file and compares its + /// declared total against this one's — the parity claim made executable rather than asserted in a + /// comment, since drift between the two apps is the failure this defect class keeps producing. + /// + [Fact] + public void BothAppsGuardTheirOwnDedupSites_SoNeitherSideCanFallBehind() + { + var darlingGuard = SourcePath(Path.Combine("Darling", "Darling.Tests", "QueryStoreSliceTieBreakSourceTests.cs")); + Assert.True(File.Exists(darlingGuard), "Darling's counterpart guard is missing: " + darlingGuard); + + var darlingSource = File.ReadAllText(darlingGuard); + + /* Darling declares its own total the same way this file does. If that guard is deleted or stops + asserting a total, this fails rather than silently becoming a one-sided check. */ + var declared = Regex.Match(darlingSource, @"Assert\.Equal\((?\d+), total\);"); + Assert.True(declared.Success, "Darling's guard no longer declares a total dedup-site count."); + Assert.Equal(12, int.Parse(declared.Groups["total"].Value, System.Globalization.CultureInfo.InvariantCulture)); + + /* And it must still enumerate its files rather than globbing, for the same reason this one does. */ + Assert.Contains("DedupSites", darlingSource, StringComparison.Ordinal); + } + + /// Walks up from the test binary to the repo root so the pin works from any run directory. + private static string SourcePath(string relative) + { + var dir = AppContext.BaseDirectory; + while (dir is not null && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln"))) + { + dir = Path.GetDirectoryName(dir); + } + + Assert.True(dir is not null, "could not locate the repository root from " + AppContext.BaseDirectory); + return Path.Combine(dir!, relative); + } +} diff --git a/Lite/Services/LocalDataService.QueryStore.cs b/Lite/Services/LocalDataService.QueryStore.cs index 3d16cac76..77f0dbcc1 100644 --- a/Lite/Services/LocalDataService.QueryStore.cs +++ b/Lite/Services/LocalDataService.QueryStore.cs @@ -60,7 +60,10 @@ WITH deduped AS -- the store, which cannot be rewritten: it deterministically picks the FLUSHED slice, the one holding the -- bulk of the interval's work, instead of flapping. Closest-available, not correct — the correct value is -- the SUM of the slices, which no read-side rule can express (#1912). This applies to EVERY dedup site in - -- both apps; a source-containment test pins all of them, so deleting one here fails loudly. + -- both apps: Lite.Tests/QueryStoreSliceTieBreakSourceTests pins all 7 of Lite's across the 3 files that + -- hold them, and Darling.Tests/QueryStoreSliceTieBreakSourceTests pins its 12, so dropping one fails + -- loudly and names the file. Both enumerate their files rather than globbing, so MOVING a read has to be + -- re-declared instead of silently shrinking the guard. SELECT collection_time, interval_start_time_utc,