Dedup Query Store intervals before aggregating (#1841 tier 1)

CHANGELOG.md

@@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **Query Store totals no longer count the same interval once per collection - live stores were overstating hot queries by one to two orders of magnitude** ([#1841]) - Query Store's runtime-stats rows are CUMULATIVE per-interval snapshots, and the collector is incremental on `last_execution_time`, so while a query keeps running its OPEN interval is re-fetched every cycle and stored again with a growing `execution_count`. The schema has no interval-id column, and every aggregate read simply summed the raw rows - so one interval's work was counted once per collection. Measured on a real store: the same (server, database, query_id, plan_id) appeared **496 times inside a single hour bucket** with `execution_count` of 1 each time (one interval, 496 collections, its CPU and duration billed 496 times), alongside cases growing 1 -> 23 cumulatively where every re-collection re-charged the accumulated total as well. Execution-count weighting did not rescue the averages: the repeated snapshots carry DIFFERENT growing weights AND different `avg_*` values, so an open interval was weighted by the triangular sum of its own growth. Every affected read now collapses each interval to its LATEST snapshot before aggregating - keyed on `first_execution_time`, the interval identity the schema does expose - which is the same `ROW_NUMBER() ... WHERE rn = 1` shape the plan-regression analysis collectors have always used. **This corrects the numbers for data already collected**; no re-collection or migration is needed. Fixed in both SKUs: Lite's Query Store time-slice bars, top-queries grid and current-vs-baseline comparison, the Darling Viewer's equivalents plus its regressions grid and slicer overlay-on-select, the service's MCP/REST `get_query_store_top`, and the Custom Views raw route. Three things this does NOT change, each stated at the source so none of them reads as an oversight. **Bar placement still lags**: a bar is keyed on when an interval was last COLLECTED, not when it ran, which on Query Store's default 60-minute interval is reliably one bucket late - window totals are right, placement is not, and correcting it needs `first_execution_time`, which is the monitored server's LOCAL wall clock against a UTC axis. **The duration-trend chart is deliberately left un-deduped and still overstates**: deduping is right for totals and wrong for that chart, because it keeps one row per interval at the collection where the interval closed, and a 60-minute interval against a 5-minute cadence collapses twelve snapshots onto one timestamp - a 1-hour window would render a single point valued zero. Better an honestly-labelled inflated line than an empty chart, until tier 2 can place the work at the time it ran. **The Custom Views rollup is not repaired**: `query_store_stats_hourly` materialized its sums from un-deduped rows, so a panel whose window reaches past the raw tier still reads the old inflated numbers, and no read-side change can undo that - a continuous aggregate cannot contain a window function, so it needs a rebuild rather than an edit. All three are tracked on [#1841]. Two further reads are correct as they stand: the per-collection history drilldown (a raw projection with no aggregate - the "show me every snapshot" surface) and the stored-plan lookups (already latest-row-only). New round-trips execute the corrected reads against real DuckDB and real Postgres and assert the interval is counted once, each watched failing against the old queries first; the composed panel gained the first test that ever RUNS the compiler's output against Postgres.
+
 - **Alert History rows stored under the old "TempDB Space" metric name show their percentage again** (field find during [#1830]'s triage) - lowercasing the tempdb token across both apps' UI also changed this alert's metric_name KEY, from `TempDB Space` to `tempdb Space`, and that change knowingly left already-stored history rows carrying the old name. The Value/Threshold formatter matches the name ordinally, so those rows missed the percent case and fell through to the bare two-decimal default: a tempdb alert archived before the rename showed `87.30` where the row beside it showed `87.3%`. Both formatters (Lite and the Darling Viewer) now accept the old spelling as well. Nothing writes it any more - it is kept solely so rows already on disk format like the new ones.
 
 - **An enumerated collector that enumerates NOTHING now says so on its collection-log row** ([#1837], minimal core) - `query_store`, `index_object_stats`, and `database_scoped_config` list their databases first and then collect per database, and when that list comes back empty the cycle returned early and logged a bare `SUCCESS` with 0 rows. That row is byte-identical to a healthy collector whose databases were simply quiet, which is what let [#1833]'s Azure defect sit behind a green Collection Health for as long as it did: Query Store enabled nowhere, a database-scoped collector filtered down to nothing, and an ordinary idle cycle all logged the same thing. The status deliberately stays `SUCCESS` - nothing failed, and health banding on empty results is the rest of [#1837] - but the row now carries a fixed, greppable message saying the enumeration yielded 0 items, on the column `SUCCESS` rows otherwise leave null. Both SKUs, from one shared constant so the wording cannot drift. Every health count, band, and self-alert keys on status rather than on that column, so the note is visible in the collection-log detail grid and inert everywhere else.
@@ -1994,3 +1996,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1830]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1830
 [#1839]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1839
 [#1832]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1832
+[#1841]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1841

Darling/Darling.Tests/DarlingComposeTests.cs

@@ -1124,6 +1124,71 @@ public void Compile_WeightedRatio_IsProductSumOverNullifSumOfWeight(string ratio
         Assert.Contains("/ 1000.0", sql, StringComparison.Ordinal);
     }
 
+    [Fact]
+    public void Compile_QueryStoreRawRoute_DedupsPerIntervalBeforeAggregating()
+    {
+        /* #1841. query_store_stats rows are CUMULATIVE per-interval snapshots re-collected every cycle,
+           so the raw route must aggregate the LATEST snapshot per interval, not every stored row. The
+           interval identity is first_execution_time; server_id is in the partition because a composed
+           panel spans the fleet. */
+        /* Scoped to specific servers on purpose — that is what puts server_name in the outer WHERE, which
+           is the whole reason it has to be in the partition too. */
+        var sql = Compile(
+            ValidPlan("{\"source\":\"query_store_stats\",\"measure\":\"qs_executions\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}"),
+            servers: ["srv-a", "srv-b"]);
+
+        Assert.Contains(
+            "PARTITION BY server_id, server_name, database_name, query_id, plan_id, first_execution_time, "
+            + "execution_type_desc, replica_role ORDER BY collection_time DESC",
+            sql, StringComparison.Ordinal);
+        Assert.Contains("AS qs_rn", sql, StringComparison.Ordinal);
+        Assert.Contains("WHERE qs_rn = 1", sql, StringComparison.Ordinal);
+
+        /* server_name must be IN the partition, not only in the outer WHERE: Postgres pushes a qual through
+           a window-function subquery only when the qual's columns appear in every PARTITION BY, so without
+           it a fleet store would rank every server's rows before narrowing to the panel's scope. */
+        var partition = sql.IndexOf("PARTITION BY", StringComparison.Ordinal);
+        var scope = sql.IndexOf("f.server_name = ANY(", StringComparison.Ordinal);
+        Assert.True(scope > partition, "the server scope stays in the outer WHERE, so the partition must carry server_name");
+
+        /* The dedup window is pushed INSIDE the derived table, so "latest" means latest within the
+           panel's window rather than latest overall — and it prunes chunks before the window function. */
+        var relation = sql.IndexOf("collect.query_store_stats ", StringComparison.Ordinal);
+        var innerWindow = sql.IndexOf("WHERE collection_time >= $1 AND collection_time <= $2", StringComparison.Ordinal);
+        Assert.True(relation >= 0 && innerWindow > relation,
+            "the dedup subquery must carry the window predicate itself");
+
+        /* Still only catalog identifiers and the two already-bound window parameters. */
+        Assert.DoesNotContain("config.", sql, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Compile_QueryStoreCaggRoute_IsNotWrappedInTheRawDedup()
+    {
+        /* The dedup belongs to the RAW route only: the rollup has no first_execution_time to partition on
+           (a CAGG cannot contain a window function at all), and its sums were materialized from
+           un-deduped rows — #1841 tier 2, deliberately not repaired here. */
+        var (compiled, error) = CompileAged(
+            "{\"source\":\"query_store_stats\",\"ratio\":\"qs_avg_duration_us\",\"timeBucket\":\"hour\",\"viz\":\"line\"}", daysOld: 10);
+        Assert.True(error is null, error);
+
+        Assert.Contains("FROM collect.query_store_stats_hourly AS f", compiled!.Sql, StringComparison.Ordinal);
+        Assert.DoesNotContain("qs_rn", compiled.Sql, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Compile_NonQueryStoreRawRoute_ReadsTheTableDirectly()
+    {
+        /* The #1841 wrapper is scoped to query_store_stats — every other source keeps its bare
+           "FROM collect.<table> AS f", so this stays a one-table exception rather than a compiler-wide
+           behavior change. */
+        var sql = Compile(ValidPlan(
+            "{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}"));
+
+        Assert.Contains("FROM collect.wait_stats AS f", sql, StringComparison.Ordinal);
+        Assert.DoesNotContain("qs_rn", sql, StringComparison.Ordinal);
+    }
+
     [Fact]
     public void WeightedQsRatios_ValidateAsRatios_AndRejectMeasureReference()
     {
@@ -1951,3 +2016,118 @@ public async Task RunComposedPanel_OldWindow_AgainstPlainPostgres_RunsCleanOnRaw
         Assert.Null(outcome.Payload["notice"]);
     }
 }
+
+/// <summary>
+/// Gated (DARLING_TEST_PG) execution of the composed Query Store panel against real Postgres. The rest of
+/// the compose suite is string-only, so nothing else ever RUNS the compiler's output — and the #1841 raw
+/// route is a hand-assembled derived table, exactly the shape where a string pin passes while Postgres
+/// rejects the statement (or silently resolves a column the dedup forgot to project).
+/// </summary>
+[Collection("live-postgres")]
+public sealed class ComposeQueryStoreLivePostgresTests
+{
+    private const int ServerId = -970820;
+    private const string ServerName = "compose-qs-dedup";
+
+    private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+
+    [Fact]
+    public async Task ComposedQueryStorePanel_RunsOnPostgres_AndCountsEachIntervalOnce()
+    {
+        var cs = ConnectionString;
+        Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to a Postgres connection string to run the live compose Query Store test.");
+
+        using var connection = new NpgsqlConnection(cs);
+        await connection.OpenAsync(TestContext.Current.CancellationToken);
+        await PgMigrations.MigrateAsync(connection, TestContext.Current.CancellationToken);
+        await DeleteAsync(connection);
+
+        var end = new DateTime(DateTime.UtcNow.Ticks - (DateTime.UtcNow.Ticks % TimeSpan.TicksPerSecond), DateTimeKind.Utc);
+        var bucket = end.AddHours(-2);
+        var firstExecA = bucket.AddMinutes(1);
+        var firstExecB = bucket.AddMinutes(2);
+
+        try
+        {
+            /* Interval A re-collected three times with a FLAT count of 1, interval B twice while it grew
+               10 -> 40. True total executions = 1 + 40 = 41; un-deduped = 3 + 50 = 53. */
+            foreach (var minute in new[] { 5, 10, 15 })
+            {
+                await InsertAsync(connection, bucket.AddMinutes(minute), queryId: 1, planId: 11, firstExecA, execCount: 1);
+            }
+
+            await InsertAsync(connection, bucket.AddMinutes(5), queryId: 2, planId: 22, firstExecB, execCount: 10);
+            await InsertAsync(connection, bucket.AddMinutes(10), queryId: 2, planId: 22, firstExecB, execCount: 40);
+
+            var (plan, parseError) = ComposeSpec.TryParsePanel(
+                (JsonObject)JsonNode.Parse("{\"source\":\"query_store_stats\",\"measure\":\"qs_executions\",\"aggregate\":\"sum\",\"timeBucket\":\"hour\",\"viz\":\"line\"}")!,
+                []);
+            Assert.True(parseError is null, parseError);
+
+            var (compiled, compileError) = ComposeCompiler.Compile(
+                plan!,
+                new ComposeRunContext([ServerName], end.AddHours(-3), end, ComposeRunContext.NoVariables,
+                    RollupAvailability.All, end, RollupCoverage.Unknown));
+            Assert.True(compileError is null, compileError);
+
+            /* A 3-hour window ending now stays inside the raw tier, so this is the deduped raw route
+               under test and not the CAGG (whose sums #1841 tier 2 still owes a fix). */
+            Assert.False(compiled!.Route.IsCagg, "the window must stay on the raw route for this test to mean anything");
+
+            await using var command = new NpgsqlCommand(compiled.Sql, connection);
+            foreach (var parameter in compiled.Parameters)
+            {
+                command.Parameters.Add(parameter);
+            }
+
+            var total = 0.0;
+            await using (var reader = await command.ExecuteReaderAsync(TestContext.Current.CancellationToken))
+            {
+                while (await reader.ReadAsync(TestContext.Current.CancellationToken))
+                {
+                    /* Time-series shape: bucket, then value. */
+                    total += reader.GetDouble(reader.GetOrdinal("value"));
+                }
+            }
+
+            Assert.Equal(41.0, total, 3);
+        }
+        finally
+        {
+            await DeleteAsync(connection);
+        }
+    }
+
+    private static async Task DeleteAsync(NpgsqlConnection connection)
+    {
+        using var command = new NpgsqlCommand("DELETE FROM collect.query_store_stats WHERE server_id = $1", connection);
+        command.Parameters.AddWithValue(ServerId);
+        await command.ExecuteNonQueryAsync(TestContext.Current.CancellationToken);
+    }
+
+    private static async Task InsertAsync(
+        NpgsqlConnection connection, DateTime collectionTimeUtc, long queryId, long planId, DateTime firstExecutionTimeUtc, long execCount)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO collect.query_store_stats
+    (collection_id, collection_time, server_id, server_name, database_name,
+     query_id, plan_id, execution_type_desc, first_execution_time, last_execution_time,
+     query_hash, execution_count, avg_duration_us, avg_cpu_time_us)
+VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14)", connection);
+        command.Parameters.AddWithValue(1L);
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(collectionTimeUtc, DateTimeKind.Unspecified));
+        command.Parameters.AddWithValue(ServerId);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue("ComposeDb");
+        command.Parameters.AddWithValue(queryId);
+        command.Parameters.AddWithValue(planId);
+        command.Parameters.AddWithValue("Regular");
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(firstExecutionTimeUtc, DateTimeKind.Unspecified));
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(collectionTimeUtc, DateTimeKind.Unspecified));
+        command.Parameters.AddWithValue("0xCOMPOSEHASH");
+        command.Parameters.AddWithValue(execCount);
+        command.Parameters.AddWithValue(1000L);
+        command.Parameters.AddWithValue(500L);
+        await command.ExecuteNonQueryAsync(TestContext.Current.CancellationToken);
+    }
+}

Darling/Darling.Tests/DarlingMcpDataToolsTests.cs

@@ -272,6 +272,23 @@ primary and secondary workload blended into one row. */
         Assert.Contains("SUM(execution_count)", sql, StringComparison.Ordinal);
     }
 
+    [Fact]
+    public void QueryStoreSql_DedupsPerIntervalBeforeAggregating()
+    {
+        /* #1841. query_store_stats rows are CUMULATIVE per-interval snapshots and the collector re-fetches
+           the OPEN interval every cycle, so SUM(execution_count) over the raw rows reports 10 + 25 + 40
+           for an interval that reached 40, and the AVG(avg_*) columns become an avg-of-avgs weighted by
+           re-collection frequency. This surface feeds BOTH the MCP tool and the REST route, so inflated
+           numbers would reach an agent's reasoning as readily as the web dashboard. replica_role is in the
+           key because the aggregate GROUPs BY it — the dedup must never drop a row the read must return. */
+        var sql = DarlingDataReader.QueryStoreTopSql;
+        Assert.Contains(
+            "PARTITION BY database_name, query_id, plan_id, first_execution_time, execution_type_desc, replica_role",
+            sql, StringComparison.Ordinal);
+        Assert.Contains("ORDER BY collection_time DESC", sql, StringComparison.Ordinal);
+        Assert.Contains("WHERE rn = 1", sql, StringComparison.Ordinal);
+    }
+
     [Fact]
     public void ServerListSql_EnabledServers_WithLastCollection()
     {

Darling/Darling.Tests/ViewerDrillDownTests.cs

@@ -215,6 +215,22 @@ public void QueryStoreItemTimelineSql_FiltersOneQueryPlanAndScalesByExecutionCou
         AssertPgPositionalDialect(sql);
     }
 
+    [Fact]
+    public void QueryStoreItemTimelineSql_DedupsPerInterval_SoTheOverlayAgreesWithTheSlicerBars()
+    {
+        /* #1841. The WHERE narrows to query_id + plan_id but NOT to an interval, and the rows are
+           CUMULATIVE per-interval snapshots re-collected every cycle — so un-deduped this series draws one
+           interval as a rising staircase of restatements, and can emit several points at the SAME
+           collection_time. It is drawn OVER QueryStoreSlicerSql's bars, which ARE deduped, so leaving it
+           raw would make the overlay contradict the bars it annotates. */
+        var sql = ViewerDataService.QueryStoreItemTimelineSql;
+        Assert.Contains(
+            "PARTITION BY database_name, query_id, plan_id, first_execution_time, execution_type_desc",
+            sql, StringComparison.Ordinal);
+        Assert.Contains("ORDER BY collection_time DESC", sql, StringComparison.Ordinal);
+        Assert.Contains("WHERE rn = 1", sql, StringComparison.Ordinal);
+    }
+
     private static void AssertPgPositionalDialect(string sql)
     {
         Assert.DoesNotContain("@", sql, StringComparison.Ordinal);   // no T-SQL named params