From f60817204e86c5900e3d53cc5a9a39d0f4677f35 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:44:18 -0400 Subject: [PATCH] Retention: re-hold a policy whose coverage list grew, on a measured shortfall only The arming gate could only ARM. add_retention_policy(if_not_exists => true) returns -1 for a policy the store already has, so nothing paused one whose coverage list had GROWN under it -- and #1869 was the first build to hand a new consumer to a gate stores had already armed. Such a store logged HELD PAUSED for the interval layer on every start and kept purging it anyway, capping how deep the new day-grain daily could ever be backfilled. The probe now answers Covered / Short / Unknown instead of a bool, and only a positive Short measurement may stop a running policy. Unknown -- a throw, a timeout, an empty result -- leaves the policy exactly as it was: a new one stays paused as before, an armed one keeps purging. That is what makes this shippable where "unsafe implies disarm" was not: the probe is deliberately fail-closed, so a disarm-capable gate reading the old bool would let one bad probe stop purging across every tier and grow disk without bound. A consumer that exists and holds no rows is a measurement, not an unknown, which is exactly the state a newly-added consumer is born in. Release is the existing arming path, unchanged, so this is a gate and not a latch. IsRawTierDropSafeAsync collapses Short and Unknown to "not safe" exactly as before, keeping the #1793 shared-predicate property -- and for the three raw tiers this removes a case where the two purge paths could already disagree, since an armed policy used to keep dropping chunks the catalog sweep was refusing to touch. Proved live on PostgreSQL 18.4 / TimescaleDB 2.28.1: a store with L1 armed and every consumer caught up, handed a rebuilt empty consumer, re-holds on the next sweep and re-arms itself once the consumer is backfilled; and a genuinely missing relation neither disarms the armed policy nor changes any other policy's verdict in the same sweep. Filed rather than folded in: #1905 (the horizon ordering is asserted by hand, not derived from the policy list) and #1906 (a re-hold reads the same as a first-time hold in the log). Closes #1877 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 10 +- .../QueryStoreCorrectedRollupLiveTests.cs | 248 ++++++++++++++++++ .../TimescaleContinuousAggregateTests.cs | 43 +++ .../TimescaleSupport.cs | 156 ++++++++--- 4 files changed, 425 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f63991b98b..65506ec380 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A retention policy whose coverage list GROWS is now re-held, instead of purging past the new tier forever** ([#1877]) - the arming gate creates every retention policy paused and arms it only once every rollup beneath it already reaches as far back as the source does. It could only ever ARM. `add_retention_policy(if_not_exists => true)` returns -1 for a policy the store already has, so nothing paused one whose COVERAGE LIST had grown under it - and [#1869] was the first build to hand a new consumer to a gate that stores had already armed. Such a store logged `HELD PAUSED` for the interval layer on every start and kept purging it anyway. Nothing a read could reach was lost, because every window older than the new tier's floor routes to a tier that holds it; what was lost is reachable DEPTH, since the new day-grain daily could then only ever be backfilled as deep as the 7-day interval layer still held rather than as deep as the store's own history. + + **The fix is a three-valued coverage verdict, and the third value is the whole of it.** The obvious form - "unsafe implies disarm" - is materially worse than the gap it closes, because the probe is deliberately fail-closed: any exception, timeout or transient failure answers "not safe". Under an arm-only gate that costs nothing, but a disarm-capable gate reading the same bool would let one bad probe on a busy store stop purging across every tier at once, and disk would grow until somebody noticed - trading a bounded depth cap for an unbounded disk risk. The probe now answers COVERED, SHORT or UNKNOWN, and only a positive SHORT measurement may stop a policy that is already running. UNKNOWN leaves the policy in whatever state it is already in: a newly created one stays paused exactly as before, and one this store already armed keeps purging. A consumer that exists and holds no rows is a SHORT measurement rather than an unknown - which is precisely the state a newly-added consumer is born in, and is what makes the re-hold fire on the upgrade it was written for. + + **Release is the existing arming path, unchanged, so this is a gate and not a latch.** A re-held policy arms itself on the first start after `--backfill-rollups` carries the new consumer past the horizon, with no manual step, exactly as a first-time hold has always released. The catalog sweep's own drop still reads the same verdict through the same map, so the two purge paths cannot judge one drop differently ([#1784]) - and for the three raw tiers this removes a case where they already could, since an armed policy used to keep dropping the very chunks the sweep was refusing to touch. The per-policy warning now names the specific tier that is short instead of the whole coverage list, and the startup summary reports how many policies were left as-is because their coverage could not be read at all. Two narrower things are filed rather than folded in: the ordering this now depends on is still asserted by hand rather than derived from the policy list, so a future policy on a daily tier could reintroduce the risk from the other side ([#1905]), and a re-hold still reads identically to a first-time hold in the log even though only one of them stopped something that was running ([#1906]). + - **A cross-database lock now names the database the contended object actually lives in, from both blocking collectors** ([#1893]) - [#1876] made the two collectors' `contentious_object` labels agree at the incident identity, and corrected the `blocked_process_report` sentinel to name the lock RESOURCE's database rather than the blocked session's. The DMV snapshot side could not follow: its normalization runs in C# against a stored row whose only database name is `database_name`, which the collector writes as the blocked SESSION's database. A cross-database lock is held in one database by a session running in another, so for exactly that case the two sides still disagreed - the report row said `database: StackOverflow2013` where the snapshot row said `database: master` - and one contended object still raised two alerts. **The resource's database id comes from `sys.dm_os_waiting_tasks.resource_description`, not from the wait resource.** MS Learn documents every lock resource type's description as carrying a `dbid=` token - `keylock`, `pagelock`, `ridlock`, `objectlock`, `databaselock`, `filelock`, `extentlock`, `applicationlock`, `metadatalock`, `hobtlock` and `allocunitlock` - so a single parse covers every lock shape, where splitting `wait_resource` positionally would need a branch per resource type and would still miss the ones whose layout differs. `DB_NAME()` then runs in the same query, which is the whole reason this had to move server-side: the id was always sitting in the row, but nothing downstream could turn it into a name. Verified live on SQL Server 2022 against a genuine cross-database KEY lock, whose description reads `keylock hobtid=72057594045726720 dbid=11 id=lock... mode=X associatedObjectId=...`; the same blocking pair produced `Unresolved: key lock, database: pm1893_res` from both collectors, byte for byte, where before the fix the snapshot row carried the raw `KEY: 11:72057594045726720 (8194443284a0)` and could only ever have been named with the session's database. @@ -111,7 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The Query Store DAILY numbers stopped over-counting by roughly 2x - the tier that is kept indefinitely is now the accurate one** ([#1869]) - the corrected rollups above dedup each Query Store interval within a collection HOUR, so an interval whose snapshots straddle an hour boundary produced two rows, each holding a cumulative value, and the daily sum counted it once per hour it was collected in rather than once. That is irreducible at the hourly grain - an interval genuinely collected across two hours has to appear in both - but not at the daily grain, and the daily tier is the one kept forever, so a permanent 2x there is a permanent lie. Two more continuous aggregates remove it: `query_store_stats_interval_daily` re-dedups the interval across the whole DAY with `last(execution_count, bucket)`, and `query_store_stats_daygrain_daily` collapses that to the same composer columns the other dailies carry, so every composed panel reads it with no change. Measured on a seeded store with the shape [#1869] published: a day whose true total is **515** reads **1,013** from the hour-grain daily - **1.97x** - and the new one reads 515. - **What it does NOT fix, because a mis-count is worth stating exactly.** An interval whose snapshots straddle MIDNIGHT is still counted once per collection DAY. It is the identical argument one grain up and equally irreducible there, but it is far smaller and the difference is structural rather than lucky: the collector fetches an interval while its `last_execution_time` keeps advancing, so a 60-minute interval is collected across an hour boundary almost always and a day boundary about once per 24 intervals - roughly a 4% over-count against the 97% removed. It is pinned by a test against a real store so the claim cannot quietly drift, and filed with the cost of the fifth aggregate that would shrink it at [#1879], along with the reason no finite dedup grain removes it outright. One upgrade note is filed the same way at [#1877]: the purge gate is arm-only, so a store whose interval-layer policy was already armed under [#1849] keeps purging while the new level catches up - nothing a read can reach is lost, but running `--backfill-rollups` promptly after upgrading is what lets the new daily reach back past 7 days. + **What it does NOT fix, because a mis-count is worth stating exactly.** An interval whose snapshots straddle MIDNIGHT is still counted once per collection DAY. It is the identical argument one grain up and equally irreducible there, but it is far smaller and the difference is structural rather than lucky: the collector fetches an interval while its `last_execution_time` keeps advancing, so a 60-minute interval is collected across an hour boundary almost always and a day boundary about once per 24 intervals - roughly a 4% over-count against the 97% removed. It is pinned by a test against a real store so the claim cannot quietly drift, and filed with the cost of the fifth aggregate that would shrink it at [#1879], along with the reason no finite dedup grain removes it outright. One upgrade note was filed the same way at [#1877] and is FIXED below in this same release: the purge gate could only arm, so a store whose interval-layer policy was already armed under [#1849] kept purging while the new level caught up. It is now re-held until the new level covers it, and running `--backfill-rollups` promptly after upgrading is what releases it. **Three dailies now, best-first, each rung the same rule.** The day-grain daily is a NEW aggregate that starts empty, so it wins only where it has actually materialized the window; older windows fall to the corrected daily, and older ones still to the superseded original - each step comparative, so a store that has been backfilled never silently reads the coarser number. The hourly tier is untouched: there is no better hourly to route to. @@ -2154,3 +2160,5 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1873]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1873 [#1896]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1896 [#1898]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1898 +[#1905]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1905 +[#1906]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1906 diff --git a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs index da89b03514..c3b116be71 100644 --- a/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs +++ b/Darling/Darling.Tests/QueryStoreCorrectedRollupLiveTests.cs @@ -458,6 +458,227 @@ the removal lands. Shallow is the real precondition and it is what the gate itse "all three consumers now cover L1, so the held purge should have armed itself on this sweep."); } + /// + /// #1877, the case the gate could not reach: a policy this store ALREADY ARMED, whose coverage list then + /// GREW a consumer. RED under an arm-only gate at step 3. + /// + /// The test above proves the gate holds a policy it is creating. This one starts from the opposite + /// state — L1's purge armed and running, every consumer caught up, exactly what a store on the #1849 build + /// looks like — and then does what #1869 did to it: hands it a new consumer that holds nothing. + /// add_retention_policy(if_not_exists => true) returns -1 for the policy that already exists, so + /// nothing pauses it, and under the old code L1 kept purging its own source while the new consumer could + /// only ever be backfilled as deep as what survived. + /// + /// The consumer is added by DROPping and rebuilding it (WITH NO DATA) rather than by editing the + /// coverage list, because that is what an upgrading store experiences: the relation appears, born empty, + /// under a policy that is already armed. Dropping it CASCADEs to the day-grain daily that reads it, and the + /// ensure sweep rebuilds both — so this is the real #1869 upgrade shape, not a simulation of it. + /// + /// WATCHED (mutation): make the Short branch of the sweep skip its hold statement — i.e. put + /// the gate back to arm-only — and step 3 goes red while every other assertion here still passes. + /// + [Fact] + public async Task ArmedIntervalPurge_IsReHeldWhenItsCoverageListGainsAnEmptyConsumer_ThenReleasesOnBackfill() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #1877 re-hold test (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(connection, null, ct)); + await TimescaleSupport.ConvertToHypertablesAsync(connection, null, ct); + + var now = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified); + var oldest = now.Date.AddDays(-9); + var span = (From: oldest.AddDays(-1), To: now.AddDays(1)); + + for (var offset = 0; oldest.AddHours(offset) < now; offset += 3) + { + await SeedIntervalAsync(connection, oldest.AddHours(offset), intervalId: 4000 + offset, queryId: 17, planId: 19, + firstExecution: oldest.AddHours(offset), snapshots: 3, secondsApart: 120, + avgDurationUs: 100, avgCpuUs: 40, ct: ct); + } + + await EnsureAggregatesWithoutRefreshPoliciesAsync(connection, ct); + + /* ── 1. A HEALTHY, FULLY-COVERED STORE. Every consumer materialized over everything L1 holds. ── */ + foreach (var view in new[] + { + TimescaleSupport.QueryStoreStatsIntervalHourlyView, + TimescaleSupport.QueryStoreStatsCorrectedHourlyView, + TimescaleSupport.QueryStoreStatsCorrectedDailyView, + TimescaleSupport.QueryStoreStatsIntervalDailyView, + TimescaleSupport.QueryStoreStatsDayGrainDailyView, + }) + { + await RefreshRangeAsync(connection, view, span.From, span.To, ct); + } + + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + Assert.True(await IsArmedAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct), + "L1's purge must be ARMED here or the test is not starting from the state #1877 is about — an " + + "already-armed policy is the only thing the arm-only gate could not re-hold."); + + /* ── 2. THE UPGRADE. The coverage list gains a consumer, born empty, exactly as #1869 delivered one to + stores already running #1849. The policy is untouched and stays armed through this. ── */ + await using (var drop = new NpgsqlCommand( + $"DROP MATERIALIZED VIEW collect.{TimescaleSupport.QueryStoreStatsIntervalDailyView} CASCADE", connection)) + { + await drop.ExecuteNonQueryAsync(ct); + } + + await EnsureAggregatesWithoutRefreshPoliciesAsync(connection, ct); + + var l1Floor = await FloorAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct); + Assert.NotNull(l1Floor); + + /* Shallow-or-empty rather than empty: the rebuild re-attaches a refresh policy whose first check runs + IMMEDIATELY (#1564), so under suite load the recent window can materialize before the strip lands. */ + var newConsumerFloor = await FloorAsync(connection, TimescaleSupport.QueryStoreStatsIntervalDailyView, ct); + Assert.True(newConsumerFloor is null || newConsumerFloor > l1Floor, + $"the rebuilt consumer should not already cover L1, or there is no coverage gap to re-hold on " + + $"(new consumer {newConsumerFloor:O}, L1 {l1Floor:O})."); + + /* ── 3. RE-HELD. The property #1877 exists for: a running purge is STOPPED once its own coverage list + no longer reaches, rather than left arming-only and quietly outrunning the new tier. ── */ + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + Assert.False(await IsArmedAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct), + "L1's purge is still armed while its newly-added consumer holds nothing — the gate is arm-only, so " + + "it keeps dropping the only interval-grain copy of history that consumer has never seen (#1877)."); + + /* ── 4. SELF-RELEASE, through the existing arming path and with no manual step — the same way a + first-time hold releases. This is what makes the re-hold safe to ship: it is not a latch. ── */ + await RefreshRangeAsync(connection, TimescaleSupport.QueryStoreStatsIntervalDailyView, span.From, span.To, ct); + + var backfilledFloor = await FloorAsync(connection, TimescaleSupport.QueryStoreStatsIntervalDailyView, ct); + Assert.NotNull(backfilledFloor); + Assert.True(backfilledFloor <= l1Floor, + $"the backfill must carry the new consumer past L1's oldest bucket before the gate can release " + + $"(consumer {backfilledFloor:O}, L1 {l1Floor:O})"); + + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + Assert.True(await IsArmedAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct), + "the new consumer now covers L1, so the re-held purge should have armed itself on this sweep."); + } + + /// + /// The property that makes #1877's re-hold shippable at all: a probe that FAILS is not a coverage + /// regression. #1877 filed the naive fix — "unsafe implies disarm" — as materially worse than the gap, + /// because the coverage probe is deliberately fail-closed and one bad probe on a busy store would have + /// stopped purging across every tier at once, growing disk without bound. + /// + /// Driven with a genuinely missing relation, which is the strongest available stand-in for the + /// timeout or permission blip that motivates the rule: the probe's statement throws 42P01 rather than + /// returning a measurement, and there is no way for the gate to tell that apart from any other failure. + /// + /// Three separate things are asserted, because a hold-nothing implementation would pass the first + /// alone: the armed policy whose probe threw stays ARMED, a DIFFERENT policy whose probe succeeded is still + /// judged on its own merits in the same sweep, and a policy that is NOT yet armed when its probe throws + /// stays PAUSED — the original fail-closed direction, which the tristate must not have inverted. + /// + /// WATCHED (mutation): return Short instead of Unknown from the probe's catch and the + /// first assertion goes red — that mutation is precisely the fleet-wide purge stop #1877 refused to ship. + /// + [Fact] + public async Task RetentionSweep_TreatsAFailedProbeAsUnknown_NeverAsACoverageRegression() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), + "Set DARLING_TEST_PG to a Postgres connection string (with TimescaleDB installed) to run the live #1877 fail-closed test (it mints its own scratch database)."); + + var ct = TestContext.Current.CancellationToken; + + await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + + Assert.True(await TimescaleSupport.TryEnableAsync(connection, null, ct)); + await TimescaleSupport.ConvertToHypertablesAsync(connection, null, ct); + + var now = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified); + var oldest = now.Date.AddDays(-9); + var span = (From: oldest.AddDays(-1), To: now.AddDays(1)); + + for (var offset = 0; oldest.AddHours(offset) < now; offset += 3) + { + await SeedIntervalAsync(connection, oldest.AddHours(offset), intervalId: 5000 + offset, queryId: 23, planId: 29, + firstExecution: oldest.AddHours(offset), snapshots: 3, secondsApart: 120, + avgDurationUs: 100, avgCpuUs: 40, ct: ct); + } + + /* The second subject: a raw tier on a completely different rollup family, so "one bad probe does not + stop the others" is a claim about independent policies rather than about one relation twice. */ + await SeedQueryStatsAsync(connection, oldest, now, ct); + + await EnsureAggregatesWithoutRefreshPoliciesAsync(connection, ct); + + foreach (var view in new[] + { + TimescaleSupport.QueryStoreStatsIntervalHourlyView, + TimescaleSupport.QueryStoreStatsCorrectedHourlyView, + TimescaleSupport.QueryStoreStatsCorrectedDailyView, + TimescaleSupport.QueryStoreStatsIntervalDailyView, + TimescaleSupport.QueryStoreStatsDayGrainDailyView, + TimescaleSupport.QueryStatsHourlyView, + }) + { + await RefreshRangeAsync(connection, view, span.From, span.To, ct); + } + + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + Assert.True(await IsArmedAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct), + "L1's purge must be armed before the probe is broken, or there is nothing for a failed probe to " + + "wrongly disarm."); + Assert.True(await IsArmedAsync(connection, "query_stats", ct), + "raw query_stats' purge must be armed here so the isolation assertion below means something."); + + /* THE BREAK: L1's third consumer stops existing, so its probe throws instead of measuring. */ + await using (var drop = new NpgsqlCommand( + $"DROP MATERIALIZED VIEW collect.{TimescaleSupport.QueryStoreStatsIntervalDailyView} CASCADE", connection)) + { + await drop.ExecuteNonQueryAsync(ct); + } + + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + + Assert.True(await IsArmedAsync(connection, TimescaleSupport.QueryStoreStatsIntervalHourlyView, ct), + "a FAILED coverage probe disarmed a running purge — an unreadable store is not evidence that " + + "coverage regressed, and treating it as one stops retention fleet-wide on one bad probe (#1877)."); + Assert.True(await IsArmedAsync(connection, "query_stats", ct), + "one relation's failed probe changed a DIFFERENT policy's verdict — the per-policy judgment is " + + "supposed to be independent."); + + /* And the original direction, unchanged: an unmeasurable store may not ARM either. Re-created from + scratch (its policy removed, its consumer dropped) so the sweep meets it as a new policy. */ + await using (var removePolicy = new NpgsqlCommand( + "SELECT remove_retention_policy('collect.query_stats', if_exists => true)", connection)) + { + await removePolicy.ExecuteNonQueryAsync(ct); + } + + await using (var drop = new NpgsqlCommand( + $"DROP MATERIALIZED VIEW collect.{TimescaleSupport.QueryStatsHourlyView} CASCADE", connection)) + { + await drop.ExecuteNonQueryAsync(ct); + } + + Assert.False(await TimescaleSupport.IsRawTierDropSafeAsync(connection, "query_stats", ct), + "an unreadable coverage state must still answer 'not safe to drop' — the catalog sweep and the " + + "tiered policy have to keep judging the same drop the same way (#1793)."); + + await TimescaleSupport.EnsureRetentionPoliciesAsync(connection, null, ct); + Assert.False(await IsArmedAsync(connection, "query_stats", ct), + "a policy whose coverage could not be established ARMED itself — the fail-closed direction the " + + "tristate had to preserve."); + } + /* ─────────────────────────── seeding + reading helpers ─────────────────────────── */ /// @@ -503,6 +724,33 @@ INSERT INTO collect.query_store_stats await command.ExecuteNonQueryAsync(ct); } + /// + /// Plants plain query_stats rows, one per hour — the second, independent raw tier the fail-closed + /// test needs so "one relation's broken probe did not change another policy's verdict" is a claim about two + /// unrelated rollup families rather than about one relation examined twice. + /// + private static async Task SeedQueryStatsAsync( + NpgsqlConnection connection, DateTime from, DateTime to, CancellationToken ct) + { + await using var insert = new NpgsqlCommand(@" +INSERT INTO collect.query_stats + (collection_id, collection_time, server_id, server_name, database_name, query_hash, sql_handle, + delta_worker_time, delta_elapsed_time, delta_execution_count) +SELECT + extract(epoch FROM g)::bigint * 100, + g, + $3, 'SQL01', 'AdventureWorks', + decode(md5('pm1877'), 'hex'), + decode(md5('pm1877handle'), 'hex'), + 1000, 2000, 10 +FROM generate_series($1::timestamp, $2::timestamp, INTERVAL '1 hour') AS g", connection); + + insert.Parameters.AddWithValue(from); + insert.Parameters.AddWithValue(to); + insert.Parameters.AddWithValue(TestServerId); + await insert.ExecuteNonQueryAsync(ct); + } + /// /// Plants one interval as EXPLICIT (collection time, cumulative count) snapshots, rather than /// 's evenly-spaced 1..n run. diff --git a/Darling/Darling.Tests/TimescaleContinuousAggregateTests.cs b/Darling/Darling.Tests/TimescaleContinuousAggregateTests.cs index acdf34ea20..97ff6c0d7f 100644 --- a/Darling/Darling.Tests/TimescaleContinuousAggregateTests.cs +++ b/Darling/Darling.Tests/TimescaleContinuousAggregateTests.cs @@ -365,6 +365,39 @@ public void ArmRetentionPolicy_TargetsExactlyOneRelationsRetentionJob() Assert.Contains("hypertable_schema = 'collect'", sql, StringComparison.Ordinal); } + /// + /// #1877: holding is the exact mirror of arming — same targeting, opposite flag — because a policy whose + /// coverage list GREW under it has to be stopped, and add_retention_policy(if_not_exists) never + /// pauses a policy the store already has. + /// + /// The two statements are asserted to disagree on the flag and agree on everything else. A hold that + /// missed the hypertable filter would stop every retention policy in the store on one shallow consumer; + /// one that said true would arm the very policy the gate just judged unsafe, which is the data-loss + /// direction and would look identical in a test that only checked the targeting. + /// + [Fact] + public void HoldRetentionPolicy_IsTheMirrorOfArming_SameTargetOppositeFlag() + { + var hold = TimescaleSupport.HoldRetentionPolicySql("query_stats"); + var arm = TimescaleSupport.ArmRetentionPolicySql("query_stats"); + + Assert.Contains("scheduled => false", hold, StringComparison.Ordinal); + Assert.DoesNotContain("scheduled => true", hold, StringComparison.Ordinal); + + foreach (var filter in new[] + { + "proc_name = 'policy_retention'", + "hypertable_schema = 'collect'", + "hypertable_name = 'query_stats'", + }) + { + Assert.Contains(filter, hold, StringComparison.Ordinal); + } + + /* Identical apart from the flag - the pair is one statement with a boolean, so they cannot drift. */ + Assert.Equal(arm, hold.Replace("scheduled => false", "scheduled => true", StringComparison.Ordinal)); + } + /// /// The safety probe compares the source's oldest row against the coverage tier's oldest bucket. Raw tables /// key on collection_time, rollups on bucket - reading the wrong column would compare against nothing and @@ -512,11 +545,21 @@ column sets one shared value mapper then mis-reads. */ /// raw (4d) < L1 (7d) < the interval-grain daily (10d), and the two composer-grain dailies are kept /// indefinitely. Written as one chain because that is the invariant — the individual numbers are tunable, /// their order is not. + /// + /// #1877 raised the stakes on this ordering. Under the old arm-only gate, inverting a pair cost + /// an arming failure: the source's policy simply never armed and that tier grew. Now that a positively + /// measured shortfall RE-HOLDS a running policy, an inverted pair would stop a purge that is already armed + /// on a perfectly healthy store, and it would not self-release. Steady state is safe by construction — + /// every consumer here outlives its source AND materialization chunks are never finer than their source's + /// (measured at 10 days against the raw tables' 1, on TimescaleDB 2.28.1), so a consumer always retains at + /// least as far back as its source. Both halves of that are what this ordering has to keep true. /// [Fact] public void RetentionHorizons_RiseDownTheQueryStoreDedupChain_SoNoGateWaitsOnSomethingAlreadyDropped() { Assert.True(TimescaleSupport.IntervalRetentionSpan > TimescaleSupport.RawRetentionSpan); + Assert.True(TimescaleSupport.HourlyRetentionSpan > TimescaleSupport.RawRetentionSpan, + "the hourly rollups gate raw's purge too, so they must outlive raw for the same reason L1 does."); Assert.True(TimescaleSupport.IntervalDailyRetentionSpan > TimescaleSupport.IntervalRetentionSpan, "the interval-grain DAILY layer must outlive the hourly one whose purge is gated on it, or L1's " + "policy can never arm and L1 grows without bound."); diff --git a/Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs b/Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs index c44be47e5a..d0c077d0e0 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/TimescaleSupport.cs @@ -742,7 +742,7 @@ public static string BaselineRelationExistsSql(string view) /// it is unconditionally true — so the gate skips exactly the tiered stores it exists for, and fires only /// on stores that do not need it. BaselineSupplyTests pins this direction. /// - /// This is the same predicate shape the retention arming uses (IsSafeToArmRetentionAsync) over + /// This is the same predicate shape the retention arming uses (MeasureRetentionCoverageAsync) over /// the same pair of relations, deliberately: what we backfill and what unblocks arming cannot be allowed /// to drift apart. /// @@ -1666,7 +1666,8 @@ concluding it was not a bug. A summary that quotes its own source cannot drift f /// twin of ; pinned equal, and /// pinned STRICTLY GREATER than , by TimescaleSupportTests — a - /// consumer that expired before its source would hold its source's purge forever. + /// consumer that expired before its source would hold its source's purge forever, and since #1877 would + /// also STOP one that is already running, on a healthy store, without self-releasing. public static readonly TimeSpan IntervalDailyRetentionSpan = TimeSpan.FromDays(10); /// twin of ; pinned equal by @@ -1708,7 +1709,28 @@ public static string AddRetentionPolicySql(string relation, string dropAfter) /// policy's first check IMMEDIATELY at creation, not on its next interval (#1680). /// public static string ArmRetentionPolicySql(string relation) - => $@"SELECT alter_job(j.job_id, scheduled => true) + => SetRetentionScheduleSql(relation, scheduled: true); + + /// + /// Re-holds a retention policy that is ALREADY ARMED (#1877). The mirror of + /// , and the statement that closes the arm-only gap: a policy created + /// paused stays paused by itself, but add_retention_policy(if_not_exists => true) returns -1 for a + /// policy this store already has, so nothing ever paused one whose COVERAGE LIST GREW under it. + /// + /// Reached ONLY from a positive coverage measurement — never from an indeterminate one. See + /// RetentionCoverage for why that distinction is the whole of #1877. + /// + public static string HoldRetentionPolicySql(string relation) + => SetRetentionScheduleSql(relation, scheduled: false); + + /// + /// The shared body of and : flip one + /// relation's retention job. Filtering by proc_name AND the hypertable is what keeps it from arming — or + /// stopping — some other policy, or every policy, by accident. Idempotent in both directions: setting a job + /// to the state it is already in is a no-op, which is what lets the sweep re-assert the verdict every start. + /// + private static string SetRetentionScheduleSql(string relation, bool scheduled) + => $@"SELECT alter_job(j.job_id, scheduled => {(scheduled ? "true" : "false")}) FROM timescaledb_information.jobs AS j WHERE j.proc_name = 'policy_retention' AND j.hypertable_schema = 'collect' @@ -1726,7 +1748,7 @@ FROM timescaledb_information.jobs AS j /// Plural since #1849, and the plurality is the point. query_store_stats now feeds TWO /// rollup families — the original inflated pair and the corrected one — and a purge that satisfied only one /// of them would destroy raw history the other has never materialized. The verdict is therefore an AND over - /// all of them, evaluated in rather than folded into SQL: + /// all of them, evaluated in rather than folded into SQL: /// GREATEST would have expressed it in one column and is exactly wrong here, because it SKIPS NULLs. /// An empty new rollup would vanish from the comparison and the gate would pass on the old rollup alone — /// which is the whole failure this exists to prevent. @@ -1806,7 +1828,13 @@ public static async Task IsRawTierDropSafeAsync( { if (string.Equals(tierRelation, relation, StringComparison.Ordinal)) { - return await IsSafeToArmRetentionAsync(connection, tierRelation, timeColumn, coverage, cancellationToken); + var (verdict, _) = await MeasureRetentionCoverageAsync(connection, tierRelation, timeColumn, coverage, cancellationToken); + + /* Only a POSITIVE all-clear permits a drop. Short and Unknown both answer "no", exactly as they + did when this probe was a bool — the tristate exists for the ARMING side, which alone needs + to tell a measured regression apart from a failed measurement (#1877). Collapsing it here + keeps the #1793 property intact: both purge paths still judge the same drop identically. */ + return verdict == RetentionCoverage.Covered; } } @@ -1814,17 +1842,43 @@ public static async Task IsRawTierDropSafeAsync( } /// - /// True when arming 's retention policy cannot drop anything ANY tier below it has - /// not already captured (#1680): the source is empty, or EVERY relation in - /// reaches at least as far back as the source does. Any failure to - /// determine this returns FALSE - an unknown coverage state must leave the policy paused, never armed on an - /// assumption. + /// What a coverage probe was able to CONCLUDE — the distinction #1877 turns on. + /// + /// Arming needs only "safe or not", and this was a bool for that reason. Re-holding needs more: an + /// already-armed policy may be stopped on evidence that its coverage genuinely fell short, and must NEVER be + /// stopped because the evidence could not be gathered. Folding a failed probe in with a measured shortfall + /// is what made "unsafe implies disarm" unshippable — one bad probe on a busy store would have stopped + /// purging across every tier and grown disk until someone noticed, trading #1877's bounded depth cap for an + /// unbounded disk risk. + /// + private enum RetentionCoverage + { + /// Every named consumer positively reaches at least as far back as the source — or the source + /// is empty, so there is no history to lose. The only verdict that permits arming or dropping. + Covered, + + /// MEASURED short: the probe ran, the source holds rows, and a named consumer either holds none + /// or starts later than the source's oldest row. A fact about the store, not a failure to read it — and + /// the only verdict that may stop a policy this store already armed. + Short, + + /// Nothing could be concluded: the probe threw, timed out, or came back empty. Refuses arming + /// exactly as before, and refuses to re-hold, because a probe error is not a coverage regression. + Unknown, + } + + /// + /// How far 's consumers reach relative to what it holds (#1680): Covered + /// when the source is empty or EVERY relation in reaches at least as far + /// back as the source does, Short when one of them is measurably behind, Unknown when the + /// probe could not answer at all. Also returns the first consumer found short, so the operator warning can + /// name the tier to backfill rather than the whole list. /// /// The verdict is an AND across consumers (#1849), and the loop below is short-circuiting in the SAFE /// direction only: one empty or shallow consumer holds the policy even if every other consumer is complete. /// A raw table with two rollup families is covered when the LEAST-covering of them covers it. /// - private static async Task IsSafeToArmRetentionAsync( + private static async Task<(RetentionCoverage Verdict, string? ShortConsumer)> MeasureRetentionCoverageAsync( NpgsqlConnection connection, string relation, string sourceTimeColumn, IReadOnlyList coverageRelations, CancellationToken cancellationToken) { try @@ -1833,37 +1887,41 @@ private static async Task IsSafeToArmRetentionAsync( using var reader = await command.ExecuteReaderAsync(cancellationToken); if (!await reader.ReadAsync(cancellationToken)) { - return false; + return (RetentionCoverage.Unknown, null); } /* Nothing in the source - a fresh store. No history to lose, so arm. */ if (await reader.IsDBNullAsync(0, cancellationToken)) { - return true; + return (RetentionCoverage.Covered, null); } var sourceOldest = reader.GetDateTime(0); for (var i = 0; i < coverageRelations.Count; i++) { /* Source has data but THIS coverage tier is empty - arming would drop history it never - materialized, whatever the other tiers hold. */ + materialized, whatever the other tiers hold. An empty consumer is a MEASUREMENT and not an + unknown: the relation exists and answered with no rows, which is precisely the state a + newly-added consumer is born in on an upgrading store (#1877). */ if (await reader.IsDBNullAsync(i + 1, cancellationToken)) { - return false; + return (RetentionCoverage.Short, coverageRelations[i]); } if (reader.GetDateTime(i + 1) > sourceOldest) { - return false; + return (RetentionCoverage.Short, coverageRelations[i]); } } - return true; + return (RetentionCoverage.Covered, null); } catch (Exception) { - /* Fail closed: if coverage cannot be established, the policy stays paused. */ - return false; + /* Fail closed: if coverage cannot be established the policy is not armed — and, since #1877, not + re-held either. Both directions read the same way here: an unmeasurable store is left exactly as + it was, because nothing was learned about it. */ + return (RetentionCoverage.Unknown, null); } } @@ -1877,12 +1935,20 @@ private static async Task IsSafeToArmRetentionAsync( /// CAGGs the hourly policies target already exist. Returns the number of policies in place. /// /// COLD START ON AN EXISTING STORE (#1759): a store that already holds raw history older than its hourly - /// CAGG has materialized does NOT lose it. is fail-closed, so that - /// store's raw policies are created and left PAUSED, and the per-policy WARN says which rollup is short and - /// by how much. This used to be documented as a caveat prescribing a manual backfill "BEFORE this policy's + /// CAGG has materialized does NOT lose it. is fail-closed, so + /// that store's raw policies are created and left PAUSED, and the per-policy WARN says which rollup is + /// short. This used to be documented as a caveat prescribing a manual backfill "BEFORE this policy's /// first run" — a step no store ever received, and a defect rather than a caveat. The backfill is now a real /// operator verb (--backfill-rollups) with a disk preflight, and once it carries a rollup past the raw /// horizon this gate arms the held policy by itself on the next start, with no manual step. + /// + /// A COVERAGE LIST THAT GROWS (#1877). Holding is not only for policies this sweep just created. + /// add_retention_policy(if_not_exists => true) returns -1 for a policy the store already has, so + /// nothing pauses it — which is right for a restart (it must not undo an operator's backfill) and was wrong + /// for a build that ADDS a consumer to a gate stores have already armed, as #1869 did. Such a policy kept + /// purging its source while the new consumer held nothing, capping how deep that consumer could ever be + /// backfilled. It is now re-held, but ONLY on a positive measurement: see the three-valued + /// RetentionCoverage, which is what keeps a probe failure from stopping retention fleet-wide. /// public static async Task EnsureRetentionPoliciesAsync(NpgsqlConnection connection, ILogger? logger, CancellationToken cancellationToken = default) { @@ -1923,9 +1989,9 @@ L1 has THREE consumers. Two because the corrected daily is its SIBLING rather th missing — and the third is the load-bearing one on a store taking this build, because that store has a fully-caught-up L1 and an empty interval_daily, which is precisely the state where a gate reading only the older two would drop the only copy of history the day-grain daily has never - seen. NOTE that this gate is arm-only: a store whose L1 policy was ALREADY armed under #1849 - keeps purging while the new consumer catches up, which caps how deep the day-grain daily can - ever be backfilled rather than losing anything a read can reach — see #1877. + seen. That store's L1 policy was ALREADY ARMED under #1849, and until #1877 the gate could only + arm — so it kept purging while the new consumer held nothing, capping how deep the day-grain + daily could ever be backfilled. The sweep below now RE-HOLDS it on the measured shortfall. The corrected HOURLY is a leaf, so the leaf rule applies (#1757): its consumer is the composed READ, which routes past HourlyRouteMaxAge to the corrected DAILY — exactly the relationship the @@ -1949,6 +2015,7 @@ Both composer-grain dailies are kept indefinitely and get no policy. */ var applied = 0; var armed = 0; var held = 0; + var indeterminate = 0; foreach (var (relation, dropAfter, timeColumn, coverage) in policies) { @@ -1986,18 +2053,45 @@ what makes a restart converge instead of re-pausing a policy this store already applied++; - if (await IsSafeToArmRetentionAsync(connection, relation, timeColumn, coverage, cancellationToken)) + var (verdict, shortConsumer) = await MeasureRetentionCoverageAsync(connection, relation, timeColumn, coverage, cancellationToken); + if (verdict == RetentionCoverage.Covered) { using var arm = new NpgsqlCommand(ArmRetentionPolicySql(relation), connection) { CommandTimeout = SetupTimeoutSeconds }; await arm.ExecuteNonQueryAsync(cancellationToken); armed++; } - else + else if (verdict == RetentionCoverage.Short) { + /* HELD — and since #1877 that is an ACTION, not just the absence of arming. A policy this + store created moments ago is already paused and this re-asserts it; a policy the store + armed under an EARLIER build, whose coverage list has since GROWN a consumer, is stopped + here. That second case is the whole issue: if_not_exists returned -1 for the existing + policy so nothing paused it, and it kept purging its source while the new consumer held + nothing — capping how deep that consumer could ever be backfilled. + + Safe to do unconditionally because the verdict is a MEASUREMENT. An indeterminate probe + lands in the branch below and touches nothing, so no store can have its purge stopped by + a timeout, a permission blip, or a relation that is mid-rebuild. And the release is the + existing arming path, unchanged: the next sweep measures Covered and arms it, with no + manual step, exactly as a first-time hold releases. */ + using var hold = new NpgsqlCommand(HoldRetentionPolicySql(relation), connection) { CommandTimeout = SetupTimeoutSeconds }; + await hold.ExecuteNonQueryAsync(cancellationToken); held++; logger?.LogWarning( - "Retention policy for {Relation} created but HELD PAUSED - {Coverage} does not yet cover everything it holds, so arming could drop history no rollup has. Backfill past the {DropAfter} horizon and the policy arms itself on the next start.", - relation, string.Join(" + ", coverage), dropAfter); + "Retention policy for {Relation} HELD PAUSED - {ShortConsumer} does not yet cover everything it holds, so arming could drop history that rollup has never materialized. Backfill past the {DropAfter} horizon and the policy arms itself on the next start.", + relation, shortConsumer, dropAfter); + } + else + { + /* Coverage could not be MEASURED, which is not the same as measuring a shortfall. Leave the + policy in whatever state it is already in: a new one is paused (fail-closed, as always), + and one this store already armed keeps running. Disarming here instead would let a single + bad probe stop purging across every tier at once and grow disk without bound — the + failure mode that kept #1877 unfixed rather than fixed badly. */ + indeterminate++; + logger?.LogWarning( + "Retention policy for {Relation} left as-is - its coverage ({Coverage}) could not be established this start, and an unreadable store is not evidence of anything. Re-judged on the next start.", + relation, string.Join(" + ", coverage)); } } catch (Exception ex) when (ex is not OperationCanceledException) @@ -2009,8 +2103,8 @@ what makes a restart converge instead of re-pausing a policy this store already } logger?.LogInformation( - "TimescaleDB: {Applied}/{Total} retention policies in place, {Armed} armed, {Held} held paused pending backfill (raw {Raw}, hourly CAGGs {Hourly}; daily CAGGs kept indefinitely)", - applied, policies.Length, armed, held, RawRetentionInterval, HourlyRetentionInterval); + "TimescaleDB: {Applied}/{Total} retention policies in place, {Armed} armed, {Held} held paused pending backfill, {Indeterminate} left as-is (coverage unreadable) (raw {Raw}, hourly CAGGs {Hourly}; daily CAGGs kept indefinitely)", + applied, policies.Length, armed, held, indeterminate, RawRetentionInterval, HourlyRetentionInterval); return applied; }