release: fix raw retention boundary - #189
Conversation
REV Code Review Report
No issues found. Reviewed the final pushed diff for security, runtime/SQL correctness, exact regression coverage, repository guidelines, API contracts, documentation, upgrade behavior, and merge sequencing. Result: PASSED
Manual evidence included the three independent #163 RED assertions on the #129 base, the copy-pasted README drill on a fresh default-start install, status-boundary self-feed, down-rounded remediation preserving all eight samples, delayed-rotation evidence, the full discovered upgrade chain, fresh/upgrade schema equivalence, and the complete reader-helper privilege closure. Merge-sequencing note (non-blocking): land B3 before rebasing #180/#182; reconcile their shared report/workflow/source-provenance regions and rerun the exact boundary tests. #181 must retain dynamic non-admin helper enumeration. REV-assisted review (parallel analysis following postgres-ai/rev) |
PR NikolayS#185 deliberately rejects sub-day rotation_period values. Keep that product contract intact and scale NikolayS#189's synthetic retention windows from minutes to whole days instead. The same entirely-past, partial-overlap, delayed-evidence, and down-rounded-boundary assertions remain in force.
NikolayS
left a comment
There was a problem hiding this comment.
Codex review report — PASS
Reviewed rebased head e724827 against current main (1fcf9e7). No remaining blocker.
- RED: all three exact #163 regressions fail independently on main: the README tie drill raises and advises the next minute; feeding
status.raw_retention_startback into the drill raises; following the advice yields{up,0,NULL,…}. - GREEN: the consolidated README/default-start/status/advice block passes on PostgreSQL 17.9, as do the full behavioral reader step and the report/compare/retention/chart step.
- Rebase conflict: combined #185's day-granular rotation and
consecutive_rotate_failuresstatus wording with #189's logical-vs-physical retention wording. - Semantic collision fixed: two #189 fixtures still assigned a 30-minute
rotation_period, which #185 deliberately forbids. Commite724827retains the product validation and scales the synthetic windows to one-day geometry; the entirely-past, partial-overlap, delayed-evidence, and down-rounding assertions are unchanged in substance. - #181 privilege bundle: 41/41 current non-admin functions are granted.
ash._raw_oldest_sample()is explicitly EXECUTE-able/callable by the reader, survivesrebuild_partitions, and is removed byrevoke_reader. The historical PR-body count of 42 became 41 because #181 intentionally moved_admin_funcs()out of the reader set. - Previous-fix cross-check: #185's cleanup/rotation regression still passes on this installer.
- Additional verification: fresh install, discovered full upgrade chain, YAML parse, and
git diff --checkpass. Automated Codex review and an independent adversarial review found no issues.
This is verification and collision resolution against the existing #160 audit; no assertions were deleted or weakened.
Preserve honest retained grain across aas, timeline, top, compare, chart, summary, and v1.5 backfills; disclose effective plans and mark legacy/incomplete hourly detail as rollup_1h_flat.\n\nThe final rebase preserves #189's physical/logical raw-retention split and #181's dynamic reader grants. The Codex review's full-archive scan finding is fixed by keeping _rollup_1h_has_flat() indexable. Fixes #161, #168, #175, and #130; supersedes #131.
Keep #180's until-relative compare defaults exactly once. The #182 reader expansion carried an older duplicate declaration block whose now-relative defaults would both fail function compilation and discard the B5 contract. Use #189's _raw_retention_start() and _active_slots() helpers for #180's alternate-coverage NOTICE instead of reintroducing the pre-B3 retention formula. This keeps the corrected logical/physical raw boundary split and excluded-slot rule intact. Adapt the sparse-hour C1 gate to #182's honest rollup_1h_flat plan: aggregate readers disclose one widened hourly datum, while report still returns silent NULL because exact per-minute coverage is not derivable.
* fix: reject inverted reader windows * docs: clarify reader and scheduler diagnostics * fix: preserve reader contracts after rebase Keep #180's until-relative compare defaults exactly once. The #182 reader expansion carried an older duplicate declaration block whose now-relative defaults would both fail function compilation and discard the B5 contract. Use #189's _raw_retention_start() and _active_slots() helpers for #180's alternate-coverage NOTICE instead of reintroducing the pre-B3 retention formula. This keeps the corrected logical/physical raw boundary split and excluded-slot rule intact. Adapt the sparse-hour C1 gate to #182's honest rollup_1h_flat plan: aggregate readers disclose one widened hourly datum, while report still returns silent NULL because exact per-minute coverage is not derivable. * fix: make diagnostic source guidance actionable When cron.database_name is unset or names the current database, explain how to configure pg_cron or use the external scheduler. Recommend installing pg_ash in another database only when the setting names a real, different target. Document ash.report's rollup_1m-only contract in the README and guard both disclosures in CI. Exercise mutable cron.database_name placeholders only on the cron-off axis because preloaded pg_cron registers the GUC as postmaster-only. This closes the Codex pre-merge review findings without changing report behavior. --------- Co-authored-by: samo-agent <280144521+samo-agent@users.noreply.github.com>
Document that retained AAS is weighted by the current sampler interval, successful idle ticks are not persisted, and intervals above one minute can overstate minute extrema. Mark the affected user stories partial and expose the limitation in the README, release notes, blueprints, and sample_interval catalog comment. Hand-resolve against current main by retaining NikolayS#182's final grain/provenance contracts, NikolayS#180's until-only window and report disclosures, NikolayS#189's raw-boundary wording, and NikolayS#187's retired-benchmark caveat.
Document that retained AAS is weighted by the current sampler interval, successful idle ticks are not persisted, and intervals above one minute can overstate minute extrema. Mark the affected user stories partial and expose the limitation in the README, release notes, blueprints, and sample_interval catalog comment. Hand-resolve against current main by retaining #182's final grain/provenance contracts, #180's until-only window and report disclosures, #189's raw-boundary wording, and #187's retired-benchmark caveat. Co-authored-by: samo-agent <280144521+samo-agent@users.noreply.github.com>
Summary
ash._raw_oldest_sample()) from the reusable minute-aligned planning/loss boundary (ash._raw_retention_start())ash.start()callRoot cause
ash._raw_retention_start()exposed the exact first sample second. Readers query minute buckets, so status could not be fed back intosince, and the guard rounded its advice up past the only partial minute. The same physical timestamp was also being used for source attribution and public planning even though those are different contracts.Verification
main(1fcf9e7): README drill errors, status self-feed errors, and advice rounds up to an empty resultgrant_readerdynamic helper closure: 41/41 current non-admin functions after release: fix reader privilege blockers and sample slot race #181, including direct EXECUTE/call ofash._raw_oldest_sample(); rebuild preserves it and revoke removes itgit diff --check: passRebase resolution
Rebased onto current
main(1fcf9e7). Theash.status()catalog-comment conflict preserves both #185's day-granular rotation /consecutive_rotate_failurescontract and this PR's logical-vs-physical retention contract. Two test fixtures that still used a now-invalid 30-minuterotation_periodwere converted to the deliberate one-day contract and their entirely-past/partial/delayed-evidence ranges were scaled without weakening any assertion.Merge sequencing
Land this retention-boundary change before rebasing sibling PRs #182 and #180. PR #181's dynamic non-admin helper enumeration is retained and verified for the new helper. Expect documentation conflicts in
blueprints/AAS_API.mdandblueprints/AAS_EXAMPLES.mdwhen #182 and #186 are rebased.Fixes #163