Repository navigation
State the tail-overlap condition where the recovery claim is made, and pin it to the cadence - #3047
Merged
Merged
Conversation
…d pin it to the cadence The schedule table described pg_deadlocks as matching pg_plan_capture; they run five minutes and sixty minutes apart. Both read the same file through the same pg_read_file call with the same 4 MB tail, so the overlap those comments rely on for truncation recovery holds only while the log grows by less than one tail per interval - about 14 KB/s at five minutes and 1.2 KB/s at sixty. Past it consecutive reads stop touching and the span between them is read by nobody. The four comments that lean on that overlap now state the condition and the rate it holds under. LogTailOverlapThresholdPinTests derives each rate from the schedule table and the collectors' own tail constants and compares it against the figure the prose states, so changing a cadence or the tail fails rather than leaving a stale number behind.
|
Reviewed. This PR is comment-only (no query/behavior changes) plus a new pin test, and it holds up:
One gap: the PR description drafts a No correctness, security, or SQL-style issues — no T-SQL/query changes at all in this diff. |
This was referenced Sep 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The schedule table described
pg_deadlocksas "Every 5 minutes against a 4 MB tail, matching pg_plan_capture."pg_plan_captureruns every sixty minutes. The two do read the same file the same way — samepg_read_filecall, same 4 MB tail, no resume marker on either — which is what makes the twelvefold cadence difference matter rather than merely be untidy.What the overlap claim actually requires
Three doc comments explain truncation recovery in terms of consecutive reads overlapping:
PgDeadlockLogParser's "re-reads an overlapping byte-window tail every cycle, so a report cut at one read's edge arrives whole in the next",PgPlanLogParser.Extract's "a block cut in half at the edge is an ordinary consequence", andPgPlanCaptureCollector'sTailBytessummary.The read is always the last
TailBytesof the current file, so two cycles see overlapping text only while the log grows by less than one tail between them:pg_deadlockspg_plan_captureAbove that the windows stop touching and the span between them is read by no cycle. That is a different outcome from the one those comments describe: not a report cut at an edge and completed next time, but whole reports and whole plans never read at all. It is silent in both directions — nothing measures the log's write rate, so neither collector can distinguish a quiet server from a truncated view of a busy one, and downstream it reads as a target with no slow queries.
1.14 KB/s is about 98 MB/day. That is a low bar; a server doing nothing but logging connections can clear it.
The two transports fail in opposite directions, and one schedule number serves both
pg_plan_capture's schedule key dispatches on target type (DarlingWorker, theIsAurora || IsAwsRdsbranch):The same is true of
pg_deadlocks, which is already at five minutes.pg_plan_capturealso sits inside a comment justifying an hourly group on the grounds that "every one of these reads a CUMULATIVE counter ... or an append-only log, so a longer interval loses no events — it only widens the window each delta covers." That reasoning is sound forpg_wait_sampling,pg_kernel_statsandpg_predicate_stats, which are genuinely cumulative counters. It does not hold for a bounded-window read of a log, where "append-only" is not the property that governs coverage — how much of it a read can see is. Plan capture was the fourth member of a group whose stated justification excludes it.What this changes, and what it deliberately does not
It states the condition at each place that leans on it, and pins the arithmetic. No cadence moves and the tail does not grow.
Why not drop
pg_plan_captureto five minutes. It would make the parity claim true, and it does move the threshold to a comfortable place. But the cadence is shared with the consume-once transport that has no such exposure, and read-time dedup on(query_id, plan_hash)is a read concern —collect.pg_plan_capturehas no unique constraint and every read groups, so twelve times the reads is twelve times the stored rows of full plan JSON against a 14-day retention, paid on every target to fix a threshold only self-hosted ones cross. That is a trade worth making deliberately rather than as a side effect of correcting a comment.Why not grow the tail. It moves the threshold without establishing one.
TailBytes' own summary cites #2565's measurement of 772 MB of log in twenty seconds at capture-everything — roughly 38 MB/s, about 34,000x the hourly threshold and 2,800x the five-minute one. No fixed tail bounds that.What the real fix is, and why it is not here. The gap is not the size of the window, it is that the collector cannot say it missed anything. The query already selects the file's
size; a stored previous size would make the skipped span a measurement instead of an absence. That is a new collector behaviour and a schema addition, so it belongs in its own change rather than riding a comment correction. Until then the interval is overridable per server (config.collector_schedule), and the comments say so.The pin
Lite.Tests/LogTailOverlapThresholdPinTests.cs. The thresholds are derived fromCollectorScheduleDefaultsand each collector's ownTailBytesLiteral, then compared against the figure the prose states — so changing a cadence or the tail fails rather than leaving a stale number behind. It also asserts both collectors read the same tail, that the two cadences are not equal, and that no comment claims they are.Three things it does deliberately:
TheParityDetector_FiresOnTheClaimItGuardsAgainstfeeds it the exact sentence being removed and a reworded variant, in both name directions. Without it the assertion would pass on any source that simply never says the word — an absence with no positive control behind it, which is the same shape as the drift.CollectorScheduleDefaults.cslegitimately states both thresholds, so each file must contain its own collector's figure and every figure stated must be one of the two the arithmetic produces. A first version asserted file-level and failed on the schedule table for the wrong reason.Verification
Lite.Teststargetsnet10.0-windowsand cannot execute on macOS (Microsoft.WindowsDesktop.Appis absent for osx-arm64), so:dotnet build Lite.Tests -p:EnableWindowsTargeting=true→ 0 errors, and none of the 9 warnings come from this file.Assert.Failand theEqual(double, double, int)overload are real, not shimmed.net10.0console harness thatCompile Includes the actual test source unmodified with an xunit shim, and a symlink so the pin's upward file walk resolves to the branch under test rather than to the main checkout.TailBytesLiteralrenamed. Three of them fail different arms of the same theory (the floor, the own-threshold arm, the known-set arm).git status --porcelainclean afterwards. The harness asserts each variant produced a test summary, because a variant that never compiled is indistinguishable from one that passed.Not verified: no live self-hosted PostgreSQL target exists in this fleet to observe the gap on — the arithmetic and the transport split are read from source and from #2565's recorded measurement, not from a target that lost a plan. The 772 MB/20s figure is quoted from the existing comment, not re-measured.
CHANGELOG entry text
Fixed — The
pg_deadlocksschedule comment claimed a cadence match withpg_plan_capture; they run five and sixty minutes apart. The comments that rely on consecutive log-tail reads overlapping now state the condition that makes that true — log growth below one tail per interval, about 14 KB/s at five minutes and 1.2 KB/s at sixty — and a new pin derives each rate from the schedule table and the collectors' tail constants so a cadence or tail change cannot leave a stale figure behind.