Skip to content

fix: ignore excluded raw ring slot - #129

Merged
NikolayS merged 3 commits into
mainfrom
release/fix-raw-retention-boundary-128
Jul 27, 2026
Merged

fix: ignore excluded raw ring slot#129
NikolayS merged 3 commits into
mainfrom
release/fix-raw-retention-boundary-128

Conversation

@NikolayS

@NikolayS NikolayS commented Jul 10, 2026

Copy link
Copy Markdown
Owner

What changed

  • makes ash._active_slots() enumerate the configured retained ring offsets 0 .. num_partitions - 2
  • filters ash._raw_retention_start() to that exact readable slot set
  • adds a behavioral regression with a populated excluded oldest slot
  • adds a discriminating six-partition assertion for the generalized slot enumeration

Why

The physical oldest ring slot remains populated until its next truncate but is deliberately excluded from raw readers. Including it in the retention minimum made status() and report coverage advertise unreadable history and allowed old event/query tie drills to pass the guard before returning a silent zero.

Impact

Raw-retention metadata now describes data the reader helpers can actually access. An event/query tie drill over excluded history raises the documented outside-retention error.

This PR intentionally does not fix the time-bound path from #128. It also moves the boundary newer, making the partial-first-minute defect in #163 strictly worse; #163 must be fixed with or immediately after this change. Frozen 1.x installers still contain the old two-slot _active_slots() body and must not be reapplied as a terminal installer over 2.0.

Validation

Postgres 17.9 after rebasing onto origin/main at ce541ac:

  • RED: the new N=6/current=2 assertion exits 3 on the base with got {2,1}
  • GREEN at 2006159: _active_slots() returns exact {2,1,0,5,4} and the complete named retention regression exits 0
  • existing N=3 fixture still verifies {0,2}, helper/status agreement, and the outside-retention error
  • actionlint and git diff --check pass
  • codex review --base origin/main reports no introduced correctness issue

Refs #128

@NikolayS
NikolayS force-pushed the release/fix-raw-retention-boundary-128 branch from cc80a64 to 2006159 Compare July 27, 2026 16:50
@NikolayS
NikolayS marked this pull request as ready for review July 27, 2026 16:50
@NikolayS

Copy link
Copy Markdown
Owner Author

Ready, with a caveat — Both halves RED on base. _active_slots() rewrite is required, not gratuitous (base returns {2,1} for N=6 while both _for variants return 5 slots). No perf regression (InitPlan Limit over MergeAppend). Do not close #128 — the time-bound path to the same fabricated zero is untouched. Fragility: frozen 1.x scripts re-create _active_slots() with the old two-slot body, now load-bearing. And see B3 — this makes B3 worse.

Rebased by hand onto origin/main at ce541ac with no conflict.

RED on the rebased base: Test 2.0: raw retention ignores the excluded ring slot exits 3 at the new discriminator with _active_slots() for N=6/current=2: expected {2,1,0,5,4}, got {2,1}.

GREEN on 2006159: the exact same complete step exits 0; the N=6 helper returns {2,1,0,5,4}, then the existing excluded-slot helper/status/error-path assertions pass.

codex review --base origin/main is standing in for REV and found no introduced correctness issue: retained-slot enumeration matches rotation semantics, and the retention boundary uses the same readable slot set. Fresh-install SQL checks and workflow syntax validation passed.

Caveats accepted explicitly: this does not fix #128's time-bound path, so #128 stays open. Moving the boundary newer makes #163 strictly worse; #163 must be fixed with or immediately after this PR. Frozen 1.x installers still carry the old two-slot helper body and are now load-bearing migration fragility.

@NikolayS

NikolayS commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Heads-up — merging this will require a one-line test change once #219 lands.

#219 (fixes #212) redefines ash.status().raw_retention_start from the exact oldest retained sample to the configured ring boundary (rotated_at - (num_partitions - 2) * rotation_period, floored to the minute). That split is the fix for #212: the field is documented as a window-planning boundary, but returning min(sample_ts) made the README's flagship drill hard-error on any young install.

This PR's assertion that raw_retention_start equals the exact oldest retained sample is correct today and becomes wrong afterwards.

Composition was tested against this PR's head 2006159 rather than assumed. The code cherry-picks with no textual conflict; only that test expectation needs updating, to compute the boundary from rotated_at, num_partitions and rotation_period. With exactly that change, both suites pass together:

NOTICE: raw retention excludes the oldest ring slot PASSED
NOTICE: issue #212 raw retention boundary tests PASSED
NOTICE: retention error split PASSED (boundary hint is followable, echoes passed start)

Recommended order: this PR first, then #219. Note also that this PR moves the boundary newer, which makes #212 strictly worse in the interim — so they should land close together.

Context: #160.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants