Re-derive HeaviestHourlyRefreshObservedCeilingSeconds from a job_history census: 594 s becomes 896 s, and the watch-line ordering inverts - #3168
Conversation
…ory census The constant was 594 s, the maximum of a 16-run record that mixed a census of the boundary day's tail with a sample of the days after it. The census read the comment named as the fix has been done: every run of job policy_refresh_continuous_aggregate query_store_stats_interval_hourly since the 13:44:23 boundary, one row per run from timescaledb_information.job_history, read at 2026-09-08 01:37Z. 57 succeeded runs after the boundary, 194 s to 896 s, median 418 s, none failed and none without a finish time; 304 runs at or before it, median 1081.7 s, maximum 13300.7 s. The estimator is unchanged, so the constant is 896. Two pre-registered triggers fired together. The 95th percentile is no longer the maximum at 57 readings (830 s, 66 s below it), so the sample-size half of the maximum-versus-percentile argument is retired and the decision is re-taken on the half that never referenced n. And the sizing figure now sits 146 s ABOVE RefreshSlotWarningSeconds, so every assertion holding the grid clear of it is false. Those are left with their conditions exactly as written.
… state it At 896 s of a 900 s slot the refresh still fits, so "inside one phase slot" is true and load-bearing; the margin that made it "well inside" is 4 s and naming it as comfort is the claim that has stopped being true.
ReviewThis PR re-derives Confirmed: an untracked test failure alongside the acknowledged onesThe new ceiling (896 s) sits above
There's a fourth spot with the identical inversion that the diff doesn't touch and doesn't mention: Assert.True(
ceiling < TimescaleSupport.RefreshSlotWarningSeconds, // 896 < 750 is now false
...);
Assert.Equal(26, (TimescaleSupport.RefreshSlotWarningSeconds - ceiling) * 100 / ceiling); // now negative, not 26With Bigger pictureGiven how meticulously every other consequence of the census re-derivation is traced through the doc comments and tests, this looks like a genuine oversight rather than an intentional omission — worth the same treatment (updated message + tracking reference) as the other three, or a fix, before merge. Separately, worth a maintainer's explicit sign-off that merging with multiple known-red assertions (pending the #3035/#3044/#3107 grid/scheduling decision) is the intended workflow here, since it means CI goes red on |
Three of the six failing methods carried a comment explaining why they are left failing and which decision owns the remedy; three did not, so the same inversion read as an oversight in half the places it appears. Conditions are unchanged in all six. NoCompressionMinuteStartsWhileTheHeaviestRefreshIsStillRunning also needed the distinction its two halves now have: the grid clearance genuinely passes (the nearest compression minute is 1,320 s past the heaviest start), and only the watch-line ordering and the 26%-gap literal derived from it are false.
… asserted The open-question paragraph closed on "the excluded run is disqualified by its duration alone", which was the belt to the boundary's braces. At a 896 s population maximum 864 s is an ordinary member of the post-boundary range, so that belt is gone and the sentence was contradicting the paragraph above it. The positional rule needs no second reason, which is why it was chosen over one.
Review summaryThis PR is a pure Darling-side change (constant + doc-comment/test re-derivation in I independently re-summed and re-sorted the published 57-value population (boundary-day tail + days-after list) by hand: total 27799, median 418, 95th-percentile-by-nearest-rank 830, 90th 786, count Main concern: this PR ships with 6 tests deliberately failing, and those tests run as required CI checksThe PR description is explicit that six test methods go RED as a direct result of this change and are left that way on purpose, pending a future scheduling decision (#3035/#3044/#3107):
I checked Concretely, e.g. So, as far as I can tell, merging this PR as-is will turn the required Darling CI checks red on Is the intent here to merge this with CI red and land the scheduling fix in a fast-follow, or should these six assertions be provisionally adjusted (with a tracking comment/issue) so CI stays green until #3035/#3044/#3107 is decided? Worth confirming explicitly before merge, since it affects everyone else's PRs through the shared required checks. Everything else in the diff (the constant's derivation, the updated regex patterns in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <para><b>The GRID half of this test still passes and the WATCH-LINE half now FAILS, on #3166's census | ||
| /// re-derivation, and the two conditions are left exactly as written.</b> The grid is genuinely clear: at | ||
| /// a 896 s ceiling the nearest compression minute is still 1,320 s past the heaviest refresh's start and | ||
| /// the discriminating minute is still excluded, so nothing overruns anything. What is false is | ||
| /// <c>ceiling < RefreshSlotWarningSeconds</c>, and the 26%-gap literal derived from it — the sizing | ||
| /// figure is 146 s ABOVE the line, so the watch now reports the grid's own sizing rather than anything | ||
| /// new. Restoring the ordering means moving the line, moving the slot, or making the refresh cheaper, | ||
| /// which is a scheduling decision (#3035, #3044, #3107); re-typing either condition here is the only | ||
| /// edit that makes the failure go away while changing nothing about the store.</para> |
There was a problem hiding this comment.
This doc comment (and the matching ones on the other five methods called out in the PR body) documents that the test's Assert.True(ceiling < TimescaleSupport.RefreshSlotWarningSeconds, ...) a few lines below now evaluates 896 < 750 and fails.
Worth confirming before merge: .github/workflows/build.yml's "Run Darling tests" step (and the darling-pg job, meant to be a required check per its own comments) run this suite with no continue-on-error and no Skip on these [Fact]s. So this isn't just a local/expected-red pin — merging as-is turns the required Darling CI checks red on dev until the follow-up scheduling decision (#3035/#3044/#3107) lands, blocking green CI for every subsequent PR that touches Darling/core/root paths in the meantime. If that's the intended trade-off, fine — but it's a different situation from this file's existing "pin written to expire" pattern (which goes red later, organically) since here the same commit causes and ships the red state together.
|
Claude posting for Erik Darling Closed as superseded by PR #3178, which merged to Its work landed. The 594 → 896 re-derivation, the census, and the doc are all on Why it could not simply merge, recorded because the reasoning outlived the branch: its six reds needed The lane's discipline here is the reason the redesign happened rather than a band being re-typed: it widened, inverted and deleted nothing, and named the 'provisionally adjust the six' option as the trap — each of those tests is an ordering or a verdict, so re-typing one greens CI while changing nothing about a 4-second clearance. |
…of the PR bodies The entry text does not only ride the PR description. #3150's own account of the previous batch says 11 of its 66 entries came from a lane's handoff file in `~/Documents/pm-issue-queue-inbox/inbox/` rather than the body, and three files there carry dedicated entry text inside this range: `3164-changelog-entry.md`, `3166-changelog-entry.md` and `3169-changelog-entry.md`. Searching only the bodies missed the first two. - **#3164** (PR #3176) is no longer written from the PR description. Its lane wrote a full entry, and that text is used verbatim: it carries the 1,036 blocks/s upper bound, the reason the decoupling #3153 deferred is unreachable, the declined command-deadline route, the census deliberately not restated at the new ceiling, and the mutation that caught nothing. `[#2997]` joins the definition block at its ascending position, which that text cites and the file did not define. - **#3166** (PR #3168, absorbed and superseded by #3178) had no entry at all. #3178 grouped it into its own citation with a one-line summary, but its lane wrote a separate entry for the census that took `HeaviestHourlyRefreshObservedCeilingSeconds` from 594 s to 896 s and inverted the watch-line ordering. It sits beside #3178's, on #3178's merge. - **#3191's entry** said "#3184 named four of the six". #3184 was squash-merged and the four-of-six state never reached `dev`; it was a commit inside that PR, corrected before merge. "shipped" in this file means merged, so the clause now says a commit in #3184, which is what the #3187 lane's own report establishes. `CHANGELOG.md` only, 60 insertions and no deletions against `origin/dev`: 33 entries (6 Added, 3 Changed, 24 Fixed) and 27 definitions. All 15 definition runs keep the ordering they had, 9 of 15 ascending before and after.
…ix merged pull requests (#3213) * Record the CHANGELOG entries for thirty-one changes across twenty-five merged pull requests Applies the `[Unreleased]` entries for the pull requests merged to `dev` from #3150 (the previous batch) through #3203. Lanes do not edit `CHANGELOG.md` -- every one of them appends to the same block, so a per-PR edit conflicts with whichever sibling merges first -- and the entry text rides each PR's own description instead. This is the periodic commit that applies it. `CHANGELOG.md` only, 56 insertions and no deletions: 31 entries prepended inside `## [Unreleased]` (6 under `### Added`, 2 under `### Changed`, 23 under `### Fixed`), newest merge first within each section, plus the 25 link-reference definitions they need appended at the foot of the file. Nothing already in the block was edited, reworded or reordered. Three of the 28 pull requests in the range earn no entry, each on measured precedent rather than judgement: #3150 is the previous batch pass itself, and no batch pass -- #2260, #2980, #3150 -- is cited anywhere in the file; #3130 and #3131 are routine Dependabot bumps, and none of the six merged Dependabot pull requests in the repository's history is cited either. Two pull requests carried no entry text anywhere, so theirs are written from their descriptions: #3176 (`pg_index_bloat`'s measured block rate) and #3184 (the PostgreSQL target's permissions section). * Record #3205's entry, which arrived on the merged base mid-batch #3205 merged while this batch was assembled, so `origin/dev` was merged in rather than rebased and its entry lands on top. An entry omitted because it arrived mid-batch is how a backlog restarts, and the batch's purpose is to make `[Unreleased]` current at its own merged base. One entry under `### Changed`, newest merge first ahead of #3195's, plus its link-reference definition. #3205 has no other pull request in the range behind it. * Take #3164's and #3166's entries from the lane handoff files instead of the PR bodies The entry text does not only ride the PR description. #3150's own account of the previous batch says 11 of its 66 entries came from a lane's handoff file in `~/Documents/pm-issue-queue-inbox/inbox/` rather than the body, and three files there carry dedicated entry text inside this range: `3164-changelog-entry.md`, `3166-changelog-entry.md` and `3169-changelog-entry.md`. Searching only the bodies missed the first two. - **#3164** (PR #3176) is no longer written from the PR description. Its lane wrote a full entry, and that text is used verbatim: it carries the 1,036 blocks/s upper bound, the reason the decoupling #3153 deferred is unreachable, the declined command-deadline route, the census deliberately not restated at the new ceiling, and the mutation that caught nothing. `[#2997]` joins the definition block at its ascending position, which that text cites and the file did not define. - **#3166** (PR #3168, absorbed and superseded by #3178) had no entry at all. #3178 grouped it into its own citation with a one-line summary, but its lane wrote a separate entry for the census that took `HeaviestHourlyRefreshObservedCeilingSeconds` from 594 s to 896 s and inverted the watch-line ordering. It sits beside #3178's, on #3178's merge. - **#3191's entry** said "#3184 named four of the six". #3184 was squash-merged and the four-of-six state never reached `dev`; it was a commit inside that PR, corrected before merge. "shipped" in this file means merged, so the clause now says a commit in #3184, which is what the #3187 lane's own report establishes. `CHANGELOG.md` only, 60 insertions and no deletions against `origin/dev`: 33 entries (6 Added, 3 Changed, 24 Fixed) and 27 definitions. All 15 definition runs keep the ordering they had, 9 of 15 ascending before and after. * Use the staged #3184 entry text instead of one written from the PR description #3184's entry now exists, staged by the lane that did the work, so the batch no longer invents one. Used verbatim, in `### Fixed` where its author targeted it and for the reason they gave: the section did not merely lack detail, it asserted "One role covers every collector", which is false in the direction that fails silently and which an operator would act on. Their text carries what a description-derived entry could not: the `pg_read_all_data` grant is PostgreSQL 14+ and the pre-14 fallback the section gave was `GRANT SELECT` on a schema, which is not valid PostgreSQL at all; the six collectors that need an extension, four of which additionally need `shared_preload_libraries` and a server restart; and why `pg_index_bloat` is unaffected by any grant. Both figures are live pre-change store reads rather than estimates. The citation is `[#3184]`, the pull request, because the change has no issue behind it, and its definition uses the `/pull/` form. `CHANGELOG.md` only, still 60 insertions and no deletions against `origin/dev`.
1 participant
Closes #3166.
HeaviestHourlyRefreshObservedCeilingSecondswas 594 s, the maximum of a 16-run record the comment itself described as half census and half sample. The comment also named the read that would settle that:timescaledb_information.job_history, every run since the boundary rather than sampled. That read has been done, and the constant is now 896 s.The census
One row per run for job
policy_refresh_continuous_aggregate/query_store_stats_interval_hourlyon the store that carries this workload, read at2026-09-08 01:37Zovertimescaledb_information.job_history(TimescaleDB 2.28.1, PostgreSQL 18.4,timescaledb.enable_job_execution_logging = on). Boundary2026-09-05 13:44:23, exclusion positional as recorded.Zero post-boundary runs failed and none is missing a finish time, so the
succeededfilter removes nothing from the span — the census is the whole of it rather than a status-selected part.The read reproduces the published record exactly where they overlap, which is what says the method is the same one: the nine boundary-day values (
194, 222, 225, 335, 594, 465, 359, 293, 355) and the seven "days after" values (219, 225, 299, 376, 418, 546, 515) come back digit for digit, and the sampled seven turn out to be seven consecutive runs of 2026-09-06 with fifteen more either side of them that the sample never saw. #3119's separately-read closed day (2026-09-06, 22 runs, maximum 778.4 s) also reproduces.The population is republished in full — 9 + 48 — so the estimator is still recomputable rather than trusted. Range 194–896 s, total 27799 s, median 418 s.
The estimator is unchanged and the decision was re-taken rather than inherited
The doc's reason for the maximum had two halves, and one was written to expire. At 16 readings the 95th percentile by nearest rank was the maximum, so a percentile bought no headroom. At 57 it is 830 s, 66 s below the maximum, and the 90th is 786 s, 110 s below.
RefreshCeilingProvenancePinTestswent red on exactly that clause, which is what it was for. The decision comes out the same way on the half that never referenced the sample size — a scheduling exclusion may accept no exceedance over its own record, and a 95th percentile is a promise to be wrong three times in sixty runs — so the estimator stays the maximum and the expiring clause is gone with the argument it guarded. There is no replacement expiry, because a reason independent ofnhas nothing left to expire; both percentiles and both gaps are now pinned as readings that track the population.Three arguments in the old derivation expired with the sample, and none was re-argued to fit
The consequence, stated and not settled here
RefreshPhaseSlotSecondsis 900. Clearance is slot minus ceiling: 306 s before, 4 s now. The compression grid's spatial clearance survives — the nearest phase minute is 1320 s past the heaviest slot's start, and the discriminating minute:14is still excluded — so nothing has overrun a slot.What broke is the watch line ordering.
RefreshSlotWarningSecondsis 750, and 896 is 146 s ABOVE it, soClassifyRefreshSlotHeadroombands the grid's own sizing figureApproachingSlot. Seven of the 57 census readings are already at or past that line, where the 16-run record had none.RefreshSlotWarningSeconds' own summary pre-registered this: "a ceiling that rose past this line would put the grid's own sizing figure inside the warning band, and the pin says so rather than leaving a reader to notice."Six test methods are therefore RED, and every condition is left exactly as written.
RefreshCeilingProvenancePinTestsTheDerivedFiguresAreExactlyStateable_AndTheConstantIsThePopulationMaximumCeiling < WatchLineTimescaleSupportTestsNoCompressionMinuteStartsWhileTheHeaviestRefreshIsStillRunningceiling < RefreshSlotWarningSeconds;Assert.Equal(26, (watch - ceiling) * 100 / ceiling)(now -16)TimescaleSupportTestsTheRefreshSlotClassifier_BandsTheLiveReadings_AndKeepsTheWatchLineAboveTheRecordedCeilingClassifyRefreshSlotHeadroom(ceiling) == InsideSlot(nowApproachingSlot);ceiling < watchTimescaleSupportTestsTheRejectedWatchLineAlternative_SitsBelowTheRecordedCeiling_WhileTheChosenLineSitsAboveTimescaleSupportTestsTheRefreshSlotReading_CarriesItsOwnVerdict_AndReportsOverrunAsNegativeHeadroomatTheCeiling.Headroom == InsideSlotTimescaleSupportTestsTheRefreshSlotLogLine_IsLeveledByBand_AndSaysNothingWithoutAReadingWarning:, notDebug:Two failure MESSAGES in the pin file were re-pointed, because the reason they gave — "the doc comment claims otherwise" — is no longer why they matter now that the comment states the crossings plainly. Both conditions are byte-identical, so the tests still go red on the same input.
What the arithmetic says about the slot, and where it stops. Restoring the five-sixths line above 896 s needs a slot of at least 1076 s — 18 minutes, which 60 does not divide, so the grid would gain slots of unequal width or lose one of its four. Moving the fraction instead, or making the refresh cheaper, are the other two routes. Choosing among them is a scheduling decision (#3035, #3044, #3107) and is deliberately not taken by re-deriving a measurement.
Trend, not calibration
The daily maximum went 594 s (boundary-day tail, 9 runs) → 778 s (2026-09-06, 22 runs) → 896 s (2026-09-07, 24 runs). #3166 listed 702 / 871 / 815 s; the census found a 896.1 s run at 2026-09-07 23:00 that the issue did not have, so the readings in the issue were not the ceiling. A census removes the "as observed" caveat and puts nothing in its place — the population is closed at a stated instant and it grows.
Verification
RefreshCeilingProvenancePinTestsis pure and reads the realTimescaleSupport.csthrough[CallerFilePath], so it was compiled unmodified into anet10.0console harness behind a minimal xUnit shim and run: 8 of 9 pass, including the full derivationVerifyand the one-number-at-a-time drift sweep over every captured figure; the 9th is the watch-line row above. Red-first proven by reverting the constant to 594 in a committed tree, confirmingEveryDerivationClaim_...fails onmax == Ceiling, and restoring viagit checkout --. The red list in the table was produced by a second harness that evaluates each ceiling-dependent condition against the real Storage assembly rather than by reading the assertions.CHANGELOG
Not edited here — entry text is in the lane report for the coordinator to batch.