release: fix rollup_1h grain honesty - #182
Conversation
Document that retained AAS is weighted by the current sampler interval and that stored activity cannot prove sampler coverage. Coordinate timeline and rollup wording with NikolayS#175 and PR NikolayS#182 without changing reader behavior.
REV Code Review Report
Result: PASSEDNo outstanding security, correctness, test, guidelines, or documentation findings remain. Findings addressed during review
Review coverage
The post-rebase audit also confirmed that the concurrently landed #127, #129, #133, and #152 behavior remains intact, including #133’s blocking hourly rollup lock and minute-watermark completeness guard. REV execution noteThe installed GitHub REV command skips draft PRs by design, and this task explicitly forbids marking the PR ready. A read-only full-diff REV-compatible parallel review using the postgres-ai/rev rubric; transparent fallback after the stock draft-aware wrapper timed out. |
Compare the bare rollup_1h.ts column against bigint bounds in _rollup_1h_has_flat(). This preserves overflow safety while allowing the (ts, datid) primary key to serve narrow historical windows instead of scanning the full archive.
NikolayS
left a comment
There was a problem hiding this comment.
Codex review (standing in for REV) — PASS
- Reviewed head:
4fa269b2f2e8934017282e07eb51d4b66bc61901 - Rebased onto:
86e0ca834d6cdcb21af0a5deea1f1250b1040df9(#189) - Verdict: no remaining correctness, security, migration, documentation, or test blocker.
Audit contract
The B1 audit required: “NULL peak_aas/p99_aas whenever the read grain exceeds the requested bucket; add source_1/source_2 (or an effective-bucket column) to ash.compare(); route the database dimension through rollup_1h_minutes; and mark legacy flat hours.” It also required the N1 backfill invariant to reject partial minute_counts.
This head satisfies that contract and genuinely covers all of #130: filtered aas, filtered timeline, every top dimension, dimensional compare, chart, and summary avoid clipped-hour denominators. Hour-only paths snap outward and disclose their effective plan; unfiltered/database-only paths retain exact minute precision. Zero-argument and now()-relative calls remain usable, so #131 is superseded without importing its regressions.
Rebase and collision audit
The four source commits rebased textually cleanly, then were checked semantically:
- #189 survives:
_raw_oldest_sample()remains the physical evidence boundary,_raw_retention_start()remains the logical planning/loss boundary, source selection and report attribution use physical evidence, and the API/examples retain that distinction. - #181 survives: the dynamic reader bundle grants all 43/43 current non-admin functions (the prior 41 plus
_minute_counts_valid()and_rollup_1h_has_flat()), including_raw_oldest_sample();_admin_funcs()remains denied. - #185/#193 rotation and deterministic-upgrade fixtures remain intact.
RED / GREEN
Exact RED on current main:
ERROR: B1a aas avg=1.15 peak=10.00 p99=4.69; top wait avg=0.77 peak=0.77 p99=0.77; database avg=1.15 peak=1.15 p99=1.15
Exact rebased GREEN:
- complete B1/#130/default-call block:
rollup_1h grain honesty, partial-hour, and default-call tests PASSED - real v1.5 upgrade: B1d returns one
rollup_1h_flatdatum with NULL extrema; N1 partial stays NULL; N1 complete has 60 slots and minute/hour sums both 4140; re-apply repairs an earlier partial array - supplemental partial-window
wait_event,query_id, and dimensionalcompare()probe passed with widened bounds and NULL extrema - #189 raw-boundary block, rollup-source/seam blocks, and #181 reader-helper closure passed
Codex finding resolved
The initial full-diff Codex review found one P2: _rollup_1h_has_flat() cast the indexed rollup_1h.ts column, forcing a full-archive scan. Commit 4fa269b moves overflow-safe arithmetic to bigint bounds while leaving the indexed column bare. PostgreSQL now reports:
Index Scan using rollup_1h_pkey
Index Cond: ((ts < ...::bigint) AND (ts > ...::bigint))
A follow-up Codex review confirmed mathematical equivalence, overflow safety, and index eligibility, with no remaining finding.
CI
Fresh rebased CI is green: docs-lint and PostgreSQL 14/15/16/17/18/19beta1 plus PG17 no-cron in Test run 30304779707; CodeQL is green in run 30304776247.
REV itself was not run; this is the required codex review substitute, labelled explicitly.
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
Fixes the
rollup_1hgrain collapse by separating requested/effective buckets from the grain actually retained, exposing provenance, and returningNULLinstead of fabricated peak/p99 values when retained data is too coarse.ash.aas(),ash.timeline(),ash.top(), andash.compare()follow the same grain-honesty ruledatabasedimension through valid per-databaseminute_countsrollup_1h_flatminute_countsin bothrollup_hour()and the v1.5 → 2.0 backfillash.summary()separate headline and wait/query drill provenance so widened drill values cannot inherit narrower labelsnow()-relative APIs workingFixes #161.
Fixes #168.
Fixes #175.
Fixes #130.
This supersedes #131; I recommend closing #131 because this branch satisfies #130 without its hard-error/default-call and database-precision regressions.
This PR needs a fresh rebase onto
origin/mainimmediately before merge. It is intentionally a draft and must not be merged as-is later without that rebase.Audit fixture behavior
aas()1.15 / 10.00 / 4.69top('wait_event')0.77 / 0.77 / 0.770.77; peak/p99NULLbecause the read is hourlytop('database')1.15 / 1.15 / 1.151.15 / 10.00 / 4.69from the per-database minute array2.30 / 2.30 / 2.30at a requested minute bucket2.30, seconds8280.00, effective bucket1 hour, peak/p99NULL; an explicit one-hour request remains2.30 / 2.300.00but peaks10.98 / 600.00and p9910.98 / 246.59rollup_1h / rollup_1m, averages10.98 / 10.98, delta0.00, and both peak/p99 pairsNULLbecause retained grains differ10.98 / 10.98, peaks600 / 600, p99246.59 / 246.59rollup_1h, points60, avg0.17, 60 non-NULL extremarollup_1h_flat, points1, avg0.17, no extrema;aas()also reports flat provenance and no extrema1800vs hourly sum3600minute_counts IS NULL4140equals hourly sum4140summary()asymmetric edge load[h+30m,h+90m)avg0.00, but hour-only drill values had no separate plan metadatarollup_1h,[h,h+2h), bucket1h, and top IO avg5.00For #130 specifically, the half-hour IO
aas()request now discloses[h, h+1h), avg1.00, seconds3600.00, peak/p99NULL; the crossing wait window discloses[h, h+2h), avg0.50, seconds3600.00, peak/p99NULL; database keeps[h, h+30m), avg/peak/p991.00 / 1.00 / 1.00, seconds1800.00.RED (captured before implementation)
B1a
B1b
B1c
B1d (real v1.5 install upgraded through
sql/migrations/ash-1.5-to-2.0.sql)N1 (same real v1.5 upgrade)
The overlapping #130 assertion also failed before implementation:
Post-review summary provenance RED:
GREEN (verbatim)
Focused reader block:
Real-release upgrade block:
Summary provenance GREEN values:
Full post-rebase local job:
Validation
2af229a44095aac4a983f38f5a07311f61c08158, rebased ontoorigin/mainat64265009e25acc8f23187e3e2f40e3754b7724afgit diff --checkpassedAPI/documentation changes
blueprints/AAS_API.mdnow documents exact return schemas and the new provenance/effective-window fields, requested versus effective bounds, upward bucket widening, hour-only dimensional snapping, database minute precision, flat legacy hours, cross-grain compare suppression, summary headline-versus-drill provenance, chart NOTICE behavior, and the #137 idle-versus-uncovered limitation. Related examples, user stories, spec, README, and catalog comments use the same contract.Deliberately not attempted
This does not add coverage markers capable of distinguishing a sampled idle minute from an uncovered minute. The current storage model cannot represent that distinction; #137 is the architectural follow-up. Wait/query dimensions also cannot recover minute extrema once only hourly aggregates remain, so these readers now return exact averages/totals but honestly return
NULLextrema instead of inventing detail.