Strengthen restored-cluster oracle in pg_combinebackup tests (amcheck + pg_visibility + IOS/seqscan comparison)#65
Draft
NikolayS wants to merge 2 commits into
Draft
Conversation
The incremental-backup comparison test 002_compare_backups.pl verified restored clusters only by comparing pg_dumpall output. A logical dump sees just live row contents through sequential scans, so it is blind to exactly the kind of physical damage pg_combinebackup could plausibly introduce: stale visibility map bits, index corruption, and other page-level problems. That oracle gap is why the ed62d26 class of VM WAL-logging bugs could survive this test. 012_vm_consistency.pl checks VM state for specific operations, but its restored-cluster verification had similar gaps. Upgrade the oracle in both tests. After each restored cluster is started, additionally: * Run pg_amcheck --all --install-missing --heapallindexed to verify heap and index consistency in every database. * Cross-check the visibility map against the heap with pg_visibility's pg_check_visible()/pg_check_frozen() over every user relation (including toast tables), asserting zero corrupt TIDs. * Run the same aggregate under a forced index-only scan (which trusts the VM) and a forced sequential scan (which does not) and require identical results -- the user-visible symptom of stale all-visible bits. Both tests gain a dedicated indexed table whose heap, index, and VM change within the incremental backup window and which is re-VACUUMed so the restored VM bits are actually consulted. In 002_compare_backups.pl the checks run against both the full-backup restore and the incremental (combined) restore, so any divergence is attributable to pg_combinebackup. Autovacuum is disabled on the restored nodes to keep the checks deterministic. A throwaway sanity experiment (not committed) confirmed the oracle bites: with manually-set stale VM bits, pg_check_visible and pg_check_frozen each reported 100 corrupt TIDs and the forced index-only scan returned rows the seqscan did not, and pg_amcheck detected a manually corrupted btree page. pg_amcheck alone does not detect stale VM bits, which is why the pg_visibility checks are also needed. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
11 tasks
Per review of the restored-cluster oracle changes: * Factor the physical-consistency checks into a shared local module, t/CombineBackupTest.pm, loaded via the FindBin pattern used by pg_rewind's RewindTest.pm, and use it from both 002_compare_backups.pl and 012_vm_consistency.pl. * Run the pg_visibility sweep in every connectable database (pg_database WHERE datallowconn) rather than only "postgres", matching the comment's claim of covering every user relation. * Add t/013_stale_vm_detection.pl, a committed negative control proving the oracle actually fires. It restores a combined (full + incremental) backup into a scratch cluster, verifies the checks pass there, then fabricates stale all-visible/all-frozen bits by writing 0xFF into the table's VM fork: the pg_visibility sweep must report the corruption (t|100|100 corrupt TIDs) and a forced index-only scan must diverge from a forced seqscan (1000|500500|1|1000 vs 900|450000|1|999). It then overwrites part of a btree page and asserts pg_amcheck fails, proving the amcheck leg too. Only the scratch cluster is corrupted. The corruption target table must not be index-scanned before its VM is corrupted: an index-only scan on the intact cluster would heap-fetch the dead tuples and set LP_DEAD kill bits in the index, hiding the divergence afterwards. The intact-cluster sanity checks therefore use a separate identical control table. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
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.
What
Upgrades the verification oracle in the incremental-backup TAP tests and adds a committed negative control proving the oracle fires:
src/bin/pg_combinebackup/t/CombineBackupTest.pm— new shared helper module (loaded via theFindBinpattern used by pg_rewind'sRewindTest.pm) exportingcheck_physical_consistency,vm_sweep_errors,ios_vs_seqscan,connectable_databases.src/bin/pg_combinebackup/t/002_compare_backups.pl— after the existing pg_dumpall comparison, runs the full oracle on both the full-backup PITR and the incremental (combined) PITR, so a divergence is attributable topg_combinebackup. Gains anindexed_table(1000 rows, PK,VACUUM (FREEZE)) whose heap, index, and VM change inside the incremental window; autovacuum disabled on restored nodes.src/bin/pg_combinebackup/t/012_vm_consistency.pl— per-operation restored-cluster validation now also runspg_check_frozen; then the full shared oracle runs against the restored cluster using a newvm_ios_testindexed table modified inside the incremental window.src/bin/pg_combinebackup/t/013_stale_vm_detection.pl— new committed negative control (kill test), see below.The oracle, per restored cluster:
pg_amcheck --all --install-missing --heapallindexed— heap and index consistency in every database.pg_database WHERE datallowconn):pg_check_visible()+pg_check_frozen()over every user relation (tables, matviews, toast;pg_classrelkind r/m/t, oid >= 16384), asserting zero corrupt TIDs.count/sum/min/maxaggregate underenable_seqscan=off, enable_indexonlyscan=onvs the inverse, asserting identical results, plus an EXPLAIN assertion that the forced plan really is an Index Only Scan — the user-visible symptom of stale all-visible bits.Why
002_compare_backups.plcompared restored clusters viapg_dumpalloutput only (its own comment admits the limitation). A logical dump reads live rows through sequential scans and is blind to stale visibility-map bits, index corruption, and other page-level damage — exactly why the ed62d26 VM/WAL-logging corruption class survived this test.012_vm_consistency.pl(5174d15) checks specific operations but its restored-cluster verification only ranpg_check_visibleon the per-operation tables.Negative control (013_stale_vm_detection.pl)
Committed, deterministic kill test proving the oracle demonstrably fires — only a scratch restored cluster is ever corrupted:
--no-data-checksums(so a fabricated stale VM bit is a valid page whose bits are simply wrong — the same shape as a real pg_combinebackup bug),summarize_wal=on,autovacuum=off. Tablet+ identicalt_control: 1000 rows each, PK,VACUUM (FREEZE).DELETE ... WHERE id % 10 = 0on both tables (clears VM bits, leaves dead tuples still present in the indexes; the predicate is non-indexable so no index kill bits are set) → incremental backup →pg_combinebackuprestore into a scratch cluster.t_control).0xFFover the first 4 bytes oft's VM fork data area (after the 24-byte page header ⇒ first 16 heap pages claimed all-visible+all-frozen), restart. Assertions that must fire:pg_visibilitysweep reports corruption:t|100|100(100 corrupt TIDs from each ofpg_check_visible/pg_check_frozen);1000|500500|1|1000vs900|450000|1|999.t_pkeybtree page, restart:pg_amcheckmust fail (it does: "line pointer points past end of tuple space").Subtlety encoded in the test: the corruption target must never be index-scanned before its VM is corrupted — an IOS on the intact cluster heap-fetches the dead tuples and sets LP_DEAD kill bits in the index, which would hide the divergence afterwards. Hence the separate
t_controlfor the intact-cluster sanity IOS. Also confirmed:pg_amcheckalone does not detect stale VM bits (verify_heapamhas no VM cross-check), so the pg_visibility + IOS/seqscan layers are necessary, not redundant.Test evidence
Key TAP lines from 013:
Provenance / reproduction
d374280("ci: Generate crashlogs on Windows"), v19devel, branchclaude/testplan-backup-oracle.meson setup build --buildtype=debug -Dcassert=true -Dtap_tests=enabled; gcc 13, Linux x86_64; combine mode--copy(defaultPG_TEST_PG_COMBINEBACKUP_MODE).summarize_wal=on(all primaries),autovacuum=off(012/013 primaries and all restored/scratch nodes), recovery settings in 002 as before (recovery_target_lsn,recovery_target_action=promote,archive_mode=off). 013 additionally usesinitdb --no-data-checksums.meson test -C build --suite setup && meson test -C build pg_combinebackup/002_compare_backups pg_combinebackup/012_vm_consistency pg_combinebackup/013_stale_vm_detection(as a non-root user). Runtimes above; total added wall time across the suite is roughly 15 s.Closes #63
🤖 Generated with Claude Code
https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP