From 95ee226afc739586a02a90a2aac43033838489c1 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 10:58:03 -0400 Subject: [PATCH 1/3] Query heatmap resolves preview text for cell winners only, not every row (#4233) base now reads query_stats and carries the digest/inline text through the window sorts; the outer SELECT resolves and truncates the preview only for the rn = 1 row of each cell, matching what v_query_stats would have resolved without paying the query_text_dim join for every row in the window. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../Darling.Tests/DarlingQueryHeatmapTests.cs | 49 ++++++++++++++++--- .../Darling.Tests/ViewerQueriesRestTests.cs | 19 +++++-- .../Mcp/DarlingQueryHeatmapReader.cs | 32 ++++++++---- .../ViewerDataService.QueryHeatmap.cs | 27 +++++++--- 4 files changed, 99 insertions(+), 28 deletions(-) diff --git a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs index 725a080f64..36fc0af6ca 100644 --- a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs +++ b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs @@ -151,10 +151,19 @@ public void HeatmapSql_KeepsTheViewersMagnitudeBands_AndFilters() } Assert.Contains("delta_execution_count > 0", sql, StringComparison.Ordinal); - /* Bound, not a literal 120 (#4198) — see HeatmapSql_PreviewWidthIsABoundParameter below. */ - Assert.Contains("LEFT(query_text, $7) AS query_preview", sql, StringComparison.Ordinal); + + /* #4233: base reads the fact table directly, never v_query_stats (#1767's payload-resolving + view), so no row in the window pays for the query_text_dim join or a truncation it will never + be shown. */ + Assert.Contains("FROM query_stats", sql, StringComparison.Ordinal); + Assert.DoesNotContain("v_query_stats", sql, StringComparison.Ordinal); + + /* The preview is resolved for the rn = 1 row of each cell only, at the #4198 bound width - inline + text when the row predates #1767, the dimension row when it does not, truncated last. */ + Assert.Contains( + "LEFT(COALESCE(query_text, (SELECT d.query_text FROM query_text_dim d WHERE d.digest = ranked.query_text_digest)), $7) AS top_query_text", + sql, StringComparison.Ordinal); Assert.DoesNotContain("LEFT(query_text, 120)", sql, StringComparison.Ordinal); - Assert.Contains("FROM v_query_stats", sql, StringComparison.Ordinal); /* DuckDB's ARG_MAX has no Postgres equivalent; the viewer's replacement is a top-1 window over the cell, with the cell's count carried alongside so one pass yields both. */ @@ -163,6 +172,28 @@ public void HeatmapSql_KeepsTheViewersMagnitudeBands_AndFilters() Assert.Contains("WHERE rn = 1", sql, StringComparison.Ordinal); } + /// + /// #4233's source pin: base carries the raw digest and the raw inline text (pre-#1767 rows) + /// through the window sorts, never a resolved-and-truncated preview - that resolution happens once, + /// in the outer SELECT, only for the row each cell keeps. Fails against the pre-#4233 SQL, which + /// computed LEFT(query_text, $7) AS query_preview inside base for every row. + /// + [Fact] + public void BaseCte_CarriesTheDigestAndInlineText_NeverATruncatedPreview() + { + foreach (var metric in AllMetrics) + { + var sql = DarlingQueryHeatmapReader.BuildQueryHeatmapSql(metric); + var baseCte = sql[..sql.IndexOf("binned AS (", StringComparison.Ordinal)]; + + Assert.DoesNotContain("query_preview", baseCte, StringComparison.Ordinal); + Assert.DoesNotContain("LEFT(", baseCte, StringComparison.Ordinal); + Assert.Contains("query_text,", baseCte, StringComparison.Ordinal); + Assert.Contains("query_text_digest", baseCte, StringComparison.Ordinal); + Assert.Contains("FROM query_stats", baseCte, StringComparison.Ordinal); + } + } + /// /// The cap orders NEWEST bin first. An MCP read has a row cap the desktop chart does not, and cutting the /// recent end of an incident window is the one thing a capped heatmap must not do. @@ -245,17 +276,19 @@ public void UnknownMetricNames_AreRefused(string name) /// table: query_stats is PERIODIC, so no rows really does mean nobody looked. /// [Fact] - public void CoverageSql_ProbesTheSameView_ForEverAndForTheWindow() + public void CoverageSql_ProbesTheSameTable_ForEverAndForTheWindow() { var sql = DarlingQueryHeatmapReader.HeatmapCoverageSql; - Assert.Equal(2, CountOf(sql, "FROM v_query_stats")); + Assert.Equal(2, CountOf(sql, "FROM query_stats")); Assert.Equal(2, CountOf(sql, "EXISTS (")); Assert.Contains("collection_time >= $2", sql, StringComparison.Ordinal); Assert.Contains("collection_time <= $3", sql, StringComparison.Ordinal); - /* The read itself is over the view, so the probe must be too - a probe on the base table could say - "collected" about rows the read cannot see. */ - Assert.Contains("FROM v_query_stats", DarlingQueryHeatmapReader.BuildQueryHeatmapSql(HeatmapMetric.Duration), StringComparison.Ordinal); + /* #4233 moved the read itself off v_query_stats onto query_stats directly; the probe follows so + this stays true by construction, not just by the (still valid) argument that the view's LEFT + JOINs never drop a fact row. */ + Assert.Contains("FROM query_stats", DarlingQueryHeatmapReader.BuildQueryHeatmapSql(HeatmapMetric.Duration), StringComparison.Ordinal); + Assert.DoesNotContain("v_query_stats", DarlingQueryHeatmapReader.BuildQueryHeatmapSql(HeatmapMetric.Duration), StringComparison.Ordinal); /* The "ever" arm is deliberately UNBOUNDED - it is the question "did anyone ever collect this server", and bounding it would make a quiet window look like a missing collector. */ diff --git a/Darling/Darling.Tests/ViewerQueriesRestTests.cs b/Darling/Darling.Tests/ViewerQueriesRestTests.cs index d108713746..98082d459c 100644 --- a/Darling/Darling.Tests/ViewerQueriesRestTests.cs +++ b/Darling/Darling.Tests/ViewerQueriesRestTests.cs @@ -227,12 +227,17 @@ public void HeatmapSql_RewritesArgMaxToTop1Window_ForEveryMetric() public void HeatmapSql_KeepsLitesMagnitudeBuckets_Filters_And_Preview() { var sql = ViewerDataService.BuildQueryHeatmapSql(HeatmapMetric.Duration); - /* v_query_stats, not the base table (#1767): the cell preview is LEFT(query_text, 120), and the - base table's inline query_text is NULL on every row written since the migration — the heatmap - would render with blank previews rather than fail. */ - Assert.Contains("FROM v_query_stats", sql, StringComparison.Ordinal); + /* #4233: base reads the fact table directly, not v_query_stats (#1767) - no row in the window + pays for the query_text_dim join or a preview truncation it will never be shown. The preview + is resolved only for the rn = 1 row of each cell: inline query_text when the row predates + #1767, the query_text_dim lookup when it does not (post-#1767 rows leave the inline column + NULL), truncated last - the same resolution v_query_stats performs, just not for every row. */ + Assert.Contains("FROM query_stats", sql, StringComparison.Ordinal); + Assert.DoesNotContain("v_query_stats", sql, StringComparison.Ordinal); Assert.Contains("delta_execution_count > 0", sql, StringComparison.Ordinal); - Assert.Contains("LEFT(query_text, 120) AS query_preview", sql, StringComparison.Ordinal); + Assert.Contains( + "LEFT(COALESCE(query_text, (SELECT d.query_text FROM query_text_dim d WHERE d.digest = ranked.query_text_digest)), 120) AS top_query_text", + sql, StringComparison.Ordinal); /* The 7-way log-magnitude CASE (only the boundaries are pinned). */ Assert.Contains("WHEN metric_value < 1 THEN 0", sql, StringComparison.Ordinal); Assert.Contains("WHEN metric_value < 100000 THEN 5", sql, StringComparison.Ordinal); @@ -490,6 +495,10 @@ public async Task QueryHeatmap_BinsByTime_BucketsByMagnitude_TopQueryByExecution Assert.Equal(1.0, result.Intensities[0, 0]); /* bucket 0: 0xLOW */ Assert.Equal("0xHOT", result.CellDetails[2, 0].TopQueryHash); /* ARG_MAX replacement: higher delta_exec wins */ Assert.Equal("0xLOW", result.CellDetails[0, 0].TopQueryHash); + /* #4233: the preview still resolves end-to-end through the C# reader for the winning row, + even though base no longer carries a pre-truncated column. */ + Assert.Equal("SELECT hot", result.CellDetails[2, 0].TopQueryText); + Assert.Equal("SELECT low", result.CellDetails[0, 0].TopQueryText); /* 0xZERO (delta_exec = 0) contributed to no cell. */ Assert.Equal(0.0, result.Intensities[6, 0]); diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingQueryHeatmapReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingQueryHeatmapReader.cs index f72ff82d6f..c4d9b59f0b 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingQueryHeatmapReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingQueryHeatmapReader.cs @@ -60,6 +60,15 @@ public enum HeatmapMetric /// nobody looked, and an existence probe on the data is the right denominator (see /// ). /// +/// #4233: the preview is resolved for the rn = 1 row of each cell only, not every row in the +/// window. base reads query_stats directly rather than v_query_stats (#1767's +/// payload-resolving view), so no row pays for the query_text_dim join or a truncation it will +/// never be shown - the raw inline query_text (pre-#1767 rows only) and the digest ride the +/// window sorts in its place. The outer rn = 1 filter runs before the SELECT list, so +/// LEFT(COALESCE(query_text, (SELECT ... FROM query_text_dim ...)), $7) - the same resolution +/// v_query_stats would have performed, at the same #4198 bound width - executes only for the +/// one row per cell the caller sees. +/// /// The SQL is built by a public method so the tests can pin the dialect and the shape without a live /// Postgres. /// @@ -180,9 +189,10 @@ WITH base AS ( date_bin(($5::integer * INTERVAL '1 minute'), collection_time, TIMESTAMP '1970-01-01 00:00:00') AS time_bin, {metricExpr} AS metric_value, query_hash, - LEFT(query_text, $7) AS query_preview, + query_text, + query_text_digest, delta_execution_count - FROM v_query_stats + FROM query_stats WHERE server_id = $1 AND collection_time >= $2 AND collection_time <= $3 @@ -203,7 +213,8 @@ WHEN metric_value < 100000 THEN 5 ELSE 6 END AS bucket_index, query_hash, - query_preview, + query_text, + query_text_digest, delta_execution_count FROM base ), @@ -212,7 +223,8 @@ ranked AS ( time_bin, bucket_index, query_hash, - query_preview, + query_text, + query_text_digest, COUNT(*) OVER (PARTITION BY time_bin, bucket_index) AS query_count, ROW_NUMBER() OVER (PARTITION BY time_bin, bucket_index ORDER BY delta_execution_count DESC) AS rn FROM binned @@ -222,7 +234,7 @@ FROM binned bucket_index, query_count, query_hash AS top_query_hash, - query_preview AS top_query_text + LEFT(COALESCE(query_text, (SELECT d.query_text FROM query_text_dim d WHERE d.digest = ranked.query_text_digest)), $7) AS top_query_text FROM ranked WHERE rn = 1 ORDER BY time_bin DESC, bucket_index @@ -238,19 +250,21 @@ FROM ranked /// writes a row every cycle for whatever sits in the plan cache, so a server with zero rows in its whole /// history is a server nobody collected — unlike blocking or deadlocks, where zero rows is the healthy /// answer and a data probe would send someone to fix collection that works. - /// Probes v_query_stats, the same relation the read itself uses, so the probe cannot - /// disagree with the read about which rows exist. $1 server_id, $2 window start, $3 window end. + /// Probes query_stats directly (#4233 moved the read itself off v_query_stats + /// onto the fact table; the view's payload-dimension joins are LEFT JOINs, so they never drop a + /// fact row, and existence here is identical either way - this keeps the probe's FROM matching the + /// read's literally rather than by that argument). $1 server_id, $2 window start, $3 window end. /// public const string HeatmapCoverageSql = """ SELECT EXISTS ( SELECT 1 - FROM v_query_stats + FROM query_stats WHERE server_id = $1 ) AS has_any, EXISTS ( SELECT 1 - FROM v_query_stats + FROM query_stats WHERE server_id = $1 AND collection_time >= $2 AND collection_time <= $3 diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryHeatmap.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryHeatmap.cs index af95663981..186134bd44 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryHeatmap.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.QueryHeatmap.cs @@ -87,7 +87,19 @@ public sealed partial class ViewerDataService /// COUNT(*) OVER the same partition carried alongside so one pass yields both the /// count and the top row. Ties resolve arbitrarily, exactly as ARG_MAX did. /// The magnitude CASE, the delta_execution_count > 0 / metric IS NOT NULL filters, - /// the LEFT(query_text, 120) preview, and the $1/$2/$3 placeholders are Lite's verbatim. + /// and the $1/$2/$3 placeholders are Lite's verbatim. + /// + /// #4233: the 120-character preview is resolved for the rn = 1 row of each cell only, + /// not every row in the window. base reads query_stats directly rather than + /// v_query_stats (#1767's payload-resolving view), so no row pays for the + /// query_text_dim join or a truncation it will never be shown — the raw inline + /// query_text (populated only on pre-#1767 rows) and the digest ride the window sorts in + /// its place, which is narrower on almost every row since a post-#1767 row's inline column is + /// NULL. The outer WHERE rn = 1 filters before the SELECT list runs, so + /// LEFT(COALESCE(query_text, (SELECT … FROM query_text_dim …)), 120) — the same resolution + /// v_query_stats would have performed — executes only for the one row per cell the caller + /// sees. Measured on a seeded 24-hour, ~180k-row window: see the PR for before/after + /// EXPLAIN (ANALYZE, BUFFERS). /// public static string BuildQueryHeatmapSql(HeatmapMetric metric) { @@ -98,9 +110,10 @@ WITH base AS ( date_bin(INTERVAL '5 minutes', collection_time, TIMESTAMP '1970-01-01 00:00:00') AS time_bin, {metricExpr} AS metric_value, query_hash, - LEFT(query_text, 120) AS query_preview, + query_text, + query_text_digest, delta_execution_count - FROM v_query_stats + FROM query_stats WHERE server_id = $1 AND collection_time >= $2 AND collection_time <= $3 @@ -121,7 +134,8 @@ WHEN metric_value < 100000 THEN 5 ELSE 6 END AS bucket_index, query_hash, - query_preview, + query_text, + query_text_digest, delta_execution_count FROM base ), @@ -130,7 +144,8 @@ ranked AS ( time_bin, bucket_index, query_hash, - query_preview, + query_text, + query_text_digest, COUNT(*) OVER (PARTITION BY time_bin, bucket_index) AS query_count, ROW_NUMBER() OVER (PARTITION BY time_bin, bucket_index ORDER BY delta_execution_count DESC) AS rn FROM binned @@ -140,7 +155,7 @@ FROM binned bucket_index, query_count, query_hash AS top_query_hash, - query_preview AS top_query_text + LEFT(COALESCE(query_text, (SELECT d.query_text FROM query_text_dim d WHERE d.digest = ranked.query_text_digest)), 120) AS top_query_text FROM ranked WHERE rn = 1 ORDER BY time_bin, bucket_index From ebf3b78a4996bb95a585a6eddb28e0c326bcbc92 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:17:35 -0400 Subject: [PATCH 2/3] Pin #4233's two open items: twin-copy parity and old-vs-new live equality Ruling item 2: BothCopies_AreByteIdentical_ThroughTheRnFilter compares ViewerDataService.BuildQueryHeatmapSql against DarlingQueryHeatmapReader's copy directly, normalizing the two differences that predate #4233 (the bin width literal vs $5, the preview width literal vs $7) and cutting before the ORDER BY/LIMIT tail (a third, deliberate difference already pinned by HeatmapSql_CapsFromTheRecentEnd). Proven live: a one-character change to either copy's COALESCE/query_text_dim subquery made this fail before being reverted. Ruling item 4: OldSqlFromDev_AndTheNewBuilder_AgreeOnCellsCountsHashAndPreview runs the pre-#4233 SQL (fetched via git show origin/dev, pinned as a literal) and the new BuildQueryHeatmapSql over one seeded window covering a query_text_dim-resolved winner, a pre-#1767 inline-text winner, and a same-cell tie, and asserts identical cells, counts, top hash and preview. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../Darling.Tests/DarlingQueryHeatmapTests.cs | 213 ++++++++++++++++++ .../QueryHeatmapSqlTwinParityTests.cs | 79 +++++++ 2 files changed, 292 insertions(+) create mode 100644 Darling/Darling.Tests/QueryHeatmapSqlTwinParityTests.cs diff --git a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs index 36fc0af6ca..8d6121104c 100644 --- a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs +++ b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs @@ -11,12 +11,15 @@ using System.ComponentModel; using System.Linq; using System.Reflection; +using System.Security.Cryptography; +using System.Text; using System.Text.Json; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; using ModelContextProtocol.Server; using Npgsql; +using NpgsqlTypes; using PerformanceMonitor.Collectors; using PerformanceMonitor.Common; using PerformanceMonitor.Darling.Service.Mcp; @@ -607,4 +610,214 @@ private static async Task DeleteRowsAsync(NpgsqlConnection connection, Cancellat await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM servers WHERE server_id = $1", ServerId); await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM config_monitored_servers WHERE server_id = $1", ServerId); } + + private const int ParityServerId = -949558; + private const string ParityServerName = "query-heatmap-parity"; + private const string ParityDb = "AppDb"; + + /// + /// #4233 ruling item 4's live pin: the pre-#4233 SQL (fetched via git show origin/dev:... before + /// this PR's rewrite landed, pinned below as so it cannot silently rot) + /// and #4233's new must return IDENTICAL + /// cells, counts, top hash and preview text over the same seeded window - proving the rewrite (read + /// query_stats directly, resolve the preview only for the rn = 1 row) is a performance change with no + /// behavior change, by measurement rather than by argument. One window carries all three shapes the + /// ruling asked for, each in its own 5-minute bin so they cannot interact: + /// + /// (a) a winner whose text lives only in query_text_dim - digest set, inline query_text NULL + /// (b) a pre-#1767 winner - inline query_text set, digest NULL + /// (c) a tie inside one cell - two rows with equal delta_execution_count, so ROW_NUMBER's + /// tiebreak is unspecified by either query. This does not assert WHICH row wins; both queries scan the + /// same base rows in the same order, so the pin is that the OLD SQL and the NEW builder agree with each + /// other, which is what "no behavior change" actually means for an unordered tie. + /// + /// + [Fact] + public async Task OldSqlFromDev_AndTheNewBuilder_AgreeOnCellsCountsHashAndPreview() + { + var cs = ConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(cs), + "Set DARLING_TEST_PG to a Postgres connection string to run the live heatmap parity test."); + + var ct = TestContext.Current.CancellationToken; + using var connection = new NpgsqlConnection(cs); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + var dimDigest = SHA256.HashData(Encoding.UTF8.GetBytes($"#4233 parity dim winner {ParityServerId}")); + await DeleteParityRowsAsync(connection, dimDigest, ct); + + var bodySucceeded = false; + try + { + await DarlingMcpTestData.RegisterServerAsync(connection, ParityServerId, ParityServerName, ct); + var t0 = FloorToHour(DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow).AddHours(-6)); + + /* (a) dimension-resolved winner. */ + await InsertDimTextAsync(connection, ct, dimDigest, "SELECT dim_winner_text FROM t"); + await SeedParityRowAsync(connection, ct, t0, "0xDIMWIN", deltaExec: 9, deltaElapsed: 450_000, queryText: null, digest: dimDigest); + + /* (b) pre-#1767 winner: inline text, no digest. */ + await SeedParityRowAsync(connection, ct, t0.AddMinutes(5), "0xINLINEWIN", deltaExec: 7, deltaElapsed: 350_000, queryText: "SELECT inline_winner_text", digest: null); + + /* (c) a tie: equal delta_execution_count, same cell. */ + await SeedParityRowAsync(connection, ct, t0.AddMinutes(10), "0xTIEA", deltaExec: 3, deltaElapsed: 150_000, queryText: "SELECT tie_a", digest: null); + await SeedParityRowAsync(connection, ct, t0.AddMinutes(10), "0xTIEB", deltaExec: 3, deltaElapsed: 150_000, queryText: "SELECT tie_b", digest: null); + + var start = t0.AddMinutes(-1); + var end = t0.AddMinutes(20); + + var oldRows = await RunHeatmapSqlAsync(connection, PreQ4233HeatmapSql, ParityServerId, start, end, ct); + var newRows = await RunHeatmapSqlAsync(connection, DarlingQueryHeatmapReader.BuildQueryHeatmapSql(HeatmapMetric.Duration), ParityServerId, start, end, ct); + + Assert.Equal(3, oldRows.Count); + Assert.Equal(oldRows.Count, newRows.Count); + + for (var i = 0; i < oldRows.Count; i++) + { + Assert.Equal(oldRows[i].TimeBin, newRows[i].TimeBin); + Assert.Equal(oldRows[i].BucketIndex, newRows[i].BucketIndex); + Assert.Equal(oldRows[i].Count, newRows[i].Count); + Assert.Equal(oldRows[i].Hash, newRows[i].Hash); + Assert.Equal(oldRows[i].Text, newRows[i].Text); + } + + /* Confirm the agreement above is not two queries independently returning the WRONG (but + matching) answer - check the new builder's rows against what was actually seeded. */ + var dimCell = newRows.Single(r => r.TimeBin == t0); + Assert.Equal("0xDIMWIN", dimCell.Hash); + Assert.Equal("SELECT dim_winner_text FROM t", dimCell.Text); + + var inlineCell = newRows.Single(r => r.TimeBin == t0.AddMinutes(5)); + Assert.Equal("0xINLINEWIN", inlineCell.Hash); + Assert.Equal("SELECT inline_winner_text", inlineCell.Text); + + var tieCell = newRows.Single(r => r.TimeBin == t0.AddMinutes(10)); + Assert.Equal(2, tieCell.Count); + Assert.Contains(tieCell.Hash, new[] { "0xTIEA", "0xTIEB" }); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) => + await DeleteParityRowsAsync(cleanup, dimDigest, cleanupCt)); + } + } + + /// + /// The pre-#4233 shape of , copied from + /// `git show origin/dev:Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingQueryHeatmapReader.cs` + /// before this PR's rewrite landed there, with the metric expression resolved to Duration inline so the + /// comparison needs no template parameter. Reads v_query_stats (#1767's payload-resolving view) and + /// truncates query_text to a preview INSIDE base, for every row in the window - the shape #4233 replaced. + /// + private const string PreQ4233HeatmapSql = """ + WITH base AS ( + SELECT + date_bin(($5::integer * INTERVAL '1 minute'), collection_time, TIMESTAMP '1970-01-01 00:00:00') AS time_bin, + (delta_elapsed_time / 1000.0) / NULLIF(delta_execution_count, 0) AS metric_value, + query_hash, + LEFT(query_text, $7) AS query_preview, + delta_execution_count + FROM v_query_stats + WHERE server_id = $1 + AND collection_time >= $2 + AND collection_time <= $3 + AND ($4::text[] IS NULL OR database_name = ANY($4)) + AND delta_execution_count > 0 + AND (delta_elapsed_time / 1000.0) / NULLIF(delta_execution_count, 0) IS NOT NULL + ), + binned AS ( + SELECT + time_bin, + CASE + WHEN metric_value < 1 THEN 0 + WHEN metric_value < 10 THEN 1 + WHEN metric_value < 100 THEN 2 + WHEN metric_value < 1000 THEN 3 + WHEN metric_value < 10000 THEN 4 + WHEN metric_value < 100000 THEN 5 + ELSE 6 + END AS bucket_index, + query_hash, + query_preview, + delta_execution_count + FROM base + ), + ranked AS ( + SELECT + time_bin, + bucket_index, + query_hash, + query_preview, + COUNT(*) OVER (PARTITION BY time_bin, bucket_index) AS query_count, + ROW_NUMBER() OVER (PARTITION BY time_bin, bucket_index ORDER BY delta_execution_count DESC) AS rn + FROM binned + ) + SELECT + time_bin, + bucket_index, + query_count, + query_hash AS top_query_hash, + query_preview AS top_query_text + FROM ranked + WHERE rn = 1 + ORDER BY time_bin DESC, bucket_index + LIMIT $6 + """; + + private static async Task SeedParityRowAsync( + NpgsqlConnection connection, CancellationToken ct, DateTime collectionTime, string queryHash, + long deltaExec, long deltaElapsed, string? queryText, byte[]? digest) => + await DarlingMcpTestData.ExecAsync(connection, ct, @" +INSERT INTO query_stats + (collection_id, collection_time, server_id, server_name, database_name, query_hash, + sample_interval_seconds, delta_execution_count, delta_worker_time, delta_elapsed_time, + delta_logical_reads, delta_logical_writes, query_text, query_text_digest) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(collectionTime), ParityServerId, ParityServerName, + ParityDb, queryHash, 60, deltaExec, 0L, deltaElapsed, 0L, 0L, queryText, digest); + + private static async Task InsertDimTextAsync(NpgsqlConnection connection, CancellationToken ct, byte[] digest, string text) => + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO query_text_dim (digest, query_text, last_seen) VALUES ($1, $2, $3) ON CONFLICT (digest) DO NOTHING", + digest, text, DarlingMcpTestData.Naive(DateTime.UtcNow)); + + /// Runs either heatmap SQL text with the same seven bound parameters ($4 database filter NULL, + /// $5 bucket_minutes 5, $6 limit 500, $7 preview length 120) and returns the cells sorted by + /// (time_bin, bucket_index) - sorted explicitly rather than trusting each query's own ORDER BY, so the + /// comparison is over the same SET of cells rather than incidentally over matching physical order. + private static async Task> RunHeatmapSqlAsync( + NpgsqlConnection connection, string sql, int serverId, DateTime start, DateTime end, CancellationToken ct) + { + var rows = new List<(DateTime TimeBin, int BucketIndex, long Count, string Hash, string Text)>(); + await using var command = new NpgsqlCommand(sql, connection); + DarlingMcpReadParameters.AddWindow(command, serverId, start, end); + command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Array | NpgsqlDbType.Text, Value = DBNull.Value }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = 5 }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = 500 }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = 120 }); + + await using var reader = await command.ExecuteReaderAsync(ct); + while (await reader.ReadAsync(ct)) + { + rows.Add(( + reader.GetDateTime(0), + reader.IsDBNull(1) ? 0 : Convert.ToInt32(reader.GetValue(1)), + reader.IsDBNull(2) ? 0 : Convert.ToInt64(reader.GetValue(2)), + reader.IsDBNull(3) ? "" : reader.GetString(3), + reader.IsDBNull(4) ? "" : reader.GetString(4))); + } + + return rows.OrderBy(r => r.TimeBin).ThenBy(r => r.BucketIndex).ToList(); + } + + private static async Task DeleteParityRowsAsync(NpgsqlConnection connection, byte[] dimDigest, CancellationToken ct) + { + await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM query_stats WHERE server_id = $1", ParityServerId); + await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM servers WHERE server_id = $1", ParityServerId); + await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM config_monitored_servers WHERE server_id = $1", ParityServerId); + await DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM query_text_dim WHERE digest = $1", dimDigest); + } } diff --git a/Darling/Darling.Tests/QueryHeatmapSqlTwinParityTests.cs b/Darling/Darling.Tests/QueryHeatmapSqlTwinParityTests.cs new file mode 100644 index 0000000000..04af9041b5 --- /dev/null +++ b/Darling/Darling.Tests/QueryHeatmapSqlTwinParityTests.cs @@ -0,0 +1,79 @@ +/* + * 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 Xunit; + +namespace Darling.Tests; + +/// +/// #4233 ruling item 2: BuildQueryHeatmapSql is hand-copied into two files - the WPF viewer's chart +/// () and the MCP/web reader behind +/// get_query_heatmap () - +/// because the headless service must not reference the WPF viewer. A hand-edit that changes only one copy +/// is exactly the kind of drift a reviewer can miss in a large diff, so this pins the two byte-identical +/// through the final WHERE rn = 1 filter - base/binned/ranked, the magnitude CASE, the top-1 window, +/// and #4233's own COALESCE/query_text_dim subquery - once the differences that predate #4233 are +/// normalized away: +/// +/// the bin width: a literal 5-minute interval in the viewer vs. the reader's bound $5 parameter; +/// the preview width: a literal 120 in the viewer vs. the reader's bound $7 parameter (#4198). +/// +/// The ORDER BY/LIMIT tail AFTER WHERE rn = 1 is a third, deliberate difference (the +/// reader caps and orders newest-first; the viewer has no cap and orders ascending) and is pinned on its +/// own by DarlingQueryHeatmapSurfaceAndSqlTests.HeatmapSql_CapsFromTheRecentEnd, so it is excluded +/// here by cutting the string rather than normalized. +/// +/// The two HeatmapMetric enums are kept fully qualified rather than imported side by side - +/// each namespace declares its own (the service must not reference the viewer's), and importing both would +/// make every unqualified HeatmapMetric in this file ambiguous. Casting the shared underlying int is +/// how "the same argument" reaches both builders without that ambiguity. +/// +/// Proven live, not just by construction: temporarily changing one character inside either copy's +/// COALESCE/query_text_dim subquery text made this test fail before the character was reverted. +/// +public sealed class QueryHeatmapSqlTwinParityTests +{ + private const string RnFilter = "WHERE rn = 1"; + + [Theory] + [InlineData(0)] // Duration + [InlineData(1)] // Cpu + [InlineData(2)] // LogicalReads + [InlineData(3)] // LogicalWrites + [InlineData(4)] // ExecutionCount + public void BothCopies_AreByteIdentical_ThroughTheRnFilter(int metric) + { + var viewerSql = PerformanceMonitor.Darling.Viewer.ViewerDataService.BuildQueryHeatmapSql( + (PerformanceMonitor.Darling.Viewer.HeatmapMetric)metric); + var readerSql = PerformanceMonitor.Darling.Service.Mcp.DarlingQueryHeatmapReader.BuildQueryHeatmapSql( + (PerformanceMonitor.Darling.Service.Mcp.HeatmapMetric)metric); + + /* Both known normalizations must actually fire - if a future edit changes the literal/parameter + spelling on either side, this catches the normalization silently becoming a no-op, which the + Assert.Equal below alone could miss if an unrelated one-sided edit happened to cancel it out. */ + const string ReaderBinWidth = "date_bin(($5::integer * INTERVAL '1 minute'), collection_time, TIMESTAMP '1970-01-01 00:00:00')"; + const string ViewerBinWidth = "date_bin(INTERVAL '5 minutes', collection_time, TIMESTAMP '1970-01-01 00:00:00')"; + const string ReaderPreviewWidth = "query_text_digest)), $7) AS top_query_text"; + const string ViewerPreviewWidth = "query_text_digest)), 120) AS top_query_text"; + + Assert.Contains(ReaderBinWidth, readerSql, StringComparison.Ordinal); + Assert.Contains(ViewerBinWidth, viewerSql, StringComparison.Ordinal); + Assert.Contains(ReaderPreviewWidth, readerSql, StringComparison.Ordinal); + Assert.Contains(ViewerPreviewWidth, viewerSql, StringComparison.Ordinal); + + var normalizedReader = readerSql + .Replace(ReaderBinWidth, ViewerBinWidth, StringComparison.Ordinal) + .Replace(ReaderPreviewWidth, ViewerPreviewWidth, StringComparison.Ordinal); + + var viewerHead = viewerSql[..(viewerSql.IndexOf(RnFilter, StringComparison.Ordinal) + RnFilter.Length)]; + var readerHead = normalizedReader[..(normalizedReader.IndexOf(RnFilter, StringComparison.Ordinal) + RnFilter.Length)]; + + Assert.Equal(viewerHead, readerHead); + } +} From 88740dd35b92f1f9bc6f98dba88b97110a1081da Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:59:33 -0400 Subject: [PATCH 3/3] Let the heatmap parity test accept either tied row (#4233) DarlingQueryHeatmapTests.cs's OldSqlFromDev_AndTheNewBuilder_AgreeOnCellsCountsHashAndPreview seeded a tie (two rows with equal delta_execution_count in one cell) and then asserted the old SQL and the new builder picked the SAME row's hash and text. Both use ROW_NUMBER() ... ORDER BY delta_execution_count DESC with no second key, and their plans differ, so PostgreSQL does not promise they pick the same row. The assertion had passed so far but could flake. The tie cell now only asserts that each query's winner is one of the two tied rows, with the hash and preview text that belong to that same row. Every other cell still asserts equality. Updated the doc comment's item (c) to match. No product SQL changed: the tie is arbitrary on dev too, and picking a winner would be a behavior change. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3 --- .../Darling.Tests/DarlingQueryHeatmapTests.cs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs index 8d6121104c..17e4e36e27 100644 --- a/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs +++ b/Darling/Darling.Tests/DarlingQueryHeatmapTests.cs @@ -627,9 +627,9 @@ private static async Task DeleteRowsAsync(NpgsqlConnection connection, Cancellat /// (a) a winner whose text lives only in query_text_dim - digest set, inline query_text NULL /// (b) a pre-#1767 winner - inline query_text set, digest NULL /// (c) a tie inside one cell - two rows with equal delta_execution_count, so ROW_NUMBER's - /// tiebreak is unspecified by either query. This does not assert WHICH row wins; both queries scan the - /// same base rows in the same order, so the pin is that the OLD SQL and the NEW builder agree with each - /// other, which is what "no behavior change" actually means for an unordered tie. + /// tiebreak is unspecified by either query, and the old and the new SQL have different plans, so + /// PostgreSQL does not promise they pick the same row. The pin is only that each query's winner is + /// one of the two tied rows, with the hash and preview text that belong to that same row. /// /// [Fact] @@ -673,11 +673,31 @@ public async Task OldSqlFromDev_AndTheNewBuilder_AgreeOnCellsCountsHashAndPrevie Assert.Equal(3, oldRows.Count); Assert.Equal(oldRows.Count, newRows.Count); + var tieBin = t0.AddMinutes(10); + var tiedPreviewByHash = new Dictionary + { + ["0xTIEA"] = "SELECT tie_a", + ["0xTIEB"] = "SELECT tie_b", + }; + for (var i = 0; i < oldRows.Count; i++) { Assert.Equal(oldRows[i].TimeBin, newRows[i].TimeBin); Assert.Equal(oldRows[i].BucketIndex, newRows[i].BucketIndex); Assert.Equal(oldRows[i].Count, newRows[i].Count); + + if (newRows[i].TimeBin == tieBin) + { + /* PostgreSQL does not order a ROW_NUMBER tie, and the two plans differ, so the old and + the new SQL can pick different tied rows. Each must pick one of the two, with the + hash and preview text of that row. */ + Assert.Contains(oldRows[i].Hash, tiedPreviewByHash.Keys); + Assert.Equal(tiedPreviewByHash[oldRows[i].Hash], oldRows[i].Text); + Assert.Contains(newRows[i].Hash, tiedPreviewByHash.Keys); + Assert.Equal(tiedPreviewByHash[newRows[i].Hash], newRows[i].Text); + continue; + } + Assert.Equal(oldRows[i].Hash, newRows[i].Hash); Assert.Equal(oldRows[i].Text, newRows[i].Text); }