diff --git a/CHANGELOG.md b/CHANGELOG.md index 3aa42876..3bc610d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The force-plan recommendation now warns when the regressed query is parameter-sensitive** ([#2138] gap 3) - each regressed_queries row carries a `parameter_sensitivity_cofired` flag, computed inside the drill-down with the PARAMETER_SENSITIVITY detector's own thresholds (one cached plan whose per-execution cost varies >= 10x across parameter values, same floors, same window) joined by query hash - so the flag can never claim evidence the detector would not report. A flagged target's force-plan preview gains a caution block naming the risk (forcing pins ONE shape for every parameter value; the population that preferred the other plan inherits the wrong one permanently, quietly, because a forced plan no longer recompiles away) and the gentler first levers (statistics updates; PSP optimization / Query Store hints on 2022+), and the copy-paste surface gets a compact two-line version of the same warning. Unflagged targets render byte-identically to before. This flag is also the standing gate for the future auto-force bot: a flagged target is never auto-forced. Both SKUs, pinned by live tests in both stores. ### Fixed +- **query_store's plan/text fetch is activity-driven: the store is the watermark** ([#2312]) - the collector's invariant 40-110s-per-run bill on big catalogs had a named mechanism at last: the #2210 watermark walk's daily expiry was supposed to be replaced by a re-verify cursor that was built, tested, documented, and **never wired** - so catalogs whose full walk needs more than a day expired MID-walk, restarted from plan_id 0, and looped the full catalog fetch forever (Finding 4). `TouchSql`, the liveness refresh the dimension GC depends on, had the same story: designed as 'the whole of the protection,' zero callers (Finding 3) - latent only because the perpetual walk's re-upserts accidentally stood in for it. The reshape retires all of it: the cycle's collected rows name their plans/texts, one touch-and-probe round trip refreshes map/dim liveness AND answers what the store lacks (plus per-cycle in-place-rewrite and Query-Store-reset detection via the live hashes the payload already carries), and the fetch selects exactly the missing ids under the same byte-budget arithmetic. A caught-up database issues NO target query - the measured 23s-to-discover-nothing becomes nothing; a Query Store reset recovers as the normal path instead of a special arm; a dormant plan resuming execution is fetched the cycle it resumes instead of waiting on a refresh horizon. V77 carries the schema strokes (nullable map digest for the content-less NULL-XML marker, `query_store_text.query_hash`, and wholesale deletion of the orphaned `planwm:`/`textwm:` state rows). Budget-deferred ids carry over in memory so a plan referenced once cannot be starved; ids the target no longer serves are dropped only on a provably-uncut pass. This also bends #2316's plan-dimension growth going forward: plans never referenced by a collected row are no longer shipped or stored at all. - **Darling Viewer crash on Queries -> Query Store by Duration** ([#2181], [#2331]) - the same uncatchable crash class as Lite's #2114, on the OTHER SKU: the grid's inline View Plan button referenced `DarkButton`, a key that IS defined in the Viewer - in `MainWindow.xaml`'s window resources, a scope a UserControl's templates cannot see, because StaticResource resolves lexically at load rather than through the runtime tree. The miss inside a cell template stack-overflows the process the moment the grid renders a row, which is also why it survived dogfooding: an EMPTY Query Store grid never applies its cell template. #2181 reported this against the Darling Viewer and was closed as a duplicate of the Lite fix on a wrong premise; #2331 re-proved it on 3.4.0. The button uses default chrome now (Lite's exact fix), and the XAML hygiene test's model is widened from per-app to per-FILE resolution (own keys + merged dictionaries + App.xaml scope - WPF's actual lookup), which flags exactly this class and produced zero false positives across both apps. - **The store self-metrics sweep's ~5-a-day "Exception while reading from stream" ERRORs were command timeouts in a network-fault costume** ([#2317]) - the sweep's sizing queries (`hypertable_detailed_size` across every hypertable - its inner `hypertable_local_size` is the frame the server log names - and `pg_database_size` over the whole store) ran on Npgsql's default 30-second timeout, which they outgrew under load on a 141-object store with a 100+ GB plan dimension. Npgsql enforces its deadline by cancelling the statement (the store side logs `canceling statement due to user request` - confirmed at the exact failure timestamps in the managed server's own log) and the client is left holding a torn stream, so the ERROR read as "the network broke" - the same misdirection #2294 named on the baseline path, one layer over. Every sweep statement now carries a five-minute timeout, and the worker caps the WHOLE sweep at the same five minutes through a linked cancellation (the sweep is awaited on the main loop, so five sequential statement timeouts must not stack into a 25-minute stall of per-server dispatch), the statement count is pinned to the timeout count so a sixth statement cannot ride the default back in, and the worker's catch names a timeout as a timeout - a sweep that still cannot finish skips the tick and the series gains a self-healing one-hour gap, deliberately NOT retrying into the same load. - **Gapped charts no longer bury their neighbours under opaque black fill** ([#2324]) - the #1944 gap markers (a NaN Y injected mid-gap so lines break across an outage) shipped first in 3.4.0, and they collide with the gradient area fill: reproduced headlessly against ScottPlot 5.1.59, one NaN in a FillY + ColorPositions series renders the ribbon as opaque black polygons with straight chord edges crossing the gap - the fill path closes its contours through the break, and its fill paint under ColorPositions is hardcoded black with the gradient shader expected to paint over it, which a NaN-bearing series defeats. On the reporter's dark theme that black buried every other series on every tab whose data had a collection gap; the one healthy tab was the one with gapless data. A gap-marked series now renders line-only - the break stays visible, nothing is buried - and continuous series keep the full gradient ribbon, pinned in both directions so the fix cannot quietly repeal the fill feature. diff --git a/Darling/Darling.Tests/ActivityDrivenPlanFetchStoreTests.cs b/Darling/Darling.Tests/ActivityDrivenPlanFetchStoreTests.cs new file mode 100644 index 00000000..f575d30b --- /dev/null +++ b/Darling/Darling.Tests/ActivityDrivenPlanFetchStoreTests.cs @@ -0,0 +1,121 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Linq; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// The V77 rung (#2312 Finding 2) — the schema strokes behind the activity-driven plan/text fetch: the +/// plan map's digest goes nullable (the content-less marker for plans whose XML the engine cannot +/// persist), query_store_text gains query_hash (the Query Store reset detector), and the +/// retired planwm:/textwm: watermark state rows are deleted wholesale. These facts pin the +/// rung's place on the ladder, the viewer probe's newest-first arm, and the migration SQL's load-bearing +/// strokes. The fetch behavior itself is pinned in QueryStorePlanFetchTests and exercised live in +/// the gated Postgres suites. +/// +public sealed class ActivityDrivenPlanFetchStoreTests +{ + /* ---------------- the rung ---------------- */ + + [Fact] + public void TheRungIsTheTopOfADenseLadder() + { + var versions = PgMigrations.Scripts.Select(s => s.Version).ToList(); + + Assert.Equal(77, versions.Max()); + Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); + Assert.Equal(versions.Distinct().OrderBy(v => v), versions); + + /* Dense above the one sanctioned historical hole at V45. */ + var above = versions.Where(v => v > 45).OrderBy(v => v).ToList(); + Assert.Equal(Enumerable.Range(above[0], above.Count), above); + + Assert.Equal("activity-driven-plan-fetch", PgMigrations.Scripts.Single(s => s.Version == 77).Name); + } + + /// The three strokes, each load-bearing and none allowed to drift out of the rung: without + /// the nullable digest the NULL-XML marker cannot land, without query_hash the reset detector has no + /// stored baseline, and without the deletes the orphaned watermark rows live forever (collector_state + /// has no retention, and the prune set no longer owns those prefixes). + [Fact] + public void TheRungCarriesAllThreeStrokes() + { + var sql = PgMigrations.Scripts.Single(s => s.Version == 77).Sql; + + Assert.Contains("ALTER TABLE collect.query_store_plan_map ALTER COLUMN digest DROP NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("ALTER TABLE collect.query_store_text ADD COLUMN IF NOT EXISTS query_hash text", sql, StringComparison.Ordinal); + Assert.Contains("DELETE FROM collector_state WHERE collector_name = 'query_store_plan_xml' AND state_key LIKE 'planwm:%'", sql, StringComparison.Ordinal); + Assert.Contains("DELETE FROM collector_state WHERE collector_name = 'query_store_text' AND state_key LIKE 'textwm:%'", sql, StringComparison.Ordinal); + } + + /* ---------------- the viewer probe ---------------- */ + + [Fact] + public void TheProbeMapsAFullyMigratedStoreTo77() + { + Assert.Equal(77, StorageVersion.SchemaVersion); + Assert.Equal(StorageVersion.SchemaVersion, ViewerDataService.RequiredStoreSchemaVersion); + + /* 52 positional sentinels then the V77 one by name — the map takes 53 parameters. Present => 77, + newest-first; absent => the previous arm still answers 76 rather than falling through. */ + var all = Enumerable.Repeat(true, 52).Cast().ToArray(); + + Assert.Equal(77, InvokeMap(all, hasQueryStoreTextHash: true)); + Assert.Equal(76, InvokeMap(all, hasQueryStoreTextHash: false)); + } + + [Fact] + public void TheProbeAsksForTheColumn_AndTheThreePlacesAgree() + { + Assert.Contains( + "table_name = 'query_store_text' AND column_name = 'query_hash'", + ViewerDataService.StoreSchemaProbeSql, StringComparison.Ordinal); + + var mapParameters = typeof(ViewerDataService) + .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)! + .GetParameters().Length; + + var viewerSource = ReadViewerSource(); + + /* The reader must hand over exactly one argument per map parameter: ordinals are 0-based, so the + highest is Count - 1, and the next one up must NOT appear. */ + Assert.Contains($"reader.GetBoolean({mapParameters - 1})", viewerSource, StringComparison.Ordinal); + Assert.DoesNotContain($"reader.GetBoolean({mapParameters})", viewerSource, StringComparison.Ordinal); + } + + /* ---------------- helpers ---------------- */ + + private static int InvokeMap(object[] leading, bool hasQueryStoreTextHash) + { + var method = typeof(ViewerDataService) + .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + + var args = leading.Concat(new object[] { hasQueryStoreTextHash }).ToArray(); + Assert.Equal(method.GetParameters().Length, args.Length); + + return (int)method.Invoke(null, args)!; + } + + private static string ReadViewerSource([System.Runtime.CompilerServices.CallerFilePath] string thisFile = "") + { + var dir = System.IO.Path.GetDirectoryName(thisFile)!; + var relative = System.IO.Path.Combine("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.cs"); + while (dir is not null && !System.IO.File.Exists(System.IO.Path.Combine(dir, relative))) + { + dir = System.IO.Path.GetDirectoryName(dir); + } + + Assert.NotNull(dir); + return System.IO.File.ReadAllText(System.IO.Path.Combine(dir!, relative)); + } +} diff --git a/Darling/Darling.Tests/AzureForeignStatePruneTests.cs b/Darling/Darling.Tests/AzureForeignStatePruneTests.cs index 345a3b43..5c0c33b4 100644 --- a/Darling/Darling.Tests/AzureForeignStatePruneTests.cs +++ b/Darling/Darling.Tests/AzureForeignStatePruneTests.cs @@ -125,17 +125,15 @@ public void ADatabaseNamedRegistrationIsPruned(string catalog) [Fact] public void BothArmsPruneEveryPerDatabasePrefix() { - Assert.Equal(5, QueryStorePerDatabaseState.PrunableKeys.Count); - Assert.Contains(QueryStorePerDatabaseState.PrunableKeys, - k => k.Prefix == QueryStorePlanXmlState.WatermarkKeyPrefix); + /* #2312 shrank this from five to three: the planwm:/textwm: watermark families retired with the + watermarks themselves (the fetches are activity-driven against the store now), and V77 deleted + their orphaned rows wholesale — a dropped-database prune has nothing left to own there. */ + Assert.Equal(3, QueryStorePerDatabaseState.PrunableKeys.Count); Assert.Contains(QueryStorePerDatabaseState.PrunableKeys, k => k.Prefix == QueryStoreBackfillState.DoneKeyPrefix); Assert.Contains(QueryStorePerDatabaseState.PrunableKeys, k => k.Prefix == QueryStoreBackfillState.HoleKeyPrefix); - /* #2150: the text watermark, keyed prefix + databaseName exactly like the plan watermark. */ - Assert.Contains(QueryStorePerDatabaseState.PrunableKeys, - k => k.Prefix == QueryStoreTextState.WatermarkKeyPrefix); - /* #2312: the open-interval refresh stamp, the fifth per-database prefix. */ + /* #2312: the open-interval refresh stamp. */ Assert.Contains(QueryStorePerDatabaseState.PrunableKeys, k => k.Prefix == QueryStoreOpenIntervalState.WatermarkKeyPrefix); diff --git a/Darling/Darling.Tests/DarlingDimensionGcBoundTests.cs b/Darling/Darling.Tests/DarlingDimensionGcBoundTests.cs index c4c9b19d..61a29680 100644 --- a/Darling/Darling.Tests/DarlingDimensionGcBoundTests.cs +++ b/Darling/Darling.Tests/DarlingDimensionGcBoundTests.cs @@ -144,32 +144,9 @@ public void NeitherPruneOrder_CanLeaveAMapRowResolvingToAnAbsentDigest(int factR } } - /// - /// #2210: the re-verify cursor paces itself off RefreshAfter and NEVER touches the watermark. The - /// slice is a row count over an id range, which is the whole point — the old expiry walked BYTES and could - /// not finish inside a day on the catalogs that mattered (15.9 to 107.5 hours measured), so those restarted - /// forever. Redstone's 77k ids at a 5-minute cadence over a 1-day sweep is ~267 ids per pass. - /// - [Fact] - public void CursorSlice_PacesASweepWithinTheRefreshPeriod_AndNeverReturnsZeroForALiveCatalog() - { - var day = TimeSpan.FromDays(1); - var cadence = TimeSpan.FromMinutes(5); - - var redstone = QueryStorePlanMap.CursorSliceWidth(77_176, day, cadence); - Assert.InRange(redstone, 200, 350); - - /* A sweep must actually cover the range within the period: slice * passes >= watermark. */ - var passes = day.Ticks / cadence.Ticks; - Assert.True(redstone * passes >= 77_176, "the sweep must cover the id range inside one refresh period"); - - /* Never zero for a live catalog, and never wider than the range itself. */ - Assert.True(QueryStorePlanMap.CursorSliceWidth(10, day, cadence) > 0); - Assert.Equal(10, QueryStorePlanMap.CursorSliceWidth(10, day, cadence)); - - /* A fresh database has no watermark to re-verify, so there is nothing to slice. */ - Assert.Equal(0, QueryStorePlanMap.CursorSliceWidth(0, day, cadence)); - } + /* #2312: the CursorSlice facts that sat here are gone with the cursor itself — it was designed in + #2210 and never wired (Finding 4), and the in-place-rewrite job it existed for now runs per-cycle + through TouchAndProbeSql's hash comparison, pinned in QueryStorePlanFetchTests. */ /// /// #2210: the DIMENSION must outlive the MAP, expressed the way it actually matters — as cutoff DATES from diff --git a/Darling/Darling.Tests/PlanContentRetentionTests.cs b/Darling/Darling.Tests/PlanContentRetentionTests.cs index d30342fe..73686927 100644 --- a/Darling/Darling.Tests/PlanContentRetentionTests.cs +++ b/Darling/Darling.Tests/PlanContentRetentionTests.cs @@ -264,9 +264,10 @@ private static int InvokeMap(object[] leading, bool hasPlanContentRetentionKnob) var method = typeof(ViewerDataService) .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; - /* #2319 appended hasQueryStoreHealth after this rung's parameter — pass it FALSE so these - facts keep exercising the V75/V74 arms rather than the newer one. */ - var args = leading.Concat(new object[] { hasPlanContentRetentionKnob, false }).ToArray(); + /* #2319 appended hasQueryStoreHealth and #2312 appended hasQueryStoreTextHash after this rung's + parameter — pass both FALSE so these facts keep exercising the V75/V74 arms rather than the + newer ones. */ + var args = leading.Concat(new object[] { hasPlanContentRetentionKnob, false, false }).ToArray(); Assert.Equal(method.GetParameters().Length, args.Length); return (int)method.Invoke(null, args)!; diff --git a/Darling/Darling.Tests/QueryStoreFetchProbeLivePostgresTests.cs b/Darling/Darling.Tests/QueryStoreFetchProbeLivePostgresTests.cs new file mode 100644 index 00000000..65e78dae --- /dev/null +++ b/Darling/Darling.Tests/QueryStoreFetchProbeLivePostgresTests.cs @@ -0,0 +1,147 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/// +/// The #2312 touch-and-probe against a REAL store — the one statement the whole activity-driven fetch +/// runs on, whose risk lives entirely in how PostgreSQL evaluates the data-modifying CTEs, the hourly +/// guard, the hash comparisons and the LEFT JOIN together; no source pin can speak to any of it. Also the +/// writer's NULL-digest content-less marker, which V77's nullable column exists for: it must land, read as +/// RESOLVED, and never re-enter the fetch list. +/// +[Collection("live-postgres")] +public sealed class QueryStoreFetchProbeLivePostgresTests +{ + private const string ServerName = "darling-fetch-probe-e2e"; + private static readonly int ServerId = ServerIdHelper.GetDeterministicHashCode(ServerName); + private const string Db = "ProbeDb"; + private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + [Fact] + public async Task TouchAndProbe_AnswersMissingStaleAndMarker_AndRefreshesLiveness() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the live fetch-probe test."); + + var ct = TestContext.Current.CancellationToken; + using var connection = new NpgsqlConnection(cs); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await DeleteRowsAsync(connection, ct); + + var bodySucceeded = false; + try + { + var landedAt = DateTime.UtcNow.AddHours(-3); + + /* Plan 1: real content with a hash. Plan 2: the engine had nothing to give — the writer must + land the NULL-digest marker rather than skipping the row. */ + var landed = await QueryStorePlanWriter.WriteAsync( + connection, ServerId, Db, + new[] + { + new FetchedPlan(1, "", "0xAAAA"), + new FetchedPlan(2, PlanXml: null, PlanHash: "0xBBBB"), + }, + landedAt, ct); + Assert.Equal(new long[] { 1, 2 }, landed); + + using (var marker = new NpgsqlCommand( + "SELECT digest IS NULL FROM collect.query_store_plan_map WHERE server_id = $1 AND database_name = $2 AND plan_id = 2", connection)) + { + marker.Parameters.AddWithValue(ServerId); + marker.Parameters.AddWithValue(Db); + Assert.Equal(true, await marker.ExecuteScalarAsync(ct)); + } + + /* The cycle references four plans: 1 current, 2 the marker, 3 never seen, and 1-with-a-new-hash + is exercised by a second batch below. The probe must return them all, in order. */ + var now = DateTime.UtcNow; + var verdicts = await QueryStoreFetchProbe.TouchAndProbePlansAsync( + connection, ServerId, Db, + new[] { (1L, (string?)"0xAAAA"), (2L, (string?)"0xBBBB"), (3L, (string?)"0xCCCC") }, + now, ct); + + Assert.Equal(3, verdicts.Count); + Assert.Equal(new FetchProbeVerdict(1, Resolved: true, HashStale: false), verdicts[0]); + /* The marker resolves — that is its entire job. */ + Assert.Equal(new FetchProbeVerdict(2, Resolved: true, HashStale: false), verdicts[1]); + Assert.Equal(new FetchProbeVerdict(3, Resolved: false, HashStale: false), verdicts[2]); + + /* Liveness: the touch advanced last_seen past the 3-hour-old landing stamp (the rows were + older than the hourly guard, so the update fired) — on the map AND on the dimension row the + real digest points at. */ + using (var freshness = new NpgsqlCommand(@" +SELECT + (SELECT COUNT(*) FROM collect.query_store_plan_map + WHERE server_id = $1 AND database_name = $2 AND last_seen > $3), + (SELECT COUNT(*) FROM collect.query_plan_dim d + JOIN collect.query_store_plan_map m ON m.digest = d.digest + WHERE m.server_id = $1 AND m.database_name = $2 AND d.last_seen > $3)", connection)) + { + freshness.Parameters.AddWithValue(ServerId); + freshness.Parameters.AddWithValue(Db); + freshness.Parameters.AddWithValue(QueryStorePlanMap.Naive(landedAt.AddMinutes(1))); + await using var reader = await freshness.ExecuteReaderAsync(ct); + Assert.True(await reader.ReadAsync(ct)); + Assert.Equal(2L, reader.GetInt64(0)); /* both referenced map rows touched */ + Assert.Equal(1L, reader.GetInt64(1)); /* the one real dim row touched */ + } + + /* An in-place rewrite: same plan_id, different live hash. Stale, and still resolved — the + caller refetches on the OR of the two. */ + var stale = await QueryStoreFetchProbe.TouchAndProbePlansAsync( + connection, ServerId, Db, new[] { (1L, (string?)"0xDEAD") }, now.AddHours(2), ct); + Assert.Equal(new FetchProbeVerdict(1, Resolved: true, HashStale: true), stale.Single()); + + /* Text side: land one row WITHOUT a hash (the legacy shape), then probe with a live hash — + NULL adopts rather than reading stale, and a second probe with a DIFFERENT hash is the + reset detector firing. */ + await QueryStoreTextWriter.WriteAsync( + connection, ServerId, Db, + new[] { new FetchedQueryText(10, "SELECT 1", QueryHash: null) }, landedAt, ct); + + var adopt = await QueryStoreFetchProbe.TouchAndProbeTextsAsync( + connection, ServerId, Db, new[] { (10L, (string?)"0x1111"), (11L, (string?)"0x2222") }, now, ct); + Assert.Equal(new FetchProbeVerdict(10, Resolved: true, HashStale: false), adopt[0]); + Assert.Equal(new FetchProbeVerdict(11, Resolved: false, HashStale: false), adopt[1]); + + var renumbered = await QueryStoreFetchProbe.TouchAndProbeTextsAsync( + connection, ServerId, Db, new[] { (10L, (string?)"0x9999") }, now.AddHours(2), ct); + Assert.Equal(new FetchProbeVerdict(10, Resolved: true, HashStale: true), renumbered.Single()); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) => + await DeleteRowsAsync(cleanup, cleanupCt)); + } + } + + private static async Task DeleteRowsAsync(NpgsqlConnection connection, CancellationToken ct) + { + var sql = + $"DELETE FROM collect.query_store_plan_map WHERE server_id = {ServerId};" + + $"DELETE FROM collect.query_store_text WHERE server_id = {ServerId};" + + $"DELETE FROM servers WHERE server_id = {ServerId};"; + using var cleanup = new NpgsqlCommand(sql, connection); + await cleanup.ExecuteNonQueryAsync(ct); + } +} diff --git a/Darling/Darling.Tests/QueryStoreHealthStoreTests.cs b/Darling/Darling.Tests/QueryStoreHealthStoreTests.cs index d6a50b0d..3c708c57 100644 --- a/Darling/Darling.Tests/QueryStoreHealthStoreTests.cs +++ b/Darling/Darling.Tests/QueryStoreHealthStoreTests.cs @@ -32,7 +32,11 @@ public void TheRungIsTheTopOfADenseLadder() { var versions = PgMigrations.Scripts.Select(s => s.Version).ToList(); - Assert.Equal(76, versions.Max()); + /* #2312 added V77, so this rung is no longer the top — the "I am the top" claim moves to the + newest rung's own test (ActivityDrivenPlanFetchStoreTests) and this one keeps the invariants + that stay true forever: the rung is PRESENT, the ladder is ordered and dense, and the build's + schema version tracks the maximum. */ + Assert.Contains(76, versions); Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); Assert.Equal(versions.Distinct().OrderBy(v => v), versions); @@ -48,7 +52,8 @@ public void TheRungIsTheTopOfADenseLadder() [Fact] public void TheProbeMapsAFullyMigratedStoreTo76() { - Assert.Equal(76, StorageVersion.SchemaVersion); + /* #2312: no longer the top (that claim lives in ActivityDrivenPlanFetchStoreTests) — this fact + keeps pinning that a store at exactly 76 maps to 76 and one at 75 maps to 75, forever. */ Assert.Equal(StorageVersion.SchemaVersion, ViewerDataService.RequiredStoreSchemaVersion); /* 51 positional sentinels then the V76 one by name — the map takes 52 parameters. Present => 76, @@ -199,7 +204,9 @@ private static int InvokeMap(object[] leading, bool hasQueryStoreHealth) var method = typeof(ViewerDataService) .GetMethod("MapProbedSchemaVersion", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; - var args = leading.Concat(new object[] { hasQueryStoreHealth }).ToArray(); + /* #2312 appended hasQueryStoreTextHash after this rung's parameter — pass it FALSE so these + facts keep exercising the V76/V75 arms rather than the newer one. */ + var args = leading.Concat(new object[] { hasQueryStoreHealth, false }).ToArray(); Assert.Equal(method.GetParameters().Length, args.Length); return (int)method.Invoke(null, args)!; diff --git a/Darling/Darling.Tests/QueryStorePlanFetchTests.cs b/Darling/Darling.Tests/QueryStorePlanFetchTests.cs new file mode 100644 index 00000000..0965eec5 --- /dev/null +++ b/Darling/Darling.Tests/QueryStorePlanFetchTests.cs @@ -0,0 +1,499 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Data; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/// +/// #2312 Finding 2: the activity-driven plan/text fetch — the STORE is the watermark. The #2164/#2210 +/// watermark design this file's predecessor pinned (per-database plan-id resume points with a daily +/// refresh expiry) is retired, because Finding 4 measured what it actually did in production: the re-verify +/// cursor that was supposed to replace the expiry never got wired, so catalogs whose full walk needed more +/// than a day expired MID-walk, restarted from plan_id 0, and looped the full catalog fetch forever — +/// 40–110s per cycle, around the clock, to mostly rediscover held content (and 23s to discover "nothing +/// new" on a caught-up catalog). +/// +/// The replacement's contract, pinned here: the cycle's collected rows name their plans/texts; the +/// touch-and-probe answers which the store lacks (refreshing map/dim liveness in the same round trip — +/// Finding 3's unwired TouchSql); the fetch selects EXACTLY those by id under the same byte-budget +/// arithmetic; and NULL content lands as a stored marker instead of a per-cycle rediscovery. Live +/// round-trips for the probe SQL are in the gated Postgres suites; these are the shape and pure-function +/// pins. +/// +public class QueryStorePlanFetchTests +{ + private const string Db = "probedb"; + private static readonly DateTime Now = new(2026, 8, 11, 12, 0, 0, DateTimeKind.Utc); + private static readonly long[] SomeIds = { 900_001, 900_007, 900_042 }; + + /* ---------- the definition still owns no state ---------- */ + + [Fact] + public void TheDefinitionDeclaresNoStateKeys() + { + /* #2312 removed the plan/text watermark state entirely, and the definition must not gain state in + its place: a state-declaring definition is a two-host contract (CollectorStateContractTests pins + default_trace_events as the only one), and the fetch's only bookkeeping now lives in the store + itself plus in-memory carry-over. The one remaining query_store state family (qsowm:) is host + bookkeeping under its own owner, exactly like the backfill pair. */ + Assert.Empty(QueryStoreCollector.Instance.StateKeys); + } + + /* ---------- the runtime-stats query: no plan narrowing, no plan XML, flag-independent ---------- */ + + [Fact] + public void LiveQuery_NeverCarriesThePlanIdPredicateOrTheRowNumberGate() + { + var sql = LiveSql(Context(capturePlanXml: true)); + + Assert.DoesNotContain("qsp.plan_id > ", sql, StringComparison.Ordinal); + Assert.DoesNotContain("ROW_NUMBER()", sql, StringComparison.Ordinal); + Assert.DoesNotContain("query_plan_text = CASE", sql, StringComparison.Ordinal); + } + + [Fact] + public void LiveQuery_EmitsThePlaceholder_RegardlessOfCapturePlanXml() + { + /* #2210: the runtime query carries no plan XML in either mode, so CapturePlanXml no longer changes + this query's text at all. Darling (on) and Lite (off) get byte-identical SQL here; the only thing + CapturePlanXml still gates is the separate by-ids fetch. */ + var on = LiveSql(Context(capturePlanXml: true)); + var off = LiveSql(Context(capturePlanXml: false)); + + Assert.Contains("query_plan_text = CONVERT(nvarchar(1), NULL),", on, StringComparison.Ordinal); + Assert.True(string.Equals(on, off, StringComparison.Ordinal), "CapturePlanXml must not change this query's text"); + } + + /// #2312: the read loop stages NO state — the write-back that advanced planwm: from + /// inline-shipped plan XML retired with the watermark. The failure mode of it creeping back is a state + /// row nothing reads and a prune set that no longer owns its prefix. + [Fact] + public async Task ReadItemAsync_StagesNoPendingState() + { + var context = Context(capturePlanXml: true); + await Read(context, Plan(10, xml: true), Plan(30, xml: true)); + + Assert.Empty(context.PendingState); + } + + /* ---------- the by-ids plan fetch ---------- */ + + /// + /// The budget predicate admits a plan on the running total BEFORE it — running - own < budget + /// — so one oversized plan ships alone instead of stalling. Under store-as-watermark the naive + /// <= form is the same stall it always was, reached through the probe: the plan never lands, + /// stays missing, and rides every cycle's fetch list forever. + /// + /// A SHAPE pin: it asserts the predicate the SQL carries, not what SQL Server does with it. The + /// CONVERT sits inside the candidate CTE and the running total measures the converted text, so a + /// shipped plan is decompressed once (measured 133ms against 274ms for the join-back form on a + /// 73,163-plan catalog). + /// + [Fact] + public void PlanFetchByIds_SelectsExactlyTheNamedIds_UnderTheBudgetArithmetic() + { + var sql = QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery( + Db, Context(capturePlanXml: true), SomeIds, budgetBytes: 12L * 1024 * 1024).Text; + + Assert.Contains("WHERE qsp.plan_id IN (900001, 900007, 900042)", sql, StringComparison.Ordinal); + Assert.DoesNotContain("qsp.plan_id > ", sql, StringComparison.Ordinal); + Assert.DoesNotContain("SELECT TOP", sql, StringComparison.Ordinal); + + Assert.Contains("b.running_bytes - b.plan_bytes < 12582912", sql, StringComparison.Ordinal); + Assert.DoesNotContain("b.running_bytes <= ", sql, StringComparison.Ordinal); + Assert.Contains("COALESCE(DATALENGTH(c.query_plan_text), 0)", sql, StringComparison.Ordinal); + Assert.Contains("ROWS UNBOUNDED PRECEDING", sql, StringComparison.Ordinal); + Assert.Contains("query_plan_text = CONVERT(nvarchar(max), qsp.query_plan)", sql, StringComparison.Ordinal); + Assert.DoesNotContain("JOIN sys.query_store_plan", sql, StringComparison.Ordinal); + + /* The hash rides along without decompressing anything, rendered the way the runtime payload + renders it — TouchAndProbeSql compares the two, so the formats must agree byte-for-byte. */ + Assert.Contains("query_plan_hash = CONVERT(varchar(64), qsp.query_plan_hash, 1)", sql, StringComparison.Ordinal); + + Assert.Contains("ORDER BY b.plan_id", sql, StringComparison.Ordinal); + Assert.Contains("OPTION(RECOMPILE)", sql, StringComparison.Ordinal); + } + + [Fact] + public void PlanFetchByIds_RunsInTheDatabasesOwnContext_WithBracketDoubling() + { + var sql = QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery( + "we]ird", Context(capturePlanXml: true), SomeIds, 1024).Text; + + Assert.Contains("EXECUTE [we]]ird].sys.sp_executesql", sql, StringComparison.Ordinal); + } + + [Fact] + public void PlanFetchByIds_GuardsItsPreconditions() + { + var context = Context(capturePlanXml: true); + + /* Empty means "nothing missing" and the caller must not have called — a silent no-op query here + would hide the missing skip, and IN () is a syntax error anyway. */ + Assert.Throws(() => + QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery(Db, context, Array.Empty(), 1024)); + + /* A non-positive budget ships nothing forever — the stall by another door. */ + Assert.Throws(() => + QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery(Db, context, SomeIds, 0)); + + /* Plan capture off means no plan fetch exists at all. */ + Assert.Throws(() => + QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery(Db, Context(capturePlanXml: false), SomeIds, 1024)); + } + + /* ---------- the by-ids text fetch ---------- */ + + [Fact] + public void TextFetchByIds_SelectsExactlyTheNamedIds_WithTheResetDetectorHash() + { + var context = Context(capturePlanXml: true, fetchTextSeparately: true); + + var sql = QueryStoreCollector.Instance.BuildTextFetchByIdsQuery( + Db, context, SomeIds, budgetBytes: 12L * 1024 * 1024).Text; + + Assert.Contains("WHERE qsq.query_id IN (900001, 900007, 900042)", sql, StringComparison.Ordinal); + Assert.DoesNotContain("qsq.query_id > ", sql, StringComparison.Ordinal); + Assert.DoesNotContain("SELECT TOP", sql, StringComparison.Ordinal); + + Assert.Contains("JOIN sys.query_store_query_text AS qst", sql, StringComparison.Ordinal); + Assert.Contains("b.running_bytes - b.text_bytes < 12582912", sql, StringComparison.Ordinal); + Assert.Contains("ROWS UNBOUNDED PRECEDING", sql, StringComparison.Ordinal); + + /* query_id is only unique until a Query Store reset renumbers it; the stored hash is how the probe + sees that id 5 now names a DIFFERENT statement. Same rendering as the runtime payload. */ + Assert.Contains("query_hash = CONVERT(varchar(64), qsq.query_hash, 1)", sql, StringComparison.Ordinal); + + Assert.Contains("ORDER BY b.query_id", sql, StringComparison.Ordinal); + Assert.Contains("OPTION(RECOMPILE)", sql, StringComparison.Ordinal); + } + + [Fact] + public void TextFetchByIds_GuardsItsPreconditions() + { + var context = Context(capturePlanXml: false, fetchTextSeparately: true); + + Assert.Throws(() => + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery(Db, context, Array.Empty(), 1024)); + Assert.Throws(() => + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery(Db, context, SomeIds, 0)); + + var inlineContext = Context(capturePlanXml: false); + Assert.Throws(() => + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery(Db, inlineContext, SomeIds, 1024)); + } + + /* ---------- the touch-and-probe: liveness + missing set + rewrite/reset detection, one trip ---------- */ + + [Fact] + public void PlanTouchAndProbe_TouchesBothTables_ProbesResolvedAndStale() + { + var sql = QueryStorePlanMap.TouchAndProbeSql; + + /* The liveness half (Finding 3): map and dim last_seen stamped by the same pass, hourly-guarded, + and the dim update skips the NULL-digest content-less markers — there is no dim row to touch. */ + Assert.Contains("UPDATE collect.query_store_plan_map", sql, StringComparison.Ordinal); + Assert.Contains("UPDATE collect.query_plan_dim", sql, StringComparison.Ordinal); + Assert.Equal(2, CountOf(sql, "interval '1 hour'")); + Assert.Contains("FROM map_touch WHERE digest IS NOT NULL", sql, StringComparison.Ordinal); + + /* Hash adoption: legacy rows (stored hash NULL) take the batch's live hash on first touch — never + the other way around, so a stored baseline is never replaced by absence. */ + Assert.Contains("plan_hash = COALESCE(m.plan_hash, t.live_hash)", sql, StringComparison.Ordinal); + + /* The probe half (Finding 2): resolved is row EXISTENCE — a NULL-digest marker still resolves, or + unpersistable plans would ride every cycle's fetch list — and hash_stale fires only when BOTH + hashes exist and differ, so legacy-NULL rows and hash-less batches can never mass-refetch. */ + Assert.Contains("(m.plan_id IS NOT NULL) AS resolved", sql, StringComparison.Ordinal); + Assert.Contains("m.plan_hash IS NOT NULL AND batch.plan_hash IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("m.plan_hash <> batch.plan_hash", sql, StringComparison.Ordinal); + + /* Five parameters: ids + hashes + the stamp. */ + Assert.Contains("$5::timestamp", sql, StringComparison.Ordinal); + Assert.Contains("unnest($1::integer[], $2::text[], $3::bigint[], $4::text[])", sql, StringComparison.Ordinal); + } + + [Fact] + public void TextTouchAndProbe_MirrorsThePlanShape_SingleTable() + { + var sql = QueryStoreTextStore.TouchAndProbeSql; + + Assert.Contains("UPDATE collect.query_store_text", sql, StringComparison.Ordinal); + Assert.Contains("interval '1 hour'", sql, StringComparison.Ordinal); + Assert.Contains("query_hash = COALESCE(t.query_hash, x.live_hash)", sql, StringComparison.Ordinal); + Assert.Contains("(t.query_id IS NOT NULL) AS resolved", sql, StringComparison.Ordinal); + Assert.Contains("t.query_hash IS NOT NULL AND batch.query_hash IS NOT NULL", sql, StringComparison.Ordinal); + Assert.Contains("t.query_hash <> batch.query_hash", sql, StringComparison.Ordinal); + Assert.Contains("$5::timestamp", sql, StringComparison.Ordinal); + } + + /* ---------- the upserts never replace knowledge with absence ---------- */ + + [Fact] + public void PlanMapUpsert_CoalescesDigestAndHash_TowardKnowledge() + { + var sql = QueryStorePlanMap.UpsertSql; + + /* A NULL-XML refetch of a plan whose content the store already holds keeps the content; a fetch + that carried no hash keeps the stored baseline. Real values still advance both — that is how an + in-place rewrite's corrected content gets pointed at. */ + Assert.Contains("digest = COALESCE(EXCLUDED.digest, query_store_plan_map.digest)", sql, StringComparison.Ordinal); + Assert.Contains("plan_hash = COALESCE(EXCLUDED.plan_hash, query_store_plan_map.plan_hash)", sql, StringComparison.Ordinal); + Assert.Contains("WHERE EXCLUDED.last_seen >= query_store_plan_map.last_seen", sql, StringComparison.Ordinal); + Assert.Contains("ORDER BY server_id, database_name, plan_id", sql, StringComparison.Ordinal); + } + + [Fact] + public void TextUpsert_CarriesTheHash_AndOverwritesTextOnPurpose() + { + var sql = QueryStoreTextStore.UpsertSql; + + /* The TEXT is overwritten (a post-reset id names a different statement and the corrected text must + land); the HASH coalesces toward knowledge like the plan side. */ + Assert.Contains("query_sql_text = EXCLUDED.query_sql_text", sql, StringComparison.Ordinal); + Assert.Contains("query_hash = COALESCE(EXCLUDED.query_hash, query_store_text.query_hash)", sql, StringComparison.Ordinal); + Assert.Contains("unnest($1::integer[], $2::text[], $3::bigint[], $4::text[], $5::text[], $6::timestamp[])", sql, StringComparison.Ordinal); + } + + /* ---------- sizing: unchanged arithmetic, still load-bearing (it caps decompression) ---------- */ + + /// + /// The candidate cap sits just past what the budget can actually ship, at every plan size the fleet + /// ACTUALLY exhibits — per-quartile averages of 162 / 80 / 39 / 15 KB measured across 2,166 budget-cut + /// passes. The point of the pin is that none of these clamp: if a real fleet plan size hit a bound, the + /// bound would be doing the sizing instead of the measurement. + /// + [Theory] + [InlineData(162, 114)] + [InlineData(80, 231)] + [InlineData(39, 473)] + [InlineData(15, 1229)] + public void CandidatePlanCount_SitsJustPastTheBudget_AtEveryMeasuredFleetPlanSize(int avgKb, int expected) + { + var k = QueryStorePlanXmlState.CandidatePlanCount(avgKb * 1024L, 12L * 1024 * 1024, out var clamped); + + Assert.Equal(expected, k); + Assert.False(clamped, "a plan size the fleet actually shows must not hit a bound"); + + var actuallyFit = (12L * 1024 * 1024) / (avgKb * 1024L); + Assert.InRange(k / (double)actuallyFit, 1.4, 1.6); + } + + /// + /// First contact assumes LARGE plans on purpose. The estimate is a divisor, so over-stating plan size + /// yields a small cap — and small is the safe direction: it only spreads the catch-up out, where too + /// large decompresses a backlog to discover what fits, which is the trap the cap exists to prevent. + /// + [Fact] + public void CandidatePlanCount_WithNoPreviousPass_IsConservativelySmall() + { + var k = QueryStorePlanXmlState.CandidatePlanCount(null, 12L * 1024 * 1024, out var clamped); + + /* ceil(12MB / 160KB * 1.5) = 116 — the arithmetic, not the issue text's rounded "~118". */ + Assert.Equal(116, k); + Assert.False(clamped); + } + + [Theory] + [InlineData(10L * 1024 * 1024, 12L * 1024 * 1024, 32)] + [InlineData(1, 12L * 1024 * 1024, 2048)] + public void CandidatePlanCount_ClampsAndSaysSo(long avgBytes, long budget, int expected) + { + var k = QueryStorePlanXmlState.CandidatePlanCount(avgBytes, budget, out var clamped); + + Assert.Equal(expected, k); + Assert.True(clamped); + } + + [Fact] + public void CandidatePlanCount_LandingNaturallyOnABound_IsNotReportedAsClamped() + { + var exactlyTheFloor = (long)(QueryStorePlanXmlState.MinCandidatePlans / QueryStorePlanXmlState.CandidatePlanMargin); + var k = QueryStorePlanXmlState.CandidatePlanCount(1, exactlyTheFloor, out var clamped); + + Assert.Equal(QueryStorePlanXmlState.MinCandidatePlans, k); + Assert.False(clamped, "the measurement produced this value; no bound changed it"); + } + + /// + /// While catch-up is in progress the observed average is floored at the seed, because the sample is + /// biased then and measurably so: on one production catalog the plans the fetch shipped averaged 15 KB + /// while the newest 300 in the same catalog averaged 46 KB. Trusting the low figure inflates the cap + /// threefold and decompresses that much more than the budget can ship. + /// + [Fact] + public void CandidatePlanCount_DuringCatchUp_FloorsTheEstimateAtTheSeed() + { + const long budget = 12L * 1024 * 1024; + var biased = 15 * 1024L; + + var duringCatchUp = QueryStorePlanXmlState.CandidatePlanCount(biased, budget, catchUpInProgress: true, out _); + var converged = QueryStorePlanXmlState.CandidatePlanCount(biased, budget, catchUpInProgress: false, out _); + var seeded = QueryStorePlanXmlState.CandidatePlanCount(null, budget, out _); + + Assert.Equal(seeded, duringCatchUp); + Assert.True(converged > duringCatchUp, "the un-floored estimate must still be trusted once converged"); + + var large = 200 * 1024L; + Assert.Equal( + QueryStorePlanXmlState.CandidatePlanCount(large, budget, catchUpInProgress: false, out _), + QueryStorePlanXmlState.CandidatePlanCount(large, budget, catchUpInProgress: true, out _)); + } + + [Fact] + public void CandidatePlanCount_WithNonPositiveBudget_FloorsAndReportsClamped() + { + var k = QueryStorePlanXmlState.CandidatePlanCount(160 * 1024L, 0, out var clamped); + + Assert.Equal(QueryStorePlanXmlState.MinCandidatePlans, k); + Assert.True(clamped); + } + + /// + /// The estimator reproduces the measured fleet numbers from the same two inputs a pass already has. + /// + [Theory] + [InlineData(1.7, 11, 158)] + [InlineData(1.7, 22, 79)] + [InlineData(1.7, 44, 39)] + [InlineData(1.7, 116, 15)] + public void ObservedAvgPlanBytes_ReproducesTheMeasuredQuartiles(double shippedMb, int plans, int expectedKb) + { + var avg = QueryStorePlanXmlState.ObservedAvgPlanBytes((long)(shippedMb * 1024 * 1024), plans); + + Assert.NotNull(avg); + Assert.Equal(expectedKb, (int)(avg!.Value / 1024)); + } + + [Fact] + public void ObservedAvgPlanBytes_OnAQuietPass_IsNull() + { + Assert.Null(QueryStorePlanXmlState.ObservedAvgPlanBytes(0, 0)); + Assert.Null(QueryStorePlanXmlState.ObservedAvgPlanBytes(0, 5)); + Assert.Null(QueryStorePlanXmlState.ObservedAvgPlanBytes(1024, 0)); + } + + /* ---------- helpers ---------- */ + + private static int CountOf(string haystack, string needle) + { + var count = 0; + var index = 0; + while ((index = haystack.IndexOf(needle, index, StringComparison.Ordinal)) >= 0) + { + count++; + index += needle.Length; + } + + return count; + } + + private static string LiveSql(CollectorContext context) => + QueryStoreCollector.Instance.BuildPerItemQuery(Db, context).Text; + + private static CollectorContext Context(bool capturePlanXml, bool fetchTextSeparately = false) + { + var context = new CollectorContext + { + ServerId = 1, + ServerName = "probe", + CollectionTime = Now, + Deltas = new CollectorDeltaCalculator(), + CapturePlanXml = capturePlanXml, + FetchQueryTextSeparately = fetchTextSeparately, + State = new Dictionary(), + }; + context.CurrentDatabaseName = Db; + return context; + } + + private static (long PlanId, bool Xml) Plan(long planId, bool xml) => (planId, xml); + + private static async Task Read(CollectorContext context, params (long PlanId, bool Xml)[] plans) + { + using var reader = MakeReader(plans); + var rows = new List(); + await QueryStoreCollector.Instance.ReadItemAsync(Db, reader, rows, context, CancellationToken.None); + } + + /// + /// A real DbDataReader over the collector's OWN payload shape, generated from + /// PayloadColumns minus database_name (which the on-prem path takes from the enumerated + /// item, not the reader). Generated rather than hand-listed so a column added to the collector cannot + /// silently shift the ordinals the read loop depends on. + /// + private static DataTableReader MakeReader((long PlanId, bool Xml)[] plans) + { + var table = new DataTable("payload"); + var columns = QueryStoreCollector.Instance.PayloadColumns.Skip(1).ToList(); + + foreach (var column in columns) + { + table.Columns.Add(column.Name, ClrType(column.Name, column.Type)); + } + + for (var i = 0; i < plans.Length; i++) + { + var row = table.NewRow(); + + foreach (var column in columns) + { + row[column.Name] = column.Type switch + { + CollectorColumnType.BigInt => 0L, + CollectorColumnType.Integer => 160, + CollectorColumnType.Boolean => false, + /* Distinct per row, so a budget cut's boundary tie group ends on the very next row. */ + CollectorColumnType.Timestamp when ClrType(column.Name, column.Type) == typeof(DateTime) + => Now.AddMinutes(i), + CollectorColumnType.Timestamp => new DateTimeOffset(Now.AddMinutes(i), TimeSpan.Zero), + _ => "x", + }; + } + + row["query_id"] = plans[i].PlanId * 10; + row["plan_id"] = plans[i].PlanId; + row["execution_count"] = 1L; + /* Must not contain the self-query marker, or the read loop skips the row entirely. */ + row["query_text"] = "SELECT 1 FROM dbo.Whatever"; + row["query_plan_text"] = plans[i].Xml ? new string('p', 4096) : (object)DBNull.Value; + + table.Rows.Add(row); + } + + var dataSet = new DataSet(); + dataSet.Tables.Add(table); + return dataSet.CreateDataReader(); + } + + /// + /// The provider types the read loop actually expects, which are NOT uniform across the timestamp + /// columns: first_execution_time / last_execution_time come out of Query Store as + /// datetimeoffset and are read as DateTimeOffset, while interval_start_time_utc is + /// computed datetime2 and read with GetDateTime. A harness that types all three the same + /// way throws InvalidCastException inside the loop — which is how this was found. + /// + private static Type ClrType(string name, CollectorColumnType type) => type switch + { + CollectorColumnType.BigInt => typeof(long), + CollectorColumnType.Integer => typeof(int), + CollectorColumnType.Boolean => typeof(bool), + CollectorColumnType.Timestamp => + name.Equals("interval_start_time_utc", StringComparison.Ordinal) ? typeof(DateTime) : typeof(DateTimeOffset), + _ => typeof(string), + }; +} diff --git a/Darling/Darling.Tests/QueryStorePlanWatermarkTests.cs b/Darling/Darling.Tests/QueryStorePlanWatermarkTests.cs deleted file mode 100644 index 13c020c2..00000000 --- a/Darling/Darling.Tests/QueryStorePlanWatermarkTests.cs +++ /dev/null @@ -1,640 +0,0 @@ -/* - * Copyright (c) 2026 Erik Darling, Darling Data LLC - * - * This file is part of the SQL Server Performance Monitor. - * - * Licensed under the MIT License. See LICENSE file in the project root for full license information. - */ - -using System; -using System.Collections.Generic; -using System.Data; -using System.Globalization; -using System.Linq; -using System.Threading; -using System.Threading.Tasks; -using PerformanceMonitor.Collectors; -using PerformanceMonitor.Darling.Storage; -using Xunit; - -namespace Darling.Tests; - -/// -/// #2164: the plan-XML watermark. 97% of the plan XML shipped in a three-hour fleet window was for plans the -/// store already held, and since drain is 94-97% of a pass and costs per-row LOB bytes, not fetching is worth -/// far more than fetching less. -/// -/// Driven entirely through the collector's PUBLIC surface — BuildPerItemQuery, -/// BuildBackfillPerItemQuery, ReadItemAsync — rather than reaching for the internal helpers, so -/// no production visibility is widened for the tests' benefit. It also makes the state format an explicit -/// pin: the stored string is written out literally here instead of being produced by the same formatter under -/// test, which would have agreed with itself no matter what it emitted. -/// -public class QueryStorePlanWatermarkTests -{ - private const string Db = "probedb"; - private static readonly DateTime Now = new(2026, 8, 11, 12, 0, 0, DateTimeKind.Utc); - - /* ---------- #2210: the SQL-side narrowing is gone; QueryStorePlanXmlState.Resolve is what remains ---------- - The ROW_NUMBER-gated CASE and its in-stream `AND qsp.plan_id > ` predicate are DELETED, not reworked — - BuildPlanFetchQuery is the only thing that reads plan XML now, and its `watermark` parameter is resolved - by the host calling Resolve directly rather than derived inline in this query. These tests used to drive - that predicate through the live SQL; they now drive Resolve directly, which is exactly what the host - still calls to get BuildPlanFetchQuery's watermark argument, so the state-format/malformed/expired/ - future-stamp/per-database coverage stays live even though the SQL it used to narrow does not exist. */ - - [Fact] - public void Resolve_Fresh_ReturnsTheStoredPlanId() - { - var resolved = QueryStorePlanXmlState.Resolve(Stored(900_000, Now), Db, Now); - - Assert.Equal(900_000, resolved); - } - - [Fact] - public void Resolve_Absent_ReturnsZero() - { - /* Absent is what a first run, a restarted host and a broken store all look like, and all three must - resolve to "fetch everything" rather than skip. */ - var resolved = QueryStorePlanXmlState.Resolve(new Dictionary(), Db, Now); - - Assert.Equal(0, resolved); - } - - [Theory] - [InlineData("")] - [InlineData(" ")] - [InlineData("900000")] /* no stamp */ - [InlineData("900000:")] /* empty stamp */ - [InlineData("notanumber:1786449600")] - [InlineData("900000:notanumber")] - [InlineData("900000:1786449600:extra")] - [InlineData("0:1786449600")] /* plan_id 0 is not a plan */ - [InlineData("-5:1786449600")] - public void Resolve_Malformed_ReturnsZero(string raw) - { - /* Anything unparseable degrades to a full fetch. Trusting a partially parsed value would suppress - plan XML based on a number nobody wrote. */ - var state = new Dictionary { [QueryStorePlanXmlState.WatermarkKeyPrefix + Db] = raw }; - - Assert.Equal(0, QueryStorePlanXmlState.Resolve(state, Db, Now)); - } - - [Fact] - public void Resolve_Expired_ReturnsZero_ButStillFreshReturnsTheStoredPlanId() - { - /* Bounded staleness is why the stamp is stored beside the id. Query Store can rewrite a plan's XML in - place (memory grant feedback and friends) without issuing a new plan_id, and a permanent watermark - would never look again. It also bounds the documented dormant-plan gap. */ - var stampedLongAgo = Now - QueryStorePlanXmlState.RefreshAfter - TimeSpan.FromMinutes(1); - - var expired = QueryStorePlanXmlState.Resolve(Stored(900_000, stampedLongAgo), Db, Now); - var stillFresh = QueryStorePlanXmlState.Resolve(Stored(900_000, Now - TimeSpan.FromMinutes(1)), Db, Now); - - Assert.Equal(0, expired); - Assert.Equal(900_000, stillFresh); - } - - [Fact] - public void Resolve_StampedInTheFuture_ReturnsZero() - { - /* A clock that moved backwards would otherwise pin the watermark for as long as the skew lasts. */ - var resolved = QueryStorePlanXmlState.Resolve(Stored(900_000, Now.AddDays(3)), Db, Now); - - Assert.Equal(0, resolved); - } - - [Fact] - public void Resolve_IsKeyedPerDatabase() - { - /* plan_id is monotonic WITHIN a database and means nothing across them, so one database's watermark - must never be read for another. */ - var state = new Dictionary - { - [QueryStorePlanXmlState.WatermarkKeyPrefix + "alpha"] = "900000:" + Unix(Now), - }; - - Assert.Equal(900_000, QueryStorePlanXmlState.Resolve(state, "alpha", Now)); - Assert.Equal(0, QueryStorePlanXmlState.Resolve(state, "beta", Now)); - } - - [Fact] - public void TheDefinitionDeclaresNoStateKeys_TheHostOwnsThisState() - { - /* The watermark keys are one per DATABASE and only known at runtime, so the definition could not - declare them even if it wanted to. More importantly it MUST NOT: a state-declaring definition is a - two-host contract (CollectorStateContractTests pins default_trace_events as the only one), while - this is host bookkeeping. The QueryStoreBackfillState seam — a separate state owner name — is what - lets the host persist per-database state without the definition claiming any. - - The failure mode if this ever flips is silent, which is why it is pinned from both ends: a row - written under the DEFINITION's name is never read back, so the watermark would resolve absent - forever and collection would quietly keep paying full price. */ - Assert.Empty(QueryStoreCollector.Instance.StateKeys); - Assert.NotEqual(QueryStorePlanXmlState.StateCollectorName, QueryStoreCollector.Instance.Name); - Assert.Equal("query_store_plan_xml", QueryStorePlanXmlState.StateCollectorName); - Assert.Equal("planwm:", QueryStorePlanXmlState.WatermarkKeyPrefix); - } - - /* ---------- #2210: the runtime-stats query itself, now flag-independent ---------- */ - - [Fact] - public void LiveQuery_NeverCarriesThePlanIdPredicateOrTheRowNumberGate_RegardlessOfWatermarkState() - { - /* The predicate and the CASE it used to narrow are gone from the query entirely, not just from the - conservative path — there is no live watermark state under which either can reappear. */ - var withState = LiveSql(Context(capturePlanXml: true, state: Stored(900_000, Now))); - var withoutState = LiveSql(Context(capturePlanXml: true, state: new Dictionary())); - - Assert.DoesNotContain("qsp.plan_id > ", withState, StringComparison.Ordinal); - Assert.DoesNotContain("qsp.plan_id > ", withoutState, StringComparison.Ordinal); - Assert.DoesNotContain("ROW_NUMBER()", withState, StringComparison.Ordinal); - Assert.DoesNotContain("query_plan_text = CASE", withState, StringComparison.Ordinal); - } - - [Fact] - public void LiveQuery_EmitsThePlaceholder_RegardlessOfCapturePlanXml() - { - /* #2210: both branches of the old ternary now emit the same nvarchar(1) NULL placeholder — the - runtime query carries no plan XML in either mode, so CapturePlanXml no longer changes this query's - text at all. Darling (on) and Lite (off) get byte-identical SQL here; the only thing CapturePlanXml - still gates is the separate BuildPlanFetchQuery fetch. */ - var on = LiveSql(Context(capturePlanXml: true, state: new Dictionary())); - var off = LiveSql(Context(capturePlanXml: false, state: new Dictionary())); - - Assert.Contains("query_plan_text = CONVERT(nvarchar(1), NULL),", on, StringComparison.Ordinal); - Assert.Contains("query_plan_text = CONVERT(nvarchar(1), NULL),", off, StringComparison.Ordinal); - Assert.True(string.Equals(on, off, StringComparison.Ordinal), "CapturePlanXml must no longer change this query's text"); - } - - /* ---------- write-back, driven through the real read loop ---------- */ - - [Fact] - public async Task WriteBack_NormalPass_AdvancesToTheHighestStoredPlanId() - { - var context = Context(capturePlanXml: true, state: new Dictionary()); - await Read(context, Plan(10, xml: true), Plan(20, xml: true), Plan(30, xml: true)); - - Assert.Equal("30:" + Unix(Now), Written(context)); - } - - [Fact] - public async Task WriteBack_CountsOnlyPlansWhoseXmlActuallyShipped() - { - /* The ROW_NUMBER gate NULLs the XML on all but one interval per plan, so "seen" and "stored" differ - on every real pass. Advancing on a plan whose XML was NULL would suppress that plan's XML from then - on without ever having sent it. */ - var context = Context(capturePlanXml: true, state: new Dictionary()); - await Read(context, Plan(10, xml: true), Plan(40, xml: false)); - - Assert.Equal("10:" + Unix(Now), Written(context)); - } - - /* WriteBack_BudgetCutPass_DoesNotAdvanceAtAll (#2164) deleted with the shape it pinned: its own comment - said it recorded known-broken behaviour under the last_execution_time-ordered fetch — the watermark - could never advance on a budget cut because a cut left an arbitrary SUBSET of plan_ids. #2210's - plan_id-ordered fetch removes that premise; the replacement is - AdvanceWatermark_OnABudgetCutPass_StillAdvances below, which pins the new behaviour directly against - QueryStorePlanXmlState.AdvanceWatermark. */ - - [Fact] - public async Task WriteBack_QuietWindow_NeverMovesTheWatermarkBackward() - { - /* This is the case that killed the first design. A window whose newest-EXECUTING plan is older than - the newest-COMPILED one is an ordinary quiet window, and on a steady workload it is most windows. - The first cut read it as a Query Store reset and dropped the watermark, which would have refetched - the whole catalog on nearly every pass — the exact cost being removed. */ - var context = Context(capturePlanXml: true, state: Stored(900_000, Now)); - await Read(context, Plan(800_000, xml: true), Plan(850_000, xml: true)); - - Assert.Null(Written(context)); - } - - [Fact] - public async Task WriteBack_Advance_CarriesTheOriginalStampForward_SoTheHorizonStillFires() - { - /* The stamp dates the last FULL fetch. If an advance re-stamped it to now, then any database that - keeps compiling new plans would push its refresh horizon out forever — and those are the busy - databases where a stale plan matters most. The bounded refresh would silently never happen. */ - var fetchedAt = Now - TimeSpan.FromHours(20); - var context = Context(capturePlanXml: true, state: Stored(900_000, fetchedAt)); - - await Read(context, Plan(950_000, xml: true)); - - Assert.Equal("950000:" + Unix(fetchedAt), Written(context)); - } - - [Fact] - public async Task WriteBack_AfterExpiry_StampsTheFullFetchAtNow() - { - /* The other half of the same rule: an expired watermark means THIS pass refetched everything, so it - is the one case that legitimately re-dates the horizon. Without this the stamp would never move and - every pass after the first expiry would be a full fetch. */ - var longAgo = Now - QueryStorePlanXmlState.RefreshAfter - TimeSpan.FromHours(1); - var context = Context(capturePlanXml: true, state: Stored(900_000, longAgo)); - - await Read(context, Plan(950_000, xml: true)); - - Assert.Equal("950000:" + Unix(Now), Written(context)); - } - - [Fact] - public async Task WriteBack_PlanCaptureOff_WritesNothing() - { - /* Lite reads the same rows with no XML. It must not leave a watermark behind that a plan-capturing - host would later honor, having never shipped a single plan. */ - var context = Context(capturePlanXml: false, state: new Dictionary()); - await Read(context, Plan(10, xml: true), Plan(30, xml: true)); - - Assert.Null(Written(context)); - } - - /* ---------- helpers ---------- */ - - private static Dictionary Stored(long planId, DateTime stampedAt) => - new() - { - [QueryStorePlanXmlState.WatermarkKeyPrefix + Db] = - planId.ToString(CultureInfo.InvariantCulture) + ":" + Unix(stampedAt), - }; - - private static string Unix(DateTime utc) => - new DateTimeOffset(DateTime.SpecifyKind(utc, DateTimeKind.Utc)).ToUnixTimeSeconds() - .ToString(CultureInfo.InvariantCulture); - - private static string? Written(CollectorContext context) => - context.PendingState.TryGetValue(QueryStorePlanXmlState.WatermarkKeyPrefix + Db, out var value) - ? value - : null; - - private static string LiveSql(CollectorContext context) => - QueryStoreCollector.Instance.BuildPerItemQuery(Db, context).Text; - - private static CollectorContext Context( - bool capturePlanXml, - IReadOnlyDictionary state, - int? budgetOverride = null) - { - var context = new CollectorContext - { - ServerId = 1, - ServerName = "probe", - CollectionTime = Now, - Deltas = new CollectorDeltaCalculator(), - CapturePlanXml = capturePlanXml, - State = state, - TextByteBudgetOverride = budgetOverride, - }; - context.CurrentDatabaseName = Db; - return context; - } - - private static (long PlanId, bool Xml) Plan(long planId, bool xml) => (planId, xml); - - private static async Task Read(CollectorContext context, params (long PlanId, bool Xml)[] plans) - { - using var reader = MakeReader(plans); - var rows = new List(); - await QueryStoreCollector.Instance.ReadItemAsync(Db, reader, rows, context, CancellationToken.None); - } - - /// - /// A real DbDataReader over the collector's OWN payload shape, generated from - /// PayloadColumns minus database_name (which the on-prem path takes from the enumerated - /// item, not the reader). Generated rather than hand-listed so a column added to the collector cannot - /// silently shift the ordinals the read loop depends on. - /// - private static DataTableReader MakeReader((long PlanId, bool Xml)[] plans) - { - var table = new DataTable("payload"); - var columns = QueryStoreCollector.Instance.PayloadColumns.Skip(1).ToList(); - - foreach (var column in columns) - { - table.Columns.Add(column.Name, ClrType(column.Name, column.Type)); - } - - for (var i = 0; i < plans.Length; i++) - { - var row = table.NewRow(); - - foreach (var column in columns) - { - row[column.Name] = column.Type switch - { - CollectorColumnType.BigInt => 0L, - CollectorColumnType.Integer => 160, - CollectorColumnType.Boolean => false, - /* Distinct per row, so a budget cut's boundary tie group ends on the very next row. */ - CollectorColumnType.Timestamp when ClrType(column.Name, column.Type) == typeof(DateTime) - => Now.AddMinutes(i), - CollectorColumnType.Timestamp => new DateTimeOffset(Now.AddMinutes(i), TimeSpan.Zero), - _ => "x", - }; - } - - row["query_id"] = plans[i].PlanId * 10; - row["plan_id"] = plans[i].PlanId; - row["execution_count"] = 1L; - /* Must not contain the self-query marker, or the read loop skips the row entirely. */ - row["query_text"] = "SELECT 1 FROM dbo.Whatever"; - row["query_plan_text"] = plans[i].Xml ? new string('p', 4096) : (object)DBNull.Value; - - table.Rows.Add(row); - } - - var dataSet = new DataSet(); - dataSet.Tables.Add(table); - return dataSet.CreateDataReader(); - } - - /// - /// The provider types the read loop actually expects, which are NOT uniform across the timestamp - /// columns: first_execution_time / last_execution_time come out of Query Store as - /// datetimeoffset and are read as DateTimeOffset, while interval_start_time_utc is - /// computed datetime2 and read with GetDateTime. A harness that types all three the same - /// way throws InvalidCastException inside the loop — which is how this was found. - /// - private static Type ClrType(string name, CollectorColumnType type) => type switch - { - CollectorColumnType.BigInt => typeof(long), - CollectorColumnType.Integer => typeof(int), - CollectorColumnType.Boolean => typeof(bool), - CollectorColumnType.Timestamp => - name.Equals("interval_start_time_utc", StringComparison.Ordinal) ? typeof(DateTime) : typeof(DateTimeOffset), - _ => typeof(string), - }; - - /* ---- #2210: the plan_id-ordered fetch policy. Pure functions, pinned like QueryStoreBackfillState - .AdaptiveSpan, because the candidate window and the watermark advance are the two places this - optimization can silently do nothing (attempt one) or silently lose plans (the ordering precondition). */ - - /// - /// The candidate window sits just past what the budget can actually ship, at every plan size the fleet - /// ACTUALLY exhibits — per-quartile averages of 162 / 80 / 39 / 15 KB measured across 2,166 budget-cut - /// passes. The point of the pin is that none of these clamp: if a real fleet plan size hit a bound, the - /// bound would be doing the sizing instead of the measurement. - /// - [Theory] - [InlineData(162, 114)] - [InlineData(80, 231)] - [InlineData(39, 473)] - [InlineData(15, 1229)] - public void CandidatePlanCount_SitsJustPastTheBudget_AtEveryMeasuredFleetPlanSize(int avgKb, int expected) - { - var k = QueryStorePlanXmlState.CandidatePlanCount(avgKb * 1024L, 12L * 1024 * 1024, out var clamped); - - Assert.Equal(expected, k); - Assert.False(clamped, "a plan size the fleet actually shows must not hit a bound"); - - /* Just past, not far past: the window is the coarse bound and the running byte total is the exact one, - and every plan IN the window is decompressed to compute that total. */ - var actuallyFit = (12L * 1024 * 1024) / (avgKb * 1024L); - Assert.InRange(k / (double)actuallyFit, 1.4, 1.6); - } - - /// - /// First contact assumes LARGE plans on purpose. The estimate is a divisor, so over-stating plan size - /// yields a small window — and small is the safe direction: it only slows the watermark down, where too - /// large decompresses a catalog to discover what fits, which is the trap the window exists to prevent. - /// - [Fact] - public void CandidatePlanCount_WithNoPreviousPass_IsConservativelySmall() - { - var seed = QueryStorePlanXmlState.CandidatePlanCount(null, 12L * 1024 * 1024, out var clamped); - var atLargestMeasured = QueryStorePlanXmlState.CandidatePlanCount(162 * 1024L, 12L * 1024 * 1024, out _); - - Assert.False(clamped); - Assert.InRange(seed, atLargestMeasured - 10, atLargestMeasured + 10); - } - - /// Bounds hold, and every clamp REPORTS itself — a window silently pinned at its ceiling reads - /// exactly like one that fit, which is how a cap becomes invisible. - [Theory] - [InlineData(1, 12L * 1024 * 1024, QueryStorePlanXmlState.MaxCandidatePlans)] - [InlineData(64 * 1024, 12L * 1024 * 1024, QueryStorePlanXmlState.MinCandidatePlans)] - public void CandidatePlanCount_ClampsAndSaysSo(long avgKb, long budget, int expected) - { - var k = QueryStorePlanXmlState.CandidatePlanCount(avgKb * 1024L, budget, out var clamped); - - Assert.Equal(expected, k); - Assert.True(clamped, "a clamped window must be reportable so the caller can log it"); - } - - /// - /// `clamped` means a bound CHANGED the answer, not that the answer equals one. A window whose measured size - /// lands naturally on a bound was sized by the measurement and needs no log line; reporting it as clamped is - /// a false positive, and a caller that logs on it trains its reader to ignore the message. - /// - [Fact] - public void CandidatePlanCount_LandingNaturallyOnABound_IsNotReportedAsClamped() - { - /* Budget chosen so budget/avg*margin is exactly MinCandidatePlans: 32 / 1.5 = 21.33 plans of 1 byte. */ - var exactlyTheFloor = (long)(QueryStorePlanXmlState.MinCandidatePlans / QueryStorePlanXmlState.CandidatePlanMargin); - var k = QueryStorePlanXmlState.CandidatePlanCount(1, exactlyTheFloor, out var clamped); - - Assert.Equal(QueryStorePlanXmlState.MinCandidatePlans, k); - Assert.False(clamped, "the measurement produced this value; no bound changed it"); - } - - /// - /// While catch-up is in progress the observed average is floored at the seed, because the sample is biased - /// then and measurably so: on one production catalog the plans the fetch shipped averaged 15 KB while the - /// newest 300 in the same catalog averaged 46 KB. Trusting the low figure inflates K threefold and - /// decompresses that much more than the budget can ship. After convergence the observed value is trusted. - /// - [Fact] - public void CandidatePlanCount_DuringCatchUp_FloorsTheEstimateAtTheSeed() - { - const long budget = 12L * 1024 * 1024; - var biased = 15 * 1024L; - - var duringCatchUp = QueryStorePlanXmlState.CandidatePlanCount(biased, budget, catchUpInProgress: true, out _); - var converged = QueryStorePlanXmlState.CandidatePlanCount(biased, budget, catchUpInProgress: false, out _); - var seeded = QueryStorePlanXmlState.CandidatePlanCount(null, budget, out _); - - Assert.Equal(seeded, duringCatchUp); - Assert.True(converged > duringCatchUp, "the un-floored estimate must still be trusted once converged"); - - /* A large observed average is NOT raised by the floor — over-estimating plan size is the safe direction - and the floor only ever makes the window smaller. */ - var large = 200 * 1024L; - Assert.Equal( - QueryStorePlanXmlState.CandidatePlanCount(large, budget, catchUpInProgress: false, out _), - QueryStorePlanXmlState.CandidatePlanCount(large, budget, catchUpInProgress: true, out _)); - } - - /// - /// #2210, ruling item 4: a FULL cursor sweep over a healthy catalog leaves the watermark byte-identical. - /// Only the reset arm may ever zero it. - /// - /// Simulated through the real functions rather than mocked, because the property is about them: a - /// healthy catalog means every slice the cursor walks finds its stored hash matching the live one, so - /// nothing is re-fetched and the pass lands no plan ids. The sweep is then ceil(range / slice) calls - /// to with nothing landed, and the persisted string - /// has to come out the same at the end — including its stamp, since re-stamping on a no-op sweep would push - /// the refresh period out forever on any database the cursor keeps visiting. - /// - [Fact] - public void AFullCursorSweepOverAHealthyCatalog_LeavesTheWatermarkByteIdentical() - { - const long watermark = 77_176; - var stamp = Now; - var before = QueryStorePlanXmlState.Format(watermark, stamp); - - var slice = QueryStorePlanMap.CursorSliceWidth(watermark, QueryStorePlanXmlState.RefreshAfter, TimeSpan.FromMinutes(5)); - Assert.True(slice > 0); - - var standing = watermark; - var passes = 0; - for (var floor = 0L; floor < watermark; floor += slice) - { - /* Healthy: hashes match across the slice, so nothing is re-fetched and nothing lands. */ - var advance = QueryStorePlanXmlState.AdvanceWatermark(standing, Array.Empty()); - - Assert.True(advance.ArrivedInPlanIdOrder); - Assert.Equal(standing, advance.Watermark); - standing = advance.Watermark; - passes++; - } - - Assert.Equal(watermark, standing); - Assert.Equal(before, QueryStorePlanXmlState.Format(standing, stamp)); - - /* And the sweep genuinely covered the range in one refresh period rather than needing a second. */ - Assert.True((long)passes * slice >= watermark); - Assert.True(passes <= QueryStorePlanXmlState.RefreshAfter.Ticks / TimeSpan.FromMinutes(5).Ticks); - } - - /// A misconfigured budget floors the window rather than producing zero or a negative one. - [Fact] - public void CandidatePlanCount_WithNonPositiveBudget_FloorsAndReportsClamped() - { - var k = QueryStorePlanXmlState.CandidatePlanCount(160 * 1024L, 0, out var clamped); - - Assert.Equal(QueryStorePlanXmlState.MinCandidatePlans, k); - Assert.True(clamped); - } - - /// - /// The estimator reproduces the measured fleet numbers from the same two inputs a pass already has, which - /// is the whole reason no probe is needed: 12.1 MB over 78 plans is the q1 average, 12.3 MB over 828 is q4. - /// - [Theory] - [InlineData(12.1, 78, 158)] - [InlineData(12.3, 828, 15)] - public void ObservedAvgPlanBytes_ReproducesTheMeasuredQuartiles(double shippedMb, int plans, int expectedKb) - { - var avg = QueryStorePlanXmlState.ObservedAvgPlanBytes((long)(shippedMb * 1024 * 1024), plans); - - Assert.NotNull(avg); - Assert.Equal(expectedKb, avg!.Value / 1024); - } - - /// A pass that shipped no plans teaches nothing about plan size and must leave the previous - /// estimate standing rather than replace it with a fallback. - [Fact] - public void ObservedAvgPlanBytes_OnAQuietPass_IsNull() - { - Assert.Null(QueryStorePlanXmlState.ObservedAvgPlanBytes(0, 0)); - Assert.Null(QueryStorePlanXmlState.ObservedAvgPlanBytes(5_000, 0)); - } - - /// - /// THE POINT OF THE WHOLE REDESIGN: a budget-cut pass still advances. Under plan_id-ordered shipping a cut - /// truncates a SUFFIX, so the highest landed id is safe. The previous design shipped in - /// last_execution_time order, where a cut left an arbitrary subset, no value was safe, and the guard that - /// followed meant the watermark could not advance on 97.8% of passes. - /// - [Fact] - public void AdvanceWatermark_OnABudgetCutPass_StillAdvances() - { - var cut = QueryStorePlanXmlState.AdvanceWatermark(100, new long[] { 101, 102 }); - - Assert.Equal(102, cut.Watermark); - Assert.True(cut.ArrivedInPlanIdOrder); - } - - /// Never backward, and a quiet pass earns nothing: lowering the watermark refetches the catalog, - /// and "no new plans this window" is an ordinary pass, not a reset. - [Theory] - [InlineData(new long[0], 100L)] - [InlineData(new[] { 98L, 99L }, 100L)] - [InlineData(new[] { 101L, 102L, 103L }, 103L)] - [InlineData(new[] { 101L, 101L, 102L }, 102L)] - public void AdvanceWatermark_NeverMovesBackward(long[] landed, long expected) - { - Assert.Equal(expected, QueryStorePlanXmlState.AdvanceWatermark(100, landed).Watermark); - } - - /// - /// A descent ABANDONS the advance rather than honouring the leading ascending run. Honouring it looks - /// safer and is not: given {105, 101} it would advance to 105, and with ordering broken there is no basis - /// for inferring that every SELECTED plan below 105 landed — so a plan whose XML never arrived would be - /// suppressed until the refresh horizon. One lost pass of progress is the cheap side of that trade. - /// - [Theory] - [InlineData(new[] { 101L, 102L, 99L, 105L })] - [InlineData(new[] { 105L, 101L })] - public void AdvanceWatermark_WhenOrderingIsViolated_RefusesToAdvance(long[] landed) - { - var refused = QueryStorePlanXmlState.AdvanceWatermark(100, landed); - - Assert.Equal(100, refused.Watermark); - Assert.False(refused.ArrivedInPlanIdOrder, - "the caller needs this to LOG the violation instead of just watching the watermark stop"); - } - - /// - /// #2210: the plan fetch admits a plan when the total BEFORE it was under budget, never on the cumulative - /// total alone. The naive running_bytes <= budget is a per-database STALL — a plan bigger than the - /// whole budget exceeds it on its own row, so it is excluded, every later row is excluded too, the pass ships - /// nothing, the watermark holds, and the next pass re-selects the same plan forever. One 13 MB plan against - /// the 12 MB default does it. - /// - /// A SHAPE pin, and worth being clear about its limit: it asserts the predicate the SQL carries, not - /// what SQL Server does with it. The two behavioural cases the reviewer asked for — an oversized plan ships - /// alone and advances the watermark, and an oversized plan mid-window cuts AFTER it rather than dropping it — - /// need a real Query Store to execute and belong to the measurement session before this leaves draft. What - /// this catches is the regression that reintroduces the naive form, which is the cheap half and the half a - /// future editor is most likely to trip. - /// - [Fact] - public void PlanFetch_AdmitsAPlanOnTheTotalBeforeIt_SoOneOversizedPlanCannotStall() - { - var sql = QueryStoreCollector.Instance.BuildPlanFetchQuery( - Db, Context(capturePlanXml: true, state: new Dictionary()), - watermark: 900_000, candidatePlans: 114, budgetBytes: 12L * 1024 * 1024).Text; - - Assert.Contains("b.running_bytes - b.plan_bytes < 12582912", sql, StringComparison.Ordinal); - Assert.DoesNotContain("b.running_bytes <= ", sql, StringComparison.Ordinal); - - /* The coarse bound sorts and filters on plan_id alone — no XML touched — which is what caps the - decompression the exact bound would otherwise pay across a whole catalog. */ - /* The CONVERT sits INSIDE the window and the running total measures the converted text, so a shipped - plan is decompressed once. Measured on a 73,163-plan production catalog: this shape 133ms against - 274ms for measuring DATALENGTH(qsp.query_plan) in the window and joining back for the text, same 114 - rows and 1.7MB out of both. Plan-id-only with no XML was 114ms. */ - Assert.Contains("query_plan_text = CONVERT(nvarchar(max), qsp.query_plan)", sql, StringComparison.Ordinal); - Assert.Contains("SELECT TOP (114)", sql, StringComparison.Ordinal); - Assert.DoesNotContain("JOIN sys.query_store_plan", sql, StringComparison.Ordinal); - - /* A NULL query_plan counts as zero bytes and still ships. Letting NULL propagate would make the budget - predicate NULL, filter the row out, and a window of all-NULL plans would then hold the watermark and - re-select forever — the oversized-plan stall by another route. */ - Assert.Contains("COALESCE(DATALENGTH(c.query_plan_text), 0)", sql, StringComparison.Ordinal); - Assert.Contains("WHERE qsp.plan_id > 900000", sql, StringComparison.Ordinal); - Assert.Contains("ROWS UNBOUNDED PRECEDING", sql, StringComparison.Ordinal); - } - - /// The ordering verdict rides along with the advance on the cases that are fine. - [Theory] - [InlineData(new[] { 101L, 102L, 103L })] - [InlineData(new[] { 101L, 101L, 102L })] - [InlineData(new[] { 7L })] - [InlineData(new long[0])] - public void AdvanceWatermark_AcceptsNonDescendingArrival(long[] landed) - { - Assert.True(QueryStorePlanXmlState.AdvanceWatermark(100, landed).ArrivedInPlanIdOrder); - } -} diff --git a/Darling/Darling.Tests/QueryStoreStatePruneLivePostgresTests.cs b/Darling/Darling.Tests/QueryStoreStatePruneLivePostgresTests.cs index 8a8793fb..14aeb71f 100644 --- a/Darling/Darling.Tests/QueryStoreStatePruneLivePostgresTests.cs +++ b/Darling/Darling.Tests/QueryStoreStatePruneLivePostgresTests.cs @@ -36,7 +36,7 @@ public sealed class QueryStoreStatePruneLivePostgresTests /// Distinctive fake ids — a real server_id is a storage-name hash, never these. private const int LiveServerId = -218800; private const int NeighborServerId = -218801; - private const string ServerName = "PLANWM-PRUNE-SRV"; + private const string ServerName = "QSOWM-PRUNE-SRV"; /// The snapshot's collection_time in every case below; state rows are dated relative to it. private static readonly DateTime Newest = new(2026, 8, 11, 9, 0, 0, DateTimeKind.Unspecified); @@ -44,7 +44,7 @@ public sealed class QueryStoreStatePruneLivePostgresTests /// Old enough to be judged by — the ordinary case for a real state row. private static readonly DateTime BeforeNewest = Newest.AddHours(-1); - private static string Planwm(string database) => QueryStorePlanXmlState.WatermarkKeyPrefix + database; + private static string Qsowm(string database) => QueryStoreOpenIntervalState.WatermarkKeyPrefix + database; private static string Done(string database) => QueryStoreBackfillState.DoneKeyPrefix + database; private static string Hole(string database) => QueryStoreBackfillState.HoleKeyPrefix + database; @@ -82,7 +82,7 @@ sys.databases keeps it. "App" is present and "AppArchive" is not, which is the name-shape trap: writing the anti-join as starts_with(state_key, prefix || ds.database_name) instead of an equality is a very - plausible variant, and it would spare planwm:AppArchive forever because "planwm:App" is a + plausible variant, and it would spare qsowm:AppArchive forever because "qsowm:App" is a prefix of it. For an issue whose subject is database name churn, that case has to be here. */ await SnapshotAsync(connection, ct, Newest, "Live", "Parked", "App"); @@ -90,11 +90,11 @@ as starts_with(state_key, prefix || ds.database_name) instead of an equality is newest, nothing would ever be retired. */ await SnapshotAsync(connection, ct, Newest.AddMinutes(-15), "Live", "Parked", "App", "Dropped", "AppArchive"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Live"), "900000:1786449600"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Parked"), "800000:1786449600"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Dropped"), "700000:1786449600"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("App"), "500000:1786449600"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("AppArchive"), "400000:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Live"), "900000:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Parked"), "800000:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Dropped"), "700000:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("App"), "500000:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("AppArchive"), "400000:1786449600"); /* The backfill worker's per-database keys, which orphan identically. Both prefixes get a survivor as well as a casualty: with only a delete case, a statement that deleted @@ -112,34 +112,34 @@ a prune written as "every key of this collector" would take it. */ here — server scoping is the difference between pruning one server and pruning the fleet. */ await StateAsync(connection, ct, LiveServerId, DefaultTraceEventsCollector.Instance.Name, DefaultTraceEventsCollector.LastTraceFilePathStateKey, @"S:\MSSQL\Log\log_766.trc"); - await StateAsync(connection, ct, NeighborServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("Dropped"), "600000:1786449600"); + await StateAsync(connection, ct, NeighborServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("Dropped"), "600000:1786449600"); await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); /* Retired: gone from the newest snapshot, on every prefix it could have left behind. */ - Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Dropped"))); + Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Dropped"))); Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreBackfillState.StateCollectorName, Done("Dropped"))); Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreBackfillState.StateCollectorName, Hole("Dropped"))); /* Retired even though a LIVE database's name is a prefix of it. */ - Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("AppArchive"))); + Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("AppArchive"))); /* Kept: still collected. */ Assert.Equal("900000:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Live"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Live"))); Assert.Equal("500000:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("App"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("App"))); Assert.Equal("2026-08-11T09:00:00.0000000Z", await ValueAsync(connection, ct, LiveServerId, QueryStoreBackfillState.StateCollectorName, Done("Live"))); Assert.Equal(EncodedHole(), await ValueAsync(connection, ct, LiveServerId, QueryStoreBackfillState.StateCollectorName, Hole("Live"))); /* Kept, and this is the assertion the change exists for: present in sys.databases, absent from - every enumeration query_store runs. Pruning it costs a full plan-XML refetch of a database that - never went anywhere, on precisely the servers that keep databases parked. */ + every enumeration query_store runs. Pruning it costs re-including the open interval for a + database that never went anywhere, on precisely the servers that keep databases parked. */ Assert.Equal("800000:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Parked"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Parked"))); /* Kept: not database-keyed, not this collector, not this server. */ Assert.Equal("keep me", @@ -148,7 +148,7 @@ every enumeration query_store runs. Pruning it costs a full plan-XML refetch of await ValueAsync(connection, ct, LiveServerId, DefaultTraceEventsCollector.Instance.Name, DefaultTraceEventsCollector.LastTraceFilePathStateKey)); Assert.Equal("600000:1786449600", - await ValueAsync(connection, ct, NeighborServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Dropped"))); + await ValueAsync(connection, ct, NeighborServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Dropped"))); /* The DIAGNOSTIC, which is the only thing that could ever make a wrong delete visible — the other symptom is a silent refetch. It comes from the statement's RETURNING clause, so if that ever @@ -159,7 +159,7 @@ await ValueAsync(connection, ct, LiveServerId, DefaultTraceEventsCollector.Insta Assert.DoesNotContain("Parked", logger.Joined, StringComparison.Ordinal); /* Idempotent — it runs on every query_store cycle of every server, so a second pass over a clean - store must touch nothing. Seven survivors: planwm for Live, Parked and App; done and hole for + store must touch nothing. Seven survivors: qsowm for Live, Parked and App; done and hole for Live; the non-database-keyed bookkeeping row; and the other collector's key. */ await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); Assert.Equal(7, await CountAsync(connection, ct, LiveServerId)); @@ -179,8 +179,8 @@ await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup /// A snapshot that EXISTS is not a snapshot that is CURRENT. If database_states stops collecting /// for a server, its newest snapshot freezes, and every database created after that instant is missing /// from it while being perfectly alive. Pruning on presence alone would delete such a database's - /// watermark on every cycle forever — paying a full plan-XML refetch each time, which is the exact cost - /// #2164 exists to remove, while logging that a live database is gone. A snapshot cannot judge a row + /// state on every cycle forever — re-deriving what the stamp existed to skip, while logging that a + /// live database is gone. A snapshot cannot judge a row /// written after it was taken. /// [Fact] @@ -206,22 +206,22 @@ public async Task Prune_LeavesStateWrittenAfterTheSnapshot_AgainstDevPostgres() await SnapshotAsync(connection, ct, Newest, "OldDb"); /* Created after the snapshot froze — absent from it, and entirely alive. */ - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("BornAfterTheSnapshot"), "10:1786449600", updatedAt: Newest.AddMinutes(30)); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("BornAfterTheSnapshot"), "10:1786449600", updatedAt: Newest.AddMinutes(30)); /* Dropped before the snapshot froze: absent from it, and its last state write PRECEDES it, which is what still makes it prunable. Without this the test would pass for a prune that had simply stopped working. */ - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("DroppedLongAgo"), "20:1786449600", updatedAt: BeforeNewest); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("DroppedLongAgo"), "20:1786449600", updatedAt: BeforeNewest); await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); Assert.Equal("10:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("BornAfterTheSnapshot"))); - Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("DroppedLongAgo"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("BornAfterTheSnapshot"))); + Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("DroppedLongAgo"))); bodySucceeded = true; } @@ -262,15 +262,15 @@ public async Task Prune_WithNoDatabaseSnapshot_RetiresNothing_AgainstDevPostgres so a prune that forgot to scope the snapshot read by server would wipe every row here. */ await SnapshotAsync(connection, ct, Newest, NeighborServerId, "SomeOtherServersDatabase"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Alpha"), "1:1786449600"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Beta"), "2:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Alpha"), "1:1786449600"); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Beta"), "2:1786449600"); await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); Assert.Equal("1:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Alpha"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Alpha"))); Assert.Equal("2:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Beta"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Beta"))); bodySucceeded = true; } @@ -314,32 +314,32 @@ public async Task Prune_RacingAnInFlightCycle_CannotLoseTheWatermark_AgainstDevP /* A snapshot that does NOT name Racer, and a state row old enough to be judged by it — the adversarial setup, since neither is true of a real live database. */ await SnapshotAsync(connection, ct, Newest, "Live"); - await StateAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, - Planwm("Racer"), "900000:1786449600", updatedAt: BeforeNewest); + await StateAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + Qsowm("Racer"), "900000:1786449600", updatedAt: BeforeNewest); /* Cycle start: the collection pass reads its state. */ - var loaded = await runner.GetCollectorStateAsync(LiveServerId, QueryStorePlanXmlState.StateCollectorName, ct); - Assert.Equal("900000:1786449600", Assert.Contains(Planwm("Racer"), loaded)); + var loaded = await runner.GetCollectorStateAsync(LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, ct); + Assert.Equal("900000:1786449600", Assert.Contains(Qsowm("Racer"), loaded)); /* Mid-flight: the prune fires and takes the row this cycle is still working from. */ await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); - Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Racer"))); + Assert.Null(await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Racer"))); /* Cycle end: the write-back is an INSERT ... ON CONFLICT, so it restores rather than failing on a row that is no longer there. The database keeps collecting; the delete cost nothing. */ await runner.SaveCollectorStateAsync( - LiveServerId, QueryStorePlanXmlState.StateCollectorName, - new Dictionary(StringComparer.Ordinal) { [Planwm("Racer")] = "950000:1786449600" }, + LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, + new Dictionary(StringComparer.Ordinal) { [Qsowm("Racer")] = "950000:1786449600" }, ct); Assert.Equal("950000:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Racer"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Racer"))); /* And it stays: the restored row is stamped NOW, which is after the snapshot, so the freshness guard keeps the next cycle's prune off it too. Without that the two would fight forever. */ await runner.PruneOrphanedQueryStoreDatabaseStateAsync(LiveServerId, ct); Assert.Equal("950000:1786449600", - await ValueAsync(connection, ct, LiveServerId, QueryStorePlanXmlState.StateCollectorName, Planwm("Racer"))); + await ValueAsync(connection, ct, LiveServerId, QueryStoreOpenIntervalState.StateCollectorName, Qsowm("Racer"))); bodySucceeded = true; } diff --git a/Darling/Darling.Tests/QueryStoreStatePruneTests.cs b/Darling/Darling.Tests/QueryStoreStatePruneTests.cs index 2c0c3e0a..a8f6d421 100644 --- a/Darling/Darling.Tests/QueryStoreStatePruneTests.cs +++ b/Darling/Darling.Tests/QueryStoreStatePruneTests.cs @@ -19,9 +19,10 @@ namespace Darling.Tests; /// /// #2188: retiring the per-database collector_state rows query_store leaves behind for databases the -/// server no longer has. The #2164 plan-XML watermark writes one planwm: row per database and the -/// #2022 backfill worker writes done: and hole: rows the same way, and until this nothing -/// deleted any of them for a dropped or renamed database. +/// server no longer has. The #2022 backfill worker writes done: and hole: rows per database +/// and the #2312 open-interval stamp writes qsowm: the same way, and until this nothing deleted any +/// of them for a dropped or renamed database. (The #2164/#2150 watermark families this prune originally +/// existed for retired with the watermarks themselves in #2312; V77 deleted their rows wholesale.) /// /// What actually needs pinning is the input, not the delete. A delete keyed on the wrong list is /// the failure mode: query_store's own enumeration is filtered by ONLINE state, AG primary-ness, the @@ -32,17 +33,16 @@ namespace Darling.Tests; /// is where that is proven against a real store; this /// class holds the policy and the cross-host wiring, which no store can see. /// -/// Both SKUs. Lite writes no planwm: (it never sets -/// CollectorContext.CapturePlanXml) but it DOES write done: and hole: through its own -/// backfill worker, and it only ever deletes a hole it services or expires — so the orphan class is real on -/// both sides and the prune is ported, not declared Darling-only. +/// Both SKUs. Lite writes done: and hole: through its own backfill worker, and +/// it only ever deletes a hole it services or expires — so the orphan class is real on both sides and the +/// prune is ported, not declared Darling-only. /// pins that in both directions, and the key set /// itself lives in the shared so a prefix cannot end up pruned on /// one SKU and orphaning on the other. /// public sealed class QueryStoreStatePruneTests { - private static string Planwm(string database) => QueryStorePlanXmlState.WatermarkKeyPrefix + database; + private static string Qsowm(string database) => QueryStoreOpenIntervalState.WatermarkKeyPrefix + database; /* ---------------- the design's premise, pinned without a store ---------------- */ @@ -121,13 +121,12 @@ It demands a DECISION rather than an addition. A prefix must appear in exactly o && type.Name.EndsWith("State", StringComparison.Ordinal)) .ToArray(); - Assert.Contains(typeof(QueryStorePlanXmlState), stateClasses); Assert.Contains(typeof(QueryStoreBackfillState), stateClasses); - /* #2150 added a third, and the discovery above found it without being told — which is the property + /* #2312: the open-interval stamp, found by the discovery without being told — which is the property this guard exists for. Named here anyway so a rename that quietly drops it out of the pattern - fails rather than silently shrinking the set under test. */ - Assert.Contains(typeof(QueryStoreTextState), stateClasses); - /* #2312 added a fourth, same treatment. */ + fails rather than silently shrinking the set under test. (QueryStorePlanXmlState still matches the + name pattern but declares no key prefixes any more — its watermark retired in #2312 — so it + contributes nothing to `declared`, which is exactly right.) */ Assert.Contains(typeof(QueryStoreOpenIntervalState), stateClasses); var declared = stateClasses @@ -159,19 +158,10 @@ fails rather than silently shrinking the set under test. */ /* Owner and prefix must travel together: a prefix pruned under the wrong collector_name silently deletes nothing, which looks exactly like "there was nothing to prune". */ - Assert.Contains( - (QueryStorePlanXmlState.StateCollectorName, QueryStorePlanXmlState.WatermarkKeyPrefix), - QueryStorePerDatabaseState.PrunableKeys); Assert.Contains( (QueryStoreBackfillState.StateCollectorName, QueryStoreBackfillState.HoleKeyPrefix), QueryStorePerDatabaseState.PrunableKeys); - /* #2150: paired with its OWN collector name, not the plan fetch's. The two watermarks are stored - separately on purpose (they walk different catalogs at different rates), so borrowing the plan - fetch's owner here would prune nothing and look exactly like having nothing to prune. */ - Assert.Contains( - (QueryStoreTextState.StateCollectorName, QueryStoreTextState.WatermarkKeyPrefix), - QueryStorePerDatabaseState.PrunableKeys); - /* #2312: the open-interval stamp, per database like the three above, under its own owner. */ + /* #2312: the open-interval stamp, per database like the backfill pair, under its own owner. */ Assert.Contains( (QueryStoreOpenIntervalState.StateCollectorName, QueryStoreOpenIntervalState.WatermarkKeyPrefix), QueryStorePerDatabaseState.PrunableKeys); @@ -225,15 +215,12 @@ change nobody chose. */ [Fact] public void LiteWritesTheBackfillKeysButNeverTheWatermark() { - /* The parity FACT, which the first cut of this change got wrong: Lite writes no planwm: (it never - sets CapturePlanXml) but it DOES write done: and hole: through its own backfill worker, and it - only ever deletes a hole it services or expires. So the orphan class is real on both SKUs and the - prune had to be ported, not declared Darling-only. - - Pinned at source in both directions so neither half can rot: if Lite ever starts capturing plans - it inherits a planwm: prune that is already there (the shared PrunableKeys carries the watermark - on both hosts precisely so that day needs no code change), and if Lite ever stops writing the - backfill keys this test says so rather than leaving a prune nobody needs. */ + /* The parity FACT: Lite writes done: and hole: through its own backfill worker, and it only ever + deletes a hole it services or expires. So the orphan class is real on both SKUs and the prune had + to be ported, not declared Darling-only. Pinned at source so if Lite ever stops writing the + backfill keys this test says so rather than leaving a prune nobody needs; the CapturePlanXml pin + below survives the watermark's retirement because the flag still gates the DARLING-only + activity-driven plan fetch, and Lite growing one would be a real design event. */ var root = FindRepoRoot(); Assert.True(root is not null, "repo root not found -- the source pin cannot run"); @@ -244,10 +231,9 @@ backfill keys this test says so rather than leaving a prune nobody needs. */ Assert.False( liteRunner.Contains("CapturePlanXml", StringComparison.Ordinal), - "Lite's definition runner now sets CapturePlanXml, so Lite writes planwm: rows too. The shared " - + "PrunableKeys already covers that prefix on both hosts, so the prune needs no change — but " - + "QueryStorePlanWatermarkTests.WriteBack_PlanCaptureOff_WritesNothing and this file's prose " - + "both describe Lite as never writing them, and that is now wrong."); + "Lite's definition runner now sets CapturePlanXml — the flag that gates the Darling-only " + + "activity-driven plan fetch (#2312). That is a real design event: Lite has no plan dimension " + + "or map to fetch into, so decide what the flag means there before shipping it."); foreach (var prefix in new[] { "DoneKeyPrefix", "HoleKeyPrefix" }) { @@ -265,43 +251,13 @@ still passed. */ } } - /// - /// The recreate-with-the-same-name case, which is the only shape here that could cost data rather than a - /// refetch: a dropped and recreated database restarts Query Store's plan_id numbering at 1, so every plan - /// in the NEW database sorts below the OLD database's watermark and has its XML suppressed. - /// - /// #2183 ships no reset detection — it was written, found unsound, and removed, because the - /// tempting test ("the highest plan_id seen this pass is below the standing watermark") is TRUE in any - /// ordinary window where nothing new compiled. What actually bounds this is - /// : the stamp dates the last FULL fetch, so a stale - /// watermark stops applying within a day no matter what. This test states that mechanism explicitly, so - /// the claim is a checked fact rather than a PR-description assertion. - /// - /// The prune strictly improves on that bound without replacing it — it removes the row outright - /// when the drop is observed between cycles — but it cannot be the guarantee, because a drop and recreate - /// entirely within one cycle is never observed as an absence at all. - /// - [Fact] - public void RecreatedDatabase_IsBoundedByTheRefreshHorizon_NotByResetDetection() - { - var now = new DateTime(2026, 8, 11, 12, 0, 0, DateTimeKind.Utc); - var state = new Dictionary(StringComparer.Ordinal) - { - [Planwm("Recreated")] = QueryStorePlanXmlState.Format(900_000, now - QueryStorePlanXmlState.RefreshAfter), - }; - - /* At the horizon the watermark stops applying, so the recreated database's plan_ids (which start at 1 - and would all fail a > 900000 predicate) are fetched again. */ - Assert.Equal(0, QueryStorePlanXmlState.Resolve(state, "Recreated", now)); - - /* And one second inside it, the stale watermark DOES still apply — which is the exposure this bounds, - and the reason the prune is worth having even though it is not the guarantee. */ - Assert.Equal(900_000, QueryStorePlanXmlState.Resolve(state, "Recreated", now - TimeSpan.FromSeconds(1))); - - /* A pruned row is simply absent, and absent is the conservative full-fetch path — so a recreate that - happens after an observed drop inherits nothing at all. */ - Assert.Equal(0, QueryStorePlanXmlState.Resolve(new Dictionary(StringComparer.Ordinal), "Recreated", now)); - } + /* #2312: the RecreatedDatabase_IsBoundedByTheRefreshHorizon fact that sat here retired with the + watermark. The recreate-with-the-same-name exposure it bounded (a recreated database restarts plan_id + numbering, so old state suppressed the new database's XML for up to a day) no longer exists in that + shape: the store-as-watermark probe sees a recreated database's plan_ids as unresolved-or-hash-stale + and refetches them within ONE cycle — pinned as SQL shape in QueryStorePlanFetchTests and as a live + round-trip in the gated Postgres suite. The prune keeps its own job either way: retiring rows for + databases that are gone for good. */ /// /// Walks up from the test output directory to the repo root — the same walk-up idiom diff --git a/Darling/Darling.Tests/QueryStoreTextStoreTests.cs b/Darling/Darling.Tests/QueryStoreTextStoreTests.cs index 1eea31cb..09ccb1ab 100644 --- a/Darling/Darling.Tests/QueryStoreTextStoreTests.cs +++ b/Darling/Darling.Tests/QueryStoreTextStoreTests.cs @@ -208,21 +208,19 @@ public void TheFetchIsOnForTheSweepAndOffForTheOnDemandRead() } /// - /// The text watermark is saved under its OWN state owner. The load merges both owners into one - /// dictionary, so writing it under the plan fetch's owner would still READ back — and then never be - /// pruned, because the shared prune set pairs textwm: with query_store_text and a prefix - /// pruned under the wrong owner deletes nothing. + /// #2312: the text watermark retired with the plan one — the fetch is activity-driven against this + /// store's own rows now, so there is no textwm: family to save, and the shared prune set must not + /// claim it (a prefix listed there without a writer is a standing invitation to delete nothing and + /// call it hygiene). The V77 migration deleted the orphaned rows wholesale. /// [Fact] - public void TheTextWatermarkIsSavedUnderItsOwnOwner() + public void TheTextWatermarkFamilyIsRetired() { var source = ReadRunnerSource(); - Assert.Contains("QueryStoreTextState.StateCollectorName, textKeys", source, StringComparison.Ordinal); - Assert.Contains("QueryStoreTextState.WatermarkKeyPrefix", source, StringComparison.Ordinal); - Assert.Contains( - (QueryStoreTextState.StateCollectorName, QueryStoreTextState.WatermarkKeyPrefix), - QueryStorePerDatabaseState.PrunableKeys); + Assert.DoesNotContain("textwm:", source, StringComparison.Ordinal); + Assert.DoesNotContain(QueryStorePerDatabaseState.PrunableKeys, + k => k.Prefix == "textwm:"); } /// @@ -237,7 +235,7 @@ private static int InvokeMap(object[] leading, bool hasQueryStoreText) /* #2316 and #2319 appended parameters after this rung's — pass them FALSE so these facts keep exercising the V74/V73 arms rather than the newer ones. */ - var args = leading.Concat(new object[] { hasQueryStoreText, false, false }).ToArray(); + var args = leading.Concat(new object[] { hasQueryStoreText, false, false, false }).ToArray(); Assert.Equal(method.GetParameters().Length, args.Length); return (int)method.Invoke(null, args)!; diff --git a/Darling/Darling.Tests/QueryStoreTextWatermarkTests.cs b/Darling/Darling.Tests/QueryStoreTextWatermarkTests.cs deleted file mode 100644 index 536f5bc9..00000000 --- a/Darling/Darling.Tests/QueryStoreTextWatermarkTests.cs +++ /dev/null @@ -1,165 +0,0 @@ -/* - * Copyright (c) 2026 Erik Darling, Darling Data LLC - * - * This file is part of the SQL Server Performance Monitor. - * - * Licensed under the MIT License. See LICENSE file in the project root for full license information. - */ - -using System; -using System.Collections.Generic; -using PerformanceMonitor.Collectors; -using Xunit; - -namespace Darling.Tests; - -/// -/// #2150: the per-database watermark for the query-text fetch — the sibling of -/// , pinning the same conservative-zero rules on a second -/// catalog. -/// -/// Why the text fetch exists. The runtime payload selected query_sql_text -/// (nvarchar(max)) inside a TOP ... WITH TIES ... ORDER BY last_execution_time. A Top-N Sort -/// carries every output column through the sort and reads ALL of its input before emitting a row, so -/// choosing the rows to ship materialized text for the entire qualifying set. With #2210's plan XML -/// already gone and that column as the only difference, time-to-first-row measured 4.67s against 0.45s at -/// 1,505 rows and 5.02s against 0.57s at 4,037. Neither the row cap nor the client byte budget bounds it: -/// TOP (500) measured the same as TOP (50000), and wall time was flat from a 4 MB to a -/// 256 MB budget, because the server is finished before the client sees a byte. -/// -/// Every zero below is the same deliberate choice: an absent, malformed, expired or future-stamped -/// watermark means "fetch everything", because a first run, a restarted host and a broken store are -/// indistinguishable from here and all three must refetch rather than skip. -/// -public sealed class QueryStoreTextWatermarkTests -{ - private const string Db = "SO"; - - private static DateTime Now => new(2026, 8, 16, 12, 0, 0, DateTimeKind.Utc); - - private static Dictionary StateWith(long queryId, DateTime stampedAt) => - new() { [QueryStoreTextState.KeyFor(Db)] = QueryStoreTextState.Format(queryId, stampedAt) }; - - [Fact] - public void AFreshWatermarkRoundTrips() - { - Assert.Equal(900, QueryStoreTextState.Resolve(StateWith(900, Now), Db, Now)); - Assert.Equal(Now, QueryStoreTextState.ResolveStamp(StateWith(900, Now), Db)); - } - - /// - /// Past the refresh horizon the watermark expires to 0 — a full re-walk. - /// - /// Not decoration: query_id is monotonic in FIRST-SEEN order, not in "we have stored it", - /// so a Query Store reset renumbers ids from the start and every text would arrive below a standing - /// watermark. Without a bounded horizon that suppresses text forever. - /// - [Fact] - public void PastTheRefreshHorizonItRefetchesEverything() - { - var state = StateWith(900, Now); - - Assert.Equal(900, QueryStoreTextState.Resolve(state, Db, Now + QueryStoreTextState.RefreshAfter - TimeSpan.FromMinutes(1))); - Assert.Equal(0, QueryStoreTextState.Resolve(state, Db, Now + QueryStoreTextState.RefreshAfter)); - } - - /// - /// A future stamp is refused, or a backwards clock would pin the watermark for as long as the skew - /// lasts. - /// - [Fact] - public void AFutureStampIsRefused() - => Assert.Equal(0, QueryStoreTextState.Resolve(StateWith(900, Now), Db, Now.AddHours(-1))); - - [Theory] - [InlineData("")] - [InlineData(" ")] - [InlineData("garbage")] - [InlineData("900")] - [InlineData(":123")] - [InlineData("900:")] - [InlineData("-5:123")] - [InlineData("900:notanumber")] - public void AMalformedWatermarkRefetchesEverything(string raw) - { - var state = new Dictionary { [QueryStoreTextState.KeyFor(Db)] = raw }; - - Assert.Equal(0, QueryStoreTextState.Resolve(state, Db, Now)); - Assert.Null(QueryStoreTextState.ResolveStamp(state, Db)); - } - - [Fact] - public void AnAbsentDatabaseRefetchesEverything() - { - Assert.Equal(0, QueryStoreTextState.Resolve(StateWith(900, Now), "somewhere-else", Now)); - Assert.Equal(0, QueryStoreTextState.Resolve(new Dictionary(), Db, Now)); - Assert.Equal(0, QueryStoreTextState.Resolve(null!, Db, Now)); - } - - [Fact] - public void TheWatermarkAdvancesToTheHighestLandedId() - { - var advance = QueryStoreTextState.AdvanceWatermark(100, new long[] { 101, 102, 103 }); - - Assert.Equal(103, advance.Watermark); - Assert.True(advance.ArrivedInQueryIdOrder); - } - - /// - /// Out-of-order arrival HOLDS the watermark, because the ordering is the whole safety argument: a - /// budget cut is only a suffix if the ids arrived sorted, and advancing past a gap would strand - /// unstored text behind a strict comparison permanently. - /// - [Fact] - public void OutOfOrderArrivalHoldsTheWatermark() - { - var advance = QueryStoreTextState.AdvanceWatermark(100, new long[] { 101, 99, 102 }); - - Assert.Equal(100, advance.Watermark); - Assert.False(advance.ArrivedInQueryIdOrder); - } - - /// - /// A quiet pass is a quiet pass, not a reset. Lowering the watermark because nothing new arrived would - /// refetch the catalog on every idle cycle. - /// - [Fact] - public void AQuietPassNeverLowersTheWatermark() - { - Assert.Equal(100, QueryStoreTextState.AdvanceWatermark(100, Array.Empty()).Watermark); - Assert.Equal(100, QueryStoreTextState.AdvanceWatermark(100, null!).Watermark); - Assert.Equal(100, QueryStoreTextState.AdvanceWatermark(100, new long[] { 5, 6 }).Watermark); - Assert.True(QueryStoreTextState.AdvanceWatermark(100, Array.Empty()).ArrivedInQueryIdOrder); - } - - /// - /// The stamp survives an advance, which is what makes the refresh horizon reachable at all: re-stamping - /// on every advance would push it out forever on any database that keeps seeing new statements — which - /// is exactly where a Query Store reset would hurt most. - /// - [Fact] - public void AnAdvanceCanCarryTheOriginalStampForward() - { - var originalStamp = Now.AddHours(-6); - var carried = QueryStoreTextState.Format(950, originalStamp); - var state = new Dictionary { [QueryStoreTextState.KeyFor(Db)] = carried }; - - Assert.Equal(950, QueryStoreTextState.Resolve(state, Db, Now)); - Assert.Equal(originalStamp, QueryStoreTextState.ResolveStamp(state, Db)); - /* Six hours in, the horizon is still six hours closer than a re-stamp would have left it. */ - Assert.Equal(0, QueryStoreTextState.Resolve(state, Db, originalStamp + QueryStoreTextState.RefreshAfter)); - } - - /// - /// Text and plan watermarks live under DIFFERENT collector names. They walk different catalogs at - /// different rates, and sharing state would let one side's reset drop the other's watermark for no - /// reason. - /// - [Fact] - public void TheTextWatermarkIsStoredSeparatelyFromThePlanWatermark() - { - Assert.NotEqual(QueryStorePlanXmlState.StateCollectorName, QueryStoreTextState.StateCollectorName); - Assert.NotEqual(QueryStorePlanXmlState.WatermarkKeyPrefix, QueryStoreTextState.WatermarkKeyPrefix); - Assert.NotEqual(QueryStorePlanXmlState.KeyFor(Db), QueryStoreTextState.KeyFor(Db)); - } -} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs index 17ab1fa7..60af5250 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs @@ -126,6 +126,32 @@ private void OnQueryStoreItemSucceeded(int serverId, string database) /// private readonly ConcurrentDictionary<(int ServerId, string Database), QueryStorePlanXmlState.PlanSizeEstimate> _observedPlanSize = new(); + /// + /// Per-database ids the activity-driven fetch (#2312 Finding 2) still owes the store: probed missing in + /// an earlier cycle but deferred by the candidate cap or the byte budget. Carried IN MEMORY because the + /// probe's input is each cycle's batch references, and a plan referenced once — its delta rows shipped, + /// never executed again — would otherwise never re-enter the probe and never get its XML. The honest + /// costs of in-memory: a restart forgets the debt, and the ids re-enter only if their plans execute + /// again — for the literal-churn plans that dominate deferrals, XML nobody can reach from a fact is the + /// cheap thing to lose. Bounded: ids are 8 bytes and a first-contact backlog is one catalog's worth. + /// + private readonly ConcurrentDictionary<(int ServerId, string Database), long[]> _planFetchCarryover = new(); + + /// Text twin of — same deferral contract, keyed by query_id. + private readonly ConcurrentDictionary<(int ServerId, string Database), long[]> _textFetchCarryover = new(); + + /// + /// Ids per IN-list statement for the plan fetch. Small on purpose: each id in the list is a plan the + /// server will DECOMPRESS to run the budget's running total, so the statement size is never the real + /// bound — the candidate cap from is — and 400 + /// keeps the SQL text itself a few KB. + /// + private const int PlanFetchIdsPerStatement = 400; + + /// Ids per IN-list statement for the text fetch. Larger than the plan side because + /// DATALENGTH(query_sql_text) is cheap — no decompression — so the only cost is statement size. + private const int TextFetchIdsPerStatement = 1000; + private static readonly TimeSpan AzureMasterRecheckInterval = TimeSpan.FromMinutes(15); public const int CommandTimeoutSeconds = 60; @@ -229,56 +255,14 @@ delete every live watermark it legitimately has. */ } } - /* #2164: the per-database plan-XML watermarks, owned by the HOST under its own state collector name - rather than declared by the definition — the QueryStoreBackfillState seam. The definition cannot - declare these: the keys are one per DATABASE and only known at runtime, and declaring a prefix - would make query_store a second state-declaring collector, which is a two-host contract change - (CollectorStateContractTests) rather than the local one this is. Loaded only when plan capture is - on, because that is the only case where anything reads or writes them. */ - if (collectorState is null - && string.Equals(definition.Name, "query_store", StringComparison.Ordinal) - && _capturePlans()) - { - collectorState = await GetCollectorStateAsync( - server.ServerId, QueryStorePlanXmlState.StateCollectorName, cancellationToken); - } - - /* #2150: the text watermark lives under its OWN state owner, so it is a second read merged into the - same dictionary — the two prefixes (planwm: / textwm:) cannot collide, and the definition still - sees one flat State. Read unconditionally for query_store rather than behind _capturePlans(), - because the text fetch is not gated on plan capture: a host that turned plans off still needs its - statement text. Merged rather than replacing, so a store that has plan state but no text state - yet (every store before this rung) keeps working. */ - if (string.Equals(definition.Name, "query_store", StringComparison.Ordinal)) - { - var textState = await GetCollectorStateAsync( - server.ServerId, QueryStoreTextState.StateCollectorName, cancellationToken); - - if (textState is { Count: > 0 }) - { - var merged = new Dictionary(StringComparer.Ordinal); - if (collectorState is not null) - { - foreach (var entry in collectorState) - { - merged[entry.Key] = entry.Value; - } - } + /* #2312: the plan and text watermark reads that used to merge in here (the #2164/#2150 host-owned + state families) are GONE — the fetches are activity-driven against the store's own map/text + tables now, so there is no persisted resume point to load. V77 deleted the orphaned rows. */ - foreach (var entry in textState) - { - merged[entry.Key] = entry.Value; - } - - collectorState = merged; - } - } - - /* #2312: the open-interval refresh stamps, the third owner merged into the same flat State — - qsowm: cannot collide with planwm:/textwm:. Read unconditionally for query_store like the - text watermark (the skip applies regardless of plan capture), and merged the same way so a + /* #2312: the open-interval refresh stamps, merged into the flat State. Read unconditionally for + query_store (the skip applies regardless of plan capture), and merged rather than replacing so a store predating this state keeps working: absent keys read as "include the open interval", - which is today's behavior exactly. */ + which is the conservative behavior. */ if (string.Equals(definition.Name, "query_store", StringComparison.Ordinal)) { var openIntervalState = await GetCollectorStateAsync( @@ -882,7 +866,8 @@ type the signature needs AND gates the engine in one expression that cannot drif per-cycle cost lives HERE rather than in the payload — a 0-row cycle's blended sql: could not distinguish them. */ var planFetchWatch = Stopwatch.StartNew(); - await FetchAndStorePlansAsync(planFetchConnection, server, item, context, itemTimeout, ct); + await FetchAndStorePlansAsync(planFetchConnection, + server, item, context, itemTimeout, ExtractPlanReferences(batch), ct); context.PerItemPlanFetchMs = planFetchWatch.ElapsedMilliseconds; } @@ -899,7 +884,8 @@ advance from a cut pass. { /* #2312 investigation: same split as the plan fetch above. */ var textFetchWatch = Stopwatch.StartNew(); - await FetchAndStoreQueryTextAsync(textFetchConnection, server, item, context, itemTimeout, ct); + await FetchAndStoreQueryTextAsync(textFetchConnection, + server, item, context, itemTimeout, ExtractTextReferences(batch), ct); context.PerItemTextFetchMs = textFetchWatch.ElapsedMilliseconds; } @@ -1053,52 +1039,32 @@ exactly what they were. */ path. Outside the storage-phase timer: this is host bookkeeping, not collected data. */ if (context.PendingState.Count > 0) { - /* #2164: query_store's pending state is the plan-XML watermark set, which belongs to the host's - own state owner, NOT to the definition's name — the definition declares no state keys, so a row - written under "query_store" would never be read back and the watermark would silently never - apply. Everything else keeps writing under its definition. */ - var stateOwner = string.Equals(definition.Name, "query_store", StringComparison.Ordinal) - ? QueryStorePlanXmlState.StateCollectorName - : definition.Name; - - /* #2150/#2312: query_store's pending state now carries THREE watermark families with three - owners, so it is split by prefix on the way out. Writing one under another's owner would - still read back (the load above merges all three), but it would then never be pruned: the - shared prune set pairs each prefix with its owner, and a prefix pruned under the wrong - owner deletes nothing — which is indistinguishable from having nothing to prune. */ - var textKeys = context.PendingState - .Where(entry => entry.Key.StartsWith(QueryStoreTextState.WatermarkKeyPrefix, StringComparison.Ordinal)) - .ToDictionary(entry => entry.Key, entry => entry.Value, StringComparer.Ordinal); + /* #2312: query_store's pending state is down to ONE family — the open-interval refresh stamps + (qsowm:), which belong to the host's own state owner rather than the definition's name (the + definition declares no state keys, so a row written under "query_store" would never be read + back). The plan/text watermark families that used to be split out here retired with the + watermarks themselves; the split-by-prefix survives only as the qsowm: extraction, so a + future fourth family cannot silently land under the wrong owner and become unprunable. */ var openIntervalKeys = context.PendingState .Where(entry => entry.Key.StartsWith(QueryStoreOpenIntervalState.WatermarkKeyPrefix, StringComparison.Ordinal)) .ToDictionary(entry => entry.Key, entry => entry.Value, StringComparer.Ordinal); - if (textKeys.Count > 0 || openIntervalKeys.Count > 0) + if (openIntervalKeys.Count > 0) { + await SaveCollectorStateAsync( + server.ServerId, QueryStoreOpenIntervalState.StateCollectorName, openIntervalKeys, cancellationToken); + var others = context.PendingState - .Where(entry => !textKeys.ContainsKey(entry.Key) && !openIntervalKeys.ContainsKey(entry.Key)) + .Where(entry => !openIntervalKeys.ContainsKey(entry.Key)) .ToDictionary(entry => entry.Key, entry => entry.Value, StringComparer.Ordinal); - - if (textKeys.Count > 0) - { - await SaveCollectorStateAsync( - server.ServerId, QueryStoreTextState.StateCollectorName, textKeys, cancellationToken); - } - - if (openIntervalKeys.Count > 0) - { - await SaveCollectorStateAsync( - server.ServerId, QueryStoreOpenIntervalState.StateCollectorName, openIntervalKeys, cancellationToken); - } - if (others.Count > 0) { - await SaveCollectorStateAsync(server.ServerId, stateOwner, others, cancellationToken); + await SaveCollectorStateAsync(server.ServerId, definition.Name, others, cancellationToken); } } else { - await SaveCollectorStateAsync(server.ServerId, stateOwner, context.PendingState, cancellationToken); + await SaveCollectorStateAsync(server.ServerId, definition.Name, context.PendingState, cancellationToken); } } @@ -1359,23 +1325,94 @@ public async Task> GetCollectorStateAsync( } /// - /// Fetches one database's un-stored plan XML in plan_id order, lands it into the shared plan dimension - /// plus the map, and advances that database's watermark to what actually LANDED (#2210). + /// The cycle's distinct referenced plans with their live hashes — the probe's whole input (#2312). + /// Generic because the dispatch loop is; any batch that is not query_store rows extracts nothing, and + /// the caller's CapturePlanXml gate means that never actually happens. When one plan appears in + /// several rows (several intervals), a non-null hash wins over a null one — the probe compares against + /// whatever the engine reported, and null only means the payload row predated the hash column. + /// + private static IReadOnlyList<(long PlanId, string? PlanHash)> ExtractPlanReferences(List batch) + { + if (batch is not List rows || rows.Count == 0) + { + return Array.Empty<(long, string?)>(); + } + + var seen = new Dictionary(); + foreach (var row in rows) + { + if (row.PlanId <= 0) + { + continue; + } + + if (!seen.TryGetValue(row.PlanId, out var hash) || (hash is null && row.QueryPlanHash is not null)) + { + seen[row.PlanId] = row.QueryPlanHash; + } + } + + var references = new List<(long PlanId, string? PlanHash)>(seen.Count); + foreach (var entry in seen) + { + references.Add((entry.Key, entry.Value)); + } + + references.Sort((a, b) => a.PlanId.CompareTo(b.PlanId)); + return references; + } + + /// Text twin of , keyed by query_id with query_hash. + private static IReadOnlyList<(long QueryId, string? QueryHash)> ExtractTextReferences(List batch) + { + if (batch is not List rows || rows.Count == 0) + { + return Array.Empty<(long, string?)>(); + } + + var seen = new Dictionary(); + foreach (var row in rows) + { + if (row.QueryId <= 0) + { + continue; + } + + if (!seen.TryGetValue(row.QueryId, out var hash) || (hash is null && row.QueryHash is not null)) + { + seen[row.QueryId] = row.QueryHash; + } + } + + var references = new List<(long QueryId, string? QueryHash)>(seen.Count); + foreach (var entry in seen) + { + references.Add((entry.Key, entry.Value)); + } + + references.Sort((a, b) => a.QueryId.CompareTo(b.QueryId)); + return references; + } + + /// + /// The activity-driven plan-XML fetch for one database (#2312 Finding 2): touch-and-probe the store for + /// the cycle's referenced plans — which refreshes map/dim liveness (Finding 3's unwired TouchSql, now + /// the same round trip) and answers which plans are missing or hash-stale — then fetch exactly those by + /// id, budget-bounded, and land them into the shared dimension plus the map. The store is the + /// watermark: a caught-up database's missing set is EMPTY and no target query runs at all, which is the + /// property the retired catalog walk lacked (measured 23s per cycle to discover "nothing new"). /// - /// Failure-isolated, and that is load-bearing rather than defensive: plan XML is an enrichment on top - /// of runtime statistics, so a fetch that throws must not cost the database its runtime stats. It logs and - /// returns with the watermark untouched, which is safe by construction — the watermark only ever advances to - /// content already written, so the next pass simply re-selects the same plans. + /// Failure-isolated, and that is load-bearing rather than defensive: plan XML is an enrichment on + /// top of runtime statistics, so a fetch that throws must not cost the database its runtime stats. It + /// logs and returns; whatever did not land is still missing from the store, so the next cycle that + /// references it re-selects it by construction. /// - /// The candidate window is seeded conservatively rather than adapted, DELIBERATELY, and this is the one - /// piece of the ratified design not yet wired: the adaptive input is the previous pass's own - /// bytes-per-plan, and there is nowhere to keep it. CollectorContext is shared with Lite, so adding a - /// field is a two-host contract change — the same reasoning that put the watermark under its own state owner - /// rather than on the definition — and the state VALUE is a parsed planId:stamp pair that cannot carry - /// a third field without a format change and a migration for readers. Passing null means K comes from - /// FirstContactAvgPlanBytes, which over-estimates plan size and therefore under-sizes the window: it - /// fetches fewer plans per pass than it could, and never more than it should. Slower convergence, never - /// unsafe. + /// Budget-deferred and capped ids go to , because the probe's + /// input is each cycle's batch references: a plan referenced ONCE whose fetch was deferred would + /// otherwise never re-enter the probe. Ids the target no longer has (Query Store cleanup took the plan + /// between reference and fetch) are dropped from the debt — but only on a pass that provably completed + /// uncut, because inside a cut pass "absent from the result" and "excluded by the budget predicate" are + /// indistinguishable from the client. /// private async Task FetchAndStorePlansAsync( SqlConnection sqlConnection, @@ -1383,122 +1420,193 @@ private async Task FetchAndStorePlansAsync( string databaseName, CollectorContext context, int itemTimeout, + IReadOnlyList<(long PlanId, string? PlanHash)> references, CancellationToken cancellationToken) { try { - var watermark = QueryStorePlanXmlState.Resolve(context.State, databaseName, context.CollectionTime); + var carryKey = (server.ServerId, databaseName); + var hasCarryover = _planFetchCarryover.TryGetValue(carryKey, out var carriedIds); + if (references.Count == 0 && !hasCarryover) + { + /* The steady quiet cycle: nothing referenced, nothing owed. Zero store reads, zero target + queries — the whole point of the reshape. */ + return; + } + + await using var pgConnection = await _postgres.OpenConnectionAsync(cancellationToken); + + var missing = new SortedSet(); + if (hasCarryover) + { + foreach (var id in carriedIds!) + { + missing.Add(id); + } + } + + if (references.Count > 0) + { + var verdicts = await QueryStoreFetchProbe.TouchAndProbePlansAsync( + pgConnection, server.ServerId, databaseName, references, context.CollectionTime, cancellationToken); + foreach (var verdict in verdicts) + { + if (!verdict.Resolved || verdict.HashStale) + { + missing.Add(verdict.Id); + } + else + { + /* Resolved and current: if it was carried debt, it is paid. */ + missing.Remove(verdict.Id); + } + } + } + + if (missing.Count == 0) + { + _planFetchCarryover.TryRemove(carryKey, out _); + return; + } + var budget = context.TextByteBudgetOverride ?? 12 * 1024 * 1024; - /* #2312 Finding 1: size the window from THIS database's learned average instead of the - 160KB seed every pass — zero AvgBytes means never learned, which is the seed's job. */ - var estimate = _observedPlanSize.TryGetValue((server.ServerId, databaseName), out var carried) - ? carried + /* #2312 Finding 1 (#2322): cap the attempt from THIS database's learned average instead of the + 160KB seed every pass — zero AvgBytes means never learned, which is the seed's job. The cap + bounds server-side DECOMPRESSION (the running total materializes every plan it measures), so + it stays load-bearing even though the walk it originally sized is gone. */ + var estimate = _observedPlanSize.TryGetValue(carryKey, out var carriedEstimate) + ? carriedEstimate : default; - var candidates = QueryStorePlanXmlState.CandidatePlanCount( + var cap = QueryStorePlanXmlState.CandidatePlanCount( estimate.AvgBytes > 0 ? estimate.AvgBytes : null, budget, estimate.CatchUpInProgress, out var clamped); - if (clamped) { _logger?.LogInformation( - "query_store plan fetch on '{Server}' database [{Database}]: candidate window clamped to {K} — a bound sized this pass, not a measurement.", - server.Config.DisplayName, databaseName, candidates); + "query_store plan fetch on '{Server}' database [{Database}]: candidate cap clamped to {K} — a bound sized this pass, not a measurement.", + server.Config.DisplayName, databaseName, cap); } - var query = QueryStoreCollector.Instance.BuildPlanFetchQuery( - databaseName, context, watermark, candidates, budget); - + /* Ascending ids (SortedSet order) so the budget's in-SQL cut and the cross-chunk break are + deterministic — the same debt is retried in the same order until paid. */ + var attempt = missing.Take(cap).ToList(); + var attempted = new List(attempt.Count); var fetched = new List(); - using (var command = CreateCollectorCommand(query, sqlConnection, itemTimeout)) - await using (var reader = await command.ExecuteReaderAsync(cancellationToken)) + var shippedBytes = 0L; + var brokeOnBudget = false; + + foreach (var chunk in attempt.Chunk(PlanFetchIdsPerStatement)) { + if (shippedBytes >= budget) + { + brokeOnBudget = true; + break; + } + + var query = QueryStoreCollector.Instance.BuildPlanFetchByIdsQuery( + databaseName, context, chunk, budget - shippedBytes); + + using var command = CreateCollectorCommand(query, sqlConnection, itemTimeout); + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + attempted.AddRange(chunk); while (await reader.ReadAsync(cancellationToken)) { + var planXml = reader.IsDBNull(2) ? null : reader.GetString(2); fetched.Add(new FetchedPlan( reader.GetInt64(0), - reader.IsDBNull(1) ? null : reader.GetString(1), - PlanHash: null)); + planXml, + reader.IsDBNull(1) ? null : reader.GetString(1))); + if (planXml is not null) + { + /* nvarchar length * 2 is DATALENGTH exactly — no server round-trip needed. */ + shippedBytes += (long)planXml.Length * 2; + } } } - /* Learn from what this pass actually decompressed and shipped — BEFORE the empty-pass - early return, because an empty pass is the one that proves the walk caught up (nvarchar - length * 2 is DATALENGTH exactly, no server round-trip needed). NULL-XML rows count for - the window (they shipped, the watermark passes them) but not for the average's divisor - (they carried no bytes to average — the review catch). */ - var shippedBytes = 0L; + /* NULL-XML rows count for the cap/catch-up comparison (they shipped, and the writer records + their content-less marker) but not for the average's divisor (they carried no bytes). */ var plansMeasured = 0; foreach (var plan in fetched) { if (plan.PlanXml is not null) { - shippedBytes += (long)plan.PlanXml.Length * 2; plansMeasured++; } } - _observedPlanSize[(server.ServerId, databaseName)] = - QueryStorePlanXmlState.Learn(estimate, shippedBytes, fetched.Count, plansMeasured, candidates, budget); + _observedPlanSize[carryKey] = + QueryStorePlanXmlState.Learn(estimate, shippedBytes, fetched.Count, plansMeasured, cap, budget); - if (fetched.Count == 0) + var returned = new HashSet(fetched.Count); + if (fetched.Count > 0) { - return; - } + var landed = await QueryStorePlanWriter.WriteAsync( + pgConnection, server.ServerId, databaseName, fetched, context.CollectionTime, cancellationToken); + foreach (var id in landed) + { + missing.Remove(id); + } - await using var pgConnection = await _postgres.OpenConnectionAsync(cancellationToken); - var landed = await QueryStorePlanWriter.WriteAsync( - pgConnection, server.ServerId, databaseName, fetched, context.CollectionTime, cancellationToken); + foreach (var plan in fetched) + { + returned.Add(plan.PlanId); + } + } - var advance = QueryStorePlanXmlState.AdvanceWatermark(watermark, landed); - if (!advance.ArrivedInPlanIdOrder) + /* Target-side-gone cleanup, only when the pass provably completed UNCUT: every chunk issued + and the in-SQL predicate never fired (a fired cut leaves shipped at or past the remaining + budget by the oversized-admission arithmetic). On such a pass an attempted id with no + returned row does not exist in sys.query_store_plan any more — Query Store cleanup took it + between reference and fetch — and carrying it forever would be the content-less stall + wearing a new hat. */ + if (!brokeOnBudget && shippedBytes < budget && attempted.Count == attempt.Count) { - /* Loud rather than swallowed: the fetch's ORDER BY is what makes a budget cut a suffix, so - out-of-order arrival means that safety argument no longer holds and the pass earns nothing. */ - _logger?.LogWarning( - "query_store plan fetch on '{Server}' database [{Database}]: plans arrived OUT OF plan_id order — watermark held at {Watermark}. The ORDER BY is what makes a cut safe, so this pass earned no advance.", - server.Config.DisplayName, databaseName, watermark); - return; + foreach (var id in attempted) + { + if (!returned.Contains(id)) + { + missing.Remove(id); + } + } } - if (advance.Watermark > watermark) + if (missing.Count > 0) + { + var owed = new long[missing.Count]; + missing.CopyTo(owed); + _planFetchCarryover[carryKey] = owed; + } + else { - /* Same stamp discipline as the runtime write-back: carried FORWARD across an advance, stamped - fresh only when the standing watermark was 0 (this pass WAS the full fetch). Re-stamping on - every advance would push the sweep period out forever on any database that keeps compiling. */ - var stamp = watermark > 0 - ? QueryStorePlanXmlState.ResolveStamp(context.State, databaseName) ?? context.CollectionTime - : context.CollectionTime; - - context.PendingState[QueryStorePlanXmlState.KeyFor(databaseName)] = - QueryStorePlanXmlState.Format(advance.Watermark, stamp); + _planFetchCarryover.TryRemove(carryKey, out _); } } catch (Exception ex) when (ex is not OperationCanceledException) { _logger?.LogWarning(ex, - "query_store plan fetch failed on '{Server}' database [{Database}] — runtime statistics are unaffected and the watermark is unchanged, so the next pass re-selects the same plans.", + "query_store plan fetch failed on '{Server}' database [{Database}] — runtime statistics are unaffected, and whatever did not land is still missing from the store, so the next cycle that references it re-selects it.", server.Config.DisplayName, databaseName); } } /// - /// One database's statement-text fetch (#2150), the sibling of . - /// - /// Exists because the runtime-stats payload stopped carrying query_sql_text: selecting it - /// inside the shipping TOP ... ORDER BY made a Top-N Sort materialize nvarchar(max) text - /// for the entire qualifying set before emitting row one (measured 4.67s against 0.45s - /// time-to-first-row, and neither the row cap nor the byte budget could bound it). + /// One database's statement-text fetch, the sibling of — the + /// #2150 split (text out of the runtime stream) driven the #2312 way (activity, not a watermark walk): + /// touch-and-probe query_store_text for the cycle's referenced query_ids, fetch exactly the + /// missing or hash-stale ones by id, land them. The hash-stale arm is the Query Store RESET detector — + /// ids renumber on a reset, so a stored hash differing from the live one means the id names a + /// different statement now, and its text is refetched within one cycle instead of waiting on the + /// retired daily re-walk. /// /// A failure here is text-only. Runtime statistics are already written by the time this - /// runs, and the watermark only advances on what LANDED — so a throw leaves the rows in place with their - /// text unresolved and the next pass re-selects the same statements. That is why this is a warning - /// rather than a failure of the collector. + /// runs, and whatever did not land is still missing from the store — so a throw leaves the rows in + /// place with their text unresolved and the next cycle that references them re-selects them. That is + /// why this is a warning rather than a failure of the collector. /// - /// Known property of a first fill, stated rather than discovered. The walk is ASCENDING by - /// query_id, because that is what makes a byte-budget cut a resumable suffix. On a store whose - /// watermark is still 0 that means the OLDEST statements resolve first, while the rows being collected - /// right now reference the newest ids — so a fresh store shows missing text for recent statements until - /// the walk catches up. Steady state is the opposite and is the case that matters: the watermark sits - /// near the top, so a newly-seen statement is fetched on the next pass. + /// Simpler than the plan fetch on purpose, in the same two ways the builders differ: no + /// candidate-cap estimator (DATALENGTH on text is cheap — no decompression to bound) and larger id + /// chunks. The budget, the carry-over debt, and the uncut-pass target-side-gone cleanup all work + /// exactly as the plan side documents. /// private async Task FetchAndStoreQueryTextAsync( SqlConnection sqlConnection, @@ -1506,67 +1614,136 @@ private async Task FetchAndStoreQueryTextAsync( string databaseName, CollectorContext context, int itemTimeout, + IReadOnlyList<(long QueryId, string? QueryHash)> references, CancellationToken cancellationToken) { try { - var watermark = QueryStoreTextState.Resolve(context.State, databaseName, context.CollectionTime); - var budget = context.TextByteBudgetOverride ?? 12 * 1024 * 1024; + var carryKey = (server.ServerId, databaseName); + var hasCarryover = _textFetchCarryover.TryGetValue(carryKey, out var carriedIds); + if (references.Count == 0 && !hasCarryover) + { + return; + } + + await using var pgConnection = await _postgres.OpenConnectionAsync(cancellationToken); - var query = QueryStoreCollector.Instance.BuildTextFetchQuery( - databaseName, context, watermark, QueryStoreTextState.CandidateTexts, budget); + var missing = new SortedSet(); + if (hasCarryover) + { + foreach (var id in carriedIds!) + { + missing.Add(id); + } + } + if (references.Count > 0) + { + var verdicts = await QueryStoreFetchProbe.TouchAndProbeTextsAsync( + pgConnection, server.ServerId, databaseName, references, context.CollectionTime, cancellationToken); + foreach (var verdict in verdicts) + { + if (!verdict.Resolved || verdict.HashStale) + { + missing.Add(verdict.Id); + } + else + { + missing.Remove(verdict.Id); + } + } + } + + if (missing.Count == 0) + { + _textFetchCarryover.TryRemove(carryKey, out _); + return; + } + + var budget = context.TextByteBudgetOverride ?? 12 * 1024 * 1024; + var attempt = new List(missing.Count); + foreach (var id in missing) + { + attempt.Add(id); + } + + var attempted = new List(attempt.Count); var fetched = new List(); - using (var command = CreateCollectorCommand(query, sqlConnection, itemTimeout)) - await using (var reader = await command.ExecuteReaderAsync(cancellationToken)) + var shippedBytes = 0L; + var brokeOnBudget = false; + + foreach (var chunk in attempt.Chunk(TextFetchIdsPerStatement)) { + if (shippedBytes >= budget) + { + brokeOnBudget = true; + break; + } + + var query = QueryStoreCollector.Instance.BuildTextFetchByIdsQuery( + databaseName, context, chunk, budget - shippedBytes); + + using var command = CreateCollectorCommand(query, sqlConnection, itemTimeout); + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + attempted.AddRange(chunk); while (await reader.ReadAsync(cancellationToken)) { + var text = reader.IsDBNull(2) ? null : reader.GetString(2); fetched.Add(new FetchedQueryText( reader.GetInt64(0), + text, reader.IsDBNull(1) ? null : reader.GetString(1))); + if (text is not null) + { + shippedBytes += (long)text.Length * 2; + } } } - if (fetched.Count == 0) + var returned = new HashSet(fetched.Count); + if (fetched.Count > 0) { - return; - } + var landed = await QueryStoreTextWriter.WriteAsync( + pgConnection, server.ServerId, databaseName, fetched, context.CollectionTime, cancellationToken); + foreach (var id in landed) + { + missing.Remove(id); + } - await using var pgConnection = await _postgres.OpenConnectionAsync(cancellationToken); - var landed = await QueryStoreTextWriter.WriteAsync( - pgConnection, server.ServerId, databaseName, fetched, context.CollectionTime, cancellationToken); + foreach (var text in fetched) + { + returned.Add(text.QueryId); + } + } - var advance = QueryStoreTextState.AdvanceWatermark(watermark, landed); - if (!advance.ArrivedInQueryIdOrder) + /* Same uncut-pass cleanup as the plan side: an id the target no longer serves must not become + permanent debt. */ + if (!brokeOnBudget && shippedBytes < budget && attempted.Count == attempt.Count) { - /* Loud rather than swallowed, same as the plan fetch: the ORDER BY is what makes a budget cut - a suffix, so out-of-order arrival means that safety argument no longer holds and the pass - earns no advance. */ - _logger?.LogWarning( - "query_store text fetch on '{Server}' database [{Database}]: statements arrived OUT OF query_id order — watermark held at {Watermark}. The ORDER BY is what makes a cut safe, so this pass earned no advance.", - server.Config.DisplayName, databaseName, watermark); - return; + foreach (var id in attempted) + { + if (!returned.Contains(id)) + { + missing.Remove(id); + } + } } - if (advance.Watermark > watermark) + if (missing.Count > 0) + { + var owed = new long[missing.Count]; + missing.CopyTo(owed); + _textFetchCarryover[carryKey] = owed; + } + else { - /* Stamp carried FORWARD across an advance and stamped fresh only when the standing watermark - was 0 (this pass WAS the full walk). Re-stamping on every advance would push the refresh - horizon out forever on any database that keeps seeing new statements — which is exactly - where a Query Store reset, the thing the horizon exists to recover from, would hurt most. */ - var stamp = watermark > 0 - ? QueryStoreTextState.ResolveStamp(context.State, databaseName) ?? context.CollectionTime - : context.CollectionTime; - - context.PendingState[QueryStoreTextState.KeyFor(databaseName)] = - QueryStoreTextState.Format(advance.Watermark, stamp); + _textFetchCarryover.TryRemove(carryKey, out _); } } catch (Exception ex) when (ex is not OperationCanceledException) { _logger?.LogWarning(ex, - "query_store text fetch failed on '{Server}' database [{Database}] — runtime statistics are already written and the watermark is unchanged, so those rows keep unresolved text and the next pass re-selects the same statements.", + "query_store text fetch failed on '{Server}' database [{Database}] — runtime statistics are already written, and whatever did not land is still missing from the store, so the next cycle that references those statements re-selects them.", server.Config.DisplayName, databaseName); } } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs index 8b75c553..fa6fcf25 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingRetention.cs @@ -721,7 +721,7 @@ internal static DateTime ComputeDimTableCutoff(string dimTable, DateTime coupled /// renders "not collected" and self-corrects; content pruned while a map row survives is a live fact /// resolving to absent XML, silently. The coupled pair keeps that gap at ChunkIntervalDays; the /// dedicated pair keeps it at one day (map at knob, dim at knob + 1 — the same one-day stamp-skew - /// margin as everywhere else, because TouchSql refreshes the map's stamp eagerly while the + /// margin as everywhere else, because TouchAndProbeSql refreshes the map's stamp eagerly while the /// dim's refresh is hourly-guarded, so the dim's stamp can trail). Both components are strictly /// ordered, so the max-of-newer composition preserves the ordering under every knob value — /// pinned in PlanContentRetentionTests across the full age sweep. diff --git a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs index 26ffdaac..663e73f1 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs @@ -133,6 +133,7 @@ here costs a fresh-through-this-rung store nothing and rung 54's own copy no-ops new Migration(74, "query-store-text", V74Sql), new Migration(75, "plan-content-retention-knob", V75Sql), new Migration(76, "query-store-health", V76Sql), + new Migration(77, "activity-driven-plan-fetch", V77Sql), }; /// @@ -1691,6 +1692,39 @@ PRIMARY KEY (server_id, queryid) CREATE INDEX IF NOT EXISTS idx_pg_statement_text_last_seen ON collect.pg_statement_text(last_seen);"; + /// + /// V77 — the activity-driven plan/text fetch (#2312 Finding 2). Three small strokes for one shape + /// change: the fetch stops walking the target's plan catalog by watermark and instead fetches exactly + /// the plans/texts the cycle's collected rows reference that the store does not hold, making the store + /// itself the watermark. + /// + /// digest goes nullable so a plan whose XML the engine cannot persist (too large, certain + /// forced-failure paths) gets a map row with a NULL digest — the content-less MARKER. Without it the + /// missing-set probe would re-select those plans on every cycle forever; with it, "seen, and the content + /// will never exist" is a stored fact. Readers are unaffected: a NULL digest joins to no dimension row, + /// which renders exactly like the absent content it records. DROP NOT NULL is metadata-only and + /// idempotent, so this rung stays instant on the largest maps. + /// + /// query_store_text.query_hash is the reset detector: query_id is only unique until + /// a Query Store reset renumbers it, and the retired design's answer was a daily watermark expiry that + /// re-walked the whole catalog. The stored hash lets the per-cycle probe see that an id now names a + /// DIFFERENT statement and refetch just that text. Nullable and unbackfilled: legacy rows adopt the + /// live hash on their first touch, which converges the fleet with zero refetches. + /// + /// The DELETEs retire the planwm:/textwm: watermark state rows wholesale — the + /// machinery that wrote them is gone, collector_state has no retention (it is state, not facts), + /// and rows nobody will ever read again should not wait for a dropped-database prune that no longer + /// iterates their prefixes. Bare table name resolves via the migrate session's + /// search_path = collect, config, public, like every rung since V8. + /// + private const string V77Sql = @" +ALTER TABLE collect.query_store_plan_map ALTER COLUMN digest DROP NOT NULL; + +ALTER TABLE collect.query_store_text ADD COLUMN IF NOT EXISTS query_hash text; + +DELETE FROM collector_state WHERE collector_name = 'query_store_plan_xml' AND state_key LIKE 'planwm:%'; +DELETE FROM collector_state WHERE collector_name = 'query_store_text' AND state_key LIKE 'textwm:%';"; + /// /// V76 — the per-database Query Store health table (#2319): what database_config's single /// is_query_store_on bit cannot say — actual vs desired state (the cap-hit READ_ONLY transition diff --git a/Darling/PerformanceMonitor.Darling.Storage/QueryStoreFetchProbe.cs b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreFetchProbe.cs new file mode 100644 index 00000000..bbf3ccf1 --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreFetchProbe.cs @@ -0,0 +1,110 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; + +namespace PerformanceMonitor.Darling.Storage; + +/// One referenced id's probe verdict: does the store resolve it, and is the stored hash stale. +/// plan_id for the plan probe, query_id for the text probe. +/// A store row exists — including the plan side's NULL-digest content-less markers, +/// which must read as known or they ride every cycle's fetch list forever. +/// The stored hash and the batch's live hash both exist and DIFFER — an in-place +/// rewrite (plans) or a post-reset renumbering (texts). Stale ids refetch even though they resolve. +public readonly record struct FetchProbeVerdict(long Id, bool Resolved, bool HashStale); + +/// +/// Executes the two touch-and-probe statements for one database's cycle (#2312): the liveness refresh the +/// dimension GC depends on and the missing-set answer the activity-driven fetch runs on, one round trip +/// each. The runner hands in the cycle's distinct referenced ids with their live hashes; what comes back is +/// the fetch list — !Resolved || HashStale — and nothing else needs to be consulted, because the +/// store IS the watermark. +/// +public static class QueryStoreFetchProbe +{ + public static Task> TouchAndProbePlansAsync( + NpgsqlConnection connection, + int serverId, + string databaseName, + IReadOnlyList<(long PlanId, string? PlanHash)> references, + DateTime collectionTimeUtc, + CancellationToken cancellationToken = default) + => ExecuteAsync(connection, QueryStorePlanMap.TouchAndProbeSql, serverId, databaseName, references, collectionTimeUtc, cancellationToken); + + public static Task> TouchAndProbeTextsAsync( + NpgsqlConnection connection, + int serverId, + string databaseName, + IReadOnlyList<(long QueryId, string? QueryHash)> references, + DateTime collectionTimeUtc, + CancellationToken cancellationToken = default) + => ExecuteAsync(connection, QueryStoreTextStore.TouchAndProbeSql, serverId, databaseName, references, collectionTimeUtc, cancellationToken); + + private static async Task> ExecuteAsync( + NpgsqlConnection connection, + string sql, + int serverId, + string databaseName, + IReadOnlyList<(long Id, string? Hash)> references, + DateTime collectionTimeUtc, + CancellationToken cancellationToken) + { + if (connection is null) + { + throw new ArgumentNullException(nameof(connection)); + } + + if (references is null) + { + throw new ArgumentNullException(nameof(references)); + } + + var verdicts = new List(references.Count); + if (references.Count == 0) + { + return verdicts; + } + + var serverIds = new int[references.Count]; + var databases = new string[references.Count]; + var ids = new long[references.Count]; + var hashes = new string?[references.Count]; + for (var i = 0; i < references.Count; i++) + { + serverIds[i] = serverId; + databases[i] = databaseName; + ids[i] = references[i].Id; + hashes[i] = references[i].Hash; + } + + using var command = new NpgsqlCommand(sql, connection); + command.Parameters.AddWithValue(serverIds); + command.Parameters.AddWithValue(databases); + command.Parameters.AddWithValue(ids); + command.Parameters.AddWithValue(hashes); + /* Naive(), the #1969 trap: a Kind=Utc value infers timestamptz and Postgres converts it into the + session zone on the way into the naive last_seen columns — hours of silent skew on the exact + stamp the GC sweeps. */ + command.Parameters.AddWithValue(QueryStorePlanMap.Naive(collectionTimeUtc)); + + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + verdicts.Add(new FetchProbeVerdict( + reader.GetInt64(2), + !reader.IsDBNull(3) && reader.GetBoolean(3), + !reader.IsDBNull(4) && reader.GetBoolean(4))); + } + + return verdicts; + } +} diff --git a/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanMap.cs b/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanMap.cs index 7669f5c7..e32fc974 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanMap.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanMap.cs @@ -19,26 +19,28 @@ namespace PerformanceMonitor.Darling.Storage; /// each plan once, in plan_id order, and this map is how a fact row finds its content. /// /// Facts deliberately do NOT gain a digest column, which is why this table exists rather than a -/// entry: a fact row is written when the runtime stats arrive, and under the -/// new ordering that is potentially several budgeted cycles BEFORE its plan's XML is fetched, so there is no -/// digest to write at fact time. The map's absence of a row IS the pending state — distinguishable from "never -/// collected" by whether the plan_id sits above the database's watermark — and a reader with no map row renders -/// "plan not yet collected" instead of resolving to nothing. +/// entry: a fact row is written when the runtime stats arrive, potentially +/// budgeted cycles BEFORE its plan's XML is fetched, so there is no digest to write at fact time. The map's +/// absence of a row IS the pending state, and a reader with no map row renders "plan not yet collected" +/// instead of resolving to nothing. Since #2312 the map is also the FETCH's source of truth — the +/// activity-driven fetch asks which of the cycle's referenced plans lack a +/// row and fetches exactly those, which is what retired the per-database watermark and its +/// daily-expiry catalog walk. /// /// THIS TABLE'S last_seen IS LOAD-BEARING, AND IT IS THE ONLY PROTECTION QUERY STORE DIGESTS HAVE. /// The dimension GC does not enumerate references — an anti-join per dim row against two hypertables is not -/// affordable at this size — so it sweeps on last_seen, which the write path refreshes on every cycle -/// that references a digest. That worked precisely BECAUSE plan XML was re-shipped every pass; ending the -/// re-shipping ends the liveness signal, and a plan fetched once would have its dim row collected while live -/// facts still referenced it. Hence , which asserts liveness for plans the batch no -/// longer carries. +/// affordable at this size — so it sweeps on last_seen, which must be refreshed on every cycle +/// that references a digest. Ending plan re-shipping ended the accidental refresh the walk provided, and a +/// plan fetched once would have its dim row collected while live facts still referenced it. Hence +/// , which asserts liveness for every plan the cycle's batch references — +/// designed for exactly this in #2210, unwired until #2312 Finding 3. /// /// The GC's second belt does not cover this either: its cutoff is clamped to one day before the oldest /// surviving DIGEST-CARRYING fact (DarlingRetention.ComputeDimensionCutoff), and Query Store facts carry /// no digest, so the measured floor is blind to them. Do not add a Query Store entry to /// to "fix" that — the entry means "this fact column holds a digest", /// which is not true here, and the clamp would still be measuring a column that does not exist. -/// is the whole of the protection. +/// is the whole of the protection. /// public static class QueryStorePlanMap { @@ -47,9 +49,13 @@ public static class QueryStorePlanMap /// than time-series and is pruned on rather than by drop_chunks. public const string TableName = "collect.query_store_plan_map"; - /// The liveness column, swept by the prune and stamped by . + /// The liveness column, swept by the prune and stamped by . public const string LastSeenColumn = "last_seen"; + /* This const mirrors the IMMUTABLE migration rung that created the table and stays byte-frozen with it. + The LIVE shape differs in one place: V77 relaxed digest to nullable for the #2312 content-less marker + rows (a plan whose XML the engine cannot persist gets a map row with a NULL digest, so the probe reads + it as known instead of refetching it forever). */ public const string CreateTableSql = @"CREATE TABLE IF NOT EXISTS collect.query_store_plan_map ( server_id integer NOT NULL, database_name text NOT NULL, @@ -69,18 +75,25 @@ index nothing queries is pure write tax on a table every plan fetch upserts into /// /// Records what the plan fetch landed: one row per plan, carrying the digest of the content written to - /// query_plan_dim. The conflict arm advances digest as well as last_seen, because a - /// plan whose XML is rewritten in place keeps its plan_id while its content digest changes — the - /// case the watermark's refresh horizon exists to catch, and this is where the corrected content gets - /// pointed at. + /// query_plan_dim — or a NULL digest for a plan whose XML the engine itself reports as absent + /// (too large to persist, certain forced-plan-failure paths). The NULL-digest row is the #2312 + /// content-less MARKER: under store-as-watermark, "seen, and the content will never exist" has to be a + /// stored fact, or the probe would re-select those plans as missing on every cycle forever — the old + /// watermark's stall reborn in miniature. V77 relaxed the column for exactly this row shape. /// - /// plan_hash is what makes re-verification cheap, and it is why it is stored here rather than - /// derived: sys.query_store_plan.query_plan_hash reads WITHOUT decompressing the plan, so the - /// re-verify cursor can walk [0..watermark] comparing hashes on cheap columns alone and re-fetch XML - /// only where a hash DIFFERS or a map row is ABSENT. That turns in-place rewrites from a full catalog walk - /// per horizon into per-changed-plan work — 0 of 38,420 plan_ids changed hash across a day of fleet data — - /// and dormant plans fall out of the same pass with no heuristic to separate them from a reset, because mass - /// absence is caught wholesale by the runtime stream's reset arm within one cycle. + /// Both COALESCEs in the conflict arm point the same direction — never replace knowledge with + /// absence. A refetch that comes back NULL for a plan whose content the store already holds keeps the + /// content (digest); a fetch path that did not carry a hash keeps the stored one + /// (plan_hash). A refetch that carries REAL content or a REAL hash still advances both, which is + /// how an in-place rewrite's corrected content gets pointed at. + /// + /// plan_hash is the in-place-rewrite detector, stored rather than derived because + /// sys.query_store_plan.query_plan_hash reads WITHOUT decompressing the plan: + /// compares it against the batch's live hash on every cycle, so an + /// active plan whose XML was rewritten in place (same plan_id, new content) is refetched within + /// one cycle — where the retired re-verify cursor would have taken up to a day to reach it, had it ever + /// been wired (#2312 Finding 4: it was not). Measured base rate: 0 of 38,420 plan_ids changed hash in a + /// day of fleet data. /// /// Ordered by the conflict key. Same reason as : concurrent /// batch upserts that take row locks in different relative orders deadlock (#1801), and a plan fetch runs @@ -94,41 +107,40 @@ FROM unnest($1::integer[], $2::text[], $3::bigint[], $4::bytea[], $5::text[], $6 AS batch(server_id, database_name, plan_id, digest, plan_hash, stamped) ORDER BY server_id, database_name, plan_id ON CONFLICT (server_id, database_name, plan_id) DO UPDATE SET - digest = EXCLUDED.digest, - plan_hash = EXCLUDED.plan_hash, + digest = COALESCE(EXCLUDED.digest, query_store_plan_map.digest), + plan_hash = COALESCE(EXCLUDED.plan_hash, query_store_plan_map.plan_hash), last_seen = EXCLUDED.last_seen WHERE EXCLUDED.last_seen >= query_store_plan_map.last_seen"; /// - /// The liveness assertion, and the reason this whole design is safe: for the distinct - /// (database_name, plan_id) a runtime-stats batch just wrote, refresh BOTH this map row's - /// last_seen and the dimension row's, so neither can age out while facts still point at the plan. - /// The batch already carries those two columns, so nothing extra is collected to make this work. + /// The liveness assertion AND the missing-set probe, one round trip (#2312): for the distinct + /// (database_name, plan_id, plan_hash) a cycle's runtime batch references, refresh BOTH this map + /// row's last_seen and the dimension row's — so neither can age out while facts still point at + /// the plan — and return, per batch row, whether the store already resolves it and whether its stored + /// hash still matches the engine's live one. The batch already carries all three columns, so nothing + /// extra is collected to make this work. /// /// Both timestamps are stamped by the SAME pass, which is what makes the map-prune-versus-dim-GC race /// structurally impossible rather than carefully avoided: a map row's last_seen can never be older /// than the newest fact batch that referenced it, so the prune cannot take a row that live facts are - /// touching. - /// - /// It also returns the RESOLVED-ness of every batch row, in the same round trip, because the batch - /// join it already does is where the reset signal lives: a plan the store has never resolved cannot be - /// produced by "no new plans this window". What it deliberately does NOT do is decide that a reset happened. - /// Two reasons, both of which bit the first version of this query: + /// touching. This is also what makes the V75 plan-content horizon mean what it says for Query Store + /// plans: content ages out N-days-since-last-REFERENCE, not since-last-refetch — before #2312 wired + /// this, the perpetual daily catalog walk was accidentally standing in for it. /// - /// • One dormant plan is not a reset. Filtering to absent rows at or below a watermark fires on - /// a SINGLE dormant plan resuming execution, which would zero that database's watermark and trigger a full - /// refetch — the opposite of what this design is for. The reset case is MASS absence, and "mass" is a - /// judgement the caller makes across the batch. A lone absence is the CURSOR's job (it fetches that plan and - /// moves on), which is what RefreshAfter's comment already says owns dormancy.
- /// • Watermarks are per database. These array parameters can carry rows for several databases in one - /// call, so comparing them all against one scalar watermark is wrong for every database but one. The caller - /// already holds the per-database watermarks; it applies them. + /// The probe columns are the activity-driven fetch's entire input. resolved is "a map row + /// exists" — INCLUDING the NULL-digest content-less markers, which is the point of storing them: a plan + /// the engine cannot persist must read as known, or it rides every cycle's fetch list forever. + /// hash_stale is the in-place-rewrite signal: a stored hash that differs from the batch's live + /// one means the plan kept its id and changed its content, and the caller refetches it. A stored hash + /// of NULL is never stale — it is adopted from the batch in the same statement (legacy rows from before + /// the fetch carried hashes), so the fleet's hash coverage backfills organically with zero refetches. /// - /// So this returns facts — (server_id, database_name, plan_id, resolved) — and the host decides. - /// When it does conclude a reset it zeroes that database's watermark and logs loudly, recovering in one - /// cycle rather than waiting on a refresh sweep. + /// What this deliberately does NOT do is detect Query Store resets, because nothing needs to any + /// more: a reset renumbers plans, the new ids come back unresolved, and the fetch list picks them up + /// budget-bounded — recovery is the normal path rather than a special arm. (The retired watermark design + /// needed the mass-absence judgement precisely because it had a watermark to zero; see #2312.) /// - /// The three preceding CTEs still run. Postgres executes data-modifying WITH statements exactly + /// The preceding CTEs still run. Postgres executes data-modifying WITH statements exactly /// once and to completion whether or not the primary query reads their output, so making the final statement /// a SELECT does not turn the liveness stamping into a no-op. That is a load-bearing detail: if it were not /// true, this restructure would silently stop refreshing last_seen and reintroduce the GC hazard the @@ -136,31 +148,34 @@ ON CONFLICT (server_id, database_name, plan_id) DO UPDATE SET /// /// Guarded at one hour like the dimension upsert's own conflict arm, and for the same reason — the /// horizons are multi-day, so an update per row per hour is enough freshness and the write amplification - /// stays bounded on a hot catalog. The margin arithmetic already accounts for this trailing hour. + /// stays bounded on a hot catalog. The margin arithmetic already accounts for this trailing hour. Hash + /// adoption rides the same guard: it is a backfill, not a correctness deadline, and un-guarding it would + /// re-write every legacy row on every cycle until the first touch landed. /// - /// The CTE is ordered by the map's primary key for the #1801 reason above — this is the one statement - /// in the design that touches many rows across two tables on every cycle of every server, so it is the most - /// likely place for an unordered-batch deadlock to form. Being precise about how much that buys, because - /// can claim more than this can: an ORDER BY inside a CTE - /// feeding UPDATE ... FROM is NOT a guaranteed lock-acquisition order in Postgres the way ordering an - /// INSERT ... ON CONFLICT's input is — the planner may reorder. It makes the common plan deterministic - /// rather than making the deadlock impossible. If one is observed, the fix is to drive the update from an - /// explicitly ordered SELECT ... FOR UPDATE, not to widen this comment. + /// The CTE is ordered by the map's primary key for the #1801 reason on — + /// this is the one statement in the design that touches many rows across two tables on every cycle of + /// every server, so it is the most likely place for an unordered-batch deadlock to form. Being precise + /// about how much that buys: an ORDER BY inside a CTE feeding UPDATE ... FROM is NOT a + /// guaranteed lock-acquisition order in Postgres the way ordering an INSERT ... ON CONFLICT's + /// input is — the planner may reorder. It makes the common plan deterministic rather than making the + /// deadlock impossible. If one is observed, the fix is to drive the update from an explicitly ordered + /// SELECT ... FOR UPDATE, not to widen this comment. /// - public const string TouchSql = @"WITH touched AS ( - SELECT m.server_id, m.database_name, m.plan_id, m.digest + public const string TouchAndProbeSql = @"WITH touched AS ( + SELECT m.server_id, m.database_name, m.plan_id, m.digest, batch.plan_hash AS live_hash FROM collect.query_store_plan_map AS m - JOIN unnest($1::integer[], $2::text[], $3::bigint[]) - AS batch(server_id, database_name, plan_id) + JOIN unnest($1::integer[], $2::text[], $3::bigint[], $4::text[]) + AS batch(server_id, database_name, plan_id, plan_hash) ON batch.server_id = m.server_id AND batch.database_name = m.database_name AND batch.plan_id = m.plan_id - WHERE m.last_seen < $4::timestamp - interval '1 hour' + WHERE m.last_seen < $5::timestamp - interval '1 hour' ORDER BY m.server_id, m.database_name, m.plan_id ), map_touch AS ( UPDATE collect.query_store_plan_map AS m - SET last_seen = $4::timestamp + SET last_seen = $5::timestamp, + plan_hash = COALESCE(m.plan_hash, t.live_hash) FROM touched AS t WHERE m.server_id = t.server_id AND m.database_name = t.database_name @@ -169,14 +184,20 @@ RETURNING t.digest ), dim_touch AS ( UPDATE collect.query_plan_dim AS d - SET last_seen = $4::timestamp - WHERE d.digest IN (SELECT digest FROM map_touch) - AND d.last_seen < $4::timestamp - interval '1 hour' + SET last_seen = $5::timestamp + WHERE d.digest IN (SELECT digest FROM map_touch WHERE digest IS NOT NULL) + AND d.last_seen < $5::timestamp - interval '1 hour' RETURNING d.digest ) -SELECT batch.server_id, batch.database_name, batch.plan_id, (m.plan_id IS NOT NULL) AS resolved -FROM unnest($1::integer[], $2::text[], $3::bigint[]) - AS batch(server_id, database_name, plan_id) +SELECT + batch.server_id, + batch.database_name, + batch.plan_id, + (m.plan_id IS NOT NULL) AS resolved, + (m.plan_id IS NOT NULL AND m.plan_hash IS NOT NULL AND batch.plan_hash IS NOT NULL + AND m.plan_hash <> batch.plan_hash) AS hash_stale +FROM unnest($1::integer[], $2::text[], $3::bigint[], $4::text[]) + AS batch(server_id, database_name, plan_id, plan_hash) LEFT JOIN collect.query_store_plan_map AS m ON m.server_id = batch.server_id AND m.database_name = batch.database_name @@ -186,7 +207,7 @@ LEFT JOIN collect.query_store_plan_map AS m /// /// Strips the off a timestamp before it is bound to any of this class's /// ::timestamp parameters. Every call site that binds a here must go through - /// this — 's stamp array and 's $4, plus the prune's + /// this — 's stamp array and 's $5, plus the prune's /// cutoff. /// /// This is the #1969 trap, and it is silent. Npgsql infers the parameter type from the value's Kind: @@ -203,69 +224,6 @@ LEFT JOIN collect.query_store_plan_map AS m /// public static DateTime Naive(DateTime utc) => DateTime.SpecifyKind(utc, DateTimeKind.Unspecified); - /// - /// The map rows a re-verify cursor slice needs to judge, for one database over a bounded - /// plan_id range: what content the store believes each plan has. - /// - /// Returns plan_id and plan_hash only — never the digest, never content. The caller - /// pairs this against the same id range read from sys.query_store_plan (also hash-only, which reads - /// without decompressing) and re-fetches XML for exactly three cases: a hash that DIFFERS (the plan was - /// rewritten in place while keeping its id), a map row that is ABSENT (a plan dormant through every - /// collected window, so the watermark passed it without its content ever landing), and a stored - /// plan_hash that is NULL (written by a build before the hash column existed — re-verify once, then - /// it self-heals). - /// - /// This is the whole reason the horizon stopped being a full refetch. The old expiry dropped the - /// watermark to zero and re-walked every plan's XML, which the walk-cost measurement showed cannot even - /// complete inside a day on the larger catalogs (2.2-15.1 GB of plan XML per catalog; 15.9 to 107.5 hours at - /// a 12 MB budget and 5-minute cadence), so those catalogs restarted forever and never reached their own - /// newest plans. A hash-only sweep over the same id range is bounded by ROW count instead of BYTE volume — - /// 77k ids at ~270 per pass — and re-fetches only what actually changed, which across a day of fleet data - /// was 0 of 38,420 plans. - /// - public const string CursorSliceSql = @"SELECT m.plan_id, m.plan_hash -FROM collect.query_store_plan_map AS m -WHERE m.server_id = $1 - AND m.database_name = $2 - AND m.plan_id > $3 - AND m.plan_id <= $4 -ORDER BY m.plan_id"; - - /// - /// The cursor's slice width for one pass: the id range divided by how many passes fit in the sweep period. - /// is no longer an expiry — it is the target period for ONE full - /// re-verification sweep — and this is where that meaning is applied. - /// - /// Floored at one so a cursor always makes progress, and floored again by - /// so a tiny catalog does not crawl an id at a time. Bounded by the range - /// itself, so a sweep never claims to cover ids that do not exist. - /// - public static long CursorSliceWidth(long watermark, TimeSpan refreshAfter, TimeSpan cadence, long minimumSlice = 64) - { - if (watermark <= 0) - { - return 0; - } - - var passes = cadence > TimeSpan.Zero ? refreshAfter.Ticks / cadence.Ticks : 1; - if (passes < 1) - { - passes = 1; - } - - /* CEILING, not floor. Truncating divides a sweep that never completes: Redstone's 77,176 ids over 288 - five-minute passes floors to 267, and 267 * 288 = 76,896 — 280 ids short, every sweep, forever. The - cursor would walk almost the whole catalog and then restart, which is a quieter version of the exact - failure this design replaced. */ - var slice = (watermark + passes - 1) / passes; - if (slice < minimumSlice) - { - slice = minimumSlice; - } - - return slice > watermark ? watermark : slice; - } - /// /// Days of margin the map prune adds past the fact-retention horizon. **Strictly less than the dimension /// GC's margin**, which is ChunkIntervalDays + 1 — see for why @@ -296,7 +254,7 @@ public static bool MarginOrderingHolds(int chunkIntervalDays) => /// /// Timestamp-driven, NOT an existence check against query_store_stats. An anti-join against a /// 43 GB hypertable per map row is exactly the cost this architecture avoids, and it is unnecessary here - /// because keeps last_seen current for anything live — the same argument the + /// because keeps last_seen current for anything live — the same argument the /// dimension GC already rests on, applied to one more timestamped table. /// /// A plan whose query goes quiet needs no special handling: it stops being touched, its facts age out diff --git a/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanWriter.cs b/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanWriter.cs index 1e89638a..50b91988 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanWriter.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/QueryStorePlanWriter.cs @@ -17,9 +17,10 @@ namespace PerformanceMonitor.Darling.Storage; /// One plan the plan-XML fetch landed, before it has been given a content digest. /// The Query Store plan id, unique within its database. /// The plan XML, or null — a plan too large to persist reads NULL and still ships, so the -/// watermark can advance past a plan whose content will never exist. -/// SQL Server's query_plan_hash, readable without decompressing the plan, which is -/// what lets the re-verify cursor detect in-place rewrites on cheap columns alone. +/// store can record the content-less marker instead of re-selecting the plan as missing forever (#2312). +/// SQL Server's query_plan_hash, readable without decompressing the plan — the +/// stored baseline QueryStorePlanMap.TouchAndProbeSql compares live hashes against to catch in-place +/// rewrites on cheap columns alone. public readonly record struct FetchedPlan(long PlanId, string? PlanXml, string? PlanHash); /// @@ -41,15 +42,17 @@ namespace PerformanceMonitor.Darling.Storage; public static class QueryStorePlanWriter { /// - /// Lands a fetch's plans for one database. Returns the plan_ids whose content actually stored, in the order - /// they were supplied, which is what the caller feeds to - /// QueryStorePlanXmlState.AdvanceWatermark — the watermark must reflect what LANDED, not what was - /// selected, or a torn pass advances past content that never arrived. + /// Lands a fetch's plans for one database. Returns the plan_ids that landed, in the order they were + /// supplied — the caller uses them to clear its budget-carry-over set, since anything landed is no + /// longer missing. /// - /// A plan with NULL XML counts as landed and gets NO dimension row and NO map row: there is no content - /// to key, and inventing a digest for absent content would make the map point at nothing. The watermark - /// still advances past it, which is correct — that plan's XML will never exist, and stalling on it forever is - /// the failure the budget predicate already had to be fixed for twice. + /// A plan with NULL XML gets NO dimension row (there is no content to key, and inventing a digest + /// for absent content would make the map point at nothing) but it DOES get a map row with a NULL digest + /// — the #2312 content-less marker. Under store-as-watermark the map row IS the fact that the plan was + /// fetched and the engine had nothing to give: without it the probe reads the plan as missing and the + /// fetch re-selects it every cycle forever, which is the old oversized-plan stall reborn through the + /// probe. Readers are unaffected — a NULL digest joins to no dimension row, which renders exactly like + /// the absent content it records. /// public static async Task> WriteAsync( NpgsqlConnection connection, @@ -79,7 +82,7 @@ public static async Task> WriteAsync( var mapServerIds = new List(plans.Count); var mapDatabases = new List(plans.Count); var mapPlanIds = new List(plans.Count); - var mapDigests = new List(plans.Count); + var mapDigests = new List(plans.Count); var mapHashes = new List(plans.Count); /* Naive() on the stamp, not the raw UTC value: these are ::timestamp parameters, and Npgsql would infer @@ -92,14 +95,13 @@ timestamptz from a Utc Kind and let Postgres convert into the session zone on th { landed.Add(plan.PlanId); - if (string.IsNullOrEmpty(plan.PlanXml)) + byte[]? digest = null; + if (!string.IsNullOrEmpty(plan.PlanXml)) { - continue; + digest = PayloadDimensions.Digest(plan.PlanXml!); + batch.Add(PayloadDimensions.QueryPlanDimTable, digest, plan.PlanXml!); } - var digest = PayloadDimensions.Digest(plan.PlanXml!); - batch.Add(PayloadDimensions.QueryPlanDimTable, digest, plan.PlanXml!); - mapServerIds.Add(serverId); mapDatabases.Add(databaseName); mapPlanIds.Add(plan.PlanId); @@ -107,11 +109,6 @@ timestamptz from a Utc Kind and let Postgres convert into the session zone on th mapHashes.Add(plan.PlanHash); } - if (mapPlanIds.Count == 0) - { - return landed; - } - using var transaction = await connection.BeginTransactionAsync(cancellationToken); /* Dimension FIRST — see the class comment on why the torn-write side matters. */ diff --git a/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextStore.cs b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextStore.cs index 3039ddc9..c52d4d8b 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextStore.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextStore.cs @@ -71,27 +71,76 @@ CREATE INDEX IF NOT EXISTS idx_query_store_text_last_seen /// The conflict arm overwrites the TEXT, not just the stamp, and that is load-bearing /// rather than defensive. query_id is unique within a database only until Query Store is reset: /// a reset renumbers from the start, so id 5 afterwards is a DIFFERENT statement than id 5 before. The - /// refresh horizon on the watermark is what brings us back to re-read it, and this is where the - /// corrected text has to land. Touching only last_seen would leave the old statement's text - /// attached to the new id forever, which reads as a plausible wrong answer rather than as missing - /// data. + /// hash comparison is what brings us back to re-read it (#2312 — the + /// retired watermark's daily expiry used to, eventually), and this is where the corrected text has to + /// land. Touching only last_seen would leave the old statement's text attached to the new id + /// forever, which reads as a plausible wrong answer rather than as missing data. /// /// ORDER BY on the conflict key because concurrent batches that touch overlapping keys in /// different orders deadlock (#1801) — the same reason the plan map's upsert carries one. The /// WHERE EXCLUDED.last_seen >= guard keeps the stamp monotonic so an out-of-order write - /// cannot age a row backwards into the prune's reach. + /// cannot age a row backwards into the prune's reach. query_hash takes EXCLUDED when the fetch + /// carried one and keeps the stored value otherwise — never replace knowledge with absence. /// public const string UpsertSql = @"INSERT INTO collect.query_store_text - (server_id, database_name, query_id, query_sql_text, last_seen) -SELECT server_id, database_name, query_id, query_sql_text, stamped -FROM unnest($1::integer[], $2::text[], $3::bigint[], $4::text[], $5::timestamp[]) - AS batch(server_id, database_name, query_id, query_sql_text, stamped) + (server_id, database_name, query_id, query_sql_text, query_hash, last_seen) +SELECT server_id, database_name, query_id, query_sql_text, query_hash, stamped +FROM unnest($1::integer[], $2::text[], $3::bigint[], $4::text[], $5::text[], $6::timestamp[]) + AS batch(server_id, database_name, query_id, query_sql_text, query_hash, stamped) ORDER BY server_id, database_name, query_id ON CONFLICT (server_id, database_name, query_id) DO UPDATE SET query_sql_text = EXCLUDED.query_sql_text, + query_hash = COALESCE(EXCLUDED.query_hash, query_store_text.query_hash), last_seen = EXCLUDED.last_seen WHERE EXCLUDED.last_seen >= query_store_text.last_seen"; + /// + /// The text side's liveness touch and missing-set probe (#2312), the single-table sibling of + /// : refresh last_seen for every statement the + /// cycle's batch references (hourly-guarded, same write-amplification argument), adopt the batch's + /// query_hash where the stored one is NULL (legacy rows from before the column existed), and + /// return per batch row whether the store already holds the text and whether the stored hash still + /// matches the live one. hash_stale is the Query Store RESET detector: ids renumber, so id 5 + /// carrying a different hash means it now names a different statement and its text must be refetched — + /// per-id, within one cycle, where the retired watermark design re-walked the whole catalog daily to + /// eventually notice. + /// + public const string TouchAndProbeSql = @"WITH touched AS ( + SELECT t.server_id, t.database_name, t.query_id, batch.query_hash AS live_hash + FROM collect.query_store_text AS t + JOIN unnest($1::integer[], $2::text[], $3::bigint[], $4::text[]) + AS batch(server_id, database_name, query_id, query_hash) + ON batch.server_id = t.server_id + AND batch.database_name = t.database_name + AND batch.query_id = t.query_id + WHERE t.last_seen < $5::timestamp - interval '1 hour' + ORDER BY t.server_id, t.database_name, t.query_id +), +text_touch AS ( + UPDATE collect.query_store_text AS t + SET last_seen = $5::timestamp, + query_hash = COALESCE(t.query_hash, x.live_hash) + FROM touched AS x + WHERE t.server_id = x.server_id + AND t.database_name = x.database_name + AND t.query_id = x.query_id + RETURNING t.query_id +) +SELECT + batch.server_id, + batch.database_name, + batch.query_id, + (t.query_id IS NOT NULL) AS resolved, + (t.query_id IS NOT NULL AND t.query_hash IS NOT NULL AND batch.query_hash IS NOT NULL + AND t.query_hash <> batch.query_hash) AS hash_stale +FROM unnest($1::integer[], $2::text[], $3::bigint[], $4::text[]) + AS batch(server_id, database_name, query_id, query_hash) +LEFT JOIN collect.query_store_text AS t + ON t.server_id = batch.server_id + AND t.database_name = batch.database_name + AND t.query_id = batch.query_id +ORDER BY batch.server_id, batch.database_name, batch.query_id"; + /// /// Retires text whose facts have all aged out, bounded to roughly one chunk-width of the oldest rows /// per call so a single sweep cannot take an unbounded row lock — the same shape and the same reason as diff --git a/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextWriter.cs b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextWriter.cs index 09aad9ed..264e81b1 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextWriter.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/QueryStoreTextWriter.cs @@ -15,7 +15,10 @@ namespace PerformanceMonitor.Darling.Storage; /// One statement's text as the fetch returned it. -public readonly record struct FetchedQueryText(long QueryId, string? QueryText); +/// SQL Server's query_hash, the renumbering detector (#2312): query_id is +/// only unique until a Query Store reset, so the stored hash is what lets the probe see that an id now +/// names a DIFFERENT statement and refetch its text. +public readonly record struct FetchedQueryText(long QueryId, string? QueryText, string? QueryHash); /// /// Lands what the query-text fetch returned into (#2150). @@ -29,16 +32,13 @@ public static class QueryStoreTextWriter { /// /// Lands a fetch's statement text for one database, returning the query_ids that stored, in the - /// order supplied — which is what the caller feeds to - /// . The watermark must - /// reflect what LANDED rather than what was selected, or a torn pass advances past text that never - /// arrived. + /// order supplied — the caller uses them to clear its budget-carry-over set, since anything landed is + /// no longer missing (#2312). /// /// Rows with null text are stored as null rather than skipped. Query Store does not produce them /// in practice, so this is about not having a special case to get wrong: a null in this store means "we /// fetched and there was nothing", the readers already COALESCE onto the fact row's own column, - /// and the watermark advances either way — stalling on a statement whose text will never exist is the - /// failure the budget predicate had to be fixed for twice on the plan side. + /// and the stored row is what stops the probe from re-selecting the id as missing forever. /// public static async Task> WriteAsync( NpgsqlConnection connection, @@ -68,6 +68,7 @@ public static async Task> WriteAsync( var databases = new string[texts.Count]; var queryIds = new long[texts.Count]; var bodies = new string?[texts.Count]; + var hashes = new string?[texts.Count]; var stamps = new DateTime[texts.Count]; /* Naive() on the stamp, not the raw UTC value: last_seen is a ::timestamp parameter, and Npgsql @@ -85,6 +86,7 @@ way in (#1969). A last_seen written at the wrong hour ages rows out ahead of the databases[i] = databaseName; queryIds[i] = text.QueryId; bodies[i] = text.QueryText; + hashes[i] = text.QueryHash; stamps[i] = stamp; } @@ -93,6 +95,7 @@ way in (#1969). A last_seen written at the wrong hour ages rows out ahead of the upsert.Parameters.AddWithValue(databases); upsert.Parameters.AddWithValue(queryIds); upsert.Parameters.AddWithValue(bodies); + upsert.Parameters.AddWithValue(hashes); upsert.Parameters.AddWithValue(stamps); await upsert.ExecuteNonQueryAsync(cancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs index 13c4693a..a606ccc0 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs @@ -16,5 +16,5 @@ namespace PerformanceMonitor.Darling.Storage; /// public static class StorageVersion { - public const int SchemaVersion = 76; + public const int SchemaVersion = 77; } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 376e8361..a9590537 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -525,7 +525,8 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'pg_statement_text'), EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'query_store_text'), EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'config_service' AND column_name = 'plan_content_retention_days'), - EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'query_store_health')"; + EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'query_store_health'), + EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'query_store_text' AND column_name = 'query_hash')"; /// The store schema version this viewer build requires — the highest migration it knows /// (). The connect-time gate blocks a store below this. @@ -546,7 +547,7 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') await using var reader = await command.ExecuteReaderAsync(cancellationToken); if (await reader.ReadAsync(cancellationToken)) { - return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51)); + return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51), reader.GetBoolean(52)); } return null; @@ -571,7 +572,7 @@ OR NOT EXISTS (SELECT 1 FROM pg_extension WHERE extname = 'timescaledb') /// is unit-tested without a live store; any schema bump past the newest arm trips the pinning test that keeps /// this in step with . /// - internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false) + internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false, bool hasQueryStoreTextHash = false) { /* V71 (the PostgreSQL blocking-edges rung): a table-existence sentinel and now the newest-first arm. A collector table would ordinarily get no arm at all — see the V63-V69 note below — but the TOP @@ -614,6 +615,15 @@ from it. */ rather than falling through. The table is named only in the probe line, not in this prose, per the V71 finding: the coverage ratchet strips information_schema lines but cannot strip a comment, so a prose mention would exempt it. */ + /* #2312: newest first. The arm below STAYS — a store migrated to exactly 76 must map to 76 + rather than falling through. The column is named only in the probe line, not in this prose, + per the V71 finding: the coverage ratchet strips information_schema lines but cannot strip + a comment, so a prose mention would exempt it. */ + if (hasQueryStoreTextHash) + { + return 77; + } + if (hasQueryStoreHealth) { return 76; diff --git a/Darling/README.md b/Darling/README.md index 77243e15..5d1a9b7a 100644 --- a/Darling/README.md +++ b/Darling/README.md @@ -621,6 +621,7 @@ The notable rungs are below. For the **complete** current schema, read `Darling/ | **V72** — Query Store plan map | `collect.query_store_plan_map` — `(server_id, database_name, plan_id)` → digest, so Query Store facts can reference plan XML they no longer carry once that content moves into the shared `query_plan_dim`. Plan XML was stored INLINE on `query_store_stats` at roughly 5x redundancy. Not a hypertable: one row per distinct plan per database, so it is dimension-shaped and pruned on `last_seen` rather than by `drop_chunks`. Its `last_seen` is load-bearing — the dimension GC sweeps on timestamps rather than counting references, so ending the re-shipping also ends the liveness signal that used to keep those dim rows alive | | **V73** — PostgreSQL statement text | `collect.pg_statement_text` — `(server_id, queryid)` → statement text, refreshed hourly, so `get_pg_top_queries` returns something readable (#2219). `pg_statement_stats` stores no text because `showtext` is a real per-collection cost and normalized text is highly repetitive; but `queryid` is NOT stable across a major version upgrade, so without this the stored history joins to nothing after one — a list of integers that used to be your slowest queries, unrecoverable because the live view no longer holds the old ids. Text is INLINE rather than a `query_text_dim` digest: the dimension route needs the GC liveness interlock whose failure mode is silently missing text, and inline cannot dangle. Not a hypertable and not a collector table, exactly like V72 — a bespoke upsert path, pruned on `last_seen` with a margin that makes text OUTLIVE the statistics referencing it | | **V76** — Query Store health | `collect.query_store_health` + its index + the `v_query_store_health` passthrough view — the #2319 per-database `sys.database_query_store_options` collector's store table: actual vs desired state (the cap-hit READ_ONLY transition and its readonly_reason), current vs max storage, cleanup thresholds, and the runtime-stats interval length. A fresh store gets the table from V1's generated schema; V76 is what an already-existing store gets | +| **V77** — Activity-driven plan fetch | Three strokes behind #2312's reshape of the Query Store plan/text fetch: `query_store_plan_map.digest` goes **nullable** (a plan whose XML the engine cannot persist gets a NULL-digest map row — the content-less marker that stops the probe re-selecting it forever), `query_store_text` gains `query_hash` (the Query Store reset detector: an id whose stored hash differs from the live one names a DIFFERENT statement now and its text refetches within one cycle), and the retired `planwm:`/`textwm:` watermark state rows are deleted wholesale. The fetch itself no longer walks the plan catalog by watermark — the cycle's collected rows name their plans, the store answers which are missing, and only those are fetched | All timestamps in the store are **naive-UTC** `timestamp` columns — the product-wide cross-store contract (Lite's DuckDB does the same). diff --git a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs index ea0d5892..4f9246af 100644 --- a/Lite.Tests/QueryStoreCollectorDefinitionTests.cs +++ b/Lite.Tests/QueryStoreCollectorDefinitionTests.cs @@ -566,55 +566,59 @@ public void WithTheFlag_TheTextIsNulledAtTheSameOrdinal() } /// - /// #2150: the text fetch resumes from a query_id watermark, is cut by an exact byte budget, and - /// ships in query_id order — the ordering being what makes a budget cut a SUFFIX, so the highest - /// stored id resumes with no hole. + /// #2150's split, driven the #2312 way: the text fetch selects exactly the query_ids the caller names — + /// the cycle's collected rows whose text the store does not hold — cut by an exact byte budget in + /// query_id order. There is no watermark to resume from any more; the STORE answers what is + /// missing, and an empty missing set issues no query at all. /// [Fact] - public void TextFetch_ResumesFromTheWatermark_AndIsBudgetCutInQueryIdOrder() + public void TextFetchByIds_SelectsTheNamedIds_AndIsBudgetCutInQueryIdOrder() { - var sql = QueryStoreCollector.Instance.BuildTextFetchQuery( - "SO", MakeContext(fetchQueryTextSeparately: true), watermark: 4242, - candidateTexts: QueryStoreTextState.CandidateTexts, budgetBytes: 12 * 1024 * 1024).Text; + var sql = QueryStoreCollector.Instance.BuildTextFetchByIdsQuery( + "SO", MakeContext(fetchQueryTextSeparately: true), new long[] { 4242, 4243 }, + budgetBytes: 12 * 1024 * 1024).Text; Assert.Contains("EXECUTE [SO].sys.sp_executesql", sql, StringComparison.Ordinal); - Assert.Contains("WHERE qsq.query_id > 4242", sql, StringComparison.Ordinal); - Assert.Contains($"TOP ({QueryStoreTextState.CandidateTexts})", sql, StringComparison.Ordinal); - Assert.Contains("ORDER BY qsq.query_id", sql, StringComparison.Ordinal); + Assert.Contains("WHERE qsq.query_id IN (4242, 4243)", sql, StringComparison.Ordinal); + Assert.DoesNotContain("qsq.query_id > ", sql, StringComparison.Ordinal); + Assert.DoesNotContain("SELECT TOP", sql, StringComparison.Ordinal); Assert.Contains("ORDER BY b.query_id", sql, StringComparison.Ordinal); Assert.Contains("b.running_bytes - b.text_bytes < 12582912", sql, StringComparison.Ordinal); + /* query_id is only unique until a Query Store reset renumbers it; the hash is the reset detector. */ + Assert.Contains("query_hash = CONVERT(varchar(64), qsq.query_hash, 1)", sql, StringComparison.Ordinal); /* ROWS, not the RANGE default: RANGE tie-groups peers and forces a spool, and the frame has to be per-row because the cut falls BETWEEN two statements. */ Assert.Contains("ROWS UNBOUNDED PRECEDING", sql, StringComparison.Ordinal); Assert.Contains("OPTION(RECOMPILE)", sql, StringComparison.Ordinal); - /* It fetches text and nothing else — plan XML has its own fetch, with its own watermark. */ + /* It fetches text and nothing else — plan XML has its own fetch. */ Assert.DoesNotContain("query_plan", sql, StringComparison.Ordinal); } /// - /// #2150: every input that would make the fetch ship nothing and silently stall the watermark throws - /// instead. A stalled watermark looks exactly like a quiet database, which is why these are exceptions - /// rather than no-ops — the plan fetch learned this the hard way from several directions. + /// Every input that would make the fetch ship nothing — and leave the ids missing forever — throws + /// instead. A permanent missing set looks exactly like a quiet database, which is why these are + /// exceptions rather than no-ops; the plan fetch learned this the hard way from several directions. /// [Fact] - public void TextFetch_RefusesInputsThatWouldStallTheWatermark() + public void TextFetchByIds_RefusesInputsThatWouldStall() { var enabled = MakeContext(fetchQueryTextSeparately: true); + var ids = new long[] { 1 }; /* Issuing it while the host still ships text inline would fetch and store text nobody reads. */ Assert.Throws(() => - QueryStoreCollector.Instance.BuildTextFetchQuery("SO", MakeContext(), 0, 5_000, 1024)); + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery("SO", MakeContext(), ids, 1024)); /* `running_bytes - text_bytes < 0` excludes even the first candidate, so the pass ships nothing. */ Assert.Throws(() => - QueryStoreCollector.Instance.BuildTextFetchQuery("SO", enabled, 0, 5_000, 0)); + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery("SO", enabled, ids, 0)); - /* TOP (0) returns no rows; a negative literal is a syntax error. */ - Assert.Throws(() => - QueryStoreCollector.Instance.BuildTextFetchQuery("SO", enabled, 0, 0, 1024)); + /* Empty means "nothing missing" — the caller must skip, not build IN (). */ + Assert.Throws(() => + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery("SO", enabled, Array.Empty(), 1024)); Assert.Throws(() => - QueryStoreCollector.Instance.BuildTextFetchQuery("SO", null!, 0, 5_000, 1024)); + QueryStoreCollector.Instance.BuildTextFetchByIdsQuery("SO", null!, ids, 1024)); } /// diff --git a/Lite.Tests/QueryStoreStatePruneTests.cs b/Lite.Tests/QueryStoreStatePruneTests.cs index c4a81332..151039db 100644 --- a/Lite.Tests/QueryStoreStatePruneTests.cs +++ b/Lite.Tests/QueryStoreStatePruneTests.cs @@ -215,19 +215,18 @@ await SeedStateAsync(ServerId, QueryStoreBackfillState.StateCollectorName, [Fact] public async Task Prune_RunsTheWatermarkStatementToo_EvenThoughLiteWritesNone() { - /* Lite iterates the SHARED QueryStorePerDatabaseState.PrunableKeys, which carries planwm: even - though Lite never writes it. That is deliberate: the day plan capture is enabled here, the prune - is already in place rather than being a thing somebody has to remember. Today it must simply be - harmless — one delete matching nothing — which is what this checks by proving a planted planwm: - row for a DROPPED database is retired by the same pass. */ + /* Lite iterates the SHARED QueryStorePerDatabaseState.PrunableKeys — including prefixes Lite may + never write itself. That is deliberate: a prefix pruned on one SKU and orphaning on the other is + the drift the shared list exists to prevent, and the cost is one delete matching nothing. Proven + here by planting a qsowm: row for a DROPPED database and watching the same pass retire it. */ await SeedSnapshotAsync(Newest, "Live"); - await SeedStateAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, - QueryStorePlanXmlState.WatermarkKeyPrefix + "Dropped", "900000:1786449600"); + await SeedStateAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, + QueryStoreOpenIntervalState.WatermarkKeyPrefix + "Dropped", "900000:1786449600"); await _pruner.PruneAsync(ServerId); - Assert.Null(await ValueAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, - QueryStorePlanXmlState.WatermarkKeyPrefix + "Dropped")); + Assert.Null(await ValueAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, + QueryStoreOpenIntervalState.WatermarkKeyPrefix + "Dropped")); } /* ---------------- helpers ---------------- */ @@ -417,23 +416,23 @@ public async Task ForeignPrune_WithNoOwnDatabase_RetiresNothing(string ownDataba } /// - /// Every per-database prefix is pruned, not just the backfill ones — the watermark prefix included, even - /// though Lite writes none today (it never sets CapturePlanXml). Pinned for the same reason the - /// on-prem twin pins it: the shared prefix list is what stops a prefix being pruned on one SKU and - /// orphaning on the other, and a Lite-only omission would be invisible on Darling. + /// Every per-database prefix is pruned, not just the backfill ones — the open-interval stamp included. + /// Pinned for the same reason the on-prem twin pins it: the shared prefix list is what stops a prefix + /// being pruned on one SKU and orphaning on the other, and a Lite-only omission would be invisible on + /// Darling. (#2312 retired the planwm:/textwm: families this fact previously exercised.) /// [Fact] public async Task ForeignPrune_CoversTheWatermarkPrefixToo() { - var foreignWatermark = QueryStorePlanXmlState.KeyFor("Sibling-A"); - var ownWatermark = QueryStorePlanXmlState.KeyFor("Payments"); + var foreignWatermark = QueryStoreOpenIntervalState.KeyFor("Sibling-A"); + var ownWatermark = QueryStoreOpenIntervalState.KeyFor("Payments"); - await SeedStateAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, foreignWatermark, "8140"); - await SeedStateAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, ownWatermark, "8150"); + await SeedStateAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, foreignWatermark, "8140"); + await SeedStateAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, ownWatermark, "8150"); await _pruner.PruneForeignAsync(ServerId, "Payments"); - Assert.Null(await ValueAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, foreignWatermark)); - Assert.Equal("8150", await ValueAsync(ServerId, QueryStorePlanXmlState.StateCollectorName, ownWatermark)); + Assert.Null(await ValueAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, foreignWatermark)); + Assert.Equal("8150", await ValueAsync(ServerId, QueryStoreOpenIntervalState.StateCollectorName, ownWatermark)); } } diff --git a/Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs b/Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs index e9076cfc..af8f1772 100644 --- a/Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs +++ b/Lite/Services/RemoteCollectorService.QueryStoreBackfill.cs @@ -563,7 +563,7 @@ protected async Task DeleteCollectorStateKeyAsync( /// collected database_states prunes nothing rather than everything. /// /// Which keys comes from the SHARED , - /// deliberately including planwm: that Lite never writes: running one no-op delete is what + /// deliberately including prefixes Lite never writes: running one no-op delete is what /// guarantees that enabling plan capture here later cannot quietly create an orphan class this forgot /// about. Best-effort like its siblings — a failed prune leaves the rows and the next tick retries. /// diff --git a/PerformanceMonitor.Collectors/CollectorContext.cs b/PerformanceMonitor.Collectors/CollectorContext.cs index 7f0194ff..596517e4 100644 --- a/PerformanceMonitor.Collectors/CollectorContext.cs +++ b/PerformanceMonitor.Collectors/CollectorContext.cs @@ -127,7 +127,7 @@ public sealed class CollectorContext /// /// When true, the query_store payload leaves query_sql_text NULL and the host is responsible - /// for resolving statement text through instead + /// for resolving statement text through instead /// (#2150). Default false, which keeps the text inline exactly as it ships today. /// /// Why the text has to leave that projection. The payload selects it inside a diff --git a/PerformanceMonitor.Collectors/QueryStoreCollector.cs b/PerformanceMonitor.Collectors/QueryStoreCollector.cs index 18b4497f..737d30e8 100644 --- a/PerformanceMonitor.Collectors/QueryStoreCollector.cs +++ b/PerformanceMonitor.Collectors/QueryStoreCollector.cs @@ -10,6 +10,7 @@ using System.Collections.Generic; using System.Data.Common; using System.Globalization; +using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -684,44 +685,21 @@ stays DESC even though the outer sort is now ASC (#1960): under oldest-first shi 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. */ - /* #2164: skip the XML for plans the store already holds. 97% of the plan XML shipped in a - three-hour fleet window was for plans held over an hour — the ROW_NUMBER gate ships each plan once - per PASS but re-ships it every pass forever, and since drain is 94-97% of a pass and is per-row LOB - cost, NOT fetching is worth far more than fetching less. The watermark is the highest plan_id whose - XML was actually STORED for this database; plan_id is monotonic within a database, so a higher id - is a plan we have never stored. Inlined as a parsed long (never operator input) because the body - nests inside sp_executesql on three paths and threading another parameter through all of them buys - nothing. Zero — absent, malformed, or expired — renders no predicate, so the conservative path is - byte-identical to the pre-#2164 query. - - NEVER on the backfill path. The watermark tracks the plans the LIVE window has stored, and backfill - digs the other way — into intervals older than anything collected, whose rows reference plans - compiled long ago and therefore numbered BELOW the live watermark. Applying it there would suppress - essentially every plan the backfill exists to fetch, silently: the slices would still ship runtime - stats, so a filled range would look complete while carrying no plan XML at all. - - KNOWN GAP, bounded by QueryStorePlanXmlState.RefreshAfter: plan_id is monotonic in COMPILE order, which is - not the same as "we have stored it". A plan compiled before monitoring began, dormant through every - collected window, then executed again, arrives with an id below the watermark and has its XML - suppressed until the refresh horizon expires. Bounding it is the reason that horizon exists. The - exact fix is a store-DERIVED watermark (the host asking its own plan dimension for the lowest - plan_id missing XML) rather than this collector-derived one; that needs host plumbing on both - products and is tracked separately. */ - /* #2210: the watermark now belongs to BuildPlanFetchQuery (the `watermark` parameter there, - resolved by the host via QueryStorePlanXmlState.Resolve). It no longer narrows anything in - this runtime-stats query, so there is nothing to compute here. */ - - /* #2210: this runtime-stats query no longer carries plan XML at all — the ROW_NUMBER-gated - CASE and its in-stream watermark predicate are DELETED, not reworked. BuildPlanFetchQuery is - the only thing that reads plan XML now: it fetches plans in plan_id order under a byte - budget and lands each plan ONCE per database LIFETIME instead of once per PASS. The shape - being replaced re-shipped every plan on every pass forever — measured at 5.0x redundancy - (871,196 plan-XML rows against 175,328 distinct database/plan pairs in a day, on a 33 GB - table). Both branches below now emit the same placeholder, so the payload is byte-identical - to Lite's regardless of the flag, and CapturePlanXml gates the separate BuildPlanFetchQuery - fetch rather than this query. Existing inline rows are NOT migrated by this change and stay - readable via the reader's existing NULL-guarded fallback; dropping the query_plan_text column - itself is a separate, later migration. */ + /* #2312: this runtime-stats query no longer carries plan XML at all — the #2164 ROW_NUMBER + gate and its in-stream watermark predicate were DELETED by #2210, and #2312 then retired + the watermark itself: the #2164 KNOWN GAP's "exact fix is a store-DERIVED watermark" is + what the activity-driven fetch now IS. BuildPlanFetchByIdsQuery is the only thing that + reads plan XML: the host probes its own store for the plans this cycle's rows reference + and fetches exactly the missing ones, so each plan lands ONCE per database lifetime, a + dormant plan resuming execution is fetched the cycle it resumes (no refresh horizon to + wait out), and a caught-up database issues no fetch at all. The shape #2210 replaced + re-shipped every plan on every pass forever — measured at 5.0x redundancy (871,196 + plan-XML rows against 175,328 distinct database/plan pairs in a day, on a 33 GB table). + Both branches below emit the same placeholder, so the payload is byte-identical to Lite's + regardless of the flag, and CapturePlanXml gates the separate by-ids fetch rather than + this query. Existing inline rows are NOT migrated and stay readable via the reader's + NULL-guarded fallback; backfill plan XML stays on its own rows for the same reason it + always did — those intervals' plans are never in the live cycle's reference set. */ const string planTextCol = "query_plan_text = CONVERT(nvarchar(1), NULL),"; /* #2150: the LAST nvarchar(max) in this projection, and now the whole remaining cost of it. The cap @@ -1128,53 +1106,58 @@ EXECUTE [{escapedDbName}].sys.sp_executesql } /// - /// The plan-XML fetch for one database (#2210): plans above the watermark, in plan_id order, bounded - /// twice — coarsely by and exactly by a running byte total. + /// The plan-XML fetch for one database (#2312 Finding 2): exactly the plans the caller names — the cycle's + /// collected runtime rows whose XML the store does not already hold — in plan_id order, cut exactly + /// by a running byte total. The STORE is the watermark: a caught-up database has an empty missing set and + /// no fetch runs at all, which is the property the #2210 catalog walk lacked (measured 23s to discover + /// "nothing new" on a warm catalog, every cycle, because the walk re-read the catalog to find out). /// - /// SEPARATE from the runtime-stats query on purpose, and that separation is the fix rather than a - /// refactor. The runtime query ships ORDER BY qsrs.last_execution_time, so a budget cut truncates it - /// in TIME order and the plans whose XML landed are an arbitrary SUBSET of plan_ids — against which no - /// watermark value is safe, because receiving plan 500 while missing 300 skips 300 forever. That is why the - /// previous shape could not advance on a cut, and 97.8% of production passes are cut. Here rows arrive in - /// plan_id order, so a cut truncates a SUFFIX and the highest landed id is safe by construction. + /// SEPARATE from the runtime-stats query on purpose, and that separation is still the fix rather than + /// a refactor. The runtime query ships ORDER BY qsrs.last_execution_time, so a budget cut truncates + /// it in TIME order — plan XML inline there re-shipped the same plans at 5.0x (measured, #2210). Here the + /// caller hands an explicit id list, so a budget cut leaves ids that are simply STILL MISSING from the + /// store, and the next cycle that references them (or the caller's own carry-over) re-selects them. No + /// watermark, no suffix-safety argument, no out-of-order hazard. /// - /// The two bounds are not redundant. The running total is exact but expensive to compute: it needs - /// DATALENGTH, and sys.query_store_plan.query_plan is decompressed BY the view on access, so an - /// unbounded candidate set pays a whole catalog's decompression to enforce a budget meant to prevent exactly - /// that. TOP (@candidate_plans) is evaluated on plan_id alone — no XML touched to sort or - /// filter — so the decompression is capped at K, sized per database by - /// from the previous pass's own bytes-per-plan. + /// The candidate bound is the ID LIST ITSELF, which the caller caps via + /// before building: the running total needs + /// DATALENGTH, and sys.query_store_plan.query_plan is decompressed BY the view on access, so + /// handing the whole missing set of a first-contact database in one statement would pay its entire + /// decompression to enforce a budget meant to prevent exactly that. Chunking and capping are caller + /// decisions; this builder's contract is only "the list you hand me is what I decompress". + /// + /// query_plan_hash rides along in the SELECT — CONVERT(varchar(64), ..., 1), the same + /// rendering the runtime payload uses — because it reads WITHOUT decompressing the plan and the map stores + /// it as the in-place-rewrite detector: a batch whose live hash differs from the stored one is the one case + /// activity-driven fetch cannot see on its own. /// /// The budget test is running_bytes - plan_bytes < budget, i.e. admit a plan when the total /// BEFORE it was still under. The obvious running_bytes <= budget is a per-database STALL: a single /// plan larger than the whole budget has a running total that already exceeds it on its own row, so it is - /// excluded, every later row is excluded too (the total is monotonic), the pass ships nothing, the watermark - /// holds, and the next pass re-selects the same plan first — forever. One 13 MB plan against the 12 MB - /// default is enough, and it is the same never-advances failure this change exists to end, reached through - /// plan SIZE instead of cut ordering. Admitting the offender ships it alone, cuts after it, and moves the - /// watermark past it. + /// excluded, every later row is excluded too (the total is monotonic), the pass ships nothing, the plan + /// stays missing, and the next pass re-selects it first — forever. One 13 MB plan against the 12 MB + /// default is enough. Admitting the offender ships it alone and cuts after it; once landed it is never + /// selected again. /// /// The honest cost of that: worst-case bytes for one pass are budget + largest single plan, /// not budget. The runtime-stats budget a few hundred lines up pays exactly the same price for the /// same reason (measured: 19.6 MB shipped against a 12 MB budget when one very large plan carried a pass /// past it), so "12 MB" is a floor on ship volume in both paths rather than a cap. /// - /// Both bounds are inlined as parsed integers rather than parameters, matching the watermark predicate - /// above and for the same reason: the body nests inside sp_executesql, and the values are host-computed - /// longs that never touch operator input. + /// The budget and the id list are inlined as parsed integers rather than parameters, and for the same + /// reason as each other: the body nests inside sp_executesql, and the values are host-computed longs + /// that never touch operator input. /// /// A NULL query_plan — a plan too large to persist, or certain forced-plan-failure paths — /// counts as ZERO bytes and STILL SHIPS, as a row with NULL text. Letting the NULL propagate through the - /// arithmetic instead would make the budget predicate NULL and filter the row out, and a window whose plans - /// are all NULL would then return nothing, hold the watermark, and re-select the same plans forever: the - /// same permanent stall as the oversized-plan case, reached through a different mechanism. Shipping the row - /// lets the watermark advance past a plan whose XML will never exist, which is correct — the store's readers - /// already guard query_plan_text IS NOT NULL because the runtime path has always been able to write - /// per-row NULLs there. + /// arithmetic instead would make the budget predicate NULL and filter the row out, and the plan would be + /// re-selected as missing forever. Shipping the row lets the writer record a content-less map row (the + /// NULL-digest marker), which is what makes "the engine says this XML will never exist" a stored fact + /// instead of a per-cycle rediscovery — the store's readers already guard + /// query_plan_text IS NOT NULL, so absent content renders as absent either way. /// - /// NEVER on the backfill path, for the reason the watermark itself is not: backfill reads intervals - /// older than anything collected, whose plans are numbered BELOW the watermark, so a plan_id-ascending fetch - /// above the watermark would return nothing the backfill needs. Backfill plan XML stays on its own rows. + /// NEVER on the backfill path: backfill plan XML stays on its own rows, exactly as before — this + /// fetch serves the live cycle's referenced plans and nothing else. /// /// The CONVERT happens ONCE, inside the candidate window, and the running total sums /// DATALENGTH of that converted text rather than of the view column. The alternative — measure with @@ -1184,7 +1167,7 @@ EXECUTE [{escapedDbName}].sys.sp_executesql /// form took 274ms cold / 262ms warm against 133ms for this one. Plan-id-only with no XML touched was 114ms, /// so this shape sits 19ms above the floor while the join-back form pays for the decompression twice. /// - public CollectorQuery BuildPlanFetchQuery(string item, CollectorContext context, long watermark, int candidatePlans, long budgetBytes) + public CollectorQuery BuildPlanFetchByIdsQuery(string item, CollectorContext context, IReadOnlyList planIds, long budgetBytes) { /* The invariant the doc comment spends a paragraph on, actually enforced rather than left to the caller: this query exists only to fetch plan XML, so building it with plan capture off is a caller bug, not a @@ -1198,48 +1181,49 @@ runtime query's plan-text CASE already reads the same flag. */ if (!context.CapturePlanXml) { throw new InvalidOperationException( - "BuildPlanFetchQuery requires CapturePlanXml; a host that does not capture plan XML must not issue the plan fetch."); + "BuildPlanFetchByIdsQuery requires CapturePlanXml; a host that does not capture plan XML must not issue the plan fetch."); } /* A non-positive budget would make the predicate `running_bytes - plan_bytes < 0`, which excludes even the FIRST candidate (its running total before it is 0, and 0 < 0 is false) — the pass ships nothing, - the watermark holds, and the next pass re-selects the same plans. The oversized-plan stall for a third - time, from a third direction. CandidatePlanCount already floors a non-positive budget for its own - sizing; this method has to guard its own input rather than assume the caller passed that value through. */ + and because the ids stay missing from the store, the next pass re-selects the same plans forever. + The oversized-plan stall, reached through the budget input rather than through cut ordering. */ if (budgetBytes <= 0) { throw new ArgumentOutOfRangeException( - nameof(budgetBytes), budgetBytes, "The plan-fetch byte budget must be positive; a zero or negative budget ships nothing and stalls the watermark."); + nameof(budgetBytes), budgetBytes, "The plan-fetch byte budget must be positive; a zero or negative budget ships nothing and the ids stay missing forever."); } - /* Same failure, fourth route: TOP (0) returns no rows and TOP with a negative literal is a syntax error, - so a bad candidate count ships nothing and holds the watermark exactly like a bad budget. Every caller - today sources this from CandidatePlanCount, which floors at MinCandidatePlans — but "the only caller - happens to be safe" is the assumption this method has already been wrong about once. */ - if (candidatePlans <= 0) + /* An empty id list means the store already holds every plan this cycle referenced — the steady state + whose whole point is that NO target query runs (#2312 Finding 2). Reaching this method with one is a + caller bug, not a no-op to swallow: an `IN ()` is a syntax error anyway, and silently returning a + no-op query would hide the caller's missing skip. */ + if (planIds is null || planIds.Count == 0) { - throw new ArgumentOutOfRangeException( - nameof(candidatePlans), candidatePlans, "The candidate plan count must be positive; TOP (0) ships nothing and stalls the watermark."); + throw new ArgumentException( + "The plan id list must be non-empty; an empty missing set means no fetch should be issued at all.", nameof(planIds)); } var escapedDbName = item.Replace("]", "]]", StringComparison.Ordinal); - var k = candidatePlans.ToString(System.Globalization.CultureInfo.InvariantCulture); var budget = budgetBytes.ToString(System.Globalization.CultureInfo.InvariantCulture); - var floor = watermark.ToString(System.Globalization.CultureInfo.InvariantCulture); + /* Host-computed longs, inlined like the budget and for the same reason: the body nests inside + sp_executesql, and none of these values ever touch operator input. */ + var idList = string.Join(", ", planIds.Select(id => id.ToString(System.Globalization.CultureInfo.InvariantCulture))); /* ROWS UNBOUNDED PRECEDING, not the RANGE default: RANGE would tie-group peers and, more to the point, forces a spool. The frame is per-row precisely because the cut has to fall between two plans. */ var body = $@"WITH candidates AS ( - SELECT TOP ({k}) + SELECT plan_id = qsp.plan_id, + query_plan_hash = CONVERT(varchar(64), qsp.query_plan_hash, 1), query_plan_text = CONVERT(nvarchar(max), qsp.query_plan) FROM sys.query_store_plan AS qsp - WHERE qsp.plan_id > {floor} - ORDER BY qsp.plan_id + WHERE qsp.plan_id IN ({idList}) ), budgeted AS ( SELECT plan_id = c.plan_id, + query_plan_hash = c.query_plan_hash, query_plan_text = c.query_plan_text, plan_bytes = COALESCE(DATALENGTH(c.query_plan_text), 0), running_bytes = SUM(COALESCE(DATALENGTH(c.query_plan_text), 0)) OVER (ORDER BY c.plan_id ROWS UNBOUNDED PRECEDING) @@ -1247,6 +1231,7 @@ FROM candidates AS c ) SELECT plan_id = b.plan_id, + query_plan_hash = b.query_plan_hash, query_plan_text = b.query_plan_text FROM budgeted AS b WHERE b.running_bytes - b.plan_bytes < {budget} @@ -1263,28 +1248,28 @@ EXECUTE [{escapedDbName}].sys.sp_executesql } /// - /// Statement text for one database, resumed from a query_id watermark and cut by a byte budget - /// (#2150) — the sibling of , and the other half of taking - /// query_sql_text out of the runtime stream. - /// - /// Ordered by query_id, which is what makes a budget cut safe. The cut falls between - /// two statements, so everything up to it is stored and the highest stored id is a resume point with no - /// hole — the same suffix argument the plan fetch rests on. query_id is also already a stored - /// payload column on the runtime row, so this needs no new fact-table column and no migration to be - /// joinable. + /// Statement text for one database (#2312 Finding 2, applying #2150's split): exactly the query_ids the + /// caller names — the cycle's collected rows whose text the store does not already hold — cut by a byte + /// budget. The sibling of , with the same store-as-watermark + /// contract: an empty missing set means no fetch runs at all. /// /// Simpler than the plan fetch on purpose. There is no candidate-window estimator here /// because DATALENGTH(query_sql_text) is cheap: sys.query_store_plan.query_plan is - /// decompressed BY the view on access, which is what forces the plan side to bound how many plans a - /// windowed running total may touch, and query_sql_text has no such cost. A flat coarse bound - /// plus the exact running total is enough. There is no content hash either — plan XML can be rewritten - /// in place, whereas a statement's text is fixed for the life of its id. + /// decompressed BY the view on access, which is what forces the plan side to cap its id list, and + /// query_sql_text has no such cost — the caller may hand the whole missing set (chunked only for + /// statement-size sanity). + /// + /// query_hash rides along — CONVERT(varchar(64), ..., 1), the runtime payload's own + /// rendering — because query_id is only unique until a Query Store reset renumbers it: a stored + /// hash that differs from the batch's live one is how the store detects that id 5 is now a DIFFERENT + /// statement and refetches, where the old design relied on a daily watermark expiry to eventually + /// re-read everything. /// /// ROWS UNBOUNDED PRECEDING rather than the RANGE default, for the same reason as /// the plan fetch: RANGE would tie-group peers and force a spool, and the frame has to be per-row /// because the cut falls between two rows. /// - public CollectorQuery BuildTextFetchQuery(string item, CollectorContext context, long watermark, int candidateTexts, long budgetBytes) + public CollectorQuery BuildTextFetchByIdsQuery(string item, CollectorContext context, IReadOnlyList queryIds, long budgetBytes) { if (context is null) { @@ -1297,44 +1282,43 @@ caller bug rather than a harmless extra round trip — it would fetch and store if (!context.FetchQueryTextSeparately) { throw new InvalidOperationException( - "BuildTextFetchQuery requires FetchQueryTextSeparately; a host that still ships query_sql_text inline must not issue the text fetch."); + "BuildTextFetchByIdsQuery requires FetchQueryTextSeparately; a host that still ships query_sql_text inline must not issue the text fetch."); } /* A non-positive budget makes the predicate `running_bytes - text_bytes < 0` exclude even the FIRST - candidate (its running total before it is 0, and 0 < 0 is false), so the pass ships nothing, the - watermark holds, and the next pass re-selects the same statements — a stall that looks like a - quiet database. */ + candidate (its running total before it is 0, and 0 < 0 is false), so the pass ships nothing and the + ids stay missing forever — a stall that looks like a quiet database. */ if (budgetBytes <= 0) { throw new ArgumentOutOfRangeException( - nameof(budgetBytes), budgetBytes, "The text-fetch byte budget must be positive; a zero or negative budget ships nothing and stalls the watermark."); + nameof(budgetBytes), budgetBytes, "The text-fetch byte budget must be positive; a zero or negative budget ships nothing and the ids stay missing forever."); } - /* Same stall, other route: TOP (0) returns no rows and a negative literal is a syntax error. */ - if (candidateTexts <= 0) + /* Same contract as the plan fetch: empty means the caller should not have called. */ + if (queryIds is null || queryIds.Count == 0) { - throw new ArgumentOutOfRangeException( - nameof(candidateTexts), candidateTexts, "The candidate text count must be positive; TOP (0) ships nothing and stalls the watermark."); + throw new ArgumentException( + "The query id list must be non-empty; an empty missing set means no fetch should be issued at all.", nameof(queryIds)); } var escapedDbName = item.Replace("]", "]]", StringComparison.Ordinal); - var k = candidateTexts.ToString(System.Globalization.CultureInfo.InvariantCulture); var budget = budgetBytes.ToString(System.Globalization.CultureInfo.InvariantCulture); - var floor = watermark.ToString(System.Globalization.CultureInfo.InvariantCulture); + var idList = string.Join(", ", queryIds.Select(id => id.ToString(System.Globalization.CultureInfo.InvariantCulture))); var body = $@"WITH candidates AS ( - SELECT TOP ({k}) + SELECT query_id = qsq.query_id, + query_hash = CONVERT(varchar(64), qsq.query_hash, 1), query_sql_text = qst.query_sql_text FROM sys.query_store_query AS qsq JOIN sys.query_store_query_text AS qst ON qst.query_text_id = qsq.query_text_id - WHERE qsq.query_id > {floor} - ORDER BY qsq.query_id + WHERE qsq.query_id IN ({idList}) ), budgeted AS ( SELECT query_id = c.query_id, + query_hash = c.query_hash, query_sql_text = c.query_sql_text, text_bytes = COALESCE(DATALENGTH(c.query_sql_text), 0), running_bytes = SUM(COALESCE(DATALENGTH(c.query_sql_text), 0)) OVER (ORDER BY c.query_id ROWS UNBOUNDED PRECEDING) @@ -1342,6 +1326,7 @@ FROM candidates AS c ) SELECT query_id = b.query_id, + query_hash = b.query_hash, query_sql_text = b.query_sql_text FROM budgeted AS b WHERE b.running_bytes - b.text_bytes < {budget} @@ -1449,12 +1434,6 @@ last_execution_time still ship (they are adjacent under the query's ASC order), var budgetSpent = false; DateTime? cutBoundary = null; - /* #2164 watermark bookkeeping: counts ONLY plans whose XML actually landed in this batch, so a - budget-cut pass cannot claim coverage it does not have. Plans observed but not stored are - deliberately not tracked — see QueryStorePlanXmlState.RefreshAfter for why the observed maximum cannot be - used to detect a Query Store reset. */ - long maxStoredPlanId = 0; - while (await reader.ReadAsync(cancellationToken)) { var row = new Row @@ -1550,11 +1529,6 @@ and finish the boundary tie group — the host surfaces the WARNING. Rows are re a bounded cycle costs latency, never data. */ textBytes += ((long)(row.QueryText?.Length ?? 0) + (row.QueryPlanText?.Length ?? 0)) * 2L; - if (row.QueryPlanText is not null && row.PlanId > maxStoredPlanId) - { - maxStoredPlanId = row.PlanId; - } - if (!budgetSpent && textBytes >= budget) { budgetSpent = true; @@ -1566,38 +1540,9 @@ and finish the boundary tie group — the host surfaces the WARNING. Rows are re context.PerItemTextBytesShipped = textBytes; context.PerItemShippedBoundary = rows.Count > 0 ? rows[^1].LastExecutionTime : null; - /* #2164: persist the plan-XML watermark for this database. - - Advance to the highest plan_id whose XML actually stored, never past it. - - Never move BACKWARD: a window whose newest-executing plan is older than the newest-COMPILED one - is an ordinary quiet window, not a reset, and lowering the watermark there would refetch the - whole catalog next cycle. (Treating it as a reset is the trap documented on - QueryStorePlanXmlState.RefreshAfter — it holds in most steady-state windows.) - - Never advance AT ALL on a budget-cut pass. Rows ship ordered by last_execution_time, NOT by - plan_id, so the cut drops an arbitrary set of plan_ids from the tail of the window — including - ids BELOW the highest one that did store. Advancing past them would suppress their XML on every - later pass (the ids no longer clear the watermark) even though it never shipped once. The cut is - already resumable on the time watermark, so declining to advance costs one repeated fetch and - nothing else. */ - if (context.CapturePlanXml && !string.IsNullOrEmpty(databaseName) && !budgetSpent && maxStoredPlanId > 0) - { - var standing = QueryStorePlanXmlState.Resolve(context.State, databaseName, context.CollectionTime); - - if (maxStoredPlanId > standing) - { - /* The stamp dates the last FULL fetch, and is carried FORWARD across advances rather than - renewed on each one. Re-stamping here would push the refresh horizon out every time a new - plan compiled, so on any database that keeps compiling — the busy ones, where a stale plan - is most likely to matter — the horizon would never fire and the watermark would effectively - be permanent. A standing watermark of 0 means this pass WAS the full fetch (absent or just - expired), so that is the one case that stamps now. */ - var stamp = standing > 0 - ? QueryStorePlanXmlState.ResolveStamp(context.State, databaseName) ?? context.CollectionTime - : context.CollectionTime; - - context.PendingState[QueryStorePlanXmlState.KeyFor(databaseName)] = - QueryStorePlanXmlState.Format(maxStoredPlanId, stamp); - } - } + /* #2312: no plan-XML watermark write-back any more. Inline-shipped plan XML (the backfill path) + lands on its own rows; the LIVE fetch is activity-driven against the store's own map, so there + is no resume point to persist here and nothing for a budget cut to corrupt. */ } diff --git a/PerformanceMonitor.Collectors/QueryStorePerDatabaseState.cs b/PerformanceMonitor.Collectors/QueryStorePerDatabaseState.cs index 949dd612..7e8a4714 100644 --- a/PerformanceMonitor.Collectors/QueryStorePerDatabaseState.cs +++ b/PerformanceMonitor.Collectors/QueryStorePerDatabaseState.cs @@ -14,21 +14,20 @@ namespace PerformanceMonitor.Collectors; /// Every collector_state key query_store owns that is keyed by DATABASE NAME (#2188) — the set both /// hosts prune when a database is dropped or renamed. /// -/// Nothing ever retired these. The #2164 plan-XML watermark writes one planwm: row per database -/// and the #2022/#2058 backfill worker writes done: and hole:, and while the worker deletes a -/// hole when it SERVICES or expires it, a dropped database will never service one — its hole can never be -/// dug and its tail can never drain. collector_state is a keyed registry rather than a hypertable -/// (pinned by CollectorStateContractTests), so no retention policy caught them either. +/// Nothing ever retired these. The #2022/#2058 backfill worker writes done: and hole:, +/// and while the worker deletes a hole when it SERVICES or expires it, a dropped database will never +/// service one — its hole can never be dug and its tail can never drain. collector_state is a keyed +/// registry rather than a hypertable (pinned by CollectorStateContractTests), so no retention policy +/// caught them either. (The #2164 planwm: and #2150 textwm: watermarks were members of this +/// list until #2312 retired the watermarks themselves — the fetches are activity-driven now, the store is +/// the watermark, and V77 deleted the orphaned rows once.) /// /// Shared rather than one list per host, which is the whole reason this file exists. The two /// stores prune with different dialects (Postgres anti-join, DuckDB NOT IN) and the SKUs write -/// different subsets — Lite never sets CollectorContext.CapturePlanXml, so it writes no -/// planwm: at all, while both write the backfill pair. A per-host list would make a fourth prefix a -/// two-place edit whose omission fails nothing: the rows would simply orphan on one SKU, invisibly, which is -/// the drift this product keeps paying for. Both hosts iterate THIS, so a prefix is pruned everywhere or -/// nowhere. Lite running the planwm: statement against rows it never writes costs one no-op delete -/// and buys the guarantee that enabling plan capture there cannot quietly create an unpruned orphan -/// class. +/// different subsets. A per-host list would make a new prefix a two-place edit whose omission fails +/// nothing: the rows would simply orphan on one SKU, invisibly, which is the drift this product keeps +/// paying for. Both hosts iterate THIS, so a prefix is pruned everywhere or nowhere — a no-op delete on +/// the SKU that never writes a prefix is the cheap price of that guarantee. /// /// Membership is a real decision, not a listing of every key: a key must be /// <prefix><databaseName>, because both prunes reconstruct it that way to test it against @@ -45,16 +44,10 @@ public static class QueryStorePerDatabaseState /// public static readonly IReadOnlyList<(string Owner, string Prefix)> PrunableKeys = new[] { - (QueryStorePlanXmlState.StateCollectorName, QueryStorePlanXmlState.WatermarkKeyPrefix), (QueryStoreBackfillState.StateCollectorName, QueryStoreBackfillState.DoneKeyPrefix), (QueryStoreBackfillState.StateCollectorName, QueryStoreBackfillState.HoleKeyPrefix), - /* #2150: the text watermark is keyed prefix + databaseName exactly like the plan watermark above, - so a dropped database's key must go with it. Paired with its OWN collector name rather than the - plan fetch's — the two watermarks are stored separately on purpose, and a prefix pruned under - the wrong owner silently deletes nothing. */ - (QueryStoreTextState.StateCollectorName, QueryStoreTextState.WatermarkKeyPrefix), - /* #2312: the open-interval refresh stamp, per database like the three above, under its own owner - for the same never-prune-under-the-wrong-name reason. */ + /* #2312: the open-interval refresh stamp, per database like the pair above, under its own owner + because a prefix pruned under the wrong collector name silently deletes nothing. */ (QueryStoreOpenIntervalState.StateCollectorName, QueryStoreOpenIntervalState.WatermarkKeyPrefix), }; diff --git a/PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs b/PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs index ef38f620..058e724f 100644 --- a/PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs +++ b/PerformanceMonitor.Collectors/QueryStorePlanXmlState.cs @@ -6,160 +6,66 @@ * Licensed under the MIT License. See LICENSE file in the project root for full license information. */ -using System; -using System.Collections.Generic; -using System.Globalization; - namespace PerformanceMonitor.Collectors; /// -/// What one plan-fetch pass earned: the watermark to persist, and whether the pass's rows actually arrived in -/// the plan_id order its ORDER BY promises (#2210). One value rather than two calls so a caller cannot -/// take the watermark without being handed the reason it may not have moved — the ordering guard is only -/// useful if the violation gets LOGGED, and a signal a caller can forget to ask for is one that eventually -/// nobody asks for. -/// -/// The plan_id to persist; the standing value when the pass earned no advance. -/// False when a descent was seen, meaning the advance was abandoned and the -/// caller should log a precondition violation rather than treat a static watermark as a quiet pass. -public readonly record struct PlanWatermarkAdvance(long Watermark, bool ArrivedInPlanIdOrder); - -/// -/// The persisted per-database plan-XML watermark (#2164) — the highest plan_id whose execution-plan -/// XML has actually been stored for a database, so collection stops re-shipping plans the store already -/// holds. 97% of the plan XML shipped in a three-hour fleet window was for plans held for over an hour, and -/// because streaming rows is 94-97% of a pass and costs per-row LOB bytes, not fetching beats fetching less. +/// Per-database SIZING for the plan-XML fetch (#2312 Finding 1, wired in #2322): how many plans one pass may +/// hand , learned from each database's own +/// bytes-per-plan rather than a fleet constant, because measured plan size spans 11x across databases +/// (162 KB to 15 KB by quartile) and no single value is right at both ends. /// -/// Owned by the HOST under its own , exactly like -/// and for the same reason: the query_store DEFINITION keeps declaring -/// no state keys, so CollectorStateContractTests stays honest and adding per-database state does not -/// silently become a two-host contract change. The keys are dynamic (one per database), which the host's -/// state read supports because it loads every row for a collector name rather than a declared key list — the -/// definition's StateKeys could not express these anyway. +/// What this class no longer is. Through #2210 it owned the persisted per-database plan-id +/// WATERMARK (planwm: under the query_store_plan_xml state owner) — the resume point for a +/// budgeted whole-catalog walk, with a daily refresh expiry standing in for a re-verify cursor that was +/// designed but never wired. #2312 Finding 4 measured what that actually did in production: catalogs whose +/// full walk needs more than a day expired MID-walk, restarted from plan_id 0, and looped the full catalog +/// fetch forever. The fetch is now activity-driven — the cycle's collected rows name their plans, the STORE +/// answers which are missing (QueryStorePlanMap's touch-and-probe), and only those are fetched — so +/// there is no watermark, no expiry, and no state rows. The retired planwm:/textwm: rows are +/// deleted once by the V77 migration. /// -/// Lives in the shared collectors project rather than either host because it is watermark-shaped state -/// that must decode identically wherever it is read: a row written by Darling today has to keep meaning the -/// same thing after an upgrade, and Lite reads the same definition. +/// What remains is the sizing estimator, which still matters: the missing set of a first-contact +/// database is its whole catalog, and the fetch's running byte total has to DECOMPRESS every plan it +/// considers to measure it, so the id list handed to one pass must be capped near what the byte budget can +/// actually ship. Lives in the shared collectors project because it is pure arithmetic pinned by tests in +/// both hosts' suites. /// public static class QueryStorePlanXmlState { - /// - /// The collector_state owner name for these rows — deliberately NOT the query_store definition's name, - /// which is the seam that lets the definition declare no state keys while the host still persists - /// per-database state for it. - /// - public const string StateCollectorName = "query_store_plan_xml"; - - /// - /// State key prefix; the remainder is the database name, because plan_id is only unique within one - /// database's Query Store and means nothing across databases. - /// - public const string WatermarkKeyPrefix = "planwm:"; - - /// - /// The target period for ONE FULL RE-VERIFICATION SWEEP of a database's plans — not an expiry, and - /// emphatically not a refetch trigger. QueryStorePlanMap.CursorSliceWidth derives the cursor's - /// per-pass id slice from it, so this constant sets the PACE of re-verification rather than a deadline - /// anything has to beat. - /// - /// It used to mean "after this long, drop the watermark to zero and refetch every plan's XML", and - /// that was measured to be unreachable on the catalogs it mattered most for: 2.2-15.1 GB of plan XML per - /// catalog on the production fleet, which at a 12 MB budget and 5-minute cadence is 15.9 to 107.5 HOURS of - /// walking — so a 1-day expiry meant the biggest catalogs restarted from their lowest plan_id forever and - /// never once reached their newest plans. The optimization could not converge on exactly the databases it - /// existed for. Raising the number does not fix that shape; the sweep has to stop being a byte-volume walk. - /// It now is one: hash-only, bounded by ROW count (77k ids at ~270 per pass), re-fetching XML solely where - /// something actually changed. - /// - /// THREE MECHANISMS, each owning one failure, none of them this constant on its own: - /// - /// • A Query Store reset — the map's absent-content signal on the runtime stream - /// (TouchSql), recovering in one cycle. The ONLY thing permitted to zero a watermark.
- /// • Dormant plans — the cursor finds a map row ABSENT at an id the watermark already passed, and - /// fetches it. No heuristic separates dormancy from a reset, because it does not have to: mass absence is - /// caught wholesale by the reset arm within a cycle.
- /// • In-place XML rewrites — the cursor finds a stored plan_hash that DIFFERS from the live - /// one and re-fetches that plan alone. Across a day of fleet data this was 0 of 38,420 plan_ids, which is - /// why paying for it with a full walk was the wrong trade. - /// - /// ONE DAY remains the right pace for a hash-only sweep, for the reason the old value was chosen and - /// for a new one: the redundancy removed is per-pass, and a sweep bounded by rows rather than bytes finishes - /// comfortably inside a day on every catalog measured. - /// - /// Historical note on the three guarantees the old expiry claimed, kept because the reasoning still - /// explains why each mechanism above exists: - /// - /// 1. In-place XML rewrites. plan_id is monotonic and a plan's identity is stable (0 of 38,420 - /// plan_ids changed their plan hash in a day of fleet data), but nothing guarantees a feature like - /// memory-grant feedback never edits grant values inside the XML of a plan that keeps its id. The - /// expiry means that question does not have to be load-bearing. - /// - /// 2. A Query Store RESET. Clearing Query Store restarts plan_id at 1, so every new plan sorts - /// below a stale watermark and its XML would be suppressed. This is NOT what covers that any more — the - /// tempting detection test ("the highest plan_id seen this pass is below the standing watermark") is TRUE in - /// any ordinary quiet window, so it would drop the watermark constantly; but the map gives the payload a - /// signal it never had on its own. A plan_id at or below the watermark whose content the store has never - /// resolved is a RENUMBERED plan, which "no new plans this window" cannot produce, and - /// QueryStorePlanMap.TouchSql surfaces exactly those rows from the batch join it already performs. - /// That is the reset mechanism, it recovers in ONE CYCLE, and it is the only thing permitted to zero the - /// watermark. - /// - /// 3. The dormant-plan gap: plan_id is monotonic in COMPILE order, which is not the same as "we - /// have stored it", so a plan compiled before monitoring began and dormant through every collected - /// window arrives below the watermark. - /// - /// ONE DAY, not a week: the redundancy removed is per-pass (a 15-minute cadence re-ships a plan - /// ~96 times a day), so a daily full fetch already eliminates ~99% of it and a weekly one adds almost - /// nothing — while buying 7x the exposure on all three guarantees above, including a reset blackout - /// measured in days. - /// - /// That trade omits a term, named here because it is the one that will move this number: expiry - /// resets the watermark to zero, so "one expensive pass" is really a full budgeted catalog WALK. At a 12 MB - /// ship budget an 82k-plan catalog spends most of a day walking, which means the largest catalogs — the ones - /// this optimization matters most for — are close to continuously refetching, and shortening the horizon - /// makes that worse rather than safer. Once the stream signal above covers resets, the walk buys only the - /// in-place-rewrite case (speculative: 0 of 38,420 plan_ids changed hash in a day of fleet data) and dormant - /// plans (real, small), and a longer horizon is likely correct. Measure the walk cost on the worst catalog - /// before changing it. - /// - public static readonly TimeSpan RefreshAfter = TimeSpan.FromDays(1); - - /// The state key for one database. - public static string KeyFor(string databaseName) => WatermarkKeyPrefix + databaseName; - /// /// The average plan size assumed for a database with no previous pass to learn from. Deliberately near the /// LARGE end of the measured fleet range (per-quartile averages of 162 / 80 / 39 / 15 KB across 2,166 /// budget-cut passes on a 52-server fleet), because the estimate feeds a DIVISOR: over-estimating plan size - /// yields a SMALL candidate window, and small is the safe direction. A window that is too small merely - /// advances the watermark more slowly; one that is too large decompresses plans it will never ship, which - /// is the exact cost the window exists to bound. + /// yields a SMALL candidate cap, and small is the safe direction. A cap that is too small merely spreads + /// the catch-up across more cycles; one that is too large decompresses plans it will never ship, which is + /// the exact cost the cap exists to bound. /// public const long FirstContactAvgPlanBytes = 160L * 1024L; /// - /// Floor on the candidate window, so progress is always possible. Even if the observed average is wildly - /// over-stated — one enormous plan in a quiet pass — a database must still be able to walk its catalog. + /// Floor on the candidate cap, so progress is always possible. Even if the observed average is wildly + /// over-stated — one enormous plan in a quiet pass — a database must still be able to work off its + /// missing set. /// public const int MinCandidatePlans = 32; /// - /// Ceiling on the candidate window. The smallest measured quartile average (15 KB) puts a 12 MB budget at + /// Ceiling on the candidate cap. The smallest measured quartile average (15 KB) puts a 12 MB budget at /// ~820 plans, so this leaves headroom for genuinely tiny plans while refusing to let a near-zero estimate - /// turn the window back into "the whole catalog" — which is the first-contact trap this window exists to - /// prevent. + /// turn one pass back into "decompress the whole catalog" — which is the first-contact trap the cap exists + /// to prevent. /// public const int MaxCandidatePlans = 2048; /// - /// How far past the budget the window reaches, in expected plans. The window is the COARSE bound and the + /// How far past the budget the cap reaches, in expected plans. The cap is the COARSE bound and the /// running byte total is the exact one, so the margin only has to cover the estimate being wrong in the /// "plans are smaller than expected" direction — where extra plans genuinely fit the budget. /// /// Kept modest at 1.5x because margin is not free: a windowed running total is evaluated over every - /// row IN the window, so the server decompresses all K plans to compute it whether the budget is reached at - /// plan 5 or plan 500. Margin buys reachability and costs decompression, which is why the estimate errs - /// large and the margin stays small. + /// row handed to the fetch, so the server decompresses all of them to compute it whether the budget is + /// reached at plan 5 or plan 500. Margin buys reachability and costs decompression, which is why the + /// estimate errs large and the margin stays small. /// public const double CandidatePlanMargin = 1.5; @@ -174,8 +80,8 @@ public static class QueryStorePlanXmlState /// /// One database's carried plan-size estimate (#2312 Finding 1): the observed average the next pass - /// sizes its candidate window from, and whether the walk is mid-backlog (which biases the sample - /// small — the overload floors it). + /// caps its id list from, and whether the fetch is mid-backlog (which biases the sample small — the + /// overload floors it). /// AvgBytes of zero means "never learned"; callers pass null to CandidatePlanCount then. /// public readonly record struct PlanSizeEstimate(long AvgBytes, bool CatchUpInProgress); @@ -183,16 +89,16 @@ public static class QueryStorePlanXmlState /// /// Folds one pass's outcome into the carried estimate. The rules, each load-bearing: /// a pass that shipped nothing teaches nothing about size (previous average stands) but DOES - /// prove the walk is caught up (nothing qualified past the watermark), so catch-up clears; - /// a pass cut by either bound — the candidate window consumed or the byte budget reached — + /// prove the fetch is caught up (nothing was missing, or nothing fit), so catch-up clears; + /// a pass cut by either bound — the candidate cap consumed or the byte budget reached — /// proves a backlog remains, so catch-up sets; an ordinary partial pass learns its average and /// clears catch-up. Pure so the table is pinnable; the runner owns only the dictionary. /// /// Two counts on purpose (the review catch): is the RAW - /// row count — NULL-XML plans deliberately ship as rows so the watermark can pass unpersistable - /// plans, and the window/catch-up comparison wants exactly that count. But the average's divisor + /// row count — NULL-XML plans deliberately ship as rows so the store can record the content-less + /// marker, and the cap/catch-up comparison wants exactly that count. But the average's divisor /// is , the rows that actually carried XML: dividing real bytes - /// by a NULL-inflated count would understate the average, which INFLATES the next window — the + /// by a NULL-inflated count would understate the average, which INFLATES the next cap — the /// unsafe direction the whole estimator errs away from. /// public static PlanSizeEstimate Learn( @@ -214,32 +120,32 @@ public static PlanSizeEstimate Learn( /// /// This is the trap mitigation. SUM(DATALENGTH(query_plan)) OVER (ORDER BY plan_id) has to /// materialize the XML to measure it — query_store_plan.query_plan is decompressed BY the TVF on - /// access — so an unbounded candidate set pays the whole catalog's decompression to enforce a budget meant - /// to prevent exactly that. Bounding the window first on the cheap columns costs nothing and caps it. + /// access — so an unbounded id list pays the whole missing set's decompression to enforce a budget meant + /// to prevent exactly that. Capping the list first on the cheap side costs nothing. /// /// Per-database rather than one fleet constant because measured plan size spans 11x (162 KB to 15 KB /// by quartile). A constant sized for the small-plan end (~820) would decompress ~134 MB to ship 12 MB on /// the large-plan end; one sized for the large end would never reach the budget on the small end. No single /// value is both, which is what makes this adaptive rather than tunable. /// - /// reports that a bound was applied, so the caller can LOG it. A window + /// reports that a bound was applied, so the caller can LOG it. A cap /// silently pinned at its ceiling looks identical to one that fit, and that is how a cap becomes invisible. /// public static int CandidatePlanCount(long? observedAvgPlanBytes, long budgetBytes, out bool clamped) => CandidatePlanCount(observedAvgPlanBytes, budgetBytes, catchUpInProgress: false, out clamped); /// - /// As above, with the catch-up guard: while — the watermark still below - /// the server's newest plan_id — the observed average is FLOORED at + /// As above, with the catch-up guard: while — the missing set still + /// larger than one pass can ship — the observed average is FLOORED at /// rather than trusted. /// /// The estimator is biased during exactly that window, and measurably so: the average is computed over /// the plans a pass actually shipped, which under plan_id-ascending shipping are the OLDEST ids in the - /// catalog. On one production catalog the plans the fetch shipped averaged 15 KB while the newest 300 plans - /// in the same catalog averaged 46 KB — a 3x under-estimate, which inflates K threefold and decompresses - /// that much more than the budget can ship. Flooring at the seed applies the same over-estimate-is-safe - /// logic the seed itself rests on, for the one window where the sample is known to be unrepresentative. - /// Once the first walk has converged the sample spans the catalog and the observed average is trusted. + /// missing set. On one production catalog the plans the fetch shipped averaged 15 KB while the newest 300 + /// plans in the same catalog averaged 46 KB — a 3x under-estimate, which inflates the cap threefold and + /// decompresses that much more than the budget can ship. Flooring at the seed applies the same + /// over-estimate-is-safe logic the seed itself rests on, for the one window where the sample is known to be + /// unrepresentative. Once caught up the sample spans the catalog and the observed average is trusted. /// public static int CandidatePlanCount(long? observedAvgPlanBytes, long budgetBytes, bool catchUpInProgress, out bool clamped) { @@ -261,133 +167,14 @@ public static int CandidatePlanCount(long? observedAvgPlanBytes, long budgetByte comparison below unable to tell a clamp from a natural landing, which is the false positive this reports on. int.MaxValue only guards the cast itself, since the budget is operator input. */ var wanted = (double)budgetBytes / avg * CandidatePlanMargin; - var unclamped = wanted >= int.MaxValue ? int.MaxValue : (int)Math.Ceiling(wanted); - var bounded = Math.Clamp(unclamped, MinCandidatePlans, MaxCandidatePlans); + var unclamped = wanted >= int.MaxValue ? int.MaxValue : (int)System.Math.Ceiling(wanted); + var bounded = System.Math.Clamp(unclamped, MinCandidatePlans, MaxCandidatePlans); - /* Reports that a bound CHANGED the answer, not that the answer happens to equal one. A window whose + /* Reports that a bound CHANGED the answer, not that the answer happens to equal one. A cap whose measured size lands naturally on 32 or 2048 was sized by the measurement and needs no log line; saying "clamped" there is a false positive against this contract, and a caller that logs on it teaches its reader to ignore the message. */ clamped = bounded != unclamped; return bounded; } - - /// - /// The watermark a pass earned, given the plan_ids whose XML actually landed. Under plan_id-ordered - /// shipping a budget cut truncates a SUFFIX, so the highest landed id is safe to keep even from a cut pass - /// — which is the whole point of the reordering (#2210): the previous design shipped in - /// last_execution_time order, where a cut left an arbitrary SUBSET and no value was safe, so the - /// watermark could not advance on 97.8% of passes and therefore never advanced at all. - /// - /// Defensive on the precondition rather than trusting it: a DESCENT anywhere in - /// abandons the advance entirely and reports itself through - /// . Honouring the leading ascending run instead - /// looks safer and is not — given {105, 101} it would advance to 105, and once ordering is broken - /// there is no longer any basis for inferring that every SELECTED plan below 105 landed, so a plan whose - /// XML never arrived gets suppressed until the refresh horizon. Ordering is what makes a cut a suffix; with - /// it gone the pass has earned nothing, and one lost pass of progress is the cheap side of that trade. - /// - /// The verdict and the signal come back TOGETHER, in one value, deliberately. Two separate functions - /// would let a caller take the watermark and never ask whether ordering held — a watermark that quietly - /// stops moving with nothing logged, which is precisely the failure this whole redesign exists to correct - /// and would be a poor thing to reintroduce one level up. - /// - /// Never moves backward: a pass that lands nothing, or only ids at or below the standing watermark, - /// returns the standing value. Lowering it would refetch the catalog, and "no new plans this window" is an - /// ordinary quiet pass, not a reset — the reset signal lives on the runtime stream, where a plan at or below - /// the watermark that the store has never resolved can actually be observed. - /// - public static PlanWatermarkAdvance AdvanceWatermark(long standing, IReadOnlyList landedPlanIdsInOrder) - { - if (landedPlanIdsInOrder is null || landedPlanIdsInOrder.Count == 0) - { - return new PlanWatermarkAdvance(standing, true); - } - - var advanced = standing; - var previous = long.MinValue; - - foreach (var planId in landedPlanIdsInOrder) - { - if (planId < previous) - { - return new PlanWatermarkAdvance(standing, false); - } - - previous = planId; - - if (planId > advanced) - { - advanced = planId; - } - } - - return new PlanWatermarkAdvance(advanced, true); - } - - /// - /// The watermark to apply for one database, or 0 — meaning "fetch every plan's XML" — for an absent, - /// malformed, EXPIRED or future-stamped one. Zero is the documented conservative path: absent is what a - /// first run, a restarted host and a broken store all look like, and all three must refetch rather than - /// skip. A future stamp means the clock moved backwards, which would otherwise pin the watermark for as - /// long as the skew lasts. - /// - public static long Resolve(IReadOnlyDictionary state, string databaseName, DateTime utcNow) - { - if (!TryParse(state, databaseName, out var planId, out var stamped)) - { - return 0; - } - - if (stamped > utcNow || utcNow - stamped >= RefreshAfter) - { - return 0; - } - - return planId; - } - - /// - /// The stored stamp — when this database last did a FULL plan-XML fetch — with no expiry applied, so a - /// write-back can carry it forward across an advance instead of renewing the refresh horizon. Null when - /// there is nothing parseable to carry, which the caller treats as "stamp now". - /// - public static DateTime? ResolveStamp(IReadOnlyDictionary state, string databaseName) => - TryParse(state, databaseName, out _, out var stamped) ? stamped : null; - - /// - /// Formats a watermark for storage: highest stored plan_id plus the stamp dating the last FULL fetch. - /// The stamp is a parameter rather than "now" precisely because it must survive advances — re-stamping - /// on every advance would push the horizon out forever on any database that keeps compiling plans, which - /// is the busy ones where a stale plan matters most, and the bounded refresh would never fire. - /// - public static string Format(long planId, DateTime fullFetchAtUtc) => - planId.ToString(CultureInfo.InvariantCulture) + ":" + - new DateTimeOffset(DateTime.SpecifyKind(fullFetchAtUtc, DateTimeKind.Utc)).ToUnixTimeSeconds() - .ToString(CultureInfo.InvariantCulture); - - private static bool TryParse( - IReadOnlyDictionary state, string databaseName, out long planId, out DateTime stamped) - { - planId = 0; - stamped = default; - - if (state is null || !state.TryGetValue(KeyFor(databaseName), out var raw) || string.IsNullOrWhiteSpace(raw)) - { - return false; - } - - var parts = raw.Split(':'); - if (parts.Length != 2 - || !long.TryParse(parts[0], NumberStyles.Integer, CultureInfo.InvariantCulture, out planId) - || !long.TryParse(parts[1], NumberStyles.Integer, CultureInfo.InvariantCulture, out var stampedUnix) - || planId <= 0) - { - planId = 0; - return false; - } - - stamped = DateTimeOffset.FromUnixTimeSeconds(stampedUnix).UtcDateTime; - return true; - } } diff --git a/PerformanceMonitor.Collectors/QueryStoreTextState.cs b/PerformanceMonitor.Collectors/QueryStoreTextState.cs deleted file mode 100644 index 10b4ec7f..00000000 --- a/PerformanceMonitor.Collectors/QueryStoreTextState.cs +++ /dev/null @@ -1,220 +0,0 @@ -/* - * Copyright (c) 2026 Erik Darling, Darling Data LLC - * - * This file is part of the SQL Server Performance Monitor. - * - * Licensed under the MIT License. See LICENSE file in the project root for full license information. - */ - -using System; -using System.Collections.Generic; -using System.Globalization; - -namespace PerformanceMonitor.Collectors; - -/// The result of advancing a text watermark: see . -public readonly record struct TextWatermarkAdvance(long Watermark, bool ArrivedInQueryIdOrder); - -/// -/// Per-database watermark for the query-text fetch (#2150), the sibling of -/// — same encoding, same conservative-zero rules, same -/// never-backward advance. -/// -/// Why this exists. The runtime-stats payload carried query_sql_text -/// (nvarchar(max)) inside a TOP ... WITH TIES ... ORDER BY last_execution_time projection. -/// A Top-N Sort carries every output column through the sort and reads ALL of its input before emitting -/// a row, so choosing the rows to ship materialized the text for the entire qualifying set. Measured on -/// a purpose-built Azure SQL DB store with the plan XML already removed by #2210 — the only difference -/// being that one column — time-to-first-row was 4.67s vs 0.45s at 1,505 rows / 12.8 MB of text -/// and 5.02s vs 0.57s at 4,037 rows / 34 MB, with full drain 8.06s vs 0.50s and -/// 16.95s vs 1.45s. Neither the row cap nor the client byte budget can bound that: TOP (500) -/// measured identical to TOP (50000), and wall time was flat across a 4 MB → 256 MB budget sweep, -/// because the server finishes before the client sees a byte. -/// -/// Why a watermarked fetch rather than a per-pass dedupe. That path was already tried on the -/// plan side and abandoned: #1556's ROW_NUMBER gate shipped each plan once per PASS, and #2164 -/// replaced it precisely because "the ROW_NUMBER gate ships each plan once per pass but re-ships it every -/// pass forever, and since drain is 94-97% of a pass and is per-row LOB cost, NOT fetching is worth far -/// more than fetching less." #2210 then took the column out of the stream entirely. -/// query_id is an identity, monotonic within a database, so the same shape applies: fetch a -/// statement's text ONCE, ever. -/// -/// Keyed on query_id, not query_text_id, and that is what keeps this cheap. -/// query_id is ALREADY a stored payload column on the runtime row, so readers get the join key for -/// free and the fact table needs no new column and no migration. Keying on query_text_id would have -/// required adding it to the payload — a schema change — to buy de-duplication across the handful of -/// query_ids that share one text (a query_id is per text PLUS context settings, so the two -/// are close to 1:1 in practice). Storing a rare duplicate is the cheaper side of that trade. -/// -/// What this deliberately does NOT mirror, and why. The plan side carries a whole candidate- -/// window estimator (FirstContactAvgPlanBytes, min/max clamps, an observed-average learning loop) -/// because SUM(DATALENGTH(query_plan)) OVER (ORDER BY plan_id) forces the server to DECOMPRESS -/// every plan in the window — sys.query_store_plan.query_plan is decompressed by the view on -/// access. sys.query_store_query_text.query_sql_text is not, so its DATALENGTH is cheap and -/// the window needs no estimate at all: a flat coarse bound plus the exact running-byte total is enough. -/// The plan side also re-verifies content hashes because plan XML can be rewritten in place; a -/// query_text_id maps to fixed text forever — a changed statement is a new id — so there is -/// nothing to re-verify and no content digest to track. -/// -public static class QueryStoreTextState -{ - /// - /// The collector name the watermark is stored under. Separate from the plan fetch's own state so the - /// two advance independently: they walk different catalogs at different rates, and sharing a key would - /// let a plan-side reset drop the text watermark (and vice versa) for no reason. - /// - public const string StateCollectorName = "query_store_text"; - - /// Prefix for the per-database state key. - public const string WatermarkKeyPrefix = "textwm:"; - - /// - /// How long a watermark stands before a full re-walk. Matched to the plan side's one day rather than - /// tuned separately, so an operator reasoning about one fetch reasons about both — and the term that - /// made the plan side's choice tight does not apply here: expiry means a budgeted catalog walk, and - /// text is roughly an order of magnitude smaller per row than plan XML (8.5 KB against 195 KB on the - /// measured store), so the walk this horizon triggers is correspondingly cheaper. - /// - /// The re-walk is not decoration. query_id is monotonic in FIRST-SEEN order, not in "we - /// have stored it", so two things arrive below a standing watermark: a statement first seen before - /// monitoring began and only executed later, and — the one that matters — a Query Store reset, which - /// renumbers ids from the start. Without a bounded horizon a reset would suppress every text forever. - /// - public static readonly TimeSpan RefreshAfter = TimeSpan.FromDays(1); - - /// - /// How many texts one pass may CONSIDER. A flat bound, not an estimate: the running byte total is the - /// exact constraint and DATALENGTH(query_sql_text) is cheap to evaluate, so this only has to be - /// large enough that the budget binds first and small enough that a pass never windows an entire - /// catalog. At the 12 MB default ship budget this covers texts averaging under ~2.5 KB, which is - /// comfortably below what a fragmenting literal-heavy statement produces. - /// - public const int CandidateTexts = 5_000; - - /// The state key for one database. - public static string KeyFor(string databaseName) => WatermarkKeyPrefix + databaseName; - - /// - /// The highest query_id landed, or the standing watermark when a pass lands nothing. - /// - /// Reports whether the ids arrived in query_id order, because that ordering is what - /// makes a budget cut a SUFFIX — everything up to the cut is stored, so the highest stored id is a - /// safe resume point. Out of order, that argument collapses and the caller must hold the watermark - /// rather than advance past statements whose text it never stored. - /// - /// Never moves backward. A pass landing nothing, or only ids at or below the standing watermark, - /// is an ordinary quiet pass — not a reset — and lowering the watermark would refetch the catalog. - /// - public static TextWatermarkAdvance AdvanceWatermark(long standing, IReadOnlyList landedQueryIdsInOrder) - { - if (landedQueryIdsInOrder is null || landedQueryIdsInOrder.Count == 0) - { - return new TextWatermarkAdvance(standing, true); - } - - var advanced = standing; - var previous = long.MinValue; - - foreach (var queryId in landedQueryIdsInOrder) - { - if (queryId < previous) - { - return new TextWatermarkAdvance(standing, false); - } - - previous = queryId; - - if (queryId > advanced) - { - advanced = queryId; - } - } - - return new TextWatermarkAdvance(advanced, true); - } - - /// - /// The watermark to apply for one database, or 0 — meaning "fetch every text" — for an absent, - /// malformed, EXPIRED or future-stamped one. Zero is the conservative path, and all three of a first - /// run, a restarted host and a broken store look identical from here: every one of them must refetch - /// rather than skip. A future stamp means the clock moved backwards, which would otherwise pin the - /// watermark for as long as the skew lasts. - /// - public static long Resolve(IReadOnlyDictionary state, string databaseName, DateTime utcNow) - { - if (!TryParse(state, databaseName, out var textId, out var stamped)) - { - return 0; - } - - if (stamped > utcNow || utcNow - stamped >= RefreshAfter) - { - return 0; - } - - return textId; - } - - /// - /// The stored stamp — when this database last did a FULL text fetch — with no expiry applied, so a - /// write-back can carry it forward across an advance instead of renewing the refresh horizon. Null - /// when there is nothing parseable to carry, which the caller treats as "stamp now". - /// - public static DateTime? ResolveStamp(IReadOnlyDictionary state, string databaseName) => - TryParse(state, databaseName, out _, out var stamped) ? stamped : null; - - /// - /// Formats a watermark for storage: highest stored query_id plus the stamp dating the last - /// FULL fetch. The stamp is a parameter rather than "now" precisely because it must survive advances — - /// re-stamping on every advance would push the horizon out forever on any database that keeps seeing - /// new statements, which is exactly where a reset would hurt most, and the bounded re-walk would never - /// fire. - /// - public static string Format(long textId, DateTime fullFetchAtUtc) => - textId.ToString(CultureInfo.InvariantCulture) + ":" + - new DateTimeOffset(DateTime.SpecifyKind(fullFetchAtUtc, DateTimeKind.Utc)).ToUnixTimeSeconds() - .ToString(CultureInfo.InvariantCulture); - - private static bool TryParse( - IReadOnlyDictionary state, string databaseName, out long textId, out DateTime stamped) - { - textId = 0; - stamped = default; - - if (state is null || !state.TryGetValue(KeyFor(databaseName), out var raw) || string.IsNullOrWhiteSpace(raw)) - { - return false; - } - - var split = raw.IndexOf(':'); - if (split <= 0 || split == raw.Length - 1) - { - return false; - } - - if (!long.TryParse(raw.AsSpan(0, split), NumberStyles.Integer, CultureInfo.InvariantCulture, out textId) - || textId < 0) - { - textId = 0; - return false; - } - - if (!long.TryParse(raw.AsSpan(split + 1), NumberStyles.Integer, CultureInfo.InvariantCulture, out var unix)) - { - textId = 0; - return false; - } - - try - { - stamped = DateTimeOffset.FromUnixTimeSeconds(unix).UtcDateTime; - } - catch (ArgumentOutOfRangeException) - { - textId = 0; - return false; - } - - return true; - } -}