The forced-plan-failures alert read gets the covering index the planner will actually take — the composite it needed was already there and priced out, so it walked the whole fleet's two-hour slice per server (#3573)

Darling/Darling.Tests/ForcePlanFailuresAccessPathTests.cs

@@ -0,0 +1,284 @@
+/*
+ * 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;
+using System.Linq;
+using System.Text;
+using System.Text.RegularExpressions;
+using System.Threading;
+using System.Threading.Tasks;
+using Npgsql;
+using PerformanceMonitor.Darling.Service;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #3573: the alerting pass's forced-plan-failures read and the covering index that gives it an access path
+/// the planner will actually take.
+///
+/// <para><b>What went wrong is not that an index was missing.</b> V1 generates
+/// <c>idx_query_store_stats_time (server_id, collection_time)</c>, the exact composite the read's predicate
+/// wants, and the production catalog carried it on the hypertable and on the chunk in the failing plan. The
+/// planner priced it out: <c>server_id</c> has near-zero physical correlation (43 servers interleaved by
+/// collection pass), so the cost model charged one random page per tuple for the composite's heap fetches
+/// and preferred streaming the entire fleet's two-hour slice through the perfectly-correlated time index
+/// and filtering 95% of it away. Forced under <c>random_page_cost = 1.1</c> the same statement took the
+/// composite and touched 5,063 buffers instead of 57,307 \u2014 the composite was right all along. A second plain
+/// composite would have been priced, and ignored, identically.</para>
+///
+/// <para><b>Covering is the fix because it deletes the term the cost model got wrong.</b> With every column
+/// the read touches in the key or INCLUDE, the plan is an Index Only Scan with no heap component to misprice,
+/// at any <c>random_page_cost</c> and at any share of the fleet the busiest server grows into. That makes the
+/// INCLUDE list load-bearing in a way most index definitions are not: a column added to the read and not to
+/// the index does not fail anything \u2014 it silently hands the read back to the fleet-wide plan. The ungated pins
+/// below hold the two against each other from source; the gated one asks the planner.</para>
+/// </summary>
+/* Live-fixture tests share one Postgres store; the collection serializes them so cross-test row churn
+   cannot race another class's assertions. */
+[Collection("live-postgres")]
+public sealed class ForcePlanFailuresAccessPathTests
+{
+    /// <summary>Distinctive fake ids \u2014 a real server_id is a storage-name hash, never these. The target is
+    /// one of six so its rows are ~17% of the seeded chunk, a share at which a seq scan is not competitive.</summary>
+    private const int TargetServerId = -735730;
+    private static readonly int[] OtherServerIds = { -735731, -735732, -735733, -735734, -735735 };
+    private const string TestServerName = "force-plan-access-path-e2e";
+
+    /// <summary>
+    /// Every <c>qs.&lt;column&gt;</c> the shipped statement references, read from the statement itself. The
+    /// alias is fixed by the SQL, so this is the complete set of columns the scan must produce.
+    /// </summary>
+    private static IReadOnlyCollection<string> ColumnsTheReadReferences()
+    {
+        return Regex.Matches(DarlingAlertReadAdapter.ForcePlanFailuresSql, @"\bqs\.([a-z_]+)")
+            .Select(m => m.Groups[1].Value)
+            .Distinct(StringComparer.Ordinal)
+            .OrderBy(c => c, StringComparer.Ordinal)
+            .ToList();
+    }
+
+    /// <summary>
+    /// The read's column references and the index's column list are the SAME set, both ways. A column the
+    /// read touches that the index lacks degrades the Index Only Scan to the heap plan it replaced, silently;
+    /// a column the index carries that the read no longer touches is dead weight on every insert into the
+    /// largest table in the store (INCLUDE disables deduplication, so each is real bytes per row).
+    /// </summary>
+    [Fact]
+    public void TheCoveringIndex_CarriesExactlyTheColumnsTheReadReferences()
+    {
+        var referenced = ColumnsTheReadReferences();
+        var carried = PgTableTuning.ForcePlanFailuresIndexColumns.OrderBy(c => c, StringComparer.Ordinal).ToList();
+
+        Assert.Equal(carried, referenced);
+
+        /* The predicate columns are the KEY, in predicate order: the equality column first so one server's
+           rows are one contiguous index range, the range column second. Everything else is INCLUDE. */
+        Assert.Equal("server_id", PgTableTuning.ForcePlanFailuresIndexColumns[0]);
+        Assert.Equal("collection_time", PgTableTuning.ForcePlanFailuresIndexColumns[1]);
+    }
+
+    /// <summary>
+    /// The statement text is BUILT from the same column list the pin above holds, so the two cannot drift: the
+    /// literal SQL carries the key as <c>(server_id, collection_time DESC)</c> and the remaining columns, in
+    /// order, as INCLUDE. Also pins the decisions the rig measured (see <see cref="PgTableTuning"/>): idempotent
+    /// <c>IF NOT EXISTS</c>; NOT <c>CONCURRENTLY</c>, which hypertables refuse; NOT
+    /// <c>timescaledb.transaction_per_chunk</c>, whose mid-build cancel leaves an invalid parent index that the
+    /// idempotent re-run then skips forever; and <c>collect.</c>-qualified like every neighbour.
+    /// </summary>
+    [Fact]
+    public void TheStatement_IsBuiltFromTheColumnList_AndTakesNeitherMeasuredTrap()
+    {
+        var statements = PgTableTuning.Statements
+            .Where(s => s.Contains(PgTableTuning.ForcePlanFailuresIndexName, StringComparison.Ordinal))
+            .ToList();
+        var statement = Assert.Single(statements);
+
+        var columns = PgTableTuning.ForcePlanFailuresIndexColumns;
+        var expected =
+            "CREATE INDEX IF NOT EXISTS " + PgTableTuning.ForcePlanFailuresIndexName
+            + " ON collect.query_store_stats (" + columns[0] + ", " + columns[1] + " DESC)"
+            + " INCLUDE (" + string.Join(", ", columns.Skip(2)) + ")";
+        Assert.Equal(expected, statement);
+
+        Assert.DoesNotContain("CONCURRENTLY", statement, StringComparison.OrdinalIgnoreCase);
+        Assert.DoesNotContain("transaction_per_chunk", statement, StringComparison.OrdinalIgnoreCase);
+        Assert.DoesNotContain(" WHERE ", statement, StringComparison.Ordinal); /* not partial \u2014 see the statement's remarks */
+    }
+
+    /// <summary>
+    /// The evidence no string pin can give: that the planner TAKES the index for the shipped statement. The
+    /// production failure was a plan choice, not a missing object \u2014 the right composite was in the catalog and
+    /// the plan walked past it \u2014 so a test that only checked <c>pg_indexes</c> would have passed on the broken
+    /// store. This builds the store the way the service does (ladder, then the hypertable conversion where
+    /// TimescaleDB is present, then <see cref="PgTableTuning.ApplyAsync"/>), seeds six servers' rows in the
+    /// collector's per-pass contiguous batches, and EXPLAINs the exact shipped SQL with its real bound
+    /// parameters: the plan must be an Index Only Scan on the covering index and must NOT be the time-index
+    /// scan filtering on <c>server_id</c> that the issue's plan showed.
+    /// </summary>
+    [Fact]
+    public async Task TheShippedRead_PlansAsAnIndexOnlyScanOnTheCoveringIndex_AgainstDevPostgres()
+    {
+        var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+        Assert.SkipWhen(string.IsNullOrEmpty(connectionString),
+            "Set DARLING_TEST_PG to a Postgres connection string to run the live access-path test.");
+
+        var ct = TestContext.Current.CancellationToken;
+
+        using var connection = new NpgsqlConnection(connectionString);
+        await connection.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(connection, ct);
+
+        /* The service's own order: hypertables first (where the extension exists), so the index is created on
+           a hypertable and propagates to chunks, then the tuning list. On a plain-PostgreSQL store the index is
+           an ordinary btree and the plan assertion below holds the same way. #1922: probe on its own connection. */
+        var timescaleEnabled = await LiveTimescaleProbe.TryEnableAsync(connectionString!, ct);
+        if (timescaleEnabled)
+        {
+            await TimescaleSupport.ConvertToHypertablesAsync(connection, null, ct);
+        }
+
+        await PgTableTuning.ApplyAsync(connection, null, ct);
+
+        var bodySucceeded = false;
+        try
+        {
+            await DeleteTestRowsAsync(connection, ct);
+
+            /* The index exists on the hypertable with the shipped definition. */
+            using (var indexDef = new NpgsqlCommand(
+                "SELECT indexdef FROM pg_indexes WHERE schemaname = 'collect' AND tablename = 'query_store_stats' AND indexname = $1", connection))
+            {
+                indexDef.Parameters.AddWithValue(PgTableTuning.ForcePlanFailuresIndexName);
+                var def = await indexDef.ExecuteScalarAsync(ct) as string;
+                Assert.NotNull(def);
+                Assert.Contains("(server_id, collection_time DESC)", def, StringComparison.Ordinal);
+                Assert.Contains("INCLUDE (" + string.Join(", ", PgTableTuning.ForcePlanFailuresIndexColumns.Skip(2)) + ")", def, StringComparison.Ordinal);
+            }
+
+            /* Eight passes fifteen minutes apart, all inside the read's two-hour window; each pass writes the six
+               servers in turn, each server's batch contiguous \u2014 the write pattern that makes server_id's
+               correlation near zero, which is the condition the production plan was chosen under. All
+               Kind-Unspecified: naive-UTC storage, see PgCollectorRowWriter. */
+            var utcNow = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified);
+            for (var pass = 7; pass >= 0; pass--)
+            {
+                var collectionTime = utcNow.AddMinutes(-2 - pass * 15);
+                foreach (var serverId in OtherServerIds.Take(3).Append(TargetServerId).Concat(OtherServerIds.Skip(3)))
+                {
+                    /* The target's forced plan climbs one failure per pass (pass 7 = 0 ... pass 0 = 7). */
+                    var forcedFailures = serverId == TargetServerId ? 7L - pass : (long?)null;
+                    await SeedPassAsync(connection, serverId, collectionTime, rows: 400, forcedFailures, ct);
+                }
+            }
+
+            /* The planner needs the visibility map current (the product keeps it so with the insert-autovacuum
+               override; a test cannot wait for autovacuum) and statistics for the rows just written. */
+            using (var vacuum = new NpgsqlCommand("VACUUM ANALYZE collect.query_store_stats", connection))
+            {
+                await vacuum.ExecuteNonQueryAsync(ct);
+            }
+
+            var plan = await ExplainShippedReadAsync(connection, utcNow - DarlingAlertReadAdapter.ForcePlanFailureWindow, ct);
+
+            /* The chunk copy of a hypertable index is named <chunk>_<index>, truncated to 63 characters, so match
+               on the name's stable prefix rather than its whole. */
+            Assert.Contains("Index Only Scan", plan, StringComparison.Ordinal);
+            Assert.Contains("idx_query_store_stats_server_time", plan, StringComparison.Ordinal);
+            Assert.DoesNotContain("collection_time_idx", plan, StringComparison.Ordinal);
+            /* The issue's signature: server_id applied as a post-scan Filter (any alias, any parenthesisation)
+               rather than inside the Index Cond. */
+            Assert.False(Regex.IsMatch(plan, @"Filter: \(+(qs(_\d+)?\.)?server_id"),
+                "server_id is being applied as a Filter after the scan — the fleet-wide plan is back:\n" + plan);
+
+            /* And the read still answers correctly through the new path: the target's forced plan's counter
+               rose between its two newest sightings (pass 1 -> pass 0), and nothing else did. */
+            await using var postgres = NpgsqlDataSource.Create(connectionString!);
+            var adapter = new DarlingAlertReadAdapter(postgres);
+            var failures = await adapter.GetForcePlanFailuresAsync(TargetServerId.ToString(CultureInfo.InvariantCulture), ct);
+            var failure = Assert.Single(failures);
+            Assert.Equal("ForcedDb", failure.DatabaseName);
+            Assert.Equal(1L, failure.QueryId);
+            Assert.Equal(10L, failure.PlanId);
+            Assert.Equal(1L, failure.FailureDelta);
+            Assert.Equal(7L, failure.TotalFailures);
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup, cleanupCt) =>
+                await DeleteTestRowsAsync(cleanup, cleanupCt));
+        }
+    }
+
+    /// <summary>
+    /// One server's batch for one pass: <paramref name="rows"/> ordinary rows plus, when
+    /// <paramref name="forcedFailures"/> is given, one forced plan carrying that <c>force_failure_count</c>, so
+    /// the caller can make the newest two sightings differ by exactly one. Written as a single multi-row INSERT
+    /// so the batch lands as one contiguous run of heap pages, the way the collector's COPY does.
+    /// </summary>
+    private static async Task SeedPassAsync(NpgsqlConnection connection, int serverId, DateTime collectionTime, int rows, long? forcedFailures, CancellationToken ct)
+    {
+        var sql = new StringBuilder(
+            "INSERT INTO collect.query_store_stats (collection_id, collection_time, server_id, server_name, database_name, query_id, plan_id, " +
+            "execution_count, avg_duration_us, plan_forcing_type, is_forced_plan, force_failure_count, last_force_failure_reason) " +
+            "SELECT $1, $2, $3, $4, 'db_' || (g % 4), 1000 + g, (1000 + g) * 10, 10 + g, 500 + g, 'NONE', FALSE, 0, NULL " +
+            "FROM generate_series(1, $5) AS g");
+        using (var insert = new NpgsqlCommand(sql.ToString(), connection))
+        {
+            insert.Parameters.AddWithValue(1L);
+            insert.Parameters.AddWithValue(collectionTime);
+            insert.Parameters.AddWithValue(serverId);
+            insert.Parameters.AddWithValue(TestServerName);
+            insert.Parameters.AddWithValue(rows);
+            await insert.ExecuteNonQueryAsync(ct);
+        }
+
+        if (forcedFailures is null)
+        {
+            return;
+        }
+
+        using var forced = new NpgsqlCommand(
+            "INSERT INTO collect.query_store_stats (collection_id, collection_time, server_id, server_name, database_name, query_id, plan_id, " +
+            "execution_count, avg_duration_us, plan_forcing_type, is_forced_plan, force_failure_count, last_force_failure_reason) " +
+            "VALUES ($1, $2, $3, $4, 'ForcedDb', 1, 10, 5, 900, 'MANUAL', TRUE, $5, 'GENERAL_FAILURE')", connection);
+        forced.Parameters.AddWithValue(1L);
+        forced.Parameters.AddWithValue(collectionTime);
+        forced.Parameters.AddWithValue(serverId);
+        forced.Parameters.AddWithValue(TestServerName);
+        forced.Parameters.AddWithValue(forcedFailures.Value);
+        await forced.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task<string> ExplainShippedReadAsync(NpgsqlConnection connection, DateTime windowStart, CancellationToken ct)
+    {
+        using var explain = new NpgsqlCommand("EXPLAIN (COSTS OFF) " + DarlingAlertReadAdapter.ForcePlanFailuresSql, connection);
+        explain.Parameters.AddWithValue(TargetServerId);
+        explain.Parameters.AddWithValue(DateTime.SpecifyKind(windowStart, DateTimeKind.Unspecified));
+        var plan = new StringBuilder();
+        using var reader = await explain.ExecuteReaderAsync(ct);
+        while (await reader.ReadAsync(ct))
+        {
+            plan.AppendLine(reader.GetString(0));
+        }
+
+        return plan.ToString();
+    }
+
+    private static async Task DeleteTestRowsAsync(NpgsqlConnection connection, CancellationToken ct)
+    {
+        var ids = string.Join(", ", OtherServerIds.Append(TargetServerId).Select(id => id.ToString(CultureInfo.InvariantCulture)));
+        using var cleanup = new NpgsqlCommand($"DELETE FROM collect.query_store_stats WHERE server_id IN ({ids})", connection);
+        await cleanup.ExecuteNonQueryAsync(ct);
+    }
+}

Darling/Darling.Tests/PgTableTuningTests.cs

@@ -17,7 +17,9 @@ namespace Darling.Tests;
 /// Pins the composer performance-tuning statements (covering indexes + per-table autovacuum-insert override,
 /// Erik's EXPLAIN-backed field fix) so the tested SQL can never silently drift. These are applied as idempotent
 /// RUNTIME setup (<see cref="PgTableTuning"/>), NOT a versioned migration, so they do not bump StorageVersion or
-/// gate the Viewer — the reason there is no schema-version change to pin here.
+/// gate the Viewer — the reason there is no schema-version change to pin here. The #3573 alerting-read covering
+/// index rides the same list for the same reason: results-invariant, so the Viewer's connect gate must not
+/// learn about it.
 /// </summary>
 public sealed class PgTableTuningTests
 {
@@ -39,8 +41,16 @@ LATERAL probes (server_id, sql_handle, newest-first — bounded by raw retention
         Assert.Contains("idx_query_stats_server_hash_time ON collect.query_stats (server_id, query_hash, collection_time DESC)", sql, StringComparison.Ordinal);
         Assert.Contains("idx_query_store_stats_server_db_query_plan_time ON collect.query_store_stats (server_id, database_name, query_id, plan_id, collection_time DESC)", sql, StringComparison.Ordinal);
 
+        /* #3573: the alerting pass's forced-plan-failures read gets the fourth COVERING index — the read's
+           (server_id, collection_time) predicate as the key and every other column it touches as INCLUDE, so
+           it runs as an Index Only Scan. The plain (server_id, collection_time) composite V1 already generates
+           was measured on the production store being priced out by the planner in favour of streaming the
+           fleet's whole two-hour slice; covering is what removes the heap component the cost model mispriced.
+           The column list is pinned against the read itself in ForcePlanFailuresAccessPathTests. */
+        Assert.Contains("idx_query_store_stats_server_time_forcing ON collect.query_store_stats (server_id, collection_time DESC) INCLUDE (database_name, query_id, plan_id, force_failure_count, is_forced_plan, plan_forcing_type, last_force_failure_reason)", sql, StringComparison.Ordinal);
+
         /* Every index is idempotent (no-op where a field box already hand-applied it, or a prior start made it). */
-        Assert.Equal(7, CountOccurrences(sql, "CREATE INDEX IF NOT EXISTS"));   /* +1: the #1981 handle index */
+        Assert.Equal(8, CountOccurrences(sql, "CREATE INDEX IF NOT EXISTS"));   /* +1: the #1981 handle index; +1: the #3573 forced-plan covering index */
         Assert.DoesNotContain("CREATE INDEX ON", sql, StringComparison.Ordinal);
 
         /* Per-table autovacuum-insert override on exactly the FOUR high-rate insert tables (NOT a global GUC
@@ -59,7 +69,7 @@ differ by one word and the wrong one would be silently inert. */
         Assert.Contains("ALTER TABLE collect.query_plan_dim SET (autovacuum_vacuum_scale_factor = 0.02, autovacuum_vacuum_threshold = 10000)", sql, StringComparison.Ordinal);
         Assert.DoesNotContain("collect.query_plan_dim SET (autovacuum_vacuum_insert_scale_factor", sql, StringComparison.Ordinal);
 
-        Assert.Equal(12, PgTableTuning.Statements.Count);   /* +1 #1981 query_stats handle index, +1 pg_statement_stats, +1 #2402 query_plan_dim */
+        Assert.Equal(13, PgTableTuning.Statements.Count);   /* +1 #1981 query_stats handle index, +1 pg_statement_stats, +1 #2402 query_plan_dim, +1 #3573 forced-plan covering index */
     }
 
     /// <summary>