[017] WAL and archiving area — one pass - #150
Merged
Merged
Conversation
… tasks forbid touching Both round-2 validator passes returned approved with zero criticals; the four major findings are applied. The consolidation of the role helper into Task 01 (Decision 19) had a side effect nobody spotted when it was made: three of Task 03's mutations, and one of Task 01's, were phrased as edits to SET ROLE and GRANT statements that now live inside the shared helper — a file Task 03's own acceptance criteria declare untouched. All four are re-aimed at the call sites. Also fixed: Task 03 claimed role names come from the helper, but the helper takes the name as an argument and Task 01 expects distinct ones, which deadlocked the executor; Task 03's grep gate demanded zero pg_ls_dir hits while its own hint requires keeping one in an explanatory comment; and Task 06's pause_test amendment replaced a false "cannot be tested" claim with another one — editPgConfig returns early on !db.Local, so that branch is drivable too. Task 05 now states honestly which half of its TestViews_Configure assertion is a real gate: the wal half, reddened by reverting Task 02's branch. The archiver half cannot be reddened by a Configure mutation, because New() already sets the same values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten tasks, four waves, two full validation rounds (five validator passes each), zero critical findings outstanding. Auto-approved per the autopilot rule. Also corrected at the source what individual tasks had been patching locally: the tech-spec still claimed Task 6's package runs without PostgreSQL (it panics), still listed "0 B" as the verbose panel's zero rendering (it prints a bare 0), and omitted two files Task 6 now touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'pgcenter report' now selects which of the two WAL-group screens to render: -W w for the pg_stat_wal report, -W a for the pg_stat_archiver one, matching the idiom -J c|t already uses for the two pg_stat_io screens (ADR [008]). options.showWAL changes from bool to string and the flag from BoolVarP to StringVarP. The mapping in selectReport is a closed whitelist with no default arm: an unmapped value falls out of both switches to the final return "", so validate() rejects it with "report type is not specified, quit". This is load-bearing rather than stylistic — ReportType is the tar-entry filter in isFilenameOK and the key into the view map in newApp, so a value leaking through would select a zero-value view and print a silently empty report. Knowingly breaking for the boolean -W. Two failure shapes replace it, both pinned by tests: bare '-W' as the last token yields pflag's own "flag needs an argument: 'W' in -W", and '-W -f dump.tar' assigns "-f" as the flag's value and exits via "report type is not specified, quit" before the input file is ever opened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add PgStatWALPG19 to internal/query/wal.go: PgStatWALDefault plus
round(wal_fpi_bytes / 1024, 2) AS "fpi,KiB", placed immediately after the
wal_fpi counter so count and volume sit adjacent and both land inside the
diffed range. SelectStatWALQuery gains a third branch returning
(PgStatWALPG19, 8, [2]int{2, 6}) for version >= PostgresV19; stats_age moves
to column 7 and stays outside the interval, since a text date_trunc value
inside it aborts the whole sample at strconv.ParseInt.
PG 14-17 and PG 18 constants and return statements are byte-identical — the
wal.go diff is additions only. internal/view/view.go needs no change: its
case "wal": already delegates to the selector.
Tests: the 190000 row of Test_SelectStatWALQuery is edited in place to
8/{2,6} and a 200000 row pins the branch as >= rather than ==; two
no-Postgres guards pin the fpi,KiB position and the untouched legacy
constants; Test_StatWALQueries is upgraded from an exec smoke test to a live
FieldDescriptions() contract asserting Ncols and the ordered PG 19 header.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 04 found two things outside its own files, both verified by running them. The specs claimed both -W failure shapes exit non-zero. They exit 0 — main() prints and returns without os.Exit(1). Pre-existing and repo-wide, but this feature makes it bite: a legacy wrapper using || alert now writes an empty file and reports success, so the failure is loud on a terminal and silent to a script. The release notes must say that; fixing the exit code belongs to the debt register. And the flag help users actually see is cmd/help.go, not cobra: printReportHelp overrides the usage template entirely, so task 04s StringVarP description never reaches pgcenter report --help. No task covered that file — every help.go reference in this feature meant top/help.go. Assigned to task 09. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds internal/query/archiver.go: the 9-column pg_stat_archiver + .ready backlog query and SelectStatArchiverQuery(_ int), version-independent because the view is schema-identical on PG 14-19 and pg_ls_archive_statusdir() exists on all of them. Adds the shared postgres.SetupTestRole helper (idempotent CREATE ROLE, optional GRANT pg_monitor, SET ROLE) that this task's privilege tests and task 03's both call. It returns an error rather than taking *testing.T: internal/postgres/ testing.go carries no build tag and ships in the released binary. Note: the round-1 content of these three files was swept into 23a7b1f by a concurrent task agent; this commit carries the review-round-2 changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Go refuses to write an executable named "cmd" next to the cmd/ directory, so the instruction handed to every task was wrong. make build (or go build -o) is the working form. Task 01 hit it and worked around it; the remaining tasks now get the right command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing tests Register the archiver view in view.New() with MinRequiredVersion=PostgresV14 and add its case to Configure(). The version floor is load-bearing: the registry serves down to PG 9.4, and pg_ls_archive_statusdir() does not exist before PG 12, so without the gate the screen would error every tick on old clusters and pgcenter record would abort the whole recording on the first query error. Update the count-based tests to new correct values: TestNew 27 -> 28, TestView_VersionOK +1 on the rows at >= PG14 only, and Test_filterViews wantV +1 on the three >= PG14 rows / wantN +1 on the four <= PG13 rows. Also close the gap task 02 pointed here: TestViews_Configure now asserts the wal layout in its 190000 and 140000 arms, which is what pins that Configure carries the PG 19 layout into the registered view. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… row Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Full suite green in the CI image with PG 14-19 fixtures, race detector on, all packages ok. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e blocked Full suite in the CI image with PG 14-19 fixtures: exit 0, 1085 pass / 86 skip (EOL clusters only, debt [019]) / 0 fail, zero races. make lint and make vuln clean. No count-based test was weakened: every changed number is a new correct one and Test_filterViews gained a membership assertion on top. CLI and report criteria verified against a real recording rather than inferred from unit tests, including a pre-0.12 PG 18 archive replayed unchanged by the new binary. The manual half did not run: the stand answers on port 22 but rejects every available credential, so ten live criteria and the Decision 9 cost measurement have no evidence. Recorded as a blocker, not skipped; Decision 9's outcome is deliberately left unapplied rather than guessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran the manual half of task 10 on pgpro@10.128.28.194 (PostgresPro ent 18.4). All seven stand scenarios pass, including the once-per-path subtitle check on both entry paths separately (hotkey w = 1, W menu = 1). Feature 017 now stands at 34/34 acceptance criteria, 0 failed, 0 not verifiable. Decision 9's cost measurement was taken under its prescribed conditions (pg_monitor-only role, verbose on, concurrent pgcenter record, archive_mode=off, 200005 .ready files) and its pre-agreed outcome applied: the numbers are bad, so throttling returns as its own decision. query wall time ~1108 ms mean vs a 1 s refresh interval view-switch latency 70-240 ms (acceptable) verbose panel A/B feature verbose=on refreshes at ~1.9 s vs ~1.0 s on master Recorded one MAJOR feature-introduced finding (the verbose panel's directory walk halves the effective refresh rate for a pg_monitor role) and one MINOR pre-existing one (segment names truncated to header width when the screen is opened before any value exists; align/print path untouched by this feature, A/B-confirmed). No code changed. Stand restored to its inventoried baseline and verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… numbers that triggered it The stand measurement under Decision 9 conditions came back bad: ~1108 ms mean query time on 200k .ready files against a 1 s refresh interval, and an A/B against master showing the feature halves the effective refresh rate on every screen for a pg_monitor role with verbose on. Decision 9 pre-agreed the outcome and the roadmap owner confirmed it: throttle now, before merge. Scoped to the panel aggregate only. The archiver screen keeps its unthrottled sub-select — it is one statement the collector runs whole, and its cost is paid only while that screen is displayed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The roadmap owner weighed the per-file cost against realistic backlog sizes and declined the throttle: ~5.5 us per file means a five-thousand-segment backlog costs ~28 ms, and the measured doubling needs 200k segments — 3.1 TB of unarchived WAL, a state that would be noticed long before the screen refresh rate mattered. Recorded in the spec rather than the tech-debt register, also at the owners direction: a debt entry is a commitment to fix, and there is none. The numbers stay so nobody re-measures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… tech debt, metrics Project knowledge: the archiver view's shape and why nothing is diffed, the w/W cycle whose group name is also a view name, the backlog function swap and its two consequences, SetupTestRole and why it cannot take *testing.T; patterns.md gains the mutation-reading rules this feature paid for. ADR log: seven entries (the has_function_privilege dead end, the backlog function swap, the discarded first sample, the empty-archive report, the closed report-flag whitelist, the shared test-role helper, the measured directory walk). The [010] archiving-backlog entry is marked superseded in part — its pg_monitor privilege claim was disproved by measurement. Tech debt: [034] report exits 0 on every failure path, [035] WAL segment names truncate when the screen opens before any value exists, [036] pflag marked indirect while a test imports it. The archive_status walk cost is deliberately NOT registered — the numbers live in the ADR log instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cmd/report/report_test.go imports pflag directly to pin the two -W failure shapes, so the indirect marking was stale and any -mod=mod build rewrote it. Tech debt [036] closed rather than carried: the fix is one line and was verified with a build and the report tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Roadmap 0.12.0 feature [017]. The WAL/archiving code area is entered once, so four related pieces ship together.
What ships
archiverscreen — nine columns overpg_stat_archiverplus a.readybacklog count, reached by cycling the existingwhotkey (wal↔archiver) with a two-itemWmenu. Nothing is diffed: counters render cumulative and never-archived cells render blank rather than0.report -Wchanges from a boolean to a string flag.fpi,KiBcolumn on thewalscreen from the newwal_fpi_bytes, placed next to thefpicounter and inside the diffed range. PG 14–18 layouts are byte-identical.pg_monitorrole can actually call.Breaking change
pgcenter report -Wis now a string flag:-W wforpg_stat_wal,-W afor the archiver.The common legacy shape fails in a way worth knowing:
pgcenter report -W -f dump.tarhas-fconsumed as the flag's value, so it exits withreport type is not specified, quit— and with code 0, becausemain()prints and returns withoutos.Exit(1). A wrapper using|| alertwill not fire and will keep an empty output file. Documented indoc/release-notes/v0.12.0.md; the exit code is registered as tech debt [034], since it affects every failure path ofreport, not just this flag.Why the verbose panel changed
pg_ls_dirhas ACL{postgres}— superuser only — whilepg_ls_waldirandpg_ls_archive_statusdirare{postgres, pg_monitor}. So the panel's archiving backlog never worked for the most common monitoring role; it showedn/a. ADR [010] assumed otherwise and is marked superseded in part.Two consequences, both accepted and recorded: a cluster with a missing
archive_statusdirectory now reports a bare0instead ofn/a(the new function ismissing_ok=true), and the walk stats every file rather than listing names.Cost, measured rather than assumed
On a live stand with 200 005
.readyfiles, under apg_monitor-only role with verbose mode on and a concurrentpgcenter record: the backlog query takes ~1108 ms against a 1 s refresh interval, and the effective refresh rate goes from 1.0 s/tick onmasterto 1.9 s/tick.Throttling was prepared and then deliberately not applied: the cost is linear at ~5.5 µs per file, so a five-thousand-segment backlog costs ~28 ms. The measured doubling needs 3.1 TB of unarchived WAL — a state noticed long before the refresh rate matters. Numbers are in the tech-spec's Decision 20 rather than the debt register, at the roadmap owner's direction.
Verification
make lintandmake vulnclean.archive_mode=off, both navigation entry paths checked separately for a duplicated subtitle, a 60-column terminal, and the privilege-degradation path.Test_filterViewswas strengthened — the counts alone would still have passed with the archiver view dropped.Known limitations
A
walrecording made on PG 19 by a pre-0.12 pgcenter replays against the new 8-column layout and fails with an error beginningdiff failed. Narrow (PG 19 is beta), documented in the release notes.report -W aprints N−1 rows for an N-tick recording — the report always discards the first sample, which for a pass-through screen is real data. Pre-existing behaviour shared withactivity.🤖 Generated with Claude Code