From 534f725b4aafcbd349fc0fcfd6c620bb8262c1d0 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 8 Aug 2026 22:20:38 +0200 Subject: [PATCH 1/6] =?UTF-8?q?Query=20Store=20payload=20stages=20its=20ag?= =?UTF-8?q?gregate=20=E2=80=94=20the=20fixed=20cost=20big=20catalogs=20cou?= =?UTF-8?q?ldn't=20pay=20(#2133)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collector joined its slice aggregate straight into the query_store_plan/query/text TVFs. With only fixed-guess cardinalities to plan from, the optimizer re-materialized a TVF per probe — a fixed ≥30s cost on an 82k-plan catalog that no catch-up window width could reduce, which is why the fleet's big databases pinned at the 15-minute shrink floor while smaller neighbors on the same servers stayed current. Bisected live on the wedged store: aggregate alone 81 ms, each TVF bare ~300 ms, aggregate-JOIN-plan >30s hinted or not. Staged through a temp table: 524 ms core; the full 55-column batch with plan capture completed a one-hour backlog in 21.3 s where the old shape never finished in 60. - SELECT INTO #pm_qs_slice, joins run FROM the temp with real row counts - LOOP JOIN hint removed for good — looping from the temp into the TVFs is the same per-probe re-materialization by another name (pinned) - interval pre-filter resolves ids from the interval catalog (20 ms) instead of scanning runtime_stats (426 ms), same superset semantics - one result set per batch; TIES/watermark/byte-budget unchanged; leading DROP covers Azure pooled reuse, sp_executesql scope self-cleans - shape pins updated: staging, catalog pre-filter, no-LOOP-JOIN Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 + .../QueryStoreCollectorDefinitionTests.cs | 62 +++++--- .../QueryStoreCollector.cs | 135 +++++++++++------- 3 files changed, 125 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23d0f0bb..68994274 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Compression Job Stuck no longer false-alarms on a job it caught mid-run** - measured live on TimescaleDB 2.x: from the moment the scheduler picks up a due job until its run completes, `job_stats.next_start` reads `-infinity` with `job_status = 'Running'`, and the real next start is only computed at completion. The detector's first arm treated `-infinity` unconditionally as "the scheduler will never run it again", so any healthy compression run the self-alert check happened to sample got flagged as stuck, alerted, and "self-healed" with a pointless re-arm - the transient stuck-then-self-healed alert pairs the field has been shrugging off were this false positive, and the stuck-detector live test's CI flake was the same race (it re-arms with `next_start => now()` and then read a single snapshot while the run it had just triggered was still executing). A RUNNING job's `-infinity` now defers to the elapsed-bound arm, which is what actually distinguishes a hung run from a healthy one - a genuinely dead job (`-infinity`, not running) still alerts exactly as before, and a hung run still trips the bound. - **The Job History tab speaks display names** ([#2126], asked by ghauan) - both the Server filter dropdown and the Server column showed the raw collected server name while every other tab shows the operator's alias, so a fleet navigated by aliases turned into a memory quiz on exactly the tab an operator visits during an incident. Both readers (job history and the Agent status header) now resolve through the servers registry - the alias when one exists, the raw name otherwise - so the filter, the column, the per-column filter popup, and the CSV export all speak the same names as the rest of the viewer, and the Agent roll-up sorts by them. Lite's Job History tab had the same gap through a different mechanism (review catch): Lite's display-name concept lives at the CONFIG layer, not in DuckDB (the stored servers.display_name column is unpopulated by design), so the shell now passes a server_id-to-alias snapshot into the tab Overview-style and rows swap in the alias on every refresh - a server no longer in config keeps its raw collected name, the durable-record case. - **The long-query completion XE session actually gets created now** ([#2129], from ghauan's field report on #2061 - they enabled the collector on two servers and the Long Queries tab stayed empty forever) - the session DDL SET a customizable attribute `collect_object_name` on `sqlserver.rpc_completed`, and no such attribute exists on that event on ANY version (it belongs to `sp_statement_completed`) - `object_name` is one of rpc_completed's DEFAULT data fields, collected with no SET at all. So the CREATE failed on every server, the session never existed, and the reconcile's follow-up START surfaced as the confusing second error ('Cannot alter the event session... does not exist'). Never caught in dogfood because the collector ships OFF by design, and the DDL test pin asserted the wrong claim, so CI enforced the bug. The SET is gone (the reader already shreds the default field generically - no reader or table change), and the pin now asserts the attribute is ABSENT, with the story attached. Anyone who flipped the collector on before this fix: it starts working on the next reconcile tick after upgrading, no re-toggle needed. +- **Query Store collection no longer has a fixed cost that big catalogs cannot pay** ([#2133], the actual root cause under the whole catch-up saga - #2102's death spiral, #2111's yield, and #2125's adaptive shrink were all mitigating it) - the collector joined its slice aggregate straight into the query_store_plan/query/text catalog TVFs, handing the optimizer nothing but fixed-guess cardinalities, and the plan it picked re-materialized a TVF per probe: on an 82k-plan catalog that was a fixed 30-second-plus cost that NO catch-up window width could reduce - which is exactly why the fleet's big databases (echo, oak, Surge, spruce, insa...) pinned at the 15-minute shrink floor and never converged while their smaller neighbors on the same servers stayed current. Bisected live: the aggregate alone ran in 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-plan could not finish in 30 s, hinted or not. The payload now STAGES the aggregate in a temp table and joins FROM it - real row counts instead of guesses, each TVF scanned exactly once, sp_QuickieStore's architecture for the same reason - and the old LOOP JOIN hint is gone for good (looping from the temp into the TVFs is the same per-probe re-materialization by another name). The interval pre-filter also resolves ids from the tiny interval catalog now instead of scanning runtime_stats itself (20 ms vs 426 ms, same id set). Measured end to end on the wedged field store: the full 55-column batch with plan capture completed a one-hour backlog in 21.3 s where the old shape never finished inside 60; the staged core is 524 ms. Same batch = one result set, TOP WITH TIES / derived-watermark / byte-budget semantics unchanged, both SKUs, both engine arms, live and backfill. ## [3.4.0] - 2026-08-06 @@ -2634,3 +2635,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2119]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2119 [#2126]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2126 [#2129]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2129 +[#2133]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2133 diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index a24d3aec..0e11af4b 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -192,10 +192,11 @@ ordinals on both paths. */ /* Interval-grain incremental filter since #1907, on this path too — the Azure body IS the shared body, so the WHERE→HAVING move lands here by construction rather than by a second edit. */ - Assert.Contains("HAVING\n MAX(qsrs.last_execution_time) > @cutoff_time", Lf(plan.Text), StringComparison.Ordinal); + Assert.Contains("HAVING\n MAX(qsrs.last_execution_time) > @cutoff_time", Lf(plan.Text), StringComparison.Ordinal); Assert.DoesNotContain("WHERE qsrs.last_execution_time > @cutoff_time", plan.Text, StringComparison.Ordinal); Assert.Contains("ORDER BY qsrs.last_execution_time ASC", plan.Text, StringComparison.Ordinal); - Assert.Contains("OPTION(RECOMPILE, LOOP JOIN);", plan.Text, StringComparison.Ordinal); + Assert.Contains("OPTION(RECOMPILE);", plan.Text, StringComparison.Ordinal); + Assert.DoesNotContain("LOOP JOIN", plan.Text, StringComparison.Ordinal); var parameter = Assert.Single(plan.Parameters); Assert.Equal("@cutoff_time", parameter.Name); @@ -481,20 +482,34 @@ public void BuildPerItemQuery_CombinesTheSlicesOfOneInterval_OnTheViewsNaturalKe Assert.Contains("max_dop = MAX(qsrs.max_dop)", text, StringComparison.Ordinal); /* 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. Without it the aggregate runs over the database's entire - retained Query Store every cycle — measured 1203ms against 375ms on a real 212k-row store. */ + keeps, so it can never subtract a row. #2133: the ids resolve from the INTERVAL CATALOG — + hundreds of rows — never by scanning runtime_stats itself (measured 20 ms vs 426 ms for the + identical id set on the field store that wedged). */ Assert.Contains("WHERE qsrs.runtime_stats_interval_id IN", text, StringComparison.Ordinal); - Assert.Contains("WHERE f.last_execution_time > @cutoff_time", text, StringComparison.Ordinal); - - /* The row SHAPE must not move: 55 selected columns, and the TOP/ORDER BY stay OUTSIDE the - aggregate so the cap counts intervals and can never truncate one interval's slices into a + Assert.Contains("FROM sys.query_store_runtime_stats_interval AS i", text, StringComparison.Ordinal); + Assert.Contains("WHERE i.end_time > @cutoff_time", text, StringComparison.Ordinal); + Assert.DoesNotContain("FROM sys.query_store_runtime_stats AS f", text, StringComparison.Ordinal); + + /* #2133 STAGING: the aggregate lands in a temp table and the plan/query/text joins run FROM it, + so the optimizer joins with real cardinalities instead of TVF fixed guesses — the monolithic + join re-materialized a TVF per probe, a fixed ≥30s cost on an 82k-plan catalog that no + catch-up width could reduce (staged: 524 ms, same store, same window). SELECT INTO emits no + result set, so the batch still returns exactly one; the leading DROP covers Azure's pooled + direct connections (on-prem the sp_executesql scope self-cleans). */ + Assert.Contains("DROP TABLE IF EXISTS #pm_qs_slice;", text, StringComparison.Ordinal); + Assert.Contains("INTO #pm_qs_slice", text, StringComparison.Ordinal); + Assert.Contains("FROM #pm_qs_slice AS qsrs\nJOIN sys.query_store_plan AS qsp", Lf(text), StringComparison.Ordinal); + + /* The row SHAPE must not move: 55 selected columns, and the TOP/ORDER BY stay on the final + SELECT so the cap counts intervals and can never truncate one interval's slices into a partial sum. WITH TIES + ASC are the #1960 never-a-hole pair: oldest-first shipping keeps the derived watermark at the shipped boundary, and WITH TIES stops a bare TOP from splitting a group of rows tied at that boundary — the strict `> @cutoff_time` would strand the - unshipped half forever. */ + unshipped half forever. The LOOP JOIN hint must never return to this query: looping from the + temp into the TVFs is the per-probe re-materialization #2133 removed. */ Assert.Contains($"TOP ({QueryStoreCollector.MaxRowsPerDatabase}) WITH TIES", text, StringComparison.Ordinal); - Assert.Contains(") AS qsrs\nJOIN sys.query_store_plan AS qsp", text, StringComparison.Ordinal); - Assert.Contains("ORDER BY qsrs.last_execution_time ASC\nOPTION(RECOMPILE, LOOP JOIN);", text, StringComparison.Ordinal); + Assert.Contains("ORDER BY qsrs.last_execution_time ASC\nOPTION(RECOMPILE);", Lf(text), StringComparison.Ordinal); + Assert.DoesNotContain("LOOP JOIN", text, StringComparison.Ordinal); } /// @@ -763,7 +778,7 @@ that would have pinned the bug. A per-slice WHERE cannot survive slice aggregati original defect with an aggregate bolted on. HAVING MAX(...) asks whether the INTERVAL saw new activity and then takes all of it. */ var normalized = plan.Text.Replace("\r\n", "\n", StringComparison.Ordinal); - Assert.Contains("HAVING\n MAX(qsrs.last_execution_time) > @cutoff_time", normalized, StringComparison.Ordinal); + Assert.Contains("HAVING\n MAX(qsrs.last_execution_time) > @cutoff_time", normalized, StringComparison.Ordinal); Assert.DoesNotContain("WHERE qsrs.last_execution_time > @cutoff_time", normalized, StringComparison.Ordinal); /* #1565: NO SQL-side self-exclusion — the old NOT LIKE was 75% of the read's elapsed time (full nvarchar(max) scan per row on a column no index can serve; field A/B: 4.3x without it), and no @@ -771,7 +786,8 @@ activity and then takes all of it. */ where the text is already materialized (pinned below). The query still CONTAINS the marker — in its own leading comment. */ Assert.DoesNotContain("NOT LIKE", plan.Text, StringComparison.Ordinal); - Assert.Contains("OPTION(RECOMPILE, LOOP JOIN);", plan.Text, StringComparison.Ordinal); + Assert.Contains("OPTION(RECOMPILE);", plan.Text, StringComparison.Ordinal); + Assert.DoesNotContain("LOOP JOIN", plan.Text, StringComparison.Ordinal); Assert.Contains("N'@cutoff_time datetime2(7)',", plan.Text, StringComparison.Ordinal); var parameter = Assert.Single(plan.Parameters); @@ -823,8 +839,11 @@ TOP from splitting a group of rows tied at that boundary (the strict `> @cutoff_ { Assert.Contains($"TOP ({QueryStoreCollector.MaxRowsPerDatabase}) WITH TIES", plan.Text, StringComparison.Ordinal); Assert.Contains("ORDER BY qsrs.last_execution_time ASC", plan.Text, StringComparison.Ordinal); - /* The row-bounding ORDER BY sits before the existing query hint, which the OPTION pin still checks. */ - Assert.Contains("OPTION(RECOMPILE, LOOP JOIN);", plan.Text, StringComparison.Ordinal); + /* The row-bounding ORDER BY sits before the existing query hint, which the OPTION pin still + checks. RECOMPILE only — the old LOOP JOIN hint is the #2133 pathology (per-probe TVF + re-materialization) and must never return. */ + Assert.Contains("OPTION(RECOMPILE);", plan.Text, StringComparison.Ordinal); + Assert.DoesNotContain("LOOP JOIN", plan.Text, StringComparison.Ordinal); } Assert.Equal(50_000, QueryStoreCollector.MaxRowsPerDatabase); @@ -1082,8 +1101,11 @@ live window instead of the backlog. */ Assert.Contains("EXECUTE [StackOverflow].sys.sp_executesql", plan.Text, StringComparison.Ordinal); Assert.Contains("N'@floor_time datetime2(7), @ceiling_time datetime2(7)'", plan.Text, StringComparison.Ordinal); - Assert.Contains("f.last_execution_time > @floor_time", plan.Text, StringComparison.Ordinal); - Assert.Contains("f.last_execution_time < @ceiling_time", plan.Text, StringComparison.Ordinal); + /* #2133: the pre-filter's two-sided window asks the INTERVAL CATALOG which intervals OVERLAP + (floor, ceiling) — end after the floor AND start before the ceiling — a superset the exact + HAVING below then narrows, exactly like the live path's one-sided form. */ + Assert.Contains("i.end_time > @floor_time", plan.Text, StringComparison.Ordinal); + Assert.Contains("i.start_time < @ceiling_time", plan.Text, StringComparison.Ordinal); Assert.Contains("MAX(qsrs.last_execution_time) > @floor_time", plan.Text, StringComparison.Ordinal); Assert.Contains("MAX(qsrs.last_execution_time) < @ceiling_time", plan.Text, StringComparison.Ordinal); Assert.Contains("ORDER BY qsrs.last_execution_time DESC", plan.Text, StringComparison.Ordinal); @@ -1109,8 +1131,8 @@ own catalog would be worse than a loud wrong-path error. */ var plan = QueryStoreCollector.Instance.BuildBackfillQuery(MakeContext(isAzureSqlDb: true), floor, ceiling); Assert.DoesNotContain("sp_executesql", plan.Text, StringComparison.Ordinal); - Assert.Contains("f.last_execution_time > @floor_time", plan.Text, StringComparison.Ordinal); - Assert.Contains("f.last_execution_time < @ceiling_time", plan.Text, StringComparison.Ordinal); + Assert.Contains("i.end_time > @floor_time", plan.Text, StringComparison.Ordinal); + Assert.Contains("i.start_time < @ceiling_time", plan.Text, StringComparison.Ordinal); Assert.Contains("ORDER BY qsrs.last_execution_time DESC", plan.Text, StringComparison.Ordinal); Assert.DoesNotContain("@cutoff_time", plan.Text, StringComparison.Ordinal); /* The same eligibility gate the live Azure query leads with. */ @@ -1132,7 +1154,7 @@ public void BuildBackfillPerItemQuery_LiveBodyStaysUntouched() one-sided cutoff and ASC order byte-for-byte, or phase 1's watermark-exact resume breaks in the same PR that builds on it. */ var live = QueryStoreCollector.Instance.BuildPerItemQuery("StackOverflow", MakeContext()); - Assert.Contains("f.last_execution_time > @cutoff_time", live.Text, StringComparison.Ordinal); + Assert.Contains("i.end_time > @cutoff_time", live.Text, StringComparison.Ordinal); Assert.Contains("MAX(qsrs.last_execution_time) > @cutoff_time", live.Text, StringComparison.Ordinal); Assert.Contains("ORDER BY qsrs.last_execution_time ASC", live.Text, StringComparison.Ordinal); Assert.DoesNotContain("@floor_time", live.Text, StringComparison.Ordinal); diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index dd5d1cef..cc01602a 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -786,18 +786,94 @@ boundary groups the same way (see ReadRowsAsync). */ its oldest shipped row, and the next slice's strict `< @ceiling_time` resumes with no hole or re-ship. Same TIES, same budget, same tie-group completion; only the window and the direction differ. */ + /* The interval pre-filter resolves candidate interval ids from the INTERVAL CATALOG + (sys.query_store_runtime_stats_interval, ~one row per interval of retained history — hundreds + of rows) rather than from runtime_stats itself (#2133; measured on the field store: 20 ms vs + 426 ms for the identical id set). end_time/start_time are datetimeoffset; the datetime2 + parameters promote with a zero offset, i.e. as the UTC instants they are — the same implicit + promotion the HAVING's last_execution_time comparison has always relied on. The catalog bound + is a SUPERSET (an interval can end after the cutoff while all its rows are older); the HAVING + below stays the exact row-level filter, so shipped semantics are unchanged. */ var intervalPreFilter = backfill - ? @"f.last_execution_time > @floor_time - AND f.last_execution_time < @ceiling_time" - : "f.last_execution_time > @cutoff_time"; + ? @"i.end_time > @floor_time + AND i.start_time < @ceiling_time" + : "i.end_time > @cutoff_time"; var intervalHaving = backfill ? @"MAX(qsrs.last_execution_time) > @floor_time AND MAX(qsrs.last_execution_time) < @ceiling_time" : "MAX(qsrs.last_execution_time) > @cutoff_time"; var shipOrder = backfill ? "DESC" : "ASC"; + /* STAGED, not monolithic (#2133). Joining the slice aggregate straight into the + query_store_plan/query/text TVFs handed the optimizer nothing but fixed-guess cardinalities, + and the shape it picked re-materialized a TVF per probe — a fixed cost no window width could + reduce. Field bisection on an 82k-plan catalog (echo, SQL 2022): the aggregate alone ran in + 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-qsp could not finish in 30 s, + hinted or not; staged through the temp the same work totaled 524 ms (56 stage + 409 join). + That fixed cost is what wedged the big-catalog databases at EVERY catch-up width and made + #2125's shrink floor-pin instead of converge. The temp gives the final join REAL row counts — + and for that reason the old LOOP JOIN hint must NOT return: looping from the temp into the + TVFs is the same per-probe re-materialization by another name; the 524 ms join is unhinted, + chosen by the optimizer from true cardinalities. sp_QuickieStore stages for the same reason. + + Batch mechanics: SELECT INTO emits no result set, so the batch still returns exactly ONE + result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql + nesting the temp's scope dies with the invocation; on Azure's direct per-database path the + leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship + order, and the derived-watermark semantics live on the final SELECT, unchanged. */ return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; +DROP TABLE IF EXISTS #pm_qs_slice; + +SELECT /* PerformanceMonitorLite */ + 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} +INTO #pm_qs_slice +FROM sys.query_store_runtime_stats AS qsrs +WHERE qsrs.runtime_stats_interval_id IN +( + SELECT + i.runtime_stats_interval_id + FROM sys.query_store_runtime_stats_interval AS i + WHERE {intervalPreFilter} +) +GROUP BY + qsrs.plan_id, + qsrs.runtime_stats_interval_id, + qsrs.execution_type_desc{replicaGroupKey} +HAVING + {intervalHaving}; + SELECT /* PerformanceMonitorLite */ TOP ({MaxRowsPerDatabase}) WITH TIES query_id = qsq.query_id, plan_id = qsp.plan_id, @@ -855,56 +931,7 @@ 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 -( - 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 {intervalPreFilter} - ) - GROUP BY - qsrs.plan_id, - qsrs.runtime_stats_interval_id, - qsrs.execution_type_desc{replicaGroupKey} - HAVING - {intervalHaving} -) AS qsrs +FROM #pm_qs_slice AS qsrs JOIN sys.query_store_plan AS qsp ON qsp.plan_id = qsrs.plan_id JOIN sys.query_store_query AS qsq @@ -915,7 +942,7 @@ LEFT JOIN sys.query_store_runtime_stats_interval AS qsrsi ON qsrsi.runtime_stats_interval_id = qsrs.runtime_stats_interval_id {replicaJoin} ORDER BY qsrs.last_execution_time {shipOrder} -OPTION(RECOMPILE, LOOP JOIN);"; +OPTION(RECOMPILE);"; } /// From d1a14ff3e9445b7fb35cb61238eacc574c0cf676 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:28:07 +0200 Subject: [PATCH 2/6] =?UTF-8?q?Retrigger=20CI=20=E2=80=94=20GitHub=20dropp?= =?UTF-8?q?ed=20the=20original=20push/PR=20events?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 From 82dea99b3b3baf904b853427afc7cf9b3a52e337 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:51:57 +0200 Subject: [PATCH 3/6] Fix the four structure pins CI caught, and normalize the staging indent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tests parsed the old derived-table structure (GROUP BY indent, the aggregate's bounds, the list-tail-to-FROM seam) — updated to the staged shape. While at it, the staging statement's spliced fragments and aggregate lines are normalized to a consistent 4-space indent so the emitted SQL doesn't carry the derived table's extra level, and the pins stay clean instead of encoding mixed indentation. Co-Authored-By: Claude Fable 5 --- .../QueryStoreCollectorDefinitionTests.cs | 26 +- .../QueryStoreCollector.cs | 892 +++++++++--------- 2 files changed, 460 insertions(+), 458 deletions(-) diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index 0e11af4b..7c5f9858 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -465,7 +465,7 @@ public void BuildPerItemQuery_CombinesTheSlicesOfOneInterval_OnTheViewsNaturalKe Anything coarser would merge work that is genuinely distinct; anything finer would leave the slices split, which is the bug. */ Assert.Contains( - "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", + "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", text, StringComparison.Ordinal); @@ -532,11 +532,12 @@ public void Payload_EveryAverageColumn_IsTheCountWeightedMean() { var text = PayloadSql(MakeContext(probeResult: 16)); - /* Only the aggregating derived table — the outer projection references the same names as plain - columns, which is correct there and must not be mistaken for an un-weighted aggregate. */ - var open = text.IndexOf("FROM\n(", StringComparison.Ordinal); - var close = text.IndexOf(") AS qsrs", StringComparison.Ordinal); - Assert.True(open > 0 && close > open, "could not locate the slice-aggregating derived table"); + /* Only the aggregating STAGING statement (#2133: the aggregate lands in #pm_qs_slice and the + joins run from it) — the final projection references the same names as plain columns, which + is correct there and must not be mistaken for an un-weighted aggregate. */ + var open = text.IndexOf("SELECT /* PerformanceMonitorLite */\n", StringComparison.Ordinal); + var close = text.IndexOf("INTO #pm_qs_slice", StringComparison.Ordinal); + Assert.True(open > 0 && close > open, "could not locate the slice-aggregating staging statement"); var aggregate = text[open..close]; var averages = System.Text.RegularExpressions.Regex @@ -581,18 +582,18 @@ public void BuildPerItemQuery_ReplicaGroupIdEntersTheGroupingKey_OnlyWhereItBind foreach (var probe in new object[] { 16, 17 }) { var attributed = PayloadSql(MakeContext(probeResult: probe)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); } var azure = AzurePayloadSql(MakeContext(isAzureSqlDb: true, probeResult: 12)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); /* Pre-2022 box and Managed Instance: the column must not be named anywhere, GROUP BY included. */ foreach (var probe in new object?[] { 13, 14, 15, null }) { var ungated = PayloadSql(MakeContext(probeResult: probe)); Assert.DoesNotContain("replica_group_id", ungated, StringComparison.Ordinal); - Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); + Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); } } @@ -619,13 +620,14 @@ public void BuildPerItemQuery_PreSql2017_GatedFamiliesLeaveTheAggregate_ButKeepT Assert.Contains("avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,", old, StringComparison.Ordinal); Assert.Contains("avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,", old, StringComparison.Ordinal); - /* The inner list must end cleanly on the last ungated column when all three are absent. */ - Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\n FROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); + /* The staging list must end cleanly on the last ungated column when all three are absent — + #2133: the aggregate now lands in #pm_qs_slice, so INTO sits between the list and FROM. */ + Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); /* On 2017+ they are present, aggregated, and the list ends with the last gated family instead. */ var newer = PayloadSql(MakeContext(probeResult: 14)); Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount),\n", newer, StringComparison.Ordinal); - Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\n FROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); + Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); } [Fact] diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index cc01602a..d8312458 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -166,11 +166,11 @@ @sql NVARCHAR(500), DECLARE db_check CURSOR LOCAL FAST_FORWARD FOR SELECT /* PerformanceMonitorLite */ - d.name + d.name FROM sys.databases AS d LEFT JOIN sys.dm_hadr_database_replica_states AS drs - ON d.database_id = drs.database_id - AND drs.is_local = 1 + ON d.database_id = drs.database_id + AND drs.is_local = 1 WHERE d.database_id > 4 AND d.database_id < 32761 AND d.state_desc = N'ONLINE' @@ -182,17 +182,17 @@ AND d.name <> N'PerformanceMonitor' Second group = common DBA-convention tooling names, screened by operator decision; the inverse case (exclude a real workload db) is what excludedDatabases handles. */ AND d.name NOT IN - ( - N'master', N'model', N'msdb', N'tempdb', - N'rdsadmin', N'gcloud_cloudsqladmin', - N'ReportServer', N'ReportServerTempDB', - N'DWConfiguration', N'DWDiagnostics', N'DWQueue', - N'DBAUtil', N'DBAUtils', N'Utility' - ) + ( + N'master', N'model', N'msdb', N'tempdb', + N'rdsadmin', N'gcloud_cloudsqladmin', + N'ReportServer', N'ReportServerTempDB', + N'DWConfiguration', N'DWDiagnostics', N'DWQueue', + N'DBAUtil', N'DBAUtils', N'Utility' + ) AND ( - drs.database_id IS NULL /*not in any AG*/ - OR drs.is_primary_replica = 1 /*primary replica*/ + drs.database_id IS NULL /*not in any AG*/ + OR drs.is_primary_replica = 1 /*primary replica*/ ) /*EXCLUSION_FILTER*/ OPTION(RECOMPILE); @@ -206,39 +206,39 @@ FROM db_check WHILE @@FETCH_STATUS = 0 BEGIN BEGIN TRY - /* actual_state IN (1,2,4) = READ_ONLY / READ_WRITE / READ_CAPTURE_SECONDARY (#1546: > 0 also - admitted 3 = ERROR). readonly_reason & 8 = 0 (#1558): bit 8 is the engine saying read-only - BECAUSE this database is a readable secondary replica — its Query Store content is the - PRIMARY's persisted QS tables arriving via replication, not local activity, so collecting it - is duplicate, lagged primary data (caught live: 24 RDS read replicas each re-reading the same - primary's QS — pathological volume for zero information). The HADR join in the cursor above - only catches Always On AG secondaries; reason bit 8 is the engine's mechanism-agnostic flag - (AG, RDS read replicas, geo-secondaries alike). An operator-set read-only QS on a PRIMARY has - reason <> 8 and still collects; a 2025 READ_CAPTURE_SECONDARY (state 4, reason 0) captures - REAL local secondary workload and still collects. */ - SET @sql = N' - SELECT ' + QUOTENAME(@db, '''') + N' - WHERE EXISTS - ( - SELECT - 1 - FROM sys.database_query_store_options - WHERE actual_state IN (1, 2, 4) - AND readonly_reason & 8 = 0 - );'; - - SET @exec_sp = QUOTENAME(@db) + N'.sys.sp_executesql'; - - INSERT @result (name) - EXECUTE @exec_sp @sql; + /* actual_state IN (1,2,4) = READ_ONLY / READ_WRITE / READ_CAPTURE_SECONDARY (#1546: > 0 also + admitted 3 = ERROR). readonly_reason & 8 = 0 (#1558): bit 8 is the engine saying read-only + BECAUSE this database is a readable secondary replica — its Query Store content is the + PRIMARY's persisted QS tables arriving via replication, not local activity, so collecting it + is duplicate, lagged primary data (caught live: 24 RDS read replicas each re-reading the same + primary's QS — pathological volume for zero information). The HADR join in the cursor above + only catches Always On AG secondaries; reason bit 8 is the engine's mechanism-agnostic flag + (AG, RDS read replicas, geo-secondaries alike). An operator-set read-only QS on a PRIMARY has + reason <> 8 and still collects; a 2025 READ_CAPTURE_SECONDARY (state 4, reason 0) captures + REAL local secondary workload and still collects. */ + SET @sql = N' + SELECT ' + QUOTENAME(@db, '''') + N' + WHERE EXISTS + ( + SELECT + 1 + FROM sys.database_query_store_options + WHERE actual_state IN (1, 2, 4) + AND readonly_reason & 8 = 0 + );'; + + SET @exec_sp = QUOTENAME(@db) + N'.sys.sp_executesql'; + + INSERT @result (name) + EXECUTE @exec_sp @sql; END TRY BEGIN CATCH - /* The failure modes this catches are ordinary and per-database (mid-restore, an AG failover - mid-cursor, a login without access, a database that went offline between the cursor and the - probe), so the cursor keeps going — but the database is now MISSING from a collection that - still reports SUCCESS, which is exactly the hole #1837 closes. */ - INSERT @probe_failures (name, error_text) - VALUES (@db, ERROR_MESSAGE()); + /* The failure modes this catches are ordinary and per-database (mid-restore, an AG failover + mid-cursor, a login without access, a database that went offline between the cursor and the + probe), so the cursor keeps going — but the database is now MISSING from a collection that + still reports SUCCESS, which is exactly the hole #1837 closes. */ + INSERT @probe_failures (name, error_text) + VALUES (@db, ERROR_MESSAGE()); END CATCH; FETCH NEXT @@ -276,7 +276,7 @@ ORDER BY IF NOT EXISTS ( SELECT - 1 + 1 FROM sys.database_query_store_options WHERE actual_state IN (1, 2, 4) AND readonly_reason & 8 = 0 @@ -289,7 +289,7 @@ WHERE actual_state IN (1, 2, 4) /// The live version probe deciding the 2017+/2022+ column gates (see class remarks). public const string ProductVersionProbeText = - "SELECT CONVERT(integer, PARSENAME(CONVERT(sysname, SERVERPROPERTY('PRODUCTVERSION')), 4))"; + "SELECT CONVERT(integer, PARSENAME(CONVERT(sysname, SERVERPROPERTY('PRODUCTVERSION')), 4))"; /// PRODUCTVERSION assumed when the probe fails or returns NULL (SQL Server 2016). public const int DefaultProductVersion = 13; @@ -347,7 +347,7 @@ WHERE actual_state IN (1, 2, 4) /// version-gated columns are selected; this gate decides whether the collector runs at all.) /// public override bool AppliesTo(CollectorTargetInfo target) => - target.SqlMajorVersion == 0 || target.SqlMajorVersion >= 13 || target.IsAzureSqlDb || target.IsAzureManagedInstance; + target.SqlMajorVersion == 0 || target.SqlMajorVersion >= 13 || target.IsAzureSqlDb || target.IsAzureManagedInstance; /// Incremental: only intervals with newer last_execution_time are fetched per cycle. public override string? WatermarkColumn => "last_execution_time"; @@ -391,14 +391,14 @@ public override bool AppliesTo(CollectorTargetInfo target) => /// public override CollectorQuery BuildQuery(CollectorContext context) { - if (!context.Target.IsAzureSqlDb) - { - throw new NotSupportedException("query_store enumerates databases on this target; BuildEnumerationQuery drives the cycle."); - } + if (!context.Target.IsAzureSqlDb) + { + throw new NotSupportedException("query_store enumerates databases on this target; BuildEnumerationQuery drives the cycle."); + } - return new CollectorQuery( - AzureEligibilityGateText + BuildPayloadBody(context), - BuildCutoffParameters(context)); + return new CollectorQuery( + AzureEligibilityGateText + BuildPayloadBody(context), + BuildCutoffParameters(context)); } /// @@ -413,18 +413,18 @@ public override CollectorQuery BuildQuery(CollectorContext context) /// public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floorUtc, DateTime ceilingUtc) { - if (!context.Target.IsAzureSqlDb) - { - throw new NotSupportedException("query_store backfills per enumerated database on this target; BuildBackfillPerItemQuery drives the slice."); - } + if (!context.Target.IsAzureSqlDb) + { + throw new NotSupportedException("query_store backfills per enumerated database on this target; BuildBackfillPerItemQuery drives the slice."); + } - return new CollectorQuery( - AzureEligibilityGateText + BuildPayloadBody(context, backfill: true), - new List - { - new("@floor_time", floorUtc, CollectorParameterType.DateTime2), - new("@ceiling_time", ceilingUtc, CollectorParameterType.DateTime2), - }); + return new CollectorQuery( + AzureEligibilityGateText + BuildPayloadBody(context, backfill: true), + new List + { + new("@floor_time", floorUtc, CollectorParameterType.DateTime2), + new("@ceiling_time", ceilingUtc, CollectorParameterType.DateTime2), + }); } /// @@ -438,20 +438,20 @@ public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floo /// public override CollectorQuery? BuildEnumerationQuery(CollectorContext context) { - if (context.Target.IsAzureSqlDb) - { - return null; - } + if (context.Target.IsAzureSqlDb) + { + return null; + } - var (exclusionClause, exclusionParameters) = DatabaseExclusionFilter.Build(context.ExcludedDatabases, "d.name"); - var text = OnPremDatabaseListQueryText - .Replace("/*EXCLUSION_FILTER*/", exclusionClause, StringComparison.Ordinal); + var (exclusionClause, exclusionParameters) = DatabaseExclusionFilter.Build(context.ExcludedDatabases, "d.name"); + var text = OnPremDatabaseListQueryText + .Replace("/*EXCLUSION_FILTER*/", exclusionClause, StringComparison.Ordinal); - return new CollectorQuery(text, exclusionParameters); + return new CollectorQuery(text, exclusionParameters); } public override CollectorQuery? BuildEnumerationProbe(CollectorContext context) - => new(ProductVersionProbeText); + => new(ProductVersionProbeText); /// /// The per-database Query Store payload — the ONE body both execution paths run (#1836). It is @@ -478,350 +478,350 @@ public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floo /// internal static string BuildPayloadBody(CollectorContext context, bool backfill = false) { - /* Detect server version for version-gated columns. - isNew = true for SQL Server 2017+ (product version > 13) or Azure SQL DB/MI. - Controls: avg_num_physical_io_reads, avg_log_bytes_used, avg_tempdb_space_used, plan_forcing_type_desc. - hasPlanType = true for SQL Server 2022+ (product version >= 16), and on Azure SQL DB/MI. - Controls: plan_type_desc. */ - var productVersion = context.EnumerationProbeResult is null - ? DefaultProductVersion - : Convert.ToInt32(context.EnumerationProbeResult, CultureInfo.InvariantCulture); - bool isNew = productVersion > 13 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* plan_type_desc: version-gated on box SQL Server, but ALWAYS on for Azure SQL DB, which the - version probe cannot speak for — it reports PRODUCTVERSION major 12 (the same reason isNew - overrides above), while the engine underneath is evergreen and never older than 2022. The - column's own catalog-view page lists Azure SQL Database in its Applies-to banner and names - exactly one platform where referencing it errors — Azure Synapse Analytics, engine edition 6, - which is never IsAzureSqlDb (edition 5). - - Managed Instance is ON as of #1886, on the same live-evidence basis Azure SQL DB was flipped on - and NOT by pattern-matching the Azure change — see the replica-attribution comment below for - the full probe, which answered both gates in one session. The short form: plan_type_desc binds - on MI (COL_LENGTH = 120), measured on an instance following the CONSERVATIVE update policy. */ - bool hasPlanType = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* Replica attribution — SQL Server 2022+ (product version >= 16). Controls: replica_role. - - "Query Store for secondary replicas" (2022+) gives an AG ONE shared Query Store that lives - on the PRIMARY: secondary-replica workload is streamed to the primary and persisted in the - primary's QS tables, distinguishable only by replica_group_id. We collect from primaries - only (is_primary_replica = 1, in the enumeration cursor) — which is correct and stays — but - without this attribution the primary's "Top Queries by CPU" silently BLENDS secondary - workload into the primary's own numbers, with no way for a reader to tell. (Microsoft's own - Query Performance Insight has this exact bug.) - - Gated on >= 16, NOT the docs' claimed 2025+: sys.query_store_replicas and - sys.query_store_runtime_stats.replica_group_id both verified present on SQL 2022 - (16.0.4255.1) and SQL 2025 (17.0.4045.5). - - LEFT JOIN, deliberately: on a 2022 standalone (non-AG) server sys.query_store_replicas has - ZERO rows, yet real runtime-stats rows still carry replica_group_id = 1. An INNER JOIN — or - a WHERE replica_name = 'Primary' filter — would match nothing and silently delete ALL Query - Store collection on every 2022 standalone server. Do not "tighten" this. - - replica_name is read DIRECTLY rather than mapped from role_type: contrary to the docs, it IS - populated on box SQL Server (observed: Primary, Secondary, Geo Secondary, Geo HA Secondary), - and the docs' sample CASEs replica_group_id as though it were role_type — it is not, it is a - replica SET number that accumulates per role across failovers. - - Resulting replica_role: NULL on a 2022 standalone, 'Primary' on a 2025 standalone, the actual - role on an AG with the feature enabled. NULL honestly means "the server did not attribute - this row" and is deliberately NOT coalesced to an invented value. - - Azure SQL DB is ON, riding the same "Azure means newest" rule plan_type_desc gets above — - but only because the live probe the previous version of this comment demanded was actually - run. It was gated OFF from #1836 until then, and that carve-out was about bind SAFETY, not - taste: a missing column is not a NULL, it fails the whole SELECT for that database, and on - Azure this collector's per-database loop would then fail in EVERY database — a worse outcome - than declining the attribution. The docs still do not settle it (replica_group_id's - applies-to note names only "SQL Server (Starting with SQL Server 2022 (16.x))", and Query - Store for secondary replicas is documented as unavailable on the Hyperscale service tier, - silent on whether the view and column still BIND there), so this is flipped on live evidence - instead — gathered on both service tiers the old comment worried about, 2026-07-31 UTC, both - running Microsoft SQL Azure (RTM) 12.0.2000.8, EngineEdition 5: - - - The exact probe the old comment specified, answered on both: General Purpose - (GP_S_Gen5_1, #1848) and Hyperscale (HS_S_Gen5_2, #1872) each returned - OBJECT_ID('sys.query_store_replicas') = -660 and - COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8. - - sys.query_store_replicas binds on both and holds the same 4 role rows box SQL Server has - (Primary, Secondary, Geo Secondary, Geo HA Secondary) — a static enumeration of ROLES, - not instances, which is why a database with 0 HA replicas does not empty it. Every - runtime-stats row carries replica_group_id = 1 and LEFT JOINs cleanly to 'Primary'. - - Decisive (#1872): the full 55-column payload composed with hasReplicaAttribution = true — - the exact text this method emits — executed on Hyperscale. 55 of 55 columns bound, exit - 0, and replica_role came back 'Primary' on every row. - - A per-tier gate was considered and rejected: Hyperscale reports EngineEdition 5, the same as - General Purpose, so the collector cannot tell the tiers apart at query-build time. It does - not need to — both bind identically. - - Managed Instance is ON as of #1886 — the probe the previous version of this comment demanded - was run, so nobody needs to provision an MI to re-answer this. MI was held back through #1844 - and #1872 for a reason that does NOT apply to Azure SQL DB and had to be retired on its own - terms: MI is not evergreen. Its feature set follows a per-instance UPDATE POLICY, so "Azure - means 2022+" is a claim about the fleet that does not transfer to a specific instance, and an - MI on an older policy genuinely might not have the catalog. The bar #1886 set for the simple - edition gate was therefore stricter than Azure's: the catalog must be present on the OLDEST - update policy still in support, not merely on whatever instance was to hand. - - Measured 2026-07-31 on a GPv2 Gen5 4-vCore Managed Instance, westus3, provisioned for the run - and torn down after — reporting ProductVersion 12.0.2000.8, EngineEdition 8, and crucially - SERVERPROPERTY('ProductUpdateType') = 'CU', i.e. the CONSERVATIVE (SQL Server 2022) update - policy rather than Always-up-to-date. That is exactly the "oldest policy still in support" - case, so the bar is met without an AlwaysUpToDate instance: catalog presence is a 2022-surface - fact, not an evergreen one. - - - OBJECT_ID('sys.query_store_replicas') = -660 and - COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8 — the same two-value - probe #1848/#1872 ran on Azure SQL DB, non-NULL on both counts. - - Answered THROUGH THE COLLECTOR'S OWN MECHANISM, not just in master: the same two values - came back from a user-database context via [db].sys.sp_executesql (msdb standing in, since - these catalog views are per-database). MI takes the on-prem enumeration path, so binding in - master would not have settled it — that path is what the issue said MI lacked. - - COL_LENGTH('sys.query_store_plan', 'plan_type_desc') = 120, which is what flips the - hasPlanType carve-out above in the same session rather than provisioning MI twice. - - A standalone MI's sys.query_store_replicas is an EMPTY enumeration — zero rows — unlike Azure - SQL DB's, which is a static 4-row roles table even with no replicas present. That difference - is worth stating because it looks alarming and is not: BINDING is the collection-safety - question and the answer is yes, while an empty enumeration only means a GP instance with no - read replicas has nothing to attribute. The LEFT JOIN below is what makes that harmless — it - is the same shape that keeps a 2022 standalone (whose view is also empty) collecting, and - tightening it would break both. replica_role simply reads NULL there, which is the honest - state rather than an invented one. */ - bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* 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). - - 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,"; - - string logBytesCols = isNew - ? "avg_log_bytes_used = qsrs.avg_log_bytes_used, min_log_bytes_used = qsrs.min_log_bytes_used, max_log_bytes_used = qsrs.max_log_bytes_used," - : "avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,"; - - string tempdbCols = isNew - ? "avg_tempdb_space_used = qsrs.avg_tempdb_space_used, min_tempdb_space_used = qsrs.min_tempdb_space_used, max_tempdb_space_used = qsrs.max_tempdb_space_used," - : "avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,"; - - string planForcingCol = isNew - ? "plan_forcing_type = qsp.plan_forcing_type_desc," - : "plan_forcing_type = NULL,"; - - string planTypeCol = hasPlanType - ? "plan_type = qsp.plan_type_desc," - : "plan_type = NULL,"; - - /* Execution-plan capture — mirrors the full Dashboard's @collect_plan path in - install/09_collect_query_store.sql: CONVERT(nvarchar(max), qsp.query_plan) from - sys.query_store_plan, no size guard. On only when the host sets CapturePlanXml (Darling); - off = the nvarchar(1) NULL placeholder (Lite), byte-identical to the no-plan form. No - single quotes, so it splices straight into the sp_executesql body. - - #1556 plan-text dedupe (ON branch only): a plan is landed ONCE per plan_id per cycle — on its - newest runtime-stats interval in the window (rn = 1) — and NULL on the older intervals, instead - of repeating the full plan XML on every interval row of the same plan. The partition ORDER BY - stays DESC even though the outer sort is now ASC (#1960): under oldest-first shipping a plan's - rn = 1 row sorts LAST among its rows, so a bounded cycle can cut before it and ship that plan's - intervals without XML — harmless, because rn is recomputed over the NEXT cycle's window, whose - newest-in-window row carries the XML then; steady-state cycles see one interval per plan and are - unaffected. The consumers tolerate the per-row NULL — Lite selects NULL for the grid and fetches - plans live, and Darling's stored-plan readers all guard `query_plan_text IS NOT NULL`. Not - mirrored into the Dashboard proc: its "Download Plan" reads by exact collection_id, where - per-row NULLs would break a real reader. */ - string planTextCol = context.CapturePlanXml - ? "query_plan_text = CASE WHEN ROW_NUMBER() OVER (PARTITION BY qsp.plan_id ORDER BY qsrs.last_execution_time DESC) = 1 THEN CONVERT(nvarchar(max), qsp.query_plan) ELSE CONVERT(nvarchar(max), NULL) END," - : "query_plan_text = CONVERT(nvarchar(1), NULL),"; - - /* The replica-attribution column + its join (see hasReplicaAttribution above). Selected after every - version-gated column, so pre-2022 targets read the nvarchar(1) NULL placeholder at the same - ordinal — byte-identical shape to the attributed form. The interval-identity pair (#1841 tier 2) - follows it and is NOT version-gated, so this fragment stays comma-free and the template supplies - the separator. */ - string replicaRoleCol = hasReplicaAttribution - ? "replica_role = qsr.replica_name" - : "replica_role = CONVERT(nvarchar(1), NULL)"; - - string replicaJoin = hasReplicaAttribution - ? "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 - a 14.9s read; the field A/B measured 4.3x faster without it) — and no predicate shape fixes - that server-side: the QS internal text table has no index on the column, so every variant is - a residual scan whose cost is the bytes it reads. The exclusion instead happens in the read - loop, where the query text is ALREADY materialized for every row — a client-side Contains at - zero SQL cost, identical semantics. Self rows cross the wire (~2% of a busy database's rows) - and are dropped before they enter the batch (never stored, never counted against the byte - budget). The query still CONTAINS the marker, in its own leading comment. */ - - /* Interval identity (#1841 tier 2), the last two SELECT items. Not version-gated: both - sys.query_store_runtime_stats.runtime_stats_interval_id and the - sys.query_store_runtime_stats_interval catalog view are original Query Store surface, verified - present on SQL Server 2016 SP3 (13.0.6300.2) — the collector's own AppliesTo floor — so there is - no target this collector runs on that lacks them. - - LEFT JOIN, not JOIN, for the same reason the replica join above is one: an INNER JOIN here would - make every runtime-stats row's survival depend on its interval row resolving, and a Query Store - that trimmed an interval row out from under us would silently delete real collection rather than - lose one column. The id comes off qsrs directly and is unaffected either way; only - interval_start_time_utc goes NULL if the join misses. - - start_time is datetimeoffset. AT TIME ZONE 'UTC' re-expresses it at +00:00 and the CONVERT drops - the offset, so the stored value is naive UTC — the same clock as collection_time and as - first_execution_time (which ReadRowsAsync already normalizes via DateTimeOffset.UtcDateTime). - That is what makes it safe to bucket on: it is NOT the monitored server's local wall clock. - 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. */ - /* Oldest-first + WITH TIES (#1960): rows ship FORWARD from the watermark, so a bounded cycle - (byte budget or this TOP) leaves the derived watermark — MAX(last_execution_time) over the - rows actually stored — sitting exactly at the shipped boundary, and the next cycle's strict - `> @cutoff_time` resumes there with no hole. WITH TIES is load-bearing for that invariant: - a bare TOP could split a group of rows sharing the boundary last_execution_time, stranding - the unshipped half behind the strict comparison forever. The client byte budget completes - boundary groups the same way (see ReadRowsAsync). */ - /* Backfill (#2022) is the mirror image: newest-first DESC inside (floor, ceiling), where the - ceiling is the DERIVED backfill boundary — MIN(last_execution_time) over the rows already - stored for the database — so each bounded slice leaves the next ceiling sitting exactly at - its oldest shipped row, and the next slice's strict `< @ceiling_time` resumes with no hole - or re-ship. Same TIES, same budget, same tie-group completion; only the window and the - direction differ. */ - /* The interval pre-filter resolves candidate interval ids from the INTERVAL CATALOG - (sys.query_store_runtime_stats_interval, ~one row per interval of retained history — hundreds - of rows) rather than from runtime_stats itself (#2133; measured on the field store: 20 ms vs - 426 ms for the identical id set). end_time/start_time are datetimeoffset; the datetime2 - parameters promote with a zero offset, i.e. as the UTC instants they are — the same implicit - promotion the HAVING's last_execution_time comparison has always relied on. The catalog bound - is a SUPERSET (an interval can end after the cutoff while all its rows are older); the HAVING - below stays the exact row-level filter, so shipped semantics are unchanged. */ - var intervalPreFilter = backfill - ? @"i.end_time > @floor_time - AND i.start_time < @ceiling_time" - : "i.end_time > @cutoff_time"; - var intervalHaving = backfill - ? @"MAX(qsrs.last_execution_time) > @floor_time - AND MAX(qsrs.last_execution_time) < @ceiling_time" - : "MAX(qsrs.last_execution_time) > @cutoff_time"; - var shipOrder = backfill ? "DESC" : "ASC"; - - /* STAGED, not monolithic (#2133). Joining the slice aggregate straight into the - query_store_plan/query/text TVFs handed the optimizer nothing but fixed-guess cardinalities, - and the shape it picked re-materialized a TVF per probe — a fixed cost no window width could - reduce. Field bisection on an 82k-plan catalog (echo, SQL 2022): the aggregate alone ran in - 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-qsp could not finish in 30 s, - hinted or not; staged through the temp the same work totaled 524 ms (56 stage + 409 join). - That fixed cost is what wedged the big-catalog databases at EVERY catch-up width and made - #2125's shrink floor-pin instead of converge. The temp gives the final join REAL row counts — - and for that reason the old LOOP JOIN hint must NOT return: looping from the temp into the - TVFs is the same per-probe re-materialization by another name; the 524 ms join is unhinted, - chosen by the optimizer from true cardinalities. sp_QuickieStore stages for the same reason. - - Batch mechanics: SELECT INTO emits no result set, so the batch still returns exactly ONE - result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql - nesting the temp's scope dies with the invocation; on Azure's direct per-database path the - leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship - order, and the derived-watermark semantics live on the final SELECT, unchanged. */ - return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; + /* Detect server version for version-gated columns. + isNew = true for SQL Server 2017+ (product version > 13) or Azure SQL DB/MI. + Controls: avg_num_physical_io_reads, avg_log_bytes_used, avg_tempdb_space_used, plan_forcing_type_desc. + hasPlanType = true for SQL Server 2022+ (product version >= 16), and on Azure SQL DB/MI. + Controls: plan_type_desc. */ + var productVersion = context.EnumerationProbeResult is null + ? DefaultProductVersion + : Convert.ToInt32(context.EnumerationProbeResult, CultureInfo.InvariantCulture); + bool isNew = productVersion > 13 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* plan_type_desc: version-gated on box SQL Server, but ALWAYS on for Azure SQL DB, which the + version probe cannot speak for — it reports PRODUCTVERSION major 12 (the same reason isNew + overrides above), while the engine underneath is evergreen and never older than 2022. The + column's own catalog-view page lists Azure SQL Database in its Applies-to banner and names + exactly one platform where referencing it errors — Azure Synapse Analytics, engine edition 6, + which is never IsAzureSqlDb (edition 5). + + Managed Instance is ON as of #1886, on the same live-evidence basis Azure SQL DB was flipped on + and NOT by pattern-matching the Azure change — see the replica-attribution comment below for + the full probe, which answered both gates in one session. The short form: plan_type_desc binds + on MI (COL_LENGTH = 120), measured on an instance following the CONSERVATIVE update policy. */ + bool hasPlanType = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* Replica attribution — SQL Server 2022+ (product version >= 16). Controls: replica_role. + + "Query Store for secondary replicas" (2022+) gives an AG ONE shared Query Store that lives + on the PRIMARY: secondary-replica workload is streamed to the primary and persisted in the + primary's QS tables, distinguishable only by replica_group_id. We collect from primaries + only (is_primary_replica = 1, in the enumeration cursor) — which is correct and stays — but + without this attribution the primary's "Top Queries by CPU" silently BLENDS secondary + workload into the primary's own numbers, with no way for a reader to tell. (Microsoft's own + Query Performance Insight has this exact bug.) + + Gated on >= 16, NOT the docs' claimed 2025+: sys.query_store_replicas and + sys.query_store_runtime_stats.replica_group_id both verified present on SQL 2022 + (16.0.4255.1) and SQL 2025 (17.0.4045.5). + + LEFT JOIN, deliberately: on a 2022 standalone (non-AG) server sys.query_store_replicas has + ZERO rows, yet real runtime-stats rows still carry replica_group_id = 1. An INNER JOIN — or + a WHERE replica_name = 'Primary' filter — would match nothing and silently delete ALL Query + Store collection on every 2022 standalone server. Do not "tighten" this. + + replica_name is read DIRECTLY rather than mapped from role_type: contrary to the docs, it IS + populated on box SQL Server (observed: Primary, Secondary, Geo Secondary, Geo HA Secondary), + and the docs' sample CASEs replica_group_id as though it were role_type — it is not, it is a + replica SET number that accumulates per role across failovers. + + Resulting replica_role: NULL on a 2022 standalone, 'Primary' on a 2025 standalone, the actual + role on an AG with the feature enabled. NULL honestly means "the server did not attribute + this row" and is deliberately NOT coalesced to an invented value. + + Azure SQL DB is ON, riding the same "Azure means newest" rule plan_type_desc gets above — + but only because the live probe the previous version of this comment demanded was actually + run. It was gated OFF from #1836 until then, and that carve-out was about bind SAFETY, not + taste: a missing column is not a NULL, it fails the whole SELECT for that database, and on + Azure this collector's per-database loop would then fail in EVERY database — a worse outcome + than declining the attribution. The docs still do not settle it (replica_group_id's + applies-to note names only "SQL Server (Starting with SQL Server 2022 (16.x))", and Query + Store for secondary replicas is documented as unavailable on the Hyperscale service tier, + silent on whether the view and column still BIND there), so this is flipped on live evidence + instead — gathered on both service tiers the old comment worried about, 2026-07-31 UTC, both + running Microsoft SQL Azure (RTM) 12.0.2000.8, EngineEdition 5: + + - The exact probe the old comment specified, answered on both: General Purpose + (GP_S_Gen5_1, #1848) and Hyperscale (HS_S_Gen5_2, #1872) each returned + OBJECT_ID('sys.query_store_replicas') = -660 and + COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8. + - sys.query_store_replicas binds on both and holds the same 4 role rows box SQL Server has + (Primary, Secondary, Geo Secondary, Geo HA Secondary) — a static enumeration of ROLES, + not instances, which is why a database with 0 HA replicas does not empty it. Every + runtime-stats row carries replica_group_id = 1 and LEFT JOINs cleanly to 'Primary'. + - Decisive (#1872): the full 55-column payload composed with hasReplicaAttribution = true — + the exact text this method emits — executed on Hyperscale. 55 of 55 columns bound, exit + 0, and replica_role came back 'Primary' on every row. + + A per-tier gate was considered and rejected: Hyperscale reports EngineEdition 5, the same as + General Purpose, so the collector cannot tell the tiers apart at query-build time. It does + not need to — both bind identically. + + Managed Instance is ON as of #1886 — the probe the previous version of this comment demanded + was run, so nobody needs to provision an MI to re-answer this. MI was held back through #1844 + and #1872 for a reason that does NOT apply to Azure SQL DB and had to be retired on its own + terms: MI is not evergreen. Its feature set follows a per-instance UPDATE POLICY, so "Azure + means 2022+" is a claim about the fleet that does not transfer to a specific instance, and an + MI on an older policy genuinely might not have the catalog. The bar #1886 set for the simple + edition gate was therefore stricter than Azure's: the catalog must be present on the OLDEST + update policy still in support, not merely on whatever instance was to hand. + + Measured 2026-07-31 on a GPv2 Gen5 4-vCore Managed Instance, westus3, provisioned for the run + and torn down after — reporting ProductVersion 12.0.2000.8, EngineEdition 8, and crucially + SERVERPROPERTY('ProductUpdateType') = 'CU', i.e. the CONSERVATIVE (SQL Server 2022) update + policy rather than Always-up-to-date. That is exactly the "oldest policy still in support" + case, so the bar is met without an AlwaysUpToDate instance: catalog presence is a 2022-surface + fact, not an evergreen one. + + - OBJECT_ID('sys.query_store_replicas') = -660 and + COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8 — the same two-value + probe #1848/#1872 ran on Azure SQL DB, non-NULL on both counts. + - Answered THROUGH THE COLLECTOR'S OWN MECHANISM, not just in master: the same two values + came back from a user-database context via [db].sys.sp_executesql (msdb standing in, since + these catalog views are per-database). MI takes the on-prem enumeration path, so binding in + master would not have settled it — that path is what the issue said MI lacked. + - COL_LENGTH('sys.query_store_plan', 'plan_type_desc') = 120, which is what flips the + hasPlanType carve-out above in the same session rather than provisioning MI twice. + + A standalone MI's sys.query_store_replicas is an EMPTY enumeration — zero rows — unlike Azure + SQL DB's, which is a static 4-row roles table even with no replicas present. That difference + is worth stating because it looks alarming and is not: BINDING is the collection-safety + question and the answer is yes, while an empty enumeration only means a GP instance with no + read replicas has nothing to attribute. The LEFT JOIN below is what makes that harmless — it + is the same shape that keeps a 2022 standalone (whose view is also empty) collecting, and + tightening it would break both. replica_role simply reads NULL there, which is the honest + state rather than an invented one. */ + bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* 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). + + 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,"; + + string logBytesCols = isNew + ? "avg_log_bytes_used = qsrs.avg_log_bytes_used, min_log_bytes_used = qsrs.min_log_bytes_used, max_log_bytes_used = qsrs.max_log_bytes_used," + : "avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,"; + + string tempdbCols = isNew + ? "avg_tempdb_space_used = qsrs.avg_tempdb_space_used, min_tempdb_space_used = qsrs.min_tempdb_space_used, max_tempdb_space_used = qsrs.max_tempdb_space_used," + : "avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,"; + + string planForcingCol = isNew + ? "plan_forcing_type = qsp.plan_forcing_type_desc," + : "plan_forcing_type = NULL,"; + + string planTypeCol = hasPlanType + ? "plan_type = qsp.plan_type_desc," + : "plan_type = NULL,"; + + /* Execution-plan capture — mirrors the full Dashboard's @collect_plan path in + install/09_collect_query_store.sql: CONVERT(nvarchar(max), qsp.query_plan) from + sys.query_store_plan, no size guard. On only when the host sets CapturePlanXml (Darling); + off = the nvarchar(1) NULL placeholder (Lite), byte-identical to the no-plan form. No + single quotes, so it splices straight into the sp_executesql body. + + #1556 plan-text dedupe (ON branch only): a plan is landed ONCE per plan_id per cycle — on its + newest runtime-stats interval in the window (rn = 1) — and NULL on the older intervals, instead + of repeating the full plan XML on every interval row of the same plan. The partition ORDER BY + stays DESC even though the outer sort is now ASC (#1960): under oldest-first shipping a plan's + rn = 1 row sorts LAST among its rows, so a bounded cycle can cut before it and ship that plan's + intervals without XML — harmless, because rn is recomputed over the NEXT cycle's window, whose + newest-in-window row carries the XML then; steady-state cycles see one interval per plan and are + unaffected. The consumers tolerate the per-row NULL — Lite selects NULL for the grid and fetches + plans live, and Darling's stored-plan readers all guard `query_plan_text IS NOT NULL`. Not + mirrored into the Dashboard proc: its "Download Plan" reads by exact collection_id, where + per-row NULLs would break a real reader. */ + string planTextCol = context.CapturePlanXml + ? "query_plan_text = CASE WHEN ROW_NUMBER() OVER (PARTITION BY qsp.plan_id ORDER BY qsrs.last_execution_time DESC) = 1 THEN CONVERT(nvarchar(max), qsp.query_plan) ELSE CONVERT(nvarchar(max), NULL) END," + : "query_plan_text = CONVERT(nvarchar(1), NULL),"; + + /* The replica-attribution column + its join (see hasReplicaAttribution above). Selected after every + version-gated column, so pre-2022 targets read the nvarchar(1) NULL placeholder at the same + ordinal — byte-identical shape to the attributed form. The interval-identity pair (#1841 tier 2) + follows it and is NOT version-gated, so this fragment stays comma-free and the template supplies + the separator. */ + string replicaRoleCol = hasReplicaAttribution + ? "replica_role = qsr.replica_name" + : "replica_role = CONVERT(nvarchar(1), NULL)"; + + string replicaJoin = hasReplicaAttribution + ? "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 + a 14.9s read; the field A/B measured 4.3x faster without it) — and no predicate shape fixes + that server-side: the QS internal text table has no index on the column, so every variant is + a residual scan whose cost is the bytes it reads. The exclusion instead happens in the read + loop, where the query text is ALREADY materialized for every row — a client-side Contains at + zero SQL cost, identical semantics. Self rows cross the wire (~2% of a busy database's rows) + and are dropped before they enter the batch (never stored, never counted against the byte + budget). The query still CONTAINS the marker, in its own leading comment. */ + + /* Interval identity (#1841 tier 2), the last two SELECT items. Not version-gated: both + sys.query_store_runtime_stats.runtime_stats_interval_id and the + sys.query_store_runtime_stats_interval catalog view are original Query Store surface, verified + present on SQL Server 2016 SP3 (13.0.6300.2) — the collector's own AppliesTo floor — so there is + no target this collector runs on that lacks them. + + LEFT JOIN, not JOIN, for the same reason the replica join above is one: an INNER JOIN here would + make every runtime-stats row's survival depend on its interval row resolving, and a Query Store + that trimmed an interval row out from under us would silently delete real collection rather than + lose one column. The id comes off qsrs directly and is unaffected either way; only + interval_start_time_utc goes NULL if the join misses. + + start_time is datetimeoffset. AT TIME ZONE 'UTC' re-expresses it at +00:00 and the CONVERT drops + the offset, so the stored value is naive UTC — the same clock as collection_time and as + first_execution_time (which ReadRowsAsync already normalizes via DateTimeOffset.UtcDateTime). + That is what makes it safe to bucket on: it is NOT the monitored server's local wall clock. + 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. */ + /* Oldest-first + WITH TIES (#1960): rows ship FORWARD from the watermark, so a bounded cycle + (byte budget or this TOP) leaves the derived watermark — MAX(last_execution_time) over the + rows actually stored — sitting exactly at the shipped boundary, and the next cycle's strict + `> @cutoff_time` resumes there with no hole. WITH TIES is load-bearing for that invariant: + a bare TOP could split a group of rows sharing the boundary last_execution_time, stranding + the unshipped half behind the strict comparison forever. The client byte budget completes + boundary groups the same way (see ReadRowsAsync). */ + /* Backfill (#2022) is the mirror image: newest-first DESC inside (floor, ceiling), where the + ceiling is the DERIVED backfill boundary — MIN(last_execution_time) over the rows already + stored for the database — so each bounded slice leaves the next ceiling sitting exactly at + its oldest shipped row, and the next slice's strict `< @ceiling_time` resumes with no hole + or re-ship. Same TIES, same budget, same tie-group completion; only the window and the + direction differ. */ + /* The interval pre-filter resolves candidate interval ids from the INTERVAL CATALOG + (sys.query_store_runtime_stats_interval, ~one row per interval of retained history — hundreds + of rows) rather than from runtime_stats itself (#2133; measured on the field store: 20 ms vs + 426 ms for the identical id set). end_time/start_time are datetimeoffset; the datetime2 + parameters promote with a zero offset, i.e. as the UTC instants they are — the same implicit + promotion the HAVING's last_execution_time comparison has always relied on. The catalog bound + is a SUPERSET (an interval can end after the cutoff while all its rows are older); the HAVING + below stays the exact row-level filter, so shipped semantics are unchanged. */ + var intervalPreFilter = backfill + ? @"i.end_time > @floor_time + AND i.start_time < @ceiling_time" + : "i.end_time > @cutoff_time"; + var intervalHaving = backfill + ? @"MAX(qsrs.last_execution_time) > @floor_time + AND MAX(qsrs.last_execution_time) < @ceiling_time" + : "MAX(qsrs.last_execution_time) > @cutoff_time"; + var shipOrder = backfill ? "DESC" : "ASC"; + + /* STAGED, not monolithic (#2133). Joining the slice aggregate straight into the + query_store_plan/query/text TVFs handed the optimizer nothing but fixed-guess cardinalities, + and the shape it picked re-materialized a TVF per probe — a fixed cost no window width could + reduce. Field bisection on an 82k-plan catalog (echo, SQL 2022): the aggregate alone ran in + 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-qsp could not finish in 30 s, + hinted or not; staged through the temp the same work totaled 524 ms (56 stage + 409 join). + That fixed cost is what wedged the big-catalog databases at EVERY catch-up width and made + #2125's shrink floor-pin instead of converge. The temp gives the final join REAL row counts — + and for that reason the old LOOP JOIN hint must NOT return: looping from the temp into the + TVFs is the same per-probe re-materialization by another name; the 524 ms join is unhinted, + chosen by the optimizer from true cardinalities. sp_QuickieStore stages for the same reason. + + Batch mechanics: SELECT INTO emits no result set, so the batch still returns exactly ONE + result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql + nesting the temp's scope dies with the invocation; on Azure's direct per-database path the + leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship + order, and the derived-watermark semantics live on the final SELECT, unchanged. */ + return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; DROP TABLE IF EXISTS #pm_qs_slice; @@ -829,35 +829,35 @@ result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_exe 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} + 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} INTO #pm_qs_slice FROM sys.query_store_runtime_stats AS qsrs WHERE qsrs.runtime_stats_interval_id IN From 503a087fb127df01f75ed95af8d32a42fbf6cd60 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:53:57 +0200 Subject: [PATCH 4/6] Revert "Fix the four structure pins CI caught, and normalize the staging indent" This reverts commit 82dea99b3b3baf904b853427afc7cf9b3a52e337. --- .../QueryStoreCollectorDefinitionTests.cs | 26 +- .../QueryStoreCollector.cs | 892 +++++++++--------- 2 files changed, 458 insertions(+), 460 deletions(-) diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index 7c5f9858..0e11af4b 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -465,7 +465,7 @@ public void BuildPerItemQuery_CombinesTheSlicesOfOneInterval_OnTheViewsNaturalKe Anything coarser would merge work that is genuinely distinct; anything finer would leave the slices split, which is the bug. */ Assert.Contains( - "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", + "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", text, StringComparison.Ordinal); @@ -532,12 +532,11 @@ public void Payload_EveryAverageColumn_IsTheCountWeightedMean() { var text = PayloadSql(MakeContext(probeResult: 16)); - /* Only the aggregating STAGING statement (#2133: the aggregate lands in #pm_qs_slice and the - joins run from it) — the final projection references the same names as plain columns, which - is correct there and must not be mistaken for an un-weighted aggregate. */ - var open = text.IndexOf("SELECT /* PerformanceMonitorLite */\n", StringComparison.Ordinal); - var close = text.IndexOf("INTO #pm_qs_slice", StringComparison.Ordinal); - Assert.True(open > 0 && close > open, "could not locate the slice-aggregating staging statement"); + /* Only the aggregating derived table — the outer projection references the same names as plain + columns, which is correct there and must not be mistaken for an un-weighted aggregate. */ + var open = text.IndexOf("FROM\n(", StringComparison.Ordinal); + var close = text.IndexOf(") AS qsrs", StringComparison.Ordinal); + Assert.True(open > 0 && close > open, "could not locate the slice-aggregating derived table"); var aggregate = text[open..close]; var averages = System.Text.RegularExpressions.Regex @@ -582,18 +581,18 @@ public void BuildPerItemQuery_ReplicaGroupIdEntersTheGroupingKey_OnlyWhereItBind foreach (var probe in new object[] { 16, 17 }) { var attributed = PayloadSql(MakeContext(probeResult: probe)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); } var azure = AzurePayloadSql(MakeContext(isAzureSqlDb: true, probeResult: 12)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); /* Pre-2022 box and Managed Instance: the column must not be named anywhere, GROUP BY included. */ foreach (var probe in new object?[] { 13, 14, 15, null }) { var ungated = PayloadSql(MakeContext(probeResult: probe)); Assert.DoesNotContain("replica_group_id", ungated, StringComparison.Ordinal); - Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); + Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); } } @@ -620,14 +619,13 @@ public void BuildPerItemQuery_PreSql2017_GatedFamiliesLeaveTheAggregate_ButKeepT Assert.Contains("avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,", old, StringComparison.Ordinal); Assert.Contains("avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,", old, StringComparison.Ordinal); - /* The staging list must end cleanly on the last ungated column when all three are absent — - #2133: the aggregate now lands in #pm_qs_slice, so INTO sits between the list and FROM. */ - Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); + /* The inner list must end cleanly on the last ungated column when all three are absent. */ + Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\n FROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); /* On 2017+ they are present, aggregated, and the list ends with the last gated family instead. */ var newer = PayloadSql(MakeContext(probeResult: 14)); Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount),\n", newer, StringComparison.Ordinal); - Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); + Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\n FROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); } [Fact] diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index d8312458..cc01602a 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -166,11 +166,11 @@ @sql NVARCHAR(500), DECLARE db_check CURSOR LOCAL FAST_FORWARD FOR SELECT /* PerformanceMonitorLite */ - d.name + d.name FROM sys.databases AS d LEFT JOIN sys.dm_hadr_database_replica_states AS drs - ON d.database_id = drs.database_id - AND drs.is_local = 1 + ON d.database_id = drs.database_id + AND drs.is_local = 1 WHERE d.database_id > 4 AND d.database_id < 32761 AND d.state_desc = N'ONLINE' @@ -182,17 +182,17 @@ AND d.name <> N'PerformanceMonitor' Second group = common DBA-convention tooling names, screened by operator decision; the inverse case (exclude a real workload db) is what excludedDatabases handles. */ AND d.name NOT IN - ( - N'master', N'model', N'msdb', N'tempdb', - N'rdsadmin', N'gcloud_cloudsqladmin', - N'ReportServer', N'ReportServerTempDB', - N'DWConfiguration', N'DWDiagnostics', N'DWQueue', - N'DBAUtil', N'DBAUtils', N'Utility' - ) + ( + N'master', N'model', N'msdb', N'tempdb', + N'rdsadmin', N'gcloud_cloudsqladmin', + N'ReportServer', N'ReportServerTempDB', + N'DWConfiguration', N'DWDiagnostics', N'DWQueue', + N'DBAUtil', N'DBAUtils', N'Utility' + ) AND ( - drs.database_id IS NULL /*not in any AG*/ - OR drs.is_primary_replica = 1 /*primary replica*/ + drs.database_id IS NULL /*not in any AG*/ + OR drs.is_primary_replica = 1 /*primary replica*/ ) /*EXCLUSION_FILTER*/ OPTION(RECOMPILE); @@ -206,39 +206,39 @@ FROM db_check WHILE @@FETCH_STATUS = 0 BEGIN BEGIN TRY - /* actual_state IN (1,2,4) = READ_ONLY / READ_WRITE / READ_CAPTURE_SECONDARY (#1546: > 0 also - admitted 3 = ERROR). readonly_reason & 8 = 0 (#1558): bit 8 is the engine saying read-only - BECAUSE this database is a readable secondary replica — its Query Store content is the - PRIMARY's persisted QS tables arriving via replication, not local activity, so collecting it - is duplicate, lagged primary data (caught live: 24 RDS read replicas each re-reading the same - primary's QS — pathological volume for zero information). The HADR join in the cursor above - only catches Always On AG secondaries; reason bit 8 is the engine's mechanism-agnostic flag - (AG, RDS read replicas, geo-secondaries alike). An operator-set read-only QS on a PRIMARY has - reason <> 8 and still collects; a 2025 READ_CAPTURE_SECONDARY (state 4, reason 0) captures - REAL local secondary workload and still collects. */ - SET @sql = N' - SELECT ' + QUOTENAME(@db, '''') + N' - WHERE EXISTS - ( - SELECT - 1 - FROM sys.database_query_store_options - WHERE actual_state IN (1, 2, 4) - AND readonly_reason & 8 = 0 - );'; - - SET @exec_sp = QUOTENAME(@db) + N'.sys.sp_executesql'; - - INSERT @result (name) - EXECUTE @exec_sp @sql; + /* actual_state IN (1,2,4) = READ_ONLY / READ_WRITE / READ_CAPTURE_SECONDARY (#1546: > 0 also + admitted 3 = ERROR). readonly_reason & 8 = 0 (#1558): bit 8 is the engine saying read-only + BECAUSE this database is a readable secondary replica — its Query Store content is the + PRIMARY's persisted QS tables arriving via replication, not local activity, so collecting it + is duplicate, lagged primary data (caught live: 24 RDS read replicas each re-reading the same + primary's QS — pathological volume for zero information). The HADR join in the cursor above + only catches Always On AG secondaries; reason bit 8 is the engine's mechanism-agnostic flag + (AG, RDS read replicas, geo-secondaries alike). An operator-set read-only QS on a PRIMARY has + reason <> 8 and still collects; a 2025 READ_CAPTURE_SECONDARY (state 4, reason 0) captures + REAL local secondary workload and still collects. */ + SET @sql = N' + SELECT ' + QUOTENAME(@db, '''') + N' + WHERE EXISTS + ( + SELECT + 1 + FROM sys.database_query_store_options + WHERE actual_state IN (1, 2, 4) + AND readonly_reason & 8 = 0 + );'; + + SET @exec_sp = QUOTENAME(@db) + N'.sys.sp_executesql'; + + INSERT @result (name) + EXECUTE @exec_sp @sql; END TRY BEGIN CATCH - /* The failure modes this catches are ordinary and per-database (mid-restore, an AG failover - mid-cursor, a login without access, a database that went offline between the cursor and the - probe), so the cursor keeps going — but the database is now MISSING from a collection that - still reports SUCCESS, which is exactly the hole #1837 closes. */ - INSERT @probe_failures (name, error_text) - VALUES (@db, ERROR_MESSAGE()); + /* The failure modes this catches are ordinary and per-database (mid-restore, an AG failover + mid-cursor, a login without access, a database that went offline between the cursor and the + probe), so the cursor keeps going — but the database is now MISSING from a collection that + still reports SUCCESS, which is exactly the hole #1837 closes. */ + INSERT @probe_failures (name, error_text) + VALUES (@db, ERROR_MESSAGE()); END CATCH; FETCH NEXT @@ -276,7 +276,7 @@ ORDER BY IF NOT EXISTS ( SELECT - 1 + 1 FROM sys.database_query_store_options WHERE actual_state IN (1, 2, 4) AND readonly_reason & 8 = 0 @@ -289,7 +289,7 @@ WHERE actual_state IN (1, 2, 4) /// The live version probe deciding the 2017+/2022+ column gates (see class remarks). public const string ProductVersionProbeText = - "SELECT CONVERT(integer, PARSENAME(CONVERT(sysname, SERVERPROPERTY('PRODUCTVERSION')), 4))"; + "SELECT CONVERT(integer, PARSENAME(CONVERT(sysname, SERVERPROPERTY('PRODUCTVERSION')), 4))"; /// PRODUCTVERSION assumed when the probe fails or returns NULL (SQL Server 2016). public const int DefaultProductVersion = 13; @@ -347,7 +347,7 @@ WHERE actual_state IN (1, 2, 4) /// version-gated columns are selected; this gate decides whether the collector runs at all.) /// public override bool AppliesTo(CollectorTargetInfo target) => - target.SqlMajorVersion == 0 || target.SqlMajorVersion >= 13 || target.IsAzureSqlDb || target.IsAzureManagedInstance; + target.SqlMajorVersion == 0 || target.SqlMajorVersion >= 13 || target.IsAzureSqlDb || target.IsAzureManagedInstance; /// Incremental: only intervals with newer last_execution_time are fetched per cycle. public override string? WatermarkColumn => "last_execution_time"; @@ -391,14 +391,14 @@ public override bool AppliesTo(CollectorTargetInfo target) => /// public override CollectorQuery BuildQuery(CollectorContext context) { - if (!context.Target.IsAzureSqlDb) - { - throw new NotSupportedException("query_store enumerates databases on this target; BuildEnumerationQuery drives the cycle."); - } + if (!context.Target.IsAzureSqlDb) + { + throw new NotSupportedException("query_store enumerates databases on this target; BuildEnumerationQuery drives the cycle."); + } - return new CollectorQuery( - AzureEligibilityGateText + BuildPayloadBody(context), - BuildCutoffParameters(context)); + return new CollectorQuery( + AzureEligibilityGateText + BuildPayloadBody(context), + BuildCutoffParameters(context)); } /// @@ -413,18 +413,18 @@ public override CollectorQuery BuildQuery(CollectorContext context) /// public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floorUtc, DateTime ceilingUtc) { - if (!context.Target.IsAzureSqlDb) - { - throw new NotSupportedException("query_store backfills per enumerated database on this target; BuildBackfillPerItemQuery drives the slice."); - } - - return new CollectorQuery( - AzureEligibilityGateText + BuildPayloadBody(context, backfill: true), - new List + if (!context.Target.IsAzureSqlDb) { - new("@floor_time", floorUtc, CollectorParameterType.DateTime2), - new("@ceiling_time", ceilingUtc, CollectorParameterType.DateTime2), - }); + throw new NotSupportedException("query_store backfills per enumerated database on this target; BuildBackfillPerItemQuery drives the slice."); + } + + return new CollectorQuery( + AzureEligibilityGateText + BuildPayloadBody(context, backfill: true), + new List + { + new("@floor_time", floorUtc, CollectorParameterType.DateTime2), + new("@ceiling_time", ceilingUtc, CollectorParameterType.DateTime2), + }); } /// @@ -438,20 +438,20 @@ public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floo /// public override CollectorQuery? BuildEnumerationQuery(CollectorContext context) { - if (context.Target.IsAzureSqlDb) - { - return null; - } + if (context.Target.IsAzureSqlDb) + { + return null; + } - var (exclusionClause, exclusionParameters) = DatabaseExclusionFilter.Build(context.ExcludedDatabases, "d.name"); - var text = OnPremDatabaseListQueryText - .Replace("/*EXCLUSION_FILTER*/", exclusionClause, StringComparison.Ordinal); + var (exclusionClause, exclusionParameters) = DatabaseExclusionFilter.Build(context.ExcludedDatabases, "d.name"); + var text = OnPremDatabaseListQueryText + .Replace("/*EXCLUSION_FILTER*/", exclusionClause, StringComparison.Ordinal); - return new CollectorQuery(text, exclusionParameters); + return new CollectorQuery(text, exclusionParameters); } public override CollectorQuery? BuildEnumerationProbe(CollectorContext context) - => new(ProductVersionProbeText); + => new(ProductVersionProbeText); /// /// The per-database Query Store payload — the ONE body both execution paths run (#1836). It is @@ -478,350 +478,350 @@ public CollectorQuery BuildBackfillQuery(CollectorContext context, DateTime floo /// internal static string BuildPayloadBody(CollectorContext context, bool backfill = false) { - /* Detect server version for version-gated columns. - isNew = true for SQL Server 2017+ (product version > 13) or Azure SQL DB/MI. - Controls: avg_num_physical_io_reads, avg_log_bytes_used, avg_tempdb_space_used, plan_forcing_type_desc. - hasPlanType = true for SQL Server 2022+ (product version >= 16), and on Azure SQL DB/MI. - Controls: plan_type_desc. */ - var productVersion = context.EnumerationProbeResult is null - ? DefaultProductVersion - : Convert.ToInt32(context.EnumerationProbeResult, CultureInfo.InvariantCulture); - bool isNew = productVersion > 13 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* plan_type_desc: version-gated on box SQL Server, but ALWAYS on for Azure SQL DB, which the - version probe cannot speak for — it reports PRODUCTVERSION major 12 (the same reason isNew - overrides above), while the engine underneath is evergreen and never older than 2022. The - column's own catalog-view page lists Azure SQL Database in its Applies-to banner and names - exactly one platform where referencing it errors — Azure Synapse Analytics, engine edition 6, - which is never IsAzureSqlDb (edition 5). - - Managed Instance is ON as of #1886, on the same live-evidence basis Azure SQL DB was flipped on - and NOT by pattern-matching the Azure change — see the replica-attribution comment below for - the full probe, which answered both gates in one session. The short form: plan_type_desc binds - on MI (COL_LENGTH = 120), measured on an instance following the CONSERVATIVE update policy. */ - bool hasPlanType = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* Replica attribution — SQL Server 2022+ (product version >= 16). Controls: replica_role. - - "Query Store for secondary replicas" (2022+) gives an AG ONE shared Query Store that lives - on the PRIMARY: secondary-replica workload is streamed to the primary and persisted in the - primary's QS tables, distinguishable only by replica_group_id. We collect from primaries - only (is_primary_replica = 1, in the enumeration cursor) — which is correct and stays — but - without this attribution the primary's "Top Queries by CPU" silently BLENDS secondary - workload into the primary's own numbers, with no way for a reader to tell. (Microsoft's own - Query Performance Insight has this exact bug.) - - Gated on >= 16, NOT the docs' claimed 2025+: sys.query_store_replicas and - sys.query_store_runtime_stats.replica_group_id both verified present on SQL 2022 - (16.0.4255.1) and SQL 2025 (17.0.4045.5). - - LEFT JOIN, deliberately: on a 2022 standalone (non-AG) server sys.query_store_replicas has - ZERO rows, yet real runtime-stats rows still carry replica_group_id = 1. An INNER JOIN — or - a WHERE replica_name = 'Primary' filter — would match nothing and silently delete ALL Query - Store collection on every 2022 standalone server. Do not "tighten" this. - - replica_name is read DIRECTLY rather than mapped from role_type: contrary to the docs, it IS - populated on box SQL Server (observed: Primary, Secondary, Geo Secondary, Geo HA Secondary), - and the docs' sample CASEs replica_group_id as though it were role_type — it is not, it is a - replica SET number that accumulates per role across failovers. - - Resulting replica_role: NULL on a 2022 standalone, 'Primary' on a 2025 standalone, the actual - role on an AG with the feature enabled. NULL honestly means "the server did not attribute - this row" and is deliberately NOT coalesced to an invented value. - - Azure SQL DB is ON, riding the same "Azure means newest" rule plan_type_desc gets above — - but only because the live probe the previous version of this comment demanded was actually - run. It was gated OFF from #1836 until then, and that carve-out was about bind SAFETY, not - taste: a missing column is not a NULL, it fails the whole SELECT for that database, and on - Azure this collector's per-database loop would then fail in EVERY database — a worse outcome - than declining the attribution. The docs still do not settle it (replica_group_id's - applies-to note names only "SQL Server (Starting with SQL Server 2022 (16.x))", and Query - Store for secondary replicas is documented as unavailable on the Hyperscale service tier, - silent on whether the view and column still BIND there), so this is flipped on live evidence - instead — gathered on both service tiers the old comment worried about, 2026-07-31 UTC, both - running Microsoft SQL Azure (RTM) 12.0.2000.8, EngineEdition 5: - - - The exact probe the old comment specified, answered on both: General Purpose - (GP_S_Gen5_1, #1848) and Hyperscale (HS_S_Gen5_2, #1872) each returned - OBJECT_ID('sys.query_store_replicas') = -660 and - COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8. - - sys.query_store_replicas binds on both and holds the same 4 role rows box SQL Server has - (Primary, Secondary, Geo Secondary, Geo HA Secondary) — a static enumeration of ROLES, - not instances, which is why a database with 0 HA replicas does not empty it. Every - runtime-stats row carries replica_group_id = 1 and LEFT JOINs cleanly to 'Primary'. - - Decisive (#1872): the full 55-column payload composed with hasReplicaAttribution = true — - the exact text this method emits — executed on Hyperscale. 55 of 55 columns bound, exit - 0, and replica_role came back 'Primary' on every row. - - A per-tier gate was considered and rejected: Hyperscale reports EngineEdition 5, the same as - General Purpose, so the collector cannot tell the tiers apart at query-build time. It does - not need to — both bind identically. - - Managed Instance is ON as of #1886 — the probe the previous version of this comment demanded - was run, so nobody needs to provision an MI to re-answer this. MI was held back through #1844 - and #1872 for a reason that does NOT apply to Azure SQL DB and had to be retired on its own - terms: MI is not evergreen. Its feature set follows a per-instance UPDATE POLICY, so "Azure - means 2022+" is a claim about the fleet that does not transfer to a specific instance, and an - MI on an older policy genuinely might not have the catalog. The bar #1886 set for the simple - edition gate was therefore stricter than Azure's: the catalog must be present on the OLDEST - update policy still in support, not merely on whatever instance was to hand. - - Measured 2026-07-31 on a GPv2 Gen5 4-vCore Managed Instance, westus3, provisioned for the run - and torn down after — reporting ProductVersion 12.0.2000.8, EngineEdition 8, and crucially - SERVERPROPERTY('ProductUpdateType') = 'CU', i.e. the CONSERVATIVE (SQL Server 2022) update - policy rather than Always-up-to-date. That is exactly the "oldest policy still in support" - case, so the bar is met without an AlwaysUpToDate instance: catalog presence is a 2022-surface - fact, not an evergreen one. - - - OBJECT_ID('sys.query_store_replicas') = -660 and - COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8 — the same two-value - probe #1848/#1872 ran on Azure SQL DB, non-NULL on both counts. - - Answered THROUGH THE COLLECTOR'S OWN MECHANISM, not just in master: the same two values - came back from a user-database context via [db].sys.sp_executesql (msdb standing in, since - these catalog views are per-database). MI takes the on-prem enumeration path, so binding in - master would not have settled it — that path is what the issue said MI lacked. - - COL_LENGTH('sys.query_store_plan', 'plan_type_desc') = 120, which is what flips the - hasPlanType carve-out above in the same session rather than provisioning MI twice. - - A standalone MI's sys.query_store_replicas is an EMPTY enumeration — zero rows — unlike Azure - SQL DB's, which is a static 4-row roles table even with no replicas present. That difference - is worth stating because it looks alarming and is not: BINDING is the collection-safety - question and the answer is yes, while an empty enumeration only means a GP instance with no - read replicas has nothing to attribute. The LEFT JOIN below is what makes that harmless — it - is the same shape that keeps a 2022 standalone (whose view is also empty) collecting, and - tightening it would break both. replica_role simply reads NULL there, which is the honest - state rather than an invented one. */ - bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; - - /* 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). - - 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,"; - - string logBytesCols = isNew - ? "avg_log_bytes_used = qsrs.avg_log_bytes_used, min_log_bytes_used = qsrs.min_log_bytes_used, max_log_bytes_used = qsrs.max_log_bytes_used," - : "avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,"; - - string tempdbCols = isNew - ? "avg_tempdb_space_used = qsrs.avg_tempdb_space_used, min_tempdb_space_used = qsrs.min_tempdb_space_used, max_tempdb_space_used = qsrs.max_tempdb_space_used," - : "avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,"; - - string planForcingCol = isNew - ? "plan_forcing_type = qsp.plan_forcing_type_desc," - : "plan_forcing_type = NULL,"; - - string planTypeCol = hasPlanType - ? "plan_type = qsp.plan_type_desc," - : "plan_type = NULL,"; - - /* Execution-plan capture — mirrors the full Dashboard's @collect_plan path in - install/09_collect_query_store.sql: CONVERT(nvarchar(max), qsp.query_plan) from - sys.query_store_plan, no size guard. On only when the host sets CapturePlanXml (Darling); - off = the nvarchar(1) NULL placeholder (Lite), byte-identical to the no-plan form. No - single quotes, so it splices straight into the sp_executesql body. - - #1556 plan-text dedupe (ON branch only): a plan is landed ONCE per plan_id per cycle — on its - newest runtime-stats interval in the window (rn = 1) — and NULL on the older intervals, instead - of repeating the full plan XML on every interval row of the same plan. The partition ORDER BY - stays DESC even though the outer sort is now ASC (#1960): under oldest-first shipping a plan's - rn = 1 row sorts LAST among its rows, so a bounded cycle can cut before it and ship that plan's - intervals without XML — harmless, because rn is recomputed over the NEXT cycle's window, whose - newest-in-window row carries the XML then; steady-state cycles see one interval per plan and are - unaffected. The consumers tolerate the per-row NULL — Lite selects NULL for the grid and fetches - plans live, and Darling's stored-plan readers all guard `query_plan_text IS NOT NULL`. Not - mirrored into the Dashboard proc: its "Download Plan" reads by exact collection_id, where - per-row NULLs would break a real reader. */ - string planTextCol = context.CapturePlanXml - ? "query_plan_text = CASE WHEN ROW_NUMBER() OVER (PARTITION BY qsp.plan_id ORDER BY qsrs.last_execution_time DESC) = 1 THEN CONVERT(nvarchar(max), qsp.query_plan) ELSE CONVERT(nvarchar(max), NULL) END," - : "query_plan_text = CONVERT(nvarchar(1), NULL),"; - - /* The replica-attribution column + its join (see hasReplicaAttribution above). Selected after every - version-gated column, so pre-2022 targets read the nvarchar(1) NULL placeholder at the same - ordinal — byte-identical shape to the attributed form. The interval-identity pair (#1841 tier 2) - follows it and is NOT version-gated, so this fragment stays comma-free and the template supplies - the separator. */ - string replicaRoleCol = hasReplicaAttribution - ? "replica_role = qsr.replica_name" - : "replica_role = CONVERT(nvarchar(1), NULL)"; - - string replicaJoin = hasReplicaAttribution - ? "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 - a 14.9s read; the field A/B measured 4.3x faster without it) — and no predicate shape fixes - that server-side: the QS internal text table has no index on the column, so every variant is - a residual scan whose cost is the bytes it reads. The exclusion instead happens in the read - loop, where the query text is ALREADY materialized for every row — a client-side Contains at - zero SQL cost, identical semantics. Self rows cross the wire (~2% of a busy database's rows) - and are dropped before they enter the batch (never stored, never counted against the byte - budget). The query still CONTAINS the marker, in its own leading comment. */ - - /* Interval identity (#1841 tier 2), the last two SELECT items. Not version-gated: both - sys.query_store_runtime_stats.runtime_stats_interval_id and the - sys.query_store_runtime_stats_interval catalog view are original Query Store surface, verified - present on SQL Server 2016 SP3 (13.0.6300.2) — the collector's own AppliesTo floor — so there is - no target this collector runs on that lacks them. - - LEFT JOIN, not JOIN, for the same reason the replica join above is one: an INNER JOIN here would - make every runtime-stats row's survival depend on its interval row resolving, and a Query Store - that trimmed an interval row out from under us would silently delete real collection rather than - lose one column. The id comes off qsrs directly and is unaffected either way; only - interval_start_time_utc goes NULL if the join misses. - - start_time is datetimeoffset. AT TIME ZONE 'UTC' re-expresses it at +00:00 and the CONVERT drops - the offset, so the stored value is naive UTC — the same clock as collection_time and as - first_execution_time (which ReadRowsAsync already normalizes via DateTimeOffset.UtcDateTime). - That is what makes it safe to bucket on: it is NOT the monitored server's local wall clock. - 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. */ - /* Oldest-first + WITH TIES (#1960): rows ship FORWARD from the watermark, so a bounded cycle - (byte budget or this TOP) leaves the derived watermark — MAX(last_execution_time) over the - rows actually stored — sitting exactly at the shipped boundary, and the next cycle's strict - `> @cutoff_time` resumes there with no hole. WITH TIES is load-bearing for that invariant: - a bare TOP could split a group of rows sharing the boundary last_execution_time, stranding - the unshipped half behind the strict comparison forever. The client byte budget completes - boundary groups the same way (see ReadRowsAsync). */ - /* Backfill (#2022) is the mirror image: newest-first DESC inside (floor, ceiling), where the - ceiling is the DERIVED backfill boundary — MIN(last_execution_time) over the rows already - stored for the database — so each bounded slice leaves the next ceiling sitting exactly at - its oldest shipped row, and the next slice's strict `< @ceiling_time` resumes with no hole - or re-ship. Same TIES, same budget, same tie-group completion; only the window and the - direction differ. */ - /* The interval pre-filter resolves candidate interval ids from the INTERVAL CATALOG - (sys.query_store_runtime_stats_interval, ~one row per interval of retained history — hundreds - of rows) rather than from runtime_stats itself (#2133; measured on the field store: 20 ms vs - 426 ms for the identical id set). end_time/start_time are datetimeoffset; the datetime2 - parameters promote with a zero offset, i.e. as the UTC instants they are — the same implicit - promotion the HAVING's last_execution_time comparison has always relied on. The catalog bound - is a SUPERSET (an interval can end after the cutoff while all its rows are older); the HAVING - below stays the exact row-level filter, so shipped semantics are unchanged. */ - var intervalPreFilter = backfill - ? @"i.end_time > @floor_time - AND i.start_time < @ceiling_time" - : "i.end_time > @cutoff_time"; - var intervalHaving = backfill - ? @"MAX(qsrs.last_execution_time) > @floor_time - AND MAX(qsrs.last_execution_time) < @ceiling_time" - : "MAX(qsrs.last_execution_time) > @cutoff_time"; - var shipOrder = backfill ? "DESC" : "ASC"; - - /* STAGED, not monolithic (#2133). Joining the slice aggregate straight into the - query_store_plan/query/text TVFs handed the optimizer nothing but fixed-guess cardinalities, - and the shape it picked re-materialized a TVF per probe — a fixed cost no window width could - reduce. Field bisection on an 82k-plan catalog (echo, SQL 2022): the aggregate alone ran in - 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-qsp could not finish in 30 s, - hinted or not; staged through the temp the same work totaled 524 ms (56 stage + 409 join). - That fixed cost is what wedged the big-catalog databases at EVERY catch-up width and made - #2125's shrink floor-pin instead of converge. The temp gives the final join REAL row counts — - and for that reason the old LOOP JOIN hint must NOT return: looping from the temp into the - TVFs is the same per-probe re-materialization by another name; the 524 ms join is unhinted, - chosen by the optimizer from true cardinalities. sp_QuickieStore stages for the same reason. - - Batch mechanics: SELECT INTO emits no result set, so the batch still returns exactly ONE - result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql - nesting the temp's scope dies with the invocation; on Azure's direct per-database path the - leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship - order, and the derived-watermark semantics live on the final SELECT, unchanged. */ - return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; + /* Detect server version for version-gated columns. + isNew = true for SQL Server 2017+ (product version > 13) or Azure SQL DB/MI. + Controls: avg_num_physical_io_reads, avg_log_bytes_used, avg_tempdb_space_used, plan_forcing_type_desc. + hasPlanType = true for SQL Server 2022+ (product version >= 16), and on Azure SQL DB/MI. + Controls: plan_type_desc. */ + var productVersion = context.EnumerationProbeResult is null + ? DefaultProductVersion + : Convert.ToInt32(context.EnumerationProbeResult, CultureInfo.InvariantCulture); + bool isNew = productVersion > 13 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* plan_type_desc: version-gated on box SQL Server, but ALWAYS on for Azure SQL DB, which the + version probe cannot speak for — it reports PRODUCTVERSION major 12 (the same reason isNew + overrides above), while the engine underneath is evergreen and never older than 2022. The + column's own catalog-view page lists Azure SQL Database in its Applies-to banner and names + exactly one platform where referencing it errors — Azure Synapse Analytics, engine edition 6, + which is never IsAzureSqlDb (edition 5). + + Managed Instance is ON as of #1886, on the same live-evidence basis Azure SQL DB was flipped on + and NOT by pattern-matching the Azure change — see the replica-attribution comment below for + the full probe, which answered both gates in one session. The short form: plan_type_desc binds + on MI (COL_LENGTH = 120), measured on an instance following the CONSERVATIVE update policy. */ + bool hasPlanType = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* Replica attribution — SQL Server 2022+ (product version >= 16). Controls: replica_role. + + "Query Store for secondary replicas" (2022+) gives an AG ONE shared Query Store that lives + on the PRIMARY: secondary-replica workload is streamed to the primary and persisted in the + primary's QS tables, distinguishable only by replica_group_id. We collect from primaries + only (is_primary_replica = 1, in the enumeration cursor) — which is correct and stays — but + without this attribution the primary's "Top Queries by CPU" silently BLENDS secondary + workload into the primary's own numbers, with no way for a reader to tell. (Microsoft's own + Query Performance Insight has this exact bug.) + + Gated on >= 16, NOT the docs' claimed 2025+: sys.query_store_replicas and + sys.query_store_runtime_stats.replica_group_id both verified present on SQL 2022 + (16.0.4255.1) and SQL 2025 (17.0.4045.5). + + LEFT JOIN, deliberately: on a 2022 standalone (non-AG) server sys.query_store_replicas has + ZERO rows, yet real runtime-stats rows still carry replica_group_id = 1. An INNER JOIN — or + a WHERE replica_name = 'Primary' filter — would match nothing and silently delete ALL Query + Store collection on every 2022 standalone server. Do not "tighten" this. + + replica_name is read DIRECTLY rather than mapped from role_type: contrary to the docs, it IS + populated on box SQL Server (observed: Primary, Secondary, Geo Secondary, Geo HA Secondary), + and the docs' sample CASEs replica_group_id as though it were role_type — it is not, it is a + replica SET number that accumulates per role across failovers. + + Resulting replica_role: NULL on a 2022 standalone, 'Primary' on a 2025 standalone, the actual + role on an AG with the feature enabled. NULL honestly means "the server did not attribute + this row" and is deliberately NOT coalesced to an invented value. + + Azure SQL DB is ON, riding the same "Azure means newest" rule plan_type_desc gets above — + but only because the live probe the previous version of this comment demanded was actually + run. It was gated OFF from #1836 until then, and that carve-out was about bind SAFETY, not + taste: a missing column is not a NULL, it fails the whole SELECT for that database, and on + Azure this collector's per-database loop would then fail in EVERY database — a worse outcome + than declining the attribution. The docs still do not settle it (replica_group_id's + applies-to note names only "SQL Server (Starting with SQL Server 2022 (16.x))", and Query + Store for secondary replicas is documented as unavailable on the Hyperscale service tier, + silent on whether the view and column still BIND there), so this is flipped on live evidence + instead — gathered on both service tiers the old comment worried about, 2026-07-31 UTC, both + running Microsoft SQL Azure (RTM) 12.0.2000.8, EngineEdition 5: + + - The exact probe the old comment specified, answered on both: General Purpose + (GP_S_Gen5_1, #1848) and Hyperscale (HS_S_Gen5_2, #1872) each returned + OBJECT_ID('sys.query_store_replicas') = -660 and + COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8. + - sys.query_store_replicas binds on both and holds the same 4 role rows box SQL Server has + (Primary, Secondary, Geo Secondary, Geo HA Secondary) — a static enumeration of ROLES, + not instances, which is why a database with 0 HA replicas does not empty it. Every + runtime-stats row carries replica_group_id = 1 and LEFT JOINs cleanly to 'Primary'. + - Decisive (#1872): the full 55-column payload composed with hasReplicaAttribution = true — + the exact text this method emits — executed on Hyperscale. 55 of 55 columns bound, exit + 0, and replica_role came back 'Primary' on every row. + + A per-tier gate was considered and rejected: Hyperscale reports EngineEdition 5, the same as + General Purpose, so the collector cannot tell the tiers apart at query-build time. It does + not need to — both bind identically. + + Managed Instance is ON as of #1886 — the probe the previous version of this comment demanded + was run, so nobody needs to provision an MI to re-answer this. MI was held back through #1844 + and #1872 for a reason that does NOT apply to Azure SQL DB and had to be retired on its own + terms: MI is not evergreen. Its feature set follows a per-instance UPDATE POLICY, so "Azure + means 2022+" is a claim about the fleet that does not transfer to a specific instance, and an + MI on an older policy genuinely might not have the catalog. The bar #1886 set for the simple + edition gate was therefore stricter than Azure's: the catalog must be present on the OLDEST + update policy still in support, not merely on whatever instance was to hand. + + Measured 2026-07-31 on a GPv2 Gen5 4-vCore Managed Instance, westus3, provisioned for the run + and torn down after — reporting ProductVersion 12.0.2000.8, EngineEdition 8, and crucially + SERVERPROPERTY('ProductUpdateType') = 'CU', i.e. the CONSERVATIVE (SQL Server 2022) update + policy rather than Always-up-to-date. That is exactly the "oldest policy still in support" + case, so the bar is met without an AlwaysUpToDate instance: catalog presence is a 2022-surface + fact, not an evergreen one. + + - OBJECT_ID('sys.query_store_replicas') = -660 and + COL_LENGTH('sys.query_store_runtime_stats', 'replica_group_id') = 8 — the same two-value + probe #1848/#1872 ran on Azure SQL DB, non-NULL on both counts. + - Answered THROUGH THE COLLECTOR'S OWN MECHANISM, not just in master: the same two values + came back from a user-database context via [db].sys.sp_executesql (msdb standing in, since + these catalog views are per-database). MI takes the on-prem enumeration path, so binding in + master would not have settled it — that path is what the issue said MI lacked. + - COL_LENGTH('sys.query_store_plan', 'plan_type_desc') = 120, which is what flips the + hasPlanType carve-out above in the same session rather than provisioning MI twice. + + A standalone MI's sys.query_store_replicas is an EMPTY enumeration — zero rows — unlike Azure + SQL DB's, which is a static 4-row roles table even with no replicas present. That difference + is worth stating because it looks alarming and is not: BINDING is the collection-safety + question and the answer is yes, while an empty enumeration only means a GP instance with no + read replicas has nothing to attribute. The LEFT JOIN below is what makes that harmless — it + is the same shape that keeps a 2022 standalone (whose view is also empty) collecting, and + tightening it would break both. replica_role simply reads NULL there, which is the honest + state rather than an invented one. */ + bool hasReplicaAttribution = productVersion >= 16 || context.Target.IsAzureSqlDb || context.Target.IsAzureManagedInstance; + + /* 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). + + 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,"; + + string logBytesCols = isNew + ? "avg_log_bytes_used = qsrs.avg_log_bytes_used, min_log_bytes_used = qsrs.min_log_bytes_used, max_log_bytes_used = qsrs.max_log_bytes_used," + : "avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,"; + + string tempdbCols = isNew + ? "avg_tempdb_space_used = qsrs.avg_tempdb_space_used, min_tempdb_space_used = qsrs.min_tempdb_space_used, max_tempdb_space_used = qsrs.max_tempdb_space_used," + : "avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,"; + + string planForcingCol = isNew + ? "plan_forcing_type = qsp.plan_forcing_type_desc," + : "plan_forcing_type = NULL,"; + + string planTypeCol = hasPlanType + ? "plan_type = qsp.plan_type_desc," + : "plan_type = NULL,"; + + /* Execution-plan capture — mirrors the full Dashboard's @collect_plan path in + install/09_collect_query_store.sql: CONVERT(nvarchar(max), qsp.query_plan) from + sys.query_store_plan, no size guard. On only when the host sets CapturePlanXml (Darling); + off = the nvarchar(1) NULL placeholder (Lite), byte-identical to the no-plan form. No + single quotes, so it splices straight into the sp_executesql body. + + #1556 plan-text dedupe (ON branch only): a plan is landed ONCE per plan_id per cycle — on its + newest runtime-stats interval in the window (rn = 1) — and NULL on the older intervals, instead + of repeating the full plan XML on every interval row of the same plan. The partition ORDER BY + stays DESC even though the outer sort is now ASC (#1960): under oldest-first shipping a plan's + rn = 1 row sorts LAST among its rows, so a bounded cycle can cut before it and ship that plan's + intervals without XML — harmless, because rn is recomputed over the NEXT cycle's window, whose + newest-in-window row carries the XML then; steady-state cycles see one interval per plan and are + unaffected. The consumers tolerate the per-row NULL — Lite selects NULL for the grid and fetches + plans live, and Darling's stored-plan readers all guard `query_plan_text IS NOT NULL`. Not + mirrored into the Dashboard proc: its "Download Plan" reads by exact collection_id, where + per-row NULLs would break a real reader. */ + string planTextCol = context.CapturePlanXml + ? "query_plan_text = CASE WHEN ROW_NUMBER() OVER (PARTITION BY qsp.plan_id ORDER BY qsrs.last_execution_time DESC) = 1 THEN CONVERT(nvarchar(max), qsp.query_plan) ELSE CONVERT(nvarchar(max), NULL) END," + : "query_plan_text = CONVERT(nvarchar(1), NULL),"; + + /* The replica-attribution column + its join (see hasReplicaAttribution above). Selected after every + version-gated column, so pre-2022 targets read the nvarchar(1) NULL placeholder at the same + ordinal — byte-identical shape to the attributed form. The interval-identity pair (#1841 tier 2) + follows it and is NOT version-gated, so this fragment stays comma-free and the template supplies + the separator. */ + string replicaRoleCol = hasReplicaAttribution + ? "replica_role = qsr.replica_name" + : "replica_role = CONVERT(nvarchar(1), NULL)"; + + string replicaJoin = hasReplicaAttribution + ? "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 + a 14.9s read; the field A/B measured 4.3x faster without it) — and no predicate shape fixes + that server-side: the QS internal text table has no index on the column, so every variant is + a residual scan whose cost is the bytes it reads. The exclusion instead happens in the read + loop, where the query text is ALREADY materialized for every row — a client-side Contains at + zero SQL cost, identical semantics. Self rows cross the wire (~2% of a busy database's rows) + and are dropped before they enter the batch (never stored, never counted against the byte + budget). The query still CONTAINS the marker, in its own leading comment. */ + + /* Interval identity (#1841 tier 2), the last two SELECT items. Not version-gated: both + sys.query_store_runtime_stats.runtime_stats_interval_id and the + sys.query_store_runtime_stats_interval catalog view are original Query Store surface, verified + present on SQL Server 2016 SP3 (13.0.6300.2) — the collector's own AppliesTo floor — so there is + no target this collector runs on that lacks them. + + LEFT JOIN, not JOIN, for the same reason the replica join above is one: an INNER JOIN here would + make every runtime-stats row's survival depend on its interval row resolving, and a Query Store + that trimmed an interval row out from under us would silently delete real collection rather than + lose one column. The id comes off qsrs directly and is unaffected either way; only + interval_start_time_utc goes NULL if the join misses. + + start_time is datetimeoffset. AT TIME ZONE 'UTC' re-expresses it at +00:00 and the CONVERT drops + the offset, so the stored value is naive UTC — the same clock as collection_time and as + first_execution_time (which ReadRowsAsync already normalizes via DateTimeOffset.UtcDateTime). + That is what makes it safe to bucket on: it is NOT the monitored server's local wall clock. + 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. */ + /* Oldest-first + WITH TIES (#1960): rows ship FORWARD from the watermark, so a bounded cycle + (byte budget or this TOP) leaves the derived watermark — MAX(last_execution_time) over the + rows actually stored — sitting exactly at the shipped boundary, and the next cycle's strict + `> @cutoff_time` resumes there with no hole. WITH TIES is load-bearing for that invariant: + a bare TOP could split a group of rows sharing the boundary last_execution_time, stranding + the unshipped half behind the strict comparison forever. The client byte budget completes + boundary groups the same way (see ReadRowsAsync). */ + /* Backfill (#2022) is the mirror image: newest-first DESC inside (floor, ceiling), where the + ceiling is the DERIVED backfill boundary — MIN(last_execution_time) over the rows already + stored for the database — so each bounded slice leaves the next ceiling sitting exactly at + its oldest shipped row, and the next slice's strict `< @ceiling_time` resumes with no hole + or re-ship. Same TIES, same budget, same tie-group completion; only the window and the + direction differ. */ + /* The interval pre-filter resolves candidate interval ids from the INTERVAL CATALOG + (sys.query_store_runtime_stats_interval, ~one row per interval of retained history — hundreds + of rows) rather than from runtime_stats itself (#2133; measured on the field store: 20 ms vs + 426 ms for the identical id set). end_time/start_time are datetimeoffset; the datetime2 + parameters promote with a zero offset, i.e. as the UTC instants they are — the same implicit + promotion the HAVING's last_execution_time comparison has always relied on. The catalog bound + is a SUPERSET (an interval can end after the cutoff while all its rows are older); the HAVING + below stays the exact row-level filter, so shipped semantics are unchanged. */ + var intervalPreFilter = backfill + ? @"i.end_time > @floor_time + AND i.start_time < @ceiling_time" + : "i.end_time > @cutoff_time"; + var intervalHaving = backfill + ? @"MAX(qsrs.last_execution_time) > @floor_time + AND MAX(qsrs.last_execution_time) < @ceiling_time" + : "MAX(qsrs.last_execution_time) > @cutoff_time"; + var shipOrder = backfill ? "DESC" : "ASC"; + + /* STAGED, not monolithic (#2133). Joining the slice aggregate straight into the + query_store_plan/query/text TVFs handed the optimizer nothing but fixed-guess cardinalities, + and the shape it picked re-materialized a TVF per probe — a fixed cost no window width could + reduce. Field bisection on an 82k-plan catalog (echo, SQL 2022): the aggregate alone ran in + 81 ms and each TVF scanned bare in ~300 ms, yet aggregate-JOIN-qsp could not finish in 30 s, + hinted or not; staged through the temp the same work totaled 524 ms (56 stage + 409 join). + That fixed cost is what wedged the big-catalog databases at EVERY catch-up width and made + #2125's shrink floor-pin instead of converge. The temp gives the final join REAL row counts — + and for that reason the old LOOP JOIN hint must NOT return: looping from the temp into the + TVFs is the same per-probe re-materialization by another name; the 524 ms join is unhinted, + chosen by the optimizer from true cardinalities. sp_QuickieStore stages for the same reason. + + Batch mechanics: SELECT INTO emits no result set, so the batch still returns exactly ONE + result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql + nesting the temp's scope dies with the invocation; on Azure's direct per-database path the + leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship + order, and the derived-watermark semantics live on the final SELECT, unchanged. */ + return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; DROP TABLE IF EXISTS #pm_qs_slice; @@ -829,35 +829,35 @@ result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_exe 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} + 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} INTO #pm_qs_slice FROM sys.query_store_runtime_stats AS qsrs WHERE qsrs.runtime_stats_interval_id IN From efeea70511fc6c8c9b7d49e197dccc9220e60b50 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 8 Aug 2026 23:55:21 +0200 Subject: [PATCH 5/6] =?UTF-8?q?Fix=20the=20four=20structure=20pins=20CI=20?= =?UTF-8?q?caught=20=E2=80=94=20staged=20shape,=20surgical=20this=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four tests parsed the old derived-table structure: the aggregate's bounds (FROM-paren to ') AS qsrs' — now the staging SELECT to INTO), the GROUP BY indent (one level shallower as a top-level statement), and the list-tail-to-FROM seam (INTO #pm_qs_slice now sits between). Pins updated to the emitted shape; no source-file rewriting involved. (The previous attempt at this — 82dea99b, reverted in 503a087f — used a python block-dedent whose start anchor matched the FIRST marker comment in the file, 700 lines early, and quietly reindented the enumeration query and everything after it. Reverted rather than rewritten; the pins below are Edit-tool surgical.) Co-Authored-By: Claude Fable 5 --- .../QueryStoreCollectorDefinitionTests.cs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index 0e11af4b..ab590aec 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -465,7 +465,7 @@ public void BuildPerItemQuery_CombinesTheSlicesOfOneInterval_OnTheViewsNaturalKe Anything coarser would merge work that is genuinely distinct; anything finer would leave the slices split, which is the bug. */ Assert.Contains( - "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", + "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", text, StringComparison.Ordinal); @@ -532,11 +532,13 @@ public void Payload_EveryAverageColumn_IsTheCountWeightedMean() { var text = PayloadSql(MakeContext(probeResult: 16)); - /* Only the aggregating derived table — the outer projection references the same names as plain - columns, which is correct there and must not be mistaken for an un-weighted aggregate. */ - var open = text.IndexOf("FROM\n(", StringComparison.Ordinal); - var close = text.IndexOf(") AS qsrs", StringComparison.Ordinal); - Assert.True(open > 0 && close > open, "could not locate the slice-aggregating derived table"); + /* Only the aggregating STAGING statement (#2133: the aggregate lands in #pm_qs_slice and the + joins run from it) — the final projection references the same names as plain columns, which + is correct there and must not be mistaken for an un-weighted aggregate. The staging SELECT + is the marker's first occurrence; the final SELECT carries TOP on the marker line. */ + var open = text.IndexOf("SELECT /* PerformanceMonitorLite */\n", StringComparison.Ordinal); + var close = text.IndexOf("INTO #pm_qs_slice", StringComparison.Ordinal); + Assert.True(open > 0 && close > open, "could not locate the slice-aggregating staging statement"); var aggregate = text[open..close]; var averages = System.Text.RegularExpressions.Regex @@ -592,7 +594,7 @@ public void BuildPerItemQuery_ReplicaGroupIdEntersTheGroupingKey_OnlyWhereItBind { var ungated = PayloadSql(MakeContext(probeResult: probe)); Assert.DoesNotContain("replica_group_id", ungated, StringComparison.Ordinal); - Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); + Assert.Contains("GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc\n", ungated, StringComparison.Ordinal); } } @@ -619,13 +621,14 @@ public void BuildPerItemQuery_PreSql2017_GatedFamiliesLeaveTheAggregate_ButKeepT Assert.Contains("avg_log_bytes_used = NULL, min_log_bytes_used = NULL, max_log_bytes_used = NULL,", old, StringComparison.Ordinal); Assert.Contains("avg_tempdb_space_used = NULL, min_tempdb_space_used = NULL, max_tempdb_space_used = NULL,", old, StringComparison.Ordinal); - /* The inner list must end cleanly on the last ungated column when all three are absent. */ - Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\n FROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); + /* The staging list must end cleanly on the last ungated column when all three are absent — + #2133: the aggregate lands in #pm_qs_slice, so INTO sits between the list and FROM. */ + Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", old, StringComparison.Ordinal); /* On 2017+ they are present, aggregated, and the list ends with the last gated family instead. */ var newer = PayloadSql(MakeContext(probeResult: 14)); Assert.Contains("max_rowcount = MAX(qsrs.max_rowcount),\n", newer, StringComparison.Ordinal); - Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\n FROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); + Assert.Contains("max_tempdb_space_used = MAX(qsrs.max_tempdb_space_used)\nINTO #pm_qs_slice\nFROM sys.query_store_runtime_stats AS qsrs", newer, StringComparison.Ordinal); } [Fact] From 92896147257523efdf17ce77fef5330d4d81eb2b Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sun, 9 Aug 2026 00:16:52 +0200 Subject: [PATCH 6/6] Staging statement gets OPTION(RECOMPILE), and the staged block dedents to 4-space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both review catches: - The staging SELECT INTO, split out on its own, would be cached via sp_executesql's parameterized text and sniffed across live vs backfill windows of wildly different selectivity — the fixed-guess failure mode this PR removes, reintroduced one statement earlier. Both statements now carry OPTION(RECOMPILE), documented in the template comment. - The staged block's leftover derived-table indentation (aggregate lines, spliced fragments, backfill window continuations) dedents to the file's consistent 4-space convention, Edit-tool surgical, and the pins assert the clean shape instead of encoding the mixed one. Co-Authored-By: Claude Fable 5 --- .../QueryStoreCollectorDefinitionTests.cs | 6 +- .../QueryStoreCollector.cs | 80 ++++++++++--------- 2 files changed, 46 insertions(+), 40 deletions(-) diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index ab590aec..1772a3de 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -465,7 +465,7 @@ public void BuildPerItemQuery_CombinesTheSlicesOfOneInterval_OnTheViewsNaturalKe Anything coarser would merge work that is genuinely distinct; anything finer would leave the slices split, which is the bug. */ Assert.Contains( - "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", + "GROUP BY\n qsrs.plan_id,\n qsrs.runtime_stats_interval_id,\n qsrs.execution_type_desc,\n qsrs.replica_group_id", text, StringComparison.Ordinal); @@ -583,11 +583,11 @@ public void BuildPerItemQuery_ReplicaGroupIdEntersTheGroupingKey_OnlyWhereItBind foreach (var probe in new object[] { 16, 17 }) { var attributed = PayloadSql(MakeContext(probeResult: probe)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", attributed, StringComparison.Ordinal); } var azure = AzurePayloadSql(MakeContext(isAzureSqlDb: true, probeResult: 12)); - Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); + Assert.Contains("qsrs.execution_type_desc,\n qsrs.replica_group_id", azure, StringComparison.Ordinal); /* Pre-2022 box and Managed Instance: the column must not be named anywhere, GROUP BY included. */ foreach (var probe in new object?[] { 13, 14, 15, null }) diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index cc01602a..3e1d37b9 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -605,15 +605,15 @@ ordinal so the 55-column reader contract never moves. The inner fragments carry 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)" + ? $",\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)" + ? $",\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)" + ? $",\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 @@ -681,7 +681,7 @@ fails the whole SELECT just as naming it in a select list would. When the gate i 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" + ? ",\n qsrs.replica_group_id" : ""; /* There is deliberately NO self-exclusion predicate in this query (#1565, actual-plan evidence @@ -796,11 +796,11 @@ promotion the HAVING's last_execution_time comparison has always relied on. The below stays the exact row-level filter, so shipped semantics are unchanged. */ var intervalPreFilter = backfill ? @"i.end_time > @floor_time - AND i.start_time < @ceiling_time" + AND i.start_time < @ceiling_time" : "i.end_time > @cutoff_time"; var intervalHaving = backfill ? @"MAX(qsrs.last_execution_time) > @floor_time - AND MAX(qsrs.last_execution_time) < @ceiling_time" + AND MAX(qsrs.last_execution_time) < @ceiling_time" : "MAX(qsrs.last_execution_time) > @cutoff_time"; var shipOrder = backfill ? "DESC" : "ASC"; @@ -820,7 +820,12 @@ AND MAX(qsrs.last_execution_time) < @ceiling_time" result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_executesql nesting the temp's scope dies with the invocation; on Azure's direct per-database path the leading DROP TABLE IF EXISTS covers pooled-connection reuse. TOP ... WITH TIES, the ship - order, and the derived-watermark semantics live on the final SELECT, unchanged. */ + order, and the derived-watermark semantics live on the final SELECT, unchanged. + + BOTH statements carry OPTION(RECOMPILE) (review catch): split out on its own, the staging + statement would otherwise be cached via sp_executesql's parameterized text and sniffed + across live vs backfill windows of wildly different selectivity — the same fixed-guess + failure mode this rewrite removes, reintroduced one statement earlier. */ return $@"SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; DROP TABLE IF EXISTS #pm_qs_slice; @@ -829,35 +834,35 @@ result set (the reader/byte-budget contract). Inside the on-prem [db].sys.sp_exe 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} + 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} INTO #pm_qs_slice FROM sys.query_store_runtime_stats AS qsrs WHERE qsrs.runtime_stats_interval_id IN @@ -872,7 +877,8 @@ GROUP BY qsrs.runtime_stats_interval_id, qsrs.execution_type_desc{replicaGroupKey} HAVING - {intervalHaving}; + {intervalHaving} +OPTION(RECOMPILE); SELECT /* PerformanceMonitorLite */ TOP ({MaxRowsPerDatabase}) WITH TIES query_id = qsq.query_id,