Repository navigation
Record what an abandoned collection cycle was doing (Fixes #2864) - #2868
Conversation
A cycle the #2673 wall-clock budget abandons stored ABANDONED and rows_collected = 0, and that zero is rows STORED -- which an abandoned cycle never does by definition. So the row could not tell a target that never sent row 1 from one that sent 149 and then went silent: a stalled target and a stalled stream, wanting unrelated fixes. #2859 made the phases queryable and the first production capture read open:104ms drain:119,945ms rows=0 -- proof the time was in the drain, and nothing about whether the drain was slow or simply empty. V109 adds five columns. drain_rows_read / drain_bytes_read come from a counting decorator around the provider reader, not from edits to 66 separate read loops. Decorating means a collector cannot forget to count; nothing in the collectors casts a reader to a provider type, so it is transparent. drain_last_read_ms carries the diagnosis, not the row count. A count alone cannot separate "streaming slowly" from "delivered everything then hung" -- both end at the budget with a positive count. Subtract it from sql_drain_ms and you have the time the reader sat with nothing arriving. NULL means no row ever arrived, unambiguous because drain_rows_read is non-null whenever the group is written, which leaves 0 free to mean what it honestly means: row 1 arrived instantly. The counters ride the SAME stopwatch as the drain they are subtracted from. drain_bytes_read is the string payload and the name says so. No DbDataReader exposes wire size; this counts UTF-16 bytes off the string and binary getters. That is the honest scope and the useful one -- these collectors are dominated by one large text column. target_session_id is read off the open connection as a client property, never SELECT @@spid: a round trip here is ~25,000 extra queries an hour to learn a number the client already holds. It is what makes a stalled run joinable to waiting_tasks / dmv_blocking_snapshot / query_snapshots, all of which record a session id. sweep_peer_max_ms separates two populations sharing one stored shape: a genuinely large query runs beside peers at or below baseline, while sweep-wide degradation shows the light collectors at 34-47x baseline BEFORE the heavies burn their budget. Budgeted collectors are excluded because they are the heavies being explained, and "is this budgeted" is asked of the CATALOG -- which required moving PerItemWallClockBudget to the base ICollectorSchemaInfo beside AppliesTo and YieldsOnLockTimeout, since the catalog is keyed by name and holds that interface. A hardcoded list would be right today and silently wrong at the fifth collector. Recorded on every row, not only abandoned ones: a ratio needs a denominator and the baseline must come from the same column. InsertCollectionLogSql is still shared with the retention run-record, so both binding blocks widened to 22 together. The #2859 pin asserting binding-count against placeholder-count was proven red first by dropping one retention binding, reproducing exactly the 08P01 that failed silently last time. No Lite twin: the counting reader is installed by Darling's server-scoped runner and the peer mark by its sweep body, neither of which Lite has, so a DuckDB twin would be five forever-NULL columns. Item 4 of the issue -- the out-of-band watchdog -- is deliberately NOT built. It is the only proposal that must break the sequential model and open a connection to a target already not responding, so it needs a design decision rather than being added alongside instrumentation that touches nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| await DarlingObservability.LogCollectionAsync( | ||
| _postgres!, runtime, collectorName, status, result.Rows, result.SqlMs, result.StorageMs, result.Note, | ||
| result.Fanout, result.ServerPhases, _logger, cancellationToken); | ||
| result.Fanout, result.ServerPhases, result.Drain, PeerMaxOrNull(server), _logger, cancellationToken); |
There was a problem hiding this comment.
sweep_peer_max_ms is stale for exactly the two collectors it's meant to diagnose (query_store, plan_correction).
PeerMaxOrNull(server) is read here after await run(...) completes. For the sequential collectors that's fine — they run one at a time within a single RunDueCollectorsAsync invocation, and the -1 reset at line 4810 happens once per body.
But query_store and plan_correction are dispatched via _ = RunDetachedAsync(...) (line 4903) — fire-and-forget, outside the await that keeps RunDueCollectorsAsync "in" a body. RunDueCollectorsAsync itself returns right after the foreach, and the outer per-server loop re-invokes it roughly every 15s (s_sweepInterval), resetting server.SweepPeerMaxMs = -1 and repopulating it from whatever later ticks' due collectors happen to run — all while the detached run is still executing in the background (query_store: p90 65s, up to 230+s; plan_correction: up to 134s).
So by the time the detached collector finishes and this line runs, server.SweepPeerMaxMs reflects the peer high-water mark of some unrelated, much later tick — not the collectors that were actually running concurrently with it. Since these two collectors are (a) the only ones excluded from updating the mark (correctly, as the heavies-being-explained) and (b) the exact motivating case in the PR description/#2840 ("was this one big query, or was the whole body slow"), the column is least trustworthy precisely where it's most needed — it'll typically read as NULL or some small unrelated value, inviting the wrong conclusion ("peers were fine") when no real peer data was ever captured.
Worth considering: snapshot PeerMaxOrNull(server) at dispatch time (before the detached task starts) rather than after it returns, or simply omit sweep_peer_max_ms for detached-collector rows since it can't mean what it's documented to mean.
Review summaryFocused review of the V109 drain-forensics change (five new Correctness issue found (posted inline): Things checked and found sound:
No SQL-injection, secrets, or missing-index concerns in this change; it's pure instrumentation plumbing. |
All three are pins doing their job, not behaviour that regressed. DocCommentHygieneTests caught a stranded doc block: inserting PeerMaxOrNull between RunOneAsync's <summary> and RunOneAsync left that summary annotating the new helper and RunOneAsync undocumented. The test's own message warns about exactly this -- "check first whether it belongs to a DIFFERENT member that an insertion pushed it away from" -- so the fix reorders rather than deletes: the helper and its own summary move above, and RunOneAsync's summary returns to RunOneAsync. I initially misdiagnosed this as an orphaned summary in ICollectorDefinition, where removing the budget declaration did leave its doc block behind. That one was NOT the failure -- a /* */ comment separated it, which terminates a run -- but it was still wrong, so its documentation is now moved to the base interface where the member lives, rather than dropped. The other two are source-text anchors this change legitimately moved. DarlingEmptyEnumerationNoteTests pins the success return's argument list so a DROPPED argument fails; four were added, so the pin names them. DarlingPayloadProbeFailureTests anchors on definition.ReadAsync(reader, now definition.ReadAsync(counting -- the read/probe/write ORDERING it exists to pin is unchanged. Verified by replicating the hygiene scan locally against both the committed tree (reproduces the CI failure at DarlingWorker.cs:5607) and the fixed one (clean, whole tree), so the scan is known to be able to fail rather than assumed to pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
…sics # Conflicts: # CHANGELOG.md
| /// ordinary bodies. Storing it only on failures would rebuild the cross-referencing this exists | ||
| /// to remove.</para> | ||
| /// </summary> | ||
| public int SweepPeerMaxMs { get; set; } = -1; |
There was a problem hiding this comment.
SweepPeerMaxMs is reset to -1 only at the top of RunDueCollectorsAsync (line ~4810), documented there as "describes ONE body... a mark carried across bodies would answer a different question with the same number."
But ServerLoopState (and this field) persists across reconnects rather than being recreated, and RunOneAsync — which reads and folds into this field — is also called from two other loops that never reset it:
TryConnectAsync's on-load dispatch loop (await RunOneAsync(server, runner, name, cancellationToken);around line 4724), which runs on every (re)connect.RunSnapshotAsync'ssnapshot_nowloop (totalRows += await RunOneAsync(...)around line 4983), which runs on an operator-triggered snapshot.
Since neither path resets SweepPeerMaxMs first, the first collector they run folds its SqlMs into whatever high-water mark the last scheduled sweep body (or a previous on-load/snapshot pass) left behind, and the sweep_peer_max_ms written to collection_log for those rows mixes bookkeeping from unrelated bodies — exactly the cross-body contamination the reset in RunDueCollectorsAsync is meant to prevent. Worth resetting at the top of both of those loops too (or wherever else the "one body" boundary is defined for them).
| command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Bigint, Value = drain.HasValue ? drain.Value.RowsRead : (object)DBNull.Value }); | ||
| command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Bigint, Value = drain.HasValue ? drain.Value.BytesRead : (object)DBNull.Value }); | ||
| command.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlDbType.Integer, Value = drain.HasValue && drain.Value.LastReadMs >= 0 ? (int)drain.Value.LastReadMs : (object)DBNull.Value }); |
There was a problem hiding this comment.
drain_last_read_ms is correctly guarded (drain.Value.LastReadMs >= 0) so the in-memory "-1 = unmeasured" sentinel never gets stored as a literal value — but drain_rows_read/drain_bytes_read (lines 285-286) aren't: they store drain.Value.RowsRead/.BytesRead directly whenever drain.HasValue, with no >= 0 check.
CollectorContext.ServerScopeRowsRead/ServerScopeBytesRead default to -1 and are only overwritten in the finally around definition.ReadAsync(counting, ...) in DarlingCollectorRunner (the block that constructs DrainCountingDataReader). If the per-item wall-clock budget fires while still inside command.ExecuteReaderAsync(itemToken) — i.e. before counting is even constructed — the catch (Exception ex) when (EnumeratedCollectorDriver.ItemBudgetExpired(...)) arm returns a CollectorRunResult with ServerPhasesMeasured: true but ServerRowsRead/ServerBytesRead still at their -1 default (only ServerOpenMs was stamped from its own finally).
That reaches here with drain.HasValue == true and RowsRead == BytesRead == -1, so drain_rows_read/drain_bytes_read get written as literal -1 in a bigint column — contradicting the documented invariant that these are "non-null whenever the group is written" (i.e. always a real non-negative count) and that -1 is "a value no real count can take" that must never be misread as a measurement. Consider guarding these two the same way LastReadMs is guarded (>= 0 ? ... : DBNull.Value).
Review summaryReviewed the V109 drain-forensics change (Darling only — DrainCountingDataReader, target_session_id, sweep_peer_max_ms, the shared InsertCollectionLogSql widening to 22 placeholders, and the PerItemWallClockBudget move to ICollectorSchemaInfo). Left 2 inline findings:
Checked and found OK:
|
DarlingEmptyEnumerationNoteTests pins the success return's arguments as one whitespace-collapsed run, so that a DROPPED argument fails there rather than reaching the store as a null. My explanatory comment sat BETWEEN two of those arguments, which broke the run and failed the pin on a change that dropped nothing -- the exact conflation of "reformatted" with "regressed" that #2851 already had to fix once by collapsing whitespace. The comment moves above the return, where it still documents why the forensics are recorded on the success path and not only on abandon, and the note about why it lives there is now in the file so the next person does not re-inline it. Verified by running all three source-scan pins locally against the real file before pushing, rather than inferring from a green build: the collapsed argument list, the read/probe/write ordering, and the test's OWN literal extracted from the test file and matched against the source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
…sics # Conflicts: # CHANGELOG.md
| await DarlingObservability.LogCollectionAsync( | ||
| _postgres!, runtime, collectorName, status, result.Rows, result.SqlMs, result.StorageMs, result.Note, | ||
| result.Fanout, result.ServerPhases, _logger, cancellationToken); | ||
| result.Fanout, result.ServerPhases, result.Drain, PeerMaxOrNull(server), _logger, cancellationToken); |
There was a problem hiding this comment.
sweep_peer_max_ms is read from live, mutable server.SweepPeerMaxMs state at the moment each run finishes — which is correct for the synchronous body, but wrong for query_store and plan_correction, the two collectors dispatched via RunDetachedAsync (fire-and-forget, line ~4903).
RunDueCollectorsAsync resets server.SweepPeerMaxMs = -1 on every invocation (line 4810), and that method runs once per s_sweepInterval (15s) per server — far more often than the 1-minute/5-minute collector cadences. A detached query_store run can take 100–230+ seconds (per the comment at line ~4872), so by the time it completes and calls LogCollectionAsync here, server.SweepPeerMaxMs has been reset and rebuilt by several unrelated, later sweep ticks. The peer value persisted alongside that query_store/plan_correction row therefore reflects whatever collectors happened to be due in the current tick when the detached task happens to finish — not the peers that ran in "the same sweep body" as documented in the migration comment and CHANGELOG ("the slowest non-budgeted collector already completed in the same sweep body").
This is exactly backwards for the feature's stated purpose: query_store and plan_correction are two of the four budgeted "heavies" this diagnostic exists to explain, and they're precisely the ones whose own sweep_peer_max_ms is attributed to the wrong body. Since neither RunDetachedAsync nor RunOneAsync captures a body-local snapshot of the peer max at dispatch time, there's no way to recover the correct value from the current design — it would need to be captured before firing the detached task and threaded through to this call instead of being re-read from live server state at completion.
| public override object this[int ordinal] => _inner[ordinal]; | ||
| public override object this[string name] => _inner[name]; |
There was a problem hiding this comment.
These two indexers forward straight to _inner, which bypasses byte counting entirely for a string column read via reader[ordinal] / reader["name"] — as opposed to double-counting, which is what the comment above (line ~1216: "so the wrapper cannot silently fall back to DbDataReader's default implementations, which route through GetValue and would double-count") is guarding against.
The safe fix for both problems at once is to route the indexers through this.GetValue(ordinal) / this.GetOrdinal(name) + this.GetValue(...) instead of _inner[ordinal] — that counts exactly once (via the overridden GetValue) rather than zero times.
No collector in PerformanceMonitor.Collectors currently reads via the indexer (verified — none use reader[...]), so this doesn't corrupt today's numbers. But it quietly breaks the class's own central claim ("a collector cannot forget to count") for any future collector that uses the idiomatic indexer syntax instead of GetString/GetValue — the byte count would silently undercount with no error or signal.
|
Reviewed the diff (Darling-only: Parity: the PR explicitly documents "no Lite twin" for this rung (both in the CHANGELOG and the V109 migration doc comment), reasoning that the counting reader and peer mark are installed by Darling-only call sites and a DuckDB twin would be five forever-NULL columns. That's a reasonable call and is the kind of explicit parity-drift justification the review is meant to catch — noting it here for visibility rather than flagging it as a defect. Left two inline comments:
Everything else — the |
All five inline findings were legitimate; three needed code. **The peer mark was wrong for the two collectors it exists to explain.** query_store and plan_correction are dispatched fire-and-forget and run 100-230s, while RunDueCollectorsAsync resets SweepPeerMaxMs every 15s. Reading PeerMaxOrNull(server) at COMPLETION therefore attributed those rows to whatever unrelated later tick happened to be in flight -- and those two are among the budgeted heavies the whole diagnostic exists to explain, so it was least trustworthy exactly where it mattered most. The mark is now captured at DISPATCH and threaded through RunOneAsync as a parameter. That is also correct for the sequential arm, where dispatch and completion are the same body. **Two callers were never a body at all.** TryConnectAsync's on-load dispatch and RunSnapshotAsync's snapshot_now loop both call RunOneAsync without resetting the mark, so their rows folded in a previous body's bookkeeping -- the cross-body contamination the reset exists to prevent. Both now pass null: an operator snapshot is not a sweep body, and saying so with NULL beats reporting a number that means something else. **The -1 sentinel could reach the store as a literal.** The budget can fire INSIDE ExecuteReaderAsync, before the counting reader is constructed -- the abandon arm then returns with phases measured (the open stamped from its own finally) but the counts at their -1 default. Only LastReadMs was guarded, so rows and bytes wrote a literal -1 into a bigint column, breaking this rung's own documented invariant that a stored count is always non-negative and -1 is a value no real count can take. All three are now guarded independently. **The indexers bypassed counting entirely.** reader[ordinal] forwarded straight to the inner reader, so a string read that way counted zero rather than once. No collector uses the indexer today, but "a collector cannot forget to count" is this decorator's central claim and it would have been quietly false for the first one that did. Three tests added for the reachable cases, and every source-scan assertion replicated locally against the real files before pushing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
| drain_last_read_ms is the one that carries the diagnosis: subtracted from sql_drain_ms it is | ||
| the time the reader spent with nothing arriving. It stores NULL when the run read no row at | ||
| all, which is NOT ambiguous with 'not recorded' here because drain_rows_read is non-null | ||
| whenever this group is written - a NULL beside a 0 count says 'nothing ever arrived', and a | ||
| NULL beside a NULL count says the row predates this rung. 0 is left free to mean what it | ||
| honestly means: row 1 arrived instantly. */ |
There was a problem hiding this comment.
The "NULL beside a NULL means the row predates this rung" claim isn't reliable, and the comment immediately below it (lines 285-290) documents the counter-example: an abandon that fires inside ExecuteReaderAsync, before the counting reader is ever constructed, leaves RowsRead/BytesRead/LastReadMs at their -1 default. That gets guarded to NULL here for all three columns — so a genuine V109 row (ServerPhasesMeasured: true, sql_open_ms populated) ends up with drain_rows_read IS NULL AND drain_last_read_ms IS NULL, the exact signature this comment says means "predates the rung." CollectionLogDrainForensicsStoreTests.TheUnmeasuredSentinelIsNeverWrittenAsALiteral even constructs this scenario directly (ServerOpenMs: 120051, ServerDrainMs: 0), so it's not a hypothetical.
It's broader than that one edge case, too: any collector that takes the per-database enumerated path (RunsPerDatabase(target) == true — query_store, procedure_stats/deadlocks/blocked_process_report on Azure SQL DB, all the Pg*BloatStats/PgColumnStats/PgPredicateStats/PgAutovacuumStats/PgIndexUsageStats collectors) never sets serverPhasesMeasured = true at all, so Drain is always null for those and every one of their rows will read drain_rows_read IS NULL forever — indistinguishable from "predates the rung" even on a store that's fully current at V109.
Worth either fixing the disambiguation (e.g. cross-check sql_drain_ms/sql_open_ms being non-null, or a dedicated sentinel for "this code path doesn't measure it") or at minimum correcting the comment (here, in PgMigrations.cs's V109Sql doc, and in DarlingDataReader.cs) so a future reader/dashboard author doesn't build a query on the "NULL/NULL = pre-rung" assumption and silently misclassify real open-stalls or whole collector families as "no data."
Review summaryScoped review of the V109 drain-forensics rung (#2864 items 1-3). This is Darling-only plus the shared Verified as correct:
One finding posted inline on No missing-index-DMV suggestions, no security issues found (all writes are parameterized; no user input reaches SQL text), no T-SQL touched by this PR. |
Review caught that "NULL beside a NULL count means the row predates this rung" is false, and it was stated in three code sites plus the CHANGELOG. Two reachable counter-examples, one of which my own previous commit created. An abandon that fires inside ExecuteReaderAsync never constructs the counting reader, so all three figures sit at their -1 default and the guard added last commit writes them as NULL -- on a genuine V109 row with sql_open_ms populated. And no per-database ENUMERATED collector sets the measured flag at all, so query_store and the whole Pg*Stats family will read NULL here forever on a store that is fully current at V109. A dashboard built on the claim would have silently misclassified both a real open-stall and entire collector families as "no data" -- worse than having no disambiguation, because it reads as one. So the claim is withdrawn rather than rescued with a new sentinel. NULL means NOT RECORDED and the docs now say only that. The one safe inference survives and is still stated: a NULL last-read beside a 0 row count means nothing ever arrived. Pinned so it cannot come back, and proven red first by reintroducing the phrase into one file's V109 region and watching the scan catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
| { | ||
| await DarlingObservability.LogCollectionAsync( | ||
| _postgres!, runtime, collectorName, "ERROR", 0, 0, 0, ex.Message, fanout: null, phases: null, _logger, cancellationToken); | ||
| _postgres!, runtime, collectorName, "ERROR", 0, 0, 0, ex.Message, fanout: null, phases: null, drain: null, sweepPeerMaxMs: null, _logger, cancellationToken); |
There was a problem hiding this comment.
sweepPeerMaxMs: null is hardcoded here and in the six other early-return LogCollectionAsync calls in this method (lines 5788, 5812, 5830, 5851, 5874, 5909), even though peerMaxAtDispatchMs is already in scope as a method parameter and costs nothing to pass through.
This seems to cut against the column's own stated design goal. The V109 migration comment says sweep_peer_max_ms is "Recorded on EVERY row rather than only abandoned ones: a ratio needs a denominator, and the baseline has to come from the same column on ordinary bodies. Storing it only on failures would rebuild exactly the cross-referencing this removes." A YIELDED (lock-timeout) or ERROR row happening mid-sweep is exactly the kind of row where "were this sweep body's other collectors also slow" is useful context — e.g. correlating a lock-timeout yield with a sweep-wide slowdown — but these rows will always read sweep_peer_max_ms = NULL regardless of what was known at dispatch, silently losing that context for every non-SUCCESS/non-ABANDONED status.
Only the main success/abandon path (line 5733) actually threads peerMaxAtDispatchMs through. Was withholding it on these early-return/exception paths intentional (e.g. "the peer mark should only describe a row that actually measured something"), or should these pass peerMaxAtDispatchMs like the main path does?
Review summaryIndependent re-review of the V109 drain-forensics rung (Darling-only + the shared
One new finding, posted inline on |
Review: the seven early-return LogCollectionAsync calls in RunOneAsync (YIELDED, PERMISSIONS x3, ERROR, SESSION_MISSING) hardcoded sweepPeerMaxMs: null while peerMaxAtDispatchMs sat in scope as a parameter, costing nothing to pass. That contradicted the column's own stated rationale, which this rung argues in its migration doc: recorded on EVERY row rather than only abandoned ones, because a ratio needs a denominator and the baseline has to come from ordinary rows. A lock-timeout YIELD during a sweep-wide slowdown is a different finding from one on an otherwise healthy body, and those rows would have read NULL forever -- rebuilding exactly the cross-referencing the column removes. drain STAYS null on those arms, and that is not an oversight: an early return never drained anything, so there is nothing to describe. The two peerMaxAtDispatchMs: null callers also stay -- TryConnectAsync's on-load dispatch and RunSnapshotAsync's operator snapshot are not sweep bodies, so for them NULL is the honest answer rather than a missed pass-through. Both distinctions are now pinned so the next reader does not "fix" one into the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy
ReviewWent through the diff in full (
Nothing to request changes on. Nice test coverage on the sentinel/NULL-semantics edge cases in particular. |
1 participant
Fixes #2864 (items 1-3; item 4 is deliberately not built — see the end).
The gap
A cycle the #2673 wall-clock budget abandons stored
ABANDONEDandrows_collected = 0— and that zero is rows stored, which an abandoned cycle never does by definition. So the row could not distinguish a target that never sent row 1 from one that sent 149 and then went silent. Those are a stalled target and a stalled stream, and they want unrelated fixes.#2859 made the phases queryable and the first production capture read:
That proved the time was in the drain and could say nothing about whether the drain was slow or simply empty.
What V109 adds
drain_rows_read/drain_bytes_readcome from a counting decorator around the provider reader, not from editing 66 separatewhile (await reader.ReadAsync(...))loops. Decorating means a collector cannot forget to count and cannot drift. Verified before writing it that nothing inPerformanceMonitor.Collectorscasts a reader to a provider type, so the wrapper is transparent; every abstract member is forwarded explicitly so a typed getter cannot fall back throughGetValueand double-count.drain_last_read_msis the column that carries the diagnosis, not the row count. A count alone still cannot separate "streaming steadily but slowly" from "delivered everything then hung" — both end at the budget with a positive count. Subtract this fromsql_drain_msand you have the time the reader sat with nothing arriving. It stores NULL when no row ever arrived, which is unambiguous becausedrain_rows_readis non-null whenever the group is written: NULL beside a 0 count means nothing came, NULL beside a NULL count means the row predates the rung. That leaves 0 free to mean what it honestly means — row 1 arrived instantly. The counters ride the same stopwatch as the drain figure they are subtracted from, so "time since last row" is not a difference of two nearly-equal numbers from different origins.drain_bytes_readis the string payload, and the name says so. NoDbDataReaderexposes wire size, so this counts UTF-16 bytes off the string and binary getters and excludes numerics and protocol framing. That is the honest scope and also the useful one: the collectors this exists for are dominated by one large text column, so string bytes are the payload to within a rounding error, and a number pretending to be the wire size would be a worse measurement wearing a better name.target_session_idis read off the open connection as a client property (SqlConnection.ServerProcessId/NpgsqlConnection.ProcessID), never withSELECT @@SPID— a round trip per collector per server per cycle is ~25,000 extra queries an hour against the fleet to learn a number the client already holds. It is what makes a stalled run joinable:waiting_tasks,dmv_blocking_snapshotandquery_snapshotsall carry a session id, so without ours "what was our own stalled session waiting on" cannot be asked even for a window where the answering snapshot was captured.sweep_peer_max_msseparates two populations that share one stored shape — the slowest non-budgeted collector already completed in the same sweep body. A genuinely large query runs beside peers at or below baseline; sweep-wide degradation shows those same light collectors at 34-47x baseline before the heavy ones burn their budget. Telling them apart previously meant cross-referencing neighbouring rows by hand.Budgeted collectors are excluded because they are the heavies being explained, and "is this one budgeted" is asked of the catalog rather than a name list kept beside it. That required moving
PerItemWallClockBudgetto the baseICollectorSchemaInfo, alongsideAppliesToandYieldsOnLockTimeoutand for the same reason those live there: the catalog is keyed by name and holds that interface. A hardcoded list would be correct today and silently wrong the moment a fifth collector earned a budget. Making it a required interface member is what made the compiler find all three test fakes rather than leaving them silently defaulted.Recorded on every row, not only abandoned ones: a ratio needs a denominator, and the baseline has to come from the same column on ordinary bodies. Storing it only on failures would rebuild exactly the cross-referencing this removes.
The shared-statement trap, proven red first
InsertCollectionLogSqlis still shared between the per-collector writer and the fleet-wide retention run-record, so both binding blocks widened to 22 together. The #2859 pin that asserts binding-count against placeholder-count across every writer was proven red before green by dropping a single retention binding — reproducing exactly the08P01: bind message supplies N parameters, but prepared statement requires Mthat failed silently last time, because both writers are failure-isolated by design and the exception goes to a Debug log.The literal
Assert.Equal(17, placeholders)is now 22. It is deliberately a hand-maintained literal: it is the tripwire that makes widening this statement a conscious act, and bumping it is the moment you are forced to ask whether every writer was widened too.Verification
The Windows suites cannot run on macOS, so pure logic was verified against the real build with a throwaway
net10.0harness — 27 checks, all passing:-1and never0— the distinction the whole change turns on, since 0 is a reachable real answerHasWallClockBudgetderives correctly for the budgeted heavies, for an ordinary peer, and degrades to light on an unknown nameStorageVersionDEFAULTNot covered: no live Postgres was available in this session, so the rung's application against a real compressed hypertable is asserted from the SQL rather than executed.
Darling PostgreSQL testsis the arbiter for that.Encoding checked after the fact: every pre-existing file kept its BOM state, both new files are BOM-free per the directory convention, all files are CRLF, and CHANGELOG.md staged as 2 insertions / 0 deletions with a plain
git add— #2858's renormalization holding.Not built: item 4
The out-of-band watchdog stays unbuilt and needs its own decision. It is the only proposal that must break the sequential model, and it opens a connection to a target that is already not responding — so it wants deliberate bounding (one shot, hard timeout, never retried) rather than being added alongside instrumentation that touches nothing and issues no new query.
🤖 Generated with Claude Code
https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy