release: keep compacted query attribution honest - #198
Conversation
NikolayS#194 deliberately changed rollup_minute() from counting inserted database rows to counting every processed completed minute. The NikolayS#136 fixture starts twenty minutes in the past, so its unbounded setup call now correctly returns roughly twenty rather than the stale expected value 1. Limit that setup call to one grain and describe the assertion as one completed minute. The expected value remains 1 and every compacted-attribution assertion is unchanged. NikolayS#194 already records the return-contract change in RELEASE_NOTES.md.
e9944ed to
12f30cf
Compare
samo-agent
left a comment
samo-agent
left a comment
There was a problem hiding this comment.
Codex review standing in for REV — audit verdict: PASS on exact head 12f30cf.
Hand rebase: based directly on main 2ccdf65. The only conflict was blueprints/AAS_USER_STORIES.md; I retained both #186s historical-cadence/#137 limitation and #198s exact-query/raw plus NULL-residual rules.
RED/GREEN: current main is discriminating RED after the #194 setup adaptation: named compacted query load is 3.00 seconds at 100.00% instead of 11.54%. Exact head is GREEN: minute named/residual 3.00/11.54 and 23.00/88.46 (the NULL residual wins top-1); hourly top-100 returns 101 rows/400.00 seconds/100%, with NULL residual 100.00/25.00; young same-minute rollups stay raw while genuinely older coarser history is rejected.
#194 assertion adaptation: before, an unbounded rollup_minute() setup asserted return=1 and described one database row. After #194, that function intentionally counts every processed completed minute, so the 20-minute-old fixture correctly returned 20. The setup now calls rollup_minute(1), still asserts 1, and says one completed minute. No #136 attribution predicate changed. RELEASE_NOTES.md already records #194s return-contract behavior.
Cross-checks: #180 until-only/inversion/cross-horizon PASS; #182 grain honesty/partial-hour PASS; #194 one/two-datid, empty-grain, caught-up, row-count, and watermark contracts PASS. #189 physical/logical raw boundaries remain; #181s dynamic non-admin reader bundle covers the new helper. #122 remains explicitly unclaimed. Workflow YAML and diff checks pass. No merge-blocking finding.
samo-agent
left a comment
samo-agent
left a comment
There was a problem hiding this comment.
Codex review standing in for REV — head correction / final delta: PASS on exact head 8660412.
The prior review was submitted after this head had already advanced, so its GitHub commit_id is correctly 8660412 although its opening sentence named the previously verified 12f30cf. The only new commit is test-only: it separately asserts #194s processed grain and physical row count, and adds until-only plus inverted-window coverage to the young exact-query fixture. The full canonical #136 step passes on exact 8660412. No product assertion was weakened and the PASS verdict stands.
samo-agent
left a comment
samo-agent
left a comment
There was a problem hiding this comment.
Codex review standing in for REV — final CI-trigger head: PASS on exact head 5ef411f. This final commit is deliberately empty: its tree is byte-identical to reviewed head 8660412 (tree 47e7e3b296727cbf10043b9362bc53d61b4c5137) and exists only because draft-to-ready did not trigger the pull_request workflow. All prior RED/GREEN and cross-check conclusions therefore apply unchanged; the newly triggered full PR workflow must still pass before merge.
|
Temporarily closing and reopening to trigger the required pull_request CI event; the draft-to-ready and fork synchronize transitions created no workflow run. |
GitHub Actions rejects workflow files above 512,000 bytes before creating a run. The NikolayS#136 inline SQL raised test.yml from 506,109 to 535,600 bytes, which left NikolayS#198 with no check suite. Move that exact 749-line payload to devel/tests/query_attribution_assert.sql and invoke it with psql -f; the SQL is byte-identical after removing YAML indentation and every assertion remains unchanged. The workflow is now 506,353 bytes.
samo-agent
left a comment
samo-agent
left a comment
There was a problem hiding this comment.
Codex review standing in for REV — final verdict: PASS on exact head e349edd. Full PR CI is green across docs lint, PG14/15/16/17/18/19beta1, and the PG17 cron-off lane: https://github.com/NikolayS/pg_ash/actions/runs/30314259819. The final CI repair is assertion-neutral: GitHub rejected the prior 535,600-byte workflow above its 512,000-byte ceiling before creating any check suite. The 749-line #136 SQL payload is byte-identical after extraction (22,068 bytes, 30 assertions, SHA-256 26cdc4e29aa87e4de9664f26a73de1c40bd1b3a82d831e889ce3059eb0b08cb7), the psql connection and ON_ERROR_STOP behavior are unchanged, and test.yml is now 506,353 bytes. Prior discriminating RED/GREEN and #180/#182/#189/#194 cross-checks stand unchanged. No merge-blocking finding.
Combining NikolayS#192 with NikolayS#198 and NikolayS#199 makes test.yml 513,005 bytes, above GitHub Actions' 512,000-byte workflow limit. Move NikolayS#199's exact 110-line transaction fixture to devel/tests and invoke it with the same psql connection and ON_ERROR_STOP behavior. The payload is byte-identical after removing YAML indentation; all nine assertions remain. The combined inventory is 848 inline plus 184 shared, 1,032 total.
After NikolayS#198, grant_reader() correctly discovers 44 non-admin reader functions rather than the pre-NikolayS#198 count of 43. Update both the explicit-role and default pg_monitor feature contracts, and name _exact_query_uses_coarser(integer,integer,name) in a direct EXECUTE assertion so an unrelated 44th grant cannot mask a missing helper. The initial exact PG14/PG17 feature run failed 44-versus-43; all four feature-bearing surfaces now pass on both majors.
* build: add pg_cron release-gate image Keep the Docker tag independent from the numeric package major so the same image definition supports PostgreSQL 14 through 19beta2. Relates to #160 * test: share release-gate SQL assertions Source cron, degraded-mode, and schema snapshots from shared SQL while preserving all 594 workflow assertions. Include attnum in schema snapshots so column-order divergence is visible. Relates to #160 * feat: add Docker release-gate runner Run every gate surface across bounded parallel PostgreSQL workers, reuse exact CI step bodies, emit validated TSV and human summaries, and guarantee ID-scoped container and volume teardown. Relates to #160 * test: add behavioral feature coverage * test: add features release-gate surface * test: align feature contracts with privilege fixes * test: expose rollup return contract mismatch * fix: normalize ash.config upgrade ordinals Rebuild the v1.5 upgrade table in canonical fresh-install order while preserving the singleton row, defaults, constraints, indexes, ownership, ACLs, and comments. Reject unsupported custom dependencies and catalog properties transactionally instead of dropping them. Add an upgrade regression that seeds non-default values and custom catalog objects, verifies preservation, and proves migration reapplication keeps the replacement table OID. * docs: clarify config normalization transaction * test: adapt release gate to current main * test: include current PG17 upgrade coverage * test: distinguish rollup progress from rows * test: isolate feature pgss state The shared feature step runs after the full workflow has accumulated hundreds of pg_stat_statements entries. Dropping and recreating the extension does not clear the preloaded shared state, so the exact marker query can be missing even though the reader code is correct. Reset pg_stat_statements immediately after recreating it. This preserves the exact ash.top()/ash.samples() enrichment assertions while making their fixture independent of prior workflow activity. * test: reap cron-path workload Tag H-CI-3's three synthetic pg_sleep clients, terminate only those tagged backends on every exit, and wait for their client processes. The prior step left all three sessions live for later workflow steps, so features_lifecycle.sql correctly observed four active sleepers instead of its single owned fixture. Keep the exact active_count = 1 and decoded-row assertions unchanged; restore the quiescent precondition they test. * test: make report diagnostics PG19-safe PostgreSQL 19 beta 2 resolves text || jsonb as JSON concatenation, so both B5 ASSERT message expressions raise on the JSON token before reporting the contract. Cast the coverage object explicitly to text. The asserted bounds and minutes-with-data predicates are unchanged; this is a diagnostic typing fix, not a behavior change. The old exact head fails on PG19beta2, while the canonical fresh-install surface passes with these casts. * test: share stale-rollup selector gate Combining #192 with #198 and #199 makes test.yml 513,005 bytes, above GitHub Actions' 512,000-byte workflow limit. Move #199's exact 110-line transaction fixture to devel/tests and invoke it with the same psql connection and ON_ERROR_STOP behavior. The payload is byte-identical after removing YAML indentation; all nine assertions remain. The combined inventory is 848 inline plus 184 shared, 1,032 total. * fix: floor report bounds before epoch conversion ash.ts_from_timestamptz() rounds fractional seconds while casting to int4. report() previously converted first and divided by 60 afterward, so an endpoint at :59.5 or later crossed into the next minute before the documented minute floor. Floor v_from/v_to in timestamp space first. Add a deterministic +59.9-second feature assertion: the old code shifts both bounds and reports three of four data-bearing minutes; the corrected code preserves the exact four-minute coverage. Record the public behavior change in RELEASE_NOTES.md. No assertion is weakened. * test: include query-history helper in reader bundle After #198, grant_reader() correctly discovers 44 non-admin reader functions rather than the pre-#198 count of 43. Update both the explicit-role and default pg_monitor feature contracts, and name _exact_query_uses_coarser(integer,integer,name) in a direct EXECUTE assertion so an unrelated 44th grant cannot mask a missing helper. The initial exact PG14/PG17 feature run failed 44-versus-43; all four feature-bearing surfaces now pass on both majors. --------- Co-authored-by: samo-agent <280144521+samo-agent@users.noreply.github.com>
2 participants
Recovery / scope
Addresses #136.
This draft replaces auto-closed #190. GitHub closed that PR when its stacked base branch,
agent/w2-b1, was deleted after #182 merged; it was not rejected and had no submitted reviews, inline threads, or issue comments.This reworks and supersedes the approach in #150 without modifying that branch. It is now based directly on
mainat2ccdf65c613b7f79e00e83fcd4d1d7566e60d5c6, which contains #180, #182, #186, #189, and #194. This PR does not resolve or close #122. It also does not claim to resolve #156.Rebase recovery
Dropped as already upstream:
6f9a5af,1182691,eddda1d,2af229a) and theirblueprints/SPEC.mddelta; release: fix rollup_1h grain honesty #182 is now squash-merged asa1f0a30.ash._raw_oldest_sample()and the old_raw_retention_start() := _raw_oldest_sample()wiring. release: fix raw retention boundary #189 already landed the physical helper plus the logical, minute-aligned ring boundary; restoring the old wiring would regress Release blocker: fresh-install tie drills reject retained raw samples #163/release: fix raw retention boundary #189._raw_oldest_sample()for physical evidence and exact-asserts a deterministiclogical < old rollup < physicalsplit.The rebased series is:
1683b78— test: expose compacted query attribution gaps21f1bab— fix: keep compacted query attribution honestac28efc— docs: explain compacted query attribution12f30cf— test: bound query-attribution rollup setup8660412— test: verify rebased query-attribution interactionsCurrent-main rebase
The only textual conflict was in
blueprints/AAS_USER_STORIES.md. It was resolved by retaining #186's cadence/coverage limitation and #180's until-only/inverted-window contract while adding this PR's exact-query/raw and compacted-residual rules.#194 changed
rollup_minute()/rollup_hour()to return processed time grains rather than physical database rows. The #136 minute fixture now callsrollup_minute(1), and both residual fixtures separately assert their processed-grain count and their single physical row.What changed
top('query_id')emits a positiveNULLresidual equal tomax(total wait counts - preserved named query counts, 0)per source row. The residual enters the ranking stream beforen, so compacted-away load can win a top-N slot.query_countsonce withGROUPING SETSand itswait_countsonce. It avoids fix: keep compacted query attribution honest #150's second query-array scan and window-wide materialized expansion.query_idfilter uses raw samples. It raises only when ordinary planning selects a matching, database-scoped rollup with retained coverage in the requested slice before the physical oldest raw sample.rollup_1h.minute_countsmust contain a non-NULL pre-raw minute before the hourly envelope counts as lost exact-query history; legacy/incomplete detail remains conservative.compare()NULL-safely pairs rollup residuals, andsummary()renders them as(other / unattributed).Combined rule on the landed #182/#189 base
aas/timelinequery_idforces raw; matching pre-raw rollup history raises, while a young same-minute rollup does not.topquery_id, or query breakdown plus a wait filter, forces raw.n.compareThe known
_pick_source_agg()end-watermark/completeness gap remains #122 and is intentionally not claimed fixed.RED / GREEN
Rollup minute residual competes for top-N
RED recorded by #190:
GREEN on the recovered branch:
The block also exact-asserts two rows,
26.00backend-seconds,100.00%, one NULL-safe compare residual with delta0.00, and the exact summary label.rollup_1htop-100 compactionRED on current
main(2ccdf65):GREEN:
The fixture stores 100 named rows / 300 seconds after hourly top-set truncation while wait totals remain 400 seconds. The result is 101 rows / 400 seconds / 100%, with a 100-second residual and NULL peak/p99 at hour grain.
Young-install exact query filters
Current-main RED is a source-honesty failure, not an exception:
GREEN:
Exact assertions cover
aas(raw, one bucket, 7 seconds),timeline(60 rows, one data row, 0.12 AAS),top(7 seconds, 100%),compare(raw/raw, delta 0, 100/100), database scoping, and matching old-history rejection.The strengthened fixture retains the young raw row and asserts the ordering between the logical ring boundary, old rollup window, and physical oldest sample. A mutation that passed
_raw_retention_start()instead of_raw_oldest_sample()failed as intended:Residual cost measurement retained from #190
PostgreSQL 17.9, 43,200
rollup_1mrows, 100 query pairs and 32 wait pairs per row:Naive #150 spilled about 20 GiB of temp reads/writes; the final stream spilled about 79.5 MiB. Naive and final outputs were identical: 4,363,200 rows and total count 29,160,000. This is adjacent to #156's cost class but does not resolve it.
Validation
git diff --check— GREEN.CI remains responsible for the full PostgreSQL 14-19 matrix. This PR stays draft.
Landing verification (current main)
rollup_minute()to count processed time grains. The Bug: compacted query rollups are presented as exact AAS #136 fixture now bounds its setup withbatch_limit = 1; both residual fixtures separately assert processed-grain counts and physical rows.RELEASE_NOTES.mdalready records release: return processed grains from rollups #194's return-contract change.8660412preserves release: reject inverted reader windows and clarify diagnostics #180 until-only/inversion, release: fix rollup_1h grain honesty #182 grain honesty, release: fix raw retention boundary #189 physical/logical retention boundaries, release: return processed grains from rollups #194 one-grain/two-datid plus empty/caught-up behavior, and release: fix reader privilege blockers and sample slot race #181's dynamic helper grant closure; it also directly asserts until-only and inverted exact-query behavior.