From da8df72ce527780abd45b34a10e4e488a8e2c7c6 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 11:55:57 -0400 Subject: [PATCH 1/3] Decide a re-created statement entry's counters row-coherently in pg_statement_stats (#4428) --- .../PgStatementStatsRowCoherentResetTests.cs | 324 ++++++++++++++++++ .../CollectorDeltaCalculator.cs | 11 + .../ICollectorDeltaCalculator.cs | 19 + .../PgStatementStatsCollector.cs | 109 +++++- 4 files changed, 460 insertions(+), 3 deletions(-) create mode 100644 Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs diff --git a/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs b/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs new file mode 100644 index 0000000000..fe2296654f --- /dev/null +++ b/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs @@ -0,0 +1,324 @@ +/* + * 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; +using System.Data.Common; +using System.Threading; +using System.Threading.Tasks; +using PerformanceMonitor.Collectors; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4428: the same row-coherent restart decision QueryStatsRowCoherentResetTests pins for +/// query_stats, applied to — with one difference the +/// PostgreSQL side forces: the series-age signal (stats_since, pg_stat_statements 1.11+) is a +/// TIMESTAMP on the monitored server's own clock, not an age relative to the collector host's clock, so +/// placement compares it ONLY against the previous pass's target_now — captured in the SAME read +/// — never against the collector's own . +/// +public sealed class PgStatementStatsRowCoherentResetTests +{ + private const int ServerId = 1; + private static DateTime T0 => new(2026, 9, 1, 12, 0, 0, DateTimeKind.Unspecified); + + /// One row, ordinals matching PgStatementStatsCollector's BuildQuery, 0-29. + private static object[] Row( + long queryId, long dbid, long userid, bool toplevel, long calls, double totalExecTimeMs, + long rowsReturned, DateTime? statsReset, DateTime? statsSince, DateTime targetNow) + => new object[] + { + queryId, dbid, userid, toplevel, calls, totalExecTimeMs, + 0.0, 0.0, 0.0, rowsReturned, + 0L, 0L, 0L, 0L, 0L, 0L, 0.0, 0.0, + (object?)null!, (object?)null!, (object?)null!, (object?)null!, + 0L, 0L, 0L, + (object?)null!, (object?)null!, + (object?)statsReset ?? DBNull.Value, + (object?)statsSince ?? DBNull.Value, + targetNow, + }; + + private static async Task> RunReadAsync( + ICollectorDeltaCalculator deltas, DateTime collectionTime, object[] row) + { + var reader = new FakeReader(row); + var context = new CollectorContext + { + ServerId = ServerId, + ServerName = "target-a", + CollectionTime = collectionTime, + Deltas = deltas, + }; + + return await PgStatementStatsCollector.Instance.ReadAsync(reader, context, CancellationToken.None); + } + + /// + /// #4431's shape, adapted: calls re-grown past the old value, total time DROPPED (the counter that a + /// per-family delta alone would call a reset), stats_since inside the gap on the TARGET clock. The + /// whole row credits current values over the real interval. + /// + [Fact] + public async Task Reset_StatsSinceInsideGap_CreditsCurrentValues() + { + var deltas = new CollectorDeltaCalculator(); + var t0Target = T0; + var t1Target = T0.AddSeconds(60); + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 1, totalExecTimeMs: 57_695_259, rowsReturned: 1, + statsReset: null, statsSince: null, targetNow: t0Target)); + + /* Restart between t0Target and t1Target: total time FELL (57,695,259 -> 703,943), while calls and + rows RE-GREW past their own old values (1 -> 16) — the #4431 field shape. A per-family delta + alone would read this as "total time reset, calls +15"; row-coherent placement instead credits + every counter as its current value over the real interval. stats_since sits inside the gap on + the TARGET clock. */ + var restartedAt = t0Target.AddSeconds(30); + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 16, totalExecTimeMs: 703_943, rowsReturned: 16, + statsReset: null, statsSince: restartedAt, targetNow: t1Target)); + + var restarted = Assert.Single(rows); + Assert.Equal(16, restarted.DeltaCalls); + Assert.Equal(703_943, restarted.DeltaTotalExecTimeMs); + Assert.Equal(16, restarted.DeltaRows); + Assert.Equal(60, restarted.SampleIntervalSeconds); + } + + /// + /// stats_since BEFORE the previous pass's target_now: the restart happened before we last looked, so + /// it is not placeable inside the gap. A decreasing row reports (0, 0). + /// + [Fact] + public async Task Reset_StatsSinceBeforePreviousTargetNow_ReportsZeroZero() + { + var deltas = new CollectorDeltaCalculator(); + var t0Target = T0; + var t1Target = T0.AddSeconds(60); + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 100, totalExecTimeMs: 5000, rowsReturned: 100, + statsReset: null, statsSince: null, targetNow: t0Target)); + + /* stats_since is BEFORE t0Target — the entry existed before our previous look, so a decrease here + cannot be a "credit the current values" restart. */ + var beforePreviousLook = t0Target.AddSeconds(-10); + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 5, totalExecTimeMs: 900, rowsReturned: 5, + statsReset: null, statsSince: beforePreviousLook, targetNow: t1Target)); + + var reset = Assert.Single(rows); + Assert.Equal(0, reset.DeltaCalls); + Assert.Equal(0, reset.DeltaTotalExecTimeMs); + Assert.Equal(0, reset.DeltaRows); + Assert.Equal(0, reset.SampleIntervalSeconds); + } + + /// stats_since absent (PostgreSQL 16, or Aurora without it): strict (0, 0) for a decreasing row. + [Fact] + public async Task Reset_StatsSinceAbsent_ReportsZeroZero() + { + var deltas = new CollectorDeltaCalculator(); + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 100, totalExecTimeMs: 5000, rowsReturned: 100, + statsReset: null, statsSince: null, targetNow: T0)); + + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 5, totalExecTimeMs: 900, rowsReturned: 5, + statsReset: null, statsSince: null, targetNow: T0.AddSeconds(60))); + + var reset = Assert.Single(rows); + Assert.Equal(0, reset.DeltaCalls); + Assert.Equal(0, reset.DeltaTotalExecTimeMs); + Assert.Equal(0, reset.DeltaRows); + Assert.Equal(0, reset.SampleIntervalSeconds); + } + + /// + /// Clock skew: the target's clock runs several minutes AHEAD of the collector host's clock. An entry + /// created BEFORE the previous pass on the TARGET's clock must not be credited, even though its + /// stats_since reads "after" the previous pass's context.CollectionTime on the collector's clock — + /// proving placement never compares against the collector's own clock. + /// + [Fact] + public async Task Reset_TargetClockSkew_NeverComparedAgainstCollectorClock() + { + var deltas = new CollectorDeltaCalculator(); + + /* Target clock runs 5 minutes ahead of the collector host: collector sees T0, target's now() is + T0 + 5 minutes. */ + var skew = TimeSpan.FromMinutes(5); + var t0Target = T0 + skew; + var t1Target = T0.AddSeconds(60) + skew; + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 100, totalExecTimeMs: 5000, rowsReturned: 100, + statsReset: null, statsSince: null, targetNow: t0Target)); + + /* The restart's stats_since is T0 + 90s on the COLLECTOR's clock — later than the collector's own + T0 (context.CollectionTime for pass one) — but on the TARGET's clock it is BEFORE t0Target + (T0 + 300s), i.e. before the previous pass. A calculator that compared stats_since against the + collector's clock would wrongly credit this; comparing against target_now correctly refuses it. */ + var statsSinceOnCollectorClock = T0.AddSeconds(90); + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 5, totalExecTimeMs: 900, rowsReturned: 5, + statsReset: null, statsSince: statsSinceOnCollectorClock, targetNow: t1Target)); + + var reset = Assert.Single(rows); + Assert.Equal(0, reset.DeltaCalls); + Assert.Equal(0, reset.DeltaTotalExecTimeMs); + Assert.Equal(0, reset.DeltaRows); + Assert.Equal(0, reset.SampleIntervalSeconds); + } + + /// No reset — every counter only ever increases — takes the ordinary per-family path unchanged. + [Fact] + public async Task NoReset_OrdinaryIncrease_Unchanged() + { + var deltas = new CollectorDeltaCalculator(); + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 100, totalExecTimeMs: 5000, rowsReturned: 100, + statsReset: null, statsSince: null, targetNow: T0)); + + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 120, totalExecTimeMs: 5200, rowsReturned: 110, + statsReset: null, statsSince: null, targetNow: T0.AddSeconds(60))); + + var ordinary = Assert.Single(rows); + Assert.Equal(20, ordinary.DeltaCalls); + Assert.Equal(200, ordinary.DeltaTotalExecTimeMs); + Assert.Equal(10, ordinary.DeltaRows); + Assert.Equal(60, ordinary.SampleIntervalSeconds); + } + + /// + /// The mutation: with forced to always report "no + /// reset" (a stub calculator whose DecideRow override never sets AnyReset), the row-coherent pins + /// above go RED — a decreasing row is read as an ordinary per-family delta instead of a restart. + /// + [Fact] + public async Task Mutation_DecideRowAlwaysNoReset_GoesRed() + { + var deltas = new NeverResetsDeltaCalculator(); + + await RunReadAsync(deltas, T0, + Row(1, 1, 1, true, calls: 1, totalExecTimeMs: 57_695_259, rowsReturned: 1, + statsReset: null, statsSince: null, targetNow: T0)); + + var restartedAt = T0.AddSeconds(30); + var rows = await RunReadAsync(deltas, T0.AddSeconds(60), + Row(1, 1, 1, true, calls: 16, totalExecTimeMs: 703_943, rowsReturned: 16, + statsReset: null, statsSince: restartedAt, targetNow: T0.AddSeconds(60))); + + var mutated = Assert.Single(rows); + + /* With the mutation, total_exec_time's own per-family call sees 703,943 < 57,695,259 and reports + (0, 0) for THAT family alone, while calls and rows (both increased) report ordinary per-family + deltas — the exact mixed reset-plus-inflated-increment #4428 exists to prevent. This assertion + is the one meant to FAIL against the mutated calculator; it demonstrates the pin catches the + mutation rather than the mutation being silently tolerated. */ + Assert.True( + mutated.DeltaTotalExecTimeMs == 703_943 && mutated.DeltaCalls == 16, + $"Mutation escaped detection: expected row-coherent credit (703943, 16) but got " + + $"(DeltaTotalExecTimeMs={mutated.DeltaTotalExecTimeMs}, DeltaCalls={mutated.DeltaCalls}) — " + + "the mutated calculator's family-by-family behaviour must differ from the row-coherent one " + + "for this pin to be meaningful."); + } + + /// + /// A calculator whose always reports no reset — the + /// #4428 mutation — with every other member delegated to a real + /// so the per-family behaviour under test is otherwise identical. Implements the interface directly + /// (rather than subclassing) because is not virtual — + /// a new hide in a subclass would not intercept a call made through the interface reference + /// actually holds, which is exactly the shape + /// uses. + /// + private sealed class NeverResetsDeltaCalculator : ICollectorDeltaCalculator + { + private readonly CollectorDeltaCalculator _inner = new(); + + public long CalculateDelta(int serverId, string collectorName, string key, long currentValue, + DateTime? collectionTime = null, int maxGapSeconds = 0) + => _inner.CalculateDelta(serverId, collectorName, key, currentValue, collectionTime, maxGapSeconds); + + public long CalculateDeltaWithInterval(int serverId, string collectorName, string key, long currentValue, + out int intervalSeconds, DateTime? collectionTime = null, int maxGapSeconds = 0) + => _inner.CalculateDeltaWithInterval(serverId, collectorName, key, currentValue, out intervalSeconds, collectionTime, maxGapSeconds); + + public long CalculateDeltaWithSeriesAge(int serverId, string collectorName, string key, long currentValue, + int? seriesAgeSeconds, out int intervalSeconds, DateTime? collectionTime = null, int maxGapSeconds = 0) + => _inner.CalculateDeltaWithSeriesAge(serverId, collectorName, key, currentValue, seriesAgeSeconds, out intervalSeconds, collectionTime, maxGapSeconds); + + public RowResetDecision DecideRow(int serverId, + System.Collections.Generic.IReadOnlyList<(string Family, long Current)> counters, string key, + int? seriesAgeSeconds, DateTime? collectionTime, int maxGapSeconds) + => default; + + public DateTime? PreviousPass(int serverId, string group, DateTime observedTime) + => _inner.PreviousPass(serverId, group, observedTime); + + public void ClearServer(int serverId, string? discontinuity = null) + => _inner.ClearServer(serverId, discontinuity); + + public void ClearGroups(int serverId, string? discontinuity, params string[] groups) + => _inner.ClearGroups(serverId, discontinuity, groups); + } + + /// A minimal multi-call over one boxed row, ordinal access only. + private sealed class FakeReader : DbDataReader + { + private readonly object[] _values; + private int _row = -1; + + public FakeReader(object[] values) => _values = values; + + private object Raw(int ordinal) => _values[ordinal]; + + public override bool IsDBNull(int ordinal) => Raw(ordinal) is DBNull; + public override DateTime GetDateTime(int ordinal) => (DateTime)Raw(ordinal); + public override long GetInt64(int ordinal) => Convert.ToInt64(Raw(ordinal)); + public override int GetInt32(int ordinal) => Convert.ToInt32(Raw(ordinal)); + public override bool GetBoolean(int ordinal) => (bool)Raw(ordinal); + public override double GetDouble(int ordinal) => Convert.ToDouble(Raw(ordinal)); + public override string GetString(int ordinal) => (string)Raw(ordinal); + public override T GetFieldValue(int ordinal) => (T)Raw(ordinal); + public override object GetValue(int ordinal) => Raw(ordinal); + public override int FieldCount => _values.Length; + public override bool Read() => ++_row == 0; + public override Task ReadAsync(CancellationToken cancellationToken) => Task.FromResult(Read()); + public override bool HasRows => true; + + public override int Depth => 0; + public override bool IsClosed => false; + public override int RecordsAffected => 0; + public override object this[int ordinal] => Raw(ordinal); + public override object this[string name] => throw new NotSupportedException("ordinal access only"); + public override int GetOrdinal(string name) => throw new NotSupportedException("ordinal access only"); + public override string GetName(int ordinal) => throw new NotSupportedException("ordinal access only"); + public override bool NextResult() => false; + public override IEnumerator GetEnumerator() => throw new NotSupportedException(); + public override int GetValues(object[] values) => throw new NotSupportedException(); + public override string GetDataTypeName(int ordinal) => throw new NotSupportedException(); + public override Type GetFieldType(int ordinal) => Raw(ordinal).GetType(); + public override byte GetByte(int ordinal) => throw new NotSupportedException(); + public override long GetBytes(int ordinal, long dataOffset, byte[]? buffer, int bufferOffset, int length) => throw new NotSupportedException(); + public override char GetChar(int ordinal) => throw new NotSupportedException(); + public override long GetChars(int ordinal, long dataOffset, char[]? buffer, int bufferOffset, int length) => throw new NotSupportedException(); + public override decimal GetDecimal(int ordinal) => throw new NotSupportedException(); + public override float GetFloat(int ordinal) => throw new NotSupportedException(); + public override Guid GetGuid(int ordinal) => throw new NotSupportedException(); + public override short GetInt16(int ordinal) => throw new NotSupportedException(); + } +} diff --git a/PerformanceMonitor.Collectors/CollectorDeltaCalculator.cs b/PerformanceMonitor.Collectors/CollectorDeltaCalculator.cs index 19f3c79e09..4960697fa3 100644 --- a/PerformanceMonitor.Collectors/CollectorDeltaCalculator.cs +++ b/PerformanceMonitor.Collectors/CollectorDeltaCalculator.cs @@ -187,6 +187,17 @@ public static DateTime SeedCutoff() return updated.Previous; } + /// + /// + /// #4428: the same window-rolling machinery + /// (private, above) already keeps for a collector-clock caller, exposed under the interface's own + /// name for a caller that tracks a DIFFERENT clock. shares the private + /// method's dictionary rather than a second one, on the contract stated on the interface member: a + /// caller's group name never collides with any collectorName an ordinary delta call passes, so + /// one dictionary safely serves both without either clock's window disturbing the other's. + public DateTime? PreviousPass(int serverId, string group, DateTime observedTime) + => PreviousPass(serverId, group, (DateTime?)observedTime); + /// /// The discontinuity accounts (#3653 A5) a definition handed to or /// and no host has logged yet: serverId -> the lines, in the order they were diff --git a/PerformanceMonitor.Collectors/ICollectorDeltaCalculator.cs b/PerformanceMonitor.Collectors/ICollectorDeltaCalculator.cs index f1fa338da7..0339162560 100644 --- a/PerformanceMonitor.Collectors/ICollectorDeltaCalculator.cs +++ b/PerformanceMonitor.Collectors/ICollectorDeltaCalculator.cs @@ -115,6 +115,25 @@ RowResetDecision DecideRow(int serverId, IReadOnlyList<(string Family, long Curr int? seriesAgeSeconds, DateTime? collectionTime, int maxGapSeconds) => default; + /// + /// #4428: rolls a caller-named pass window forward when is new for + /// (, ), and returns the PREVIOUS value of that + /// window — the same bookkeeping already keeps on the collector's own clock + /// (collectionTime), exposed generically so a definition that must place a restart on a + /// DIFFERENT clock — a source's own now(), captured in the same read, rather than the collector + /// host's — can track that clock's own previous pass without a second cache of its own. + /// + /// is a caller-chosen namespace. It shares nothing with any + /// collectorName a delta family uses elsewhere — the point of the method is that a target-clock + /// window and a collector-clock window never mix, so a caller MUST pick a name no ordinary delta call + /// also passes as its collectorName. + /// + /// Default-implemented to return null, like every other member here, so an implementer that + /// tracks no such window — every test double in this repo until it opts in — keeps compiling. + /// + DateTime? PreviousPass(int serverId, string group, DateTime observedTime) + => null; + /// /// Forgets every baseline and every pass window cached for , because the /// counters behind that id are no longer the counters the baselines were read from (#3653 A5, the diff --git a/PerformanceMonitor.Collectors/PgStatementStatsCollector.cs b/PerformanceMonitor.Collectors/PgStatementStatsCollector.cs index edf70ca292..7496c5edfe 100644 --- a/PerformanceMonitor.Collectors/PgStatementStatsCollector.cs +++ b/PerformanceMonitor.Collectors/PgStatementStatsCollector.cs @@ -182,6 +182,25 @@ reads it unguarded. */ with the measurement behind its shape (#3818). */ var statsReset = StatementsEpochSql; + /* #4428: stats_since arrived in pg_stat_statements 1.11, bundled with PostgreSQL 17 — the moment + THIS entry was (re-)created in the extension's hashtable, the same series-age signal #2235 + already uses for query_stats' compile_age, reported as a timestamp rather than an age. It is a + COLUMN of the base view, exactly like toplevel above, so it takes the SAME proxy toplevel + already takes and for the same accepted reason (#3818's remarks on toplevel): the server major + is a stand-in for the extension's catalog version, which is usually right and loudly wrong + (42703, unclassified) on the cluster it misses — a 17+ engine whose extension predates the + in-place major upgrade. aurora_stat_statements() is documented to carry every pg_stat_statements + column, stats_since included, wherever the function itself exists, so the Aurora flavor reads + it unguarded the same way it already reads toplevel unguarded. */ + var statsSince = postgresMajorVersion >= 17 ? "stats_since" : "NULL::timestamp with time zone"; + + /* #4428: the TARGET's own clock, captured in the SAME read as stats_since, once per row (cheap — + PostgreSQL evaluates now() once per statement, not per row, so this is one clock read per pass + either way). The restart placement below compares stats_since ONLY against this column's value + from the PREVIOUS pass, never against the collector host's own clock — the two clocks can be + minutes apart (#4428's skew pin), and the collector's clock is not even the same MACHINE. */ + var targetNow = "now()"; + if (!isAurora) { return $@" @@ -213,7 +232,9 @@ with the measurement behind its shape (#3818). */ wal_bytes::bigint AS wal_bytes, NULL::bigint AS total_exec_peakmem, NULL::bigint AS max_exec_peakmem, - {statsReset} AS statements_stats_reset + {statsReset} AS statements_stats_reset, + {statsSince} AS stats_since, + {targetNow} AS target_now FROM public.pg_stat_statements WHERE calls > 0"; } @@ -247,7 +268,9 @@ FROM public.pg_stat_statements wal_bytes::bigint AS wal_bytes, total_exec_peakmem::bigint AS total_exec_peakmem, max_exec_peakmem::bigint AS max_exec_peakmem, - {statsReset} AS statements_stats_reset + {statsReset} AS statements_stats_reset, + {statsSince} AS stats_since, + {targetNow} AS target_now FROM aurora_stat_statements(false) WHERE calls > 0"; } @@ -478,6 +501,42 @@ it the same way it covers a restart. */ var key = string.Create(CultureInfo.InvariantCulture, $"{queryId}|{databaseId}|{userId}|{(topLevel ? 1 : 0)}"); + /* #4428: stats_since (ordinal 28, NULL below PostgreSQL 17 or when the column does not exist + on this cluster's extension catalog) and target_now (ordinal 29, always present — now() never + returns NULL) are read TOGETHER, in this same row, before any delta call below can touch a + baseline. Both travel on the TARGET's own clock; neither is ever compared against anything + measured on the collector host's clock — see the class remarks and #4428's skew pin. */ + var statsSince = reader.IsDBNull(28) ? (DateTime?)null : reader.GetDateTime(28); + var targetNow = reader.GetDateTime(29); + + /* #4428: peeked BEFORE any of the three per-family calls below mutate a baseline — the same + ordering DecideRow's own doc comment requires and QueryStatsCollector already follows for + query_stats. seriesAgeSeconds is passed null deliberately: DecideRow's OWN gap-placement + branch (reached only when seriesAgeSeconds.HasValue) measures the gap on collectionTime, + the COLLECTOR's clock, which this definition must never use for placement. Passing null + skips that branch entirely and leaves this call a pure AnyReset peek; placement is decided + below, on the target's own clock, using stats_since and the target-clock pass window. */ + var rowReset = context.Deltas.DecideRow( + context.ServerId, + new (string Family, long Current)[] + { + ("pg_statement_stats_calls", calls), + ("pg_statement_stats_time", (long)totalExecTimeMs), + ("pg_statement_stats_rows", rowsReturned), + }, + key, + seriesAgeSeconds: null, + collectionTime: context.CollectionTime, + maxGapSeconds: CollectorDeltaCalculator.DefaultMaxGapSeconds); + + /* #4428: the TARGET-clock pass window, tracked under a group name no ordinary delta call ever + passes as its own collectorName (see ICollectorDeltaCalculator.PreviousPass's contract), so + it never collides with the collector-clock windows the three CalculateDeltaWithInterval + calls below keep for themselves. Rolled every pass — target_now moves forward on every + genuine pass, so this always advances — and returns the PREVIOUS pass's target_now, the + only clock a restart may be placed against. */ + var previousTargetNow = context.Deltas.PreviousPass(context.ServerId, "pg_statement_stats_target_clock", targetNow); + var deltaCalls = context.Deltas.CalculateDeltaWithInterval( context.ServerId, "pg_statement_stats_calls", key, calls, out var callsIntervalSeconds, collectionTime: context.CollectionTime, maxGapSeconds: CollectorDeltaCalculator.DefaultMaxGapSeconds); @@ -496,11 +555,55 @@ false counter-reset on a real accrual. See the class remarks. */ var deltaRows = context.Deltas.CalculateDeltaWithInterval( context.ServerId, "pg_statement_stats_rows", key, rowsReturned, out var rowsIntervalSeconds, collectionTime: context.CollectionTime, maxGapSeconds: CollectorDeltaCalculator.DefaultMaxGapSeconds); + + /* #4428: the row-coherent decision, applied AFTER every per-family call above has already run + and stored its own new baseline — exactly QueryStatsCollector's ordering, so the row is + ready for an ordinary delta on the NEXT pass regardless of which branch this row takes now. + Any family decreasing makes the WHOLE row a restart (rowReset.AnyReset, decided above on the + unmutated baseline). Placement: stats_since strictly AFTER the previous pass's target-clock + now() means the restart happened inside the gap, so every counter's delta becomes its + CURRENT value over the real target-clock gap; anything else — stats_since absent (rule 3), + previousTargetNow unknown (first pass), or stats_since at or before the previous target_now + — makes the whole row unknowable, (0, 0), same as an ordinary single-family reset already + reports. NEVER compared against context.CollectionTime — the collector host's own clock — + which is the whole point of carrying target_now beside stats_since in the same read. */ + if (rowReset.AnyReset) + { + var gapSeconds = previousTargetNow.HasValue + ? (int)(targetNow - previousTargetNow.Value).TotalSeconds + : 0; + + var creditedInGap = statsSince.HasValue && previousTargetNow.HasValue + && statsSince.Value > previousTargetNow.Value + && gapSeconds > 0; + + if (creditedInGap) + { + deltaCalls = calls; + deltaTotalTime = (long)totalExecTimeMs; + deltaRows = rowsReturned; + callsIntervalSeconds = gapSeconds; + timeIntervalSeconds = gapSeconds; + rowsIntervalSeconds = gapSeconds; + } + else + { + deltaCalls = 0; + deltaTotalTime = 0; + deltaRows = 0; + callsIntervalSeconds = 0; + timeIntervalSeconds = 0; + rowsIntervalSeconds = 0; + } + } + /* #3540 (V128): the stored interval is the MINIMUM over the row's three groups — the V127 rule (WaitStatsCollector). The groups share a key and a collection time, so they agree in every case but an independent single-counter reset, and pg_stat_statements resets an entry's counters together; the minimum makes the stored pair mean "every delta in this row is - knowable", so a reader never divides one group's reset 0 by a sibling's real span. */ + knowable", so a reader never divides one group's reset 0 by a sibling's real span. After the + #4428 override above the three intervals already agree, so this is a no-op for a restarted + row and unchanged behaviour for every other one. */ var sampleIntervalSeconds = Math.Min(callsIntervalSeconds, Math.Min(timeIntervalSeconds, rowsIntervalSeconds)); /* The skip: a REAL interval (this is not a first sighting, a counter reset, or a gap this From dae420ebe034fbcd2b8291248972ee03b08dd73e Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 12:04:54 -0400 Subject: [PATCH 2/3] Pin the pg_statement_stats restart without a failing fact (#4428) Remove the committed intended-failure mutation fact; the mutation instead gets exercised as an uncommitted, restored code change to CollectorDeltaCalculator.DecideRow, confirmed RED then GREEN. --- .../PgStatementStatsRowCoherentResetTests.cs | 74 ------------------- 1 file changed, 74 deletions(-) diff --git a/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs b/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs index fe2296654f..f7d9fb2a5a 100644 --- a/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs +++ b/Darling/Darling.Tests/PgStatementStatsRowCoherentResetTests.cs @@ -202,80 +202,6 @@ await RunReadAsync(deltas, T0, Assert.Equal(60, ordinary.SampleIntervalSeconds); } - /// - /// The mutation: with forced to always report "no - /// reset" (a stub calculator whose DecideRow override never sets AnyReset), the row-coherent pins - /// above go RED — a decreasing row is read as an ordinary per-family delta instead of a restart. - /// - [Fact] - public async Task Mutation_DecideRowAlwaysNoReset_GoesRed() - { - var deltas = new NeverResetsDeltaCalculator(); - - await RunReadAsync(deltas, T0, - Row(1, 1, 1, true, calls: 1, totalExecTimeMs: 57_695_259, rowsReturned: 1, - statsReset: null, statsSince: null, targetNow: T0)); - - var restartedAt = T0.AddSeconds(30); - var rows = await RunReadAsync(deltas, T0.AddSeconds(60), - Row(1, 1, 1, true, calls: 16, totalExecTimeMs: 703_943, rowsReturned: 16, - statsReset: null, statsSince: restartedAt, targetNow: T0.AddSeconds(60))); - - var mutated = Assert.Single(rows); - - /* With the mutation, total_exec_time's own per-family call sees 703,943 < 57,695,259 and reports - (0, 0) for THAT family alone, while calls and rows (both increased) report ordinary per-family - deltas — the exact mixed reset-plus-inflated-increment #4428 exists to prevent. This assertion - is the one meant to FAIL against the mutated calculator; it demonstrates the pin catches the - mutation rather than the mutation being silently tolerated. */ - Assert.True( - mutated.DeltaTotalExecTimeMs == 703_943 && mutated.DeltaCalls == 16, - $"Mutation escaped detection: expected row-coherent credit (703943, 16) but got " + - $"(DeltaTotalExecTimeMs={mutated.DeltaTotalExecTimeMs}, DeltaCalls={mutated.DeltaCalls}) — " + - "the mutated calculator's family-by-family behaviour must differ from the row-coherent one " + - "for this pin to be meaningful."); - } - - /// - /// A calculator whose always reports no reset — the - /// #4428 mutation — with every other member delegated to a real - /// so the per-family behaviour under test is otherwise identical. Implements the interface directly - /// (rather than subclassing) because is not virtual — - /// a new hide in a subclass would not intercept a call made through the interface reference - /// actually holds, which is exactly the shape - /// uses. - /// - private sealed class NeverResetsDeltaCalculator : ICollectorDeltaCalculator - { - private readonly CollectorDeltaCalculator _inner = new(); - - public long CalculateDelta(int serverId, string collectorName, string key, long currentValue, - DateTime? collectionTime = null, int maxGapSeconds = 0) - => _inner.CalculateDelta(serverId, collectorName, key, currentValue, collectionTime, maxGapSeconds); - - public long CalculateDeltaWithInterval(int serverId, string collectorName, string key, long currentValue, - out int intervalSeconds, DateTime? collectionTime = null, int maxGapSeconds = 0) - => _inner.CalculateDeltaWithInterval(serverId, collectorName, key, currentValue, out intervalSeconds, collectionTime, maxGapSeconds); - - public long CalculateDeltaWithSeriesAge(int serverId, string collectorName, string key, long currentValue, - int? seriesAgeSeconds, out int intervalSeconds, DateTime? collectionTime = null, int maxGapSeconds = 0) - => _inner.CalculateDeltaWithSeriesAge(serverId, collectorName, key, currentValue, seriesAgeSeconds, out intervalSeconds, collectionTime, maxGapSeconds); - - public RowResetDecision DecideRow(int serverId, - System.Collections.Generic.IReadOnlyList<(string Family, long Current)> counters, string key, - int? seriesAgeSeconds, DateTime? collectionTime, int maxGapSeconds) - => default; - - public DateTime? PreviousPass(int serverId, string group, DateTime observedTime) - => _inner.PreviousPass(serverId, group, observedTime); - - public void ClearServer(int serverId, string? discontinuity = null) - => _inner.ClearServer(serverId, discontinuity); - - public void ClearGroups(int serverId, string? discontinuity, params string[] groups) - => _inner.ClearGroups(serverId, discontinuity, groups); - } - /// A minimal multi-call over one boxed row, ordinal access only. private sealed class FakeReader : DbDataReader { From 0189a0200da6f4e8579d605baae5b3b50a0d2762 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 26 Sep 2026 12:44:28 -0400 Subject: [PATCH 3/3] Give the pg_statement_stats test rows the stats_since and target clock columns (#4428) --- Lite.Tests/PgStatementStatsDeltaSkipTests.cs | 7 ++++++- Lite.Tests/PgStatementStatsFlavorTests.cs | 16 ++++++++++------ Lite.Tests/ServerEpochTests.cs | 9 ++++++++- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/Lite.Tests/PgStatementStatsDeltaSkipTests.cs b/Lite.Tests/PgStatementStatsDeltaSkipTests.cs index c20c094d4c..7d66f019e8 100644 --- a/Lite.Tests/PgStatementStatsDeltaSkipTests.cs +++ b/Lite.Tests/PgStatementStatsDeltaSkipTests.cs @@ -56,8 +56,11 @@ public class PgStatementStatsDeltaSkipTests /// on the Aurora-only six, and — since #3653 A5 — a NULL statements_stats_reset at ordinal 27, /// which the epoch check reads as "unknown" and so never as a change; these tests are about the skip, /// not the epoch, and a NULL keeps the epoch inert. ServerEpochTests drives the epoch itself.) + /// Since #4428, ordinal 28 is stats_since (NULL by default here — the row-coherent restart + /// placement is tested separately in Darling.Tests) and ordinal 29 is target_now, the + /// target's own clock, defaulted to the collection time a scenario is about to pass to ReadAsync. /// - private static object[] Row(long queryId, long calls, double totalExecTimeMs, long rowsReturned = 0, long databaseId = 1, long userId = 1) => new object[] + private static object[] Row(long queryId, long calls, double totalExecTimeMs, long rowsReturned = 0, long databaseId = 1, long userId = 1, DateTime? statsSince = null, DateTime? targetNow = null) => new object[] { queryId, databaseId, userId, true, calls, totalExecTimeMs, 0d, 0d, 0d, rowsReturned, @@ -67,6 +70,8 @@ public class PgStatementStatsDeltaSkipTests 0L, 0L, 0L, DBNull.Value, DBNull.Value, DBNull.Value, + statsSince.HasValue ? (object)statsSince.Value : DBNull.Value, + targetNow ?? T0, }; private static async Task> ReadAsync( diff --git a/Lite.Tests/PgStatementStatsFlavorTests.cs b/Lite.Tests/PgStatementStatsFlavorTests.cs index 9a1a51442c..90d125f712 100644 --- a/Lite.Tests/PgStatementStatsFlavorTests.cs +++ b/Lite.Tests/PgStatementStatsFlavorTests.cs @@ -30,8 +30,9 @@ namespace Lite.Tests; /// /// /// -/// The ordinals are the load-bearing detail. Both queries select the same 28 columns in the same order (the -/// 28th, appended by #3653 A5, is the statements epoch stats_reset, read and not stored) — the +/// The ordinals are the load-bearing detail. Both queries select the same 30 columns in the same order (the +/// 28th, appended by #3653 A5, is the statements epoch stats_reset, read and not stored; the 29th and +/// 30th, appended by #4428, are stats_since and target_now, also read and not stored) — the /// vanilla one fills Aurora's six with typed NULL literals — so ReadAsync, PayloadColumns and /// WritePayload stay single implementations. A shorter vanilla SELECT would have meant a second reader /// whose ordinals could drift from this one, which is exactly the failure the per-major column naming in this @@ -110,10 +111,13 @@ public void BothFlavorsSelectTheSameColumnsInTheSameOrder() Assert.Equal(aurora, vanilla); /* The SELECT list is the payload minus the four columns computed on the client (the three deltas - and, since V128 (#3540), the interval they accrued over) PLUS the one column read and not stored: - the statements epoch stats_reset (#3653 A5), last, so every stored ordinal is where it was. */ - Assert.Equal(PgStatementStatsCollector.Instance.PayloadColumns.Count - 4 + 1, aurora.Count); - Assert.Equal("statements_stats_reset", aurora[^1]); + and, since V128 (#3540), the interval they accrued over) PLUS the three columns read and not + stored: the statements epoch stats_reset (#3653 A5), and — since #4428 — stats_since and + target_now, last, so every stored ordinal is where it was. */ + Assert.Equal(PgStatementStatsCollector.Instance.PayloadColumns.Count - 4 + 3, aurora.Count); + Assert.Equal("statements_stats_reset", aurora[^3]); + Assert.Equal("stats_since", aurora[^2]); + Assert.Equal("target_now", aurora[^1]); } /// diff --git a/Lite.Tests/ServerEpochTests.cs b/Lite.Tests/ServerEpochTests.cs index b2e2c303cd..52ecd59198 100644 --- a/Lite.Tests/ServerEpochTests.cs +++ b/Lite.Tests/ServerEpochTests.cs @@ -835,7 +835,12 @@ public void DrainDiscontinuities_ReturnsInOrder_ThenEmpty() /* ---------------- helpers ---------------- */ - /// A pg_statement_stats reader row in ordinal order, the statements epoch at 27. + /// + /// A pg_statement_stats reader row in ordinal order, the statements epoch at 27. Since #4428, ordinal + /// 28 is stats_since (NULL here — these tests are about the epoch forget, not the row-coherent + /// restart placement, which Darling.Tests covers separately) and ordinal 29 is target_now, + /// defaulted to the same moment as when it is a . + /// private static object[] StatementRow(long queryId, long calls, object statsReset) => new object[] { queryId, 1L, 1L, true, calls, 100d, @@ -846,6 +851,8 @@ public void DrainDiscontinuities_ReturnsInOrder_ThenEmpty() 0L, 0L, 0L, DBNull.Value, DBNull.Value, statsReset, + DBNull.Value, + statsReset is DateTime resetTime ? resetTime : new DateTime(2026, 9, 19, 3, 59, 0, DateTimeKind.Utc), }; /// Reaches the protected restart-seed hook, to stage the host-restart case.