Pass fork number to rm_mask so wal_consistency_checking covers visibility map pages#78
Draft
NikolayS wants to merge 2 commits into
Draft
Pass fork number to rm_mask so wal_consistency_checking covers visibility map pages#78NikolayS wants to merge 2 commits into
NikolayS wants to merge 2 commits into
Conversation
RelationCopyStorageUsingBuffer() WAL-logged every copied page with log_newpage_buffer(..., page_std = true), for every fork. Visibility map and FSM pages keep their entire payload between pd_lower and pd_upper (they are initialized with PageInit(page, BLCKSZ, 0), so pd_lower = SizeOfPageHeaderData and pd_upper = BLCKSZ), which means the standard-page "hole" optimization elided the whole payload from the full-page image. Replaying such an FPI zero-fills the hole, so a standby, a PITR restore, or crash recovery on the primary itself reconstructed the copied database's visibility map and FSM forks as all-zeros, while a non-crashed primary kept the template's bits. The damage direction is safe but bad: lost VM bits and FSM knowledge cannot cause false visibility, but they force extra heap reads for index-only scans, redundant vacuum work (including re-freezing), and poorer free space reuse in the copied database. That is presumably why this went unnoticed since the WAL_LOG strategy was introduced in commit 9c08aea (PostgreSQL 15). It was also undetectable by wal_consistency_checking, whose heap_mask() wipes the region between pd_lower and pd_upper on VM pages before comparison; a follow-up commit addresses that blindness, and with it 027_stream_regress under PG_TEST_EXTRA=wal_consistency_checking reliably caught this bug on the first heap insert into a copied catalog: FATAL: inconsistent page found, rel 1663/16384/1247, forknum 2, blkno 0 CONTEXT: WAL redo at 0/03442088 for Heap/INSERT: ... blkref #1: rel 1663/16384/1247, fork 2, blk 0 FPW The 73-byte XLOG/FPI record for that VM page (a full image would be ~8kB) confirms the payload was elided as a hole. Fix by claiming the standard page layout for all forks except the visibility map and FSM forks; main and init fork pages do use the standard layout, so hole compression remains in effect for them. The other bulk-copy paths (RelationCopyStorage() via bulk_write.c and the log_newpage_range() callers) already pass page_std = false where appropriate. Add a recovery TAP test that copies a template database with STRATEGY=WAL_LOG and verifies that a standby reproduces the same visibility map and FSM contents as the primary. Backpatch to 15, where the WAL_LOG strategy was introduced. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
wal_consistency_checking was structurally blind to visibility map corruption. verifyBackupPageConsistency() called rm_mask() with only the block number, so heap_mask() was applied to visibility map pages as if they were heap pages. Its mask_unused_space() call memsets the region between pd_lower and pd_upper -- but VM pages are initialized with PageInit(page, BLCKSZ, 0) and store the entire all-visible/ all-frozen bitmap exactly there, so both the primary and replay images of a VM page were wiped before comparison and every VM page check passed vacuously. The stale-VM-bit corruption class addressed by ed62d26 (registering VM blocks in heap WAL records) is exactly the kind of divergence this tool should catch, yet it could not. Fix this by adding the fork number to the rm_mask API: void (*rm_mask) (char *pagedata, BlockNumber blkno, ForkNumber forknum); and updating all in-core mask routines (heap_mask, btree_mask, hash_mask, gin_mask, gist_mask, seq_mask, spg_mask, brin_mask, generic_mask) plus verifyBackupPageConsistency(), which now passes the fork number from the decoded block reference. heap_mask() -- Heap and Heap2 are the only resource managers whose records register visibility map blocks -- now detects VISIBILITYMAP_FORKNUM and masks only the page LSN and checksum, leaving the bitmap intact so that VM pages are actually compared between primary and standby. PD_ALL_VISIBLE masking in mask_page_hint_bits() is kept: although all PD_ALL_VISIBLE changes are WAL-logged nowadays, a flag-only VM set intentionally skips the heap page FPI and LSN stamp (see log_heap_prune_and_freeze()), so the flag's on-disk state is not strictly LSN-ordered and can transiently diverge around recovery restarts without indicating corruption. The comment there now documents this. Note that this is an API break for out-of-core custom resource managers that implement rm_mask; the custom-rmgr documentation is updated accordingly. Validated by running src/test/recovery/t/027_stream_regress.pl with PG_TEST_EXTRA=wal_consistency_checking, which replays the full regression suite on a standby with wal_consistency_checking=all. Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP
NikolayS
force-pushed
the
claude/testplan-rmmask-forknum
branch
from
July 18, 2026 15:45
84d9953 to
77d654c
Compare
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.
Two-patch series. Patch 1 is the backpatch candidate (15–19) and is fully independent of patch 2; patch 2 is master-only (it changes the
rm_masksignature, an API break for out-of-core custom resource managers).[PATCH 1/2]
19c6691— Don't log VM/FSM pages as standard in CREATE DATABASE WAL_LOG copy (backpatch 15–19)RelationCopyStorageUsingBuffer()— the CREATE DATABASESTRATEGY=WAL_LOGcopy path, introduced in upstream commit9c08aea6a3090a396be334cc58c511edab05776a(PG 15) — WAL-logged every copied page withlog_newpage_buffer(dstBuf, true), claiming the standard page layout for every fork. Visibility map and FSM pages keep their entire payload betweenpd_lower(24) andpd_upper(BLCKSZ), so the standard-page "hole" optimization elided the whole payload from the FPI, and replay zero-fills the hole. Result: a standby, a PITR restore, or crash recovery on the primary itself reconstructs the copied database's VM and FSM forks as all-zeros while a non-crashed primary keeps the template's bits.The damage direction is safe-but-bad: lost VM bits / FSM knowledge cannot cause false visibility, but they force extra heap reads for index-only scans, redundant vacuum work (including re-freezing), and poorer free space reuse — which is presumably why it went unnoticed for four releases, and it was invisible to
wal_consistency_checkingbecause of the masking blindness fixed in patch 2.Fix: claim the standard layout for all forks except
VISIBILITYMAP_FORKNUMandFSM_FORKNUM(init forks share the standard layout with main, so they keep hole compression):The other bulk-copy paths (
RelationCopyStorage()viabulk_write.c,log_newpage_range()callers) already passpage_std = falsewhere appropriate.New always-running TAP test (in patch 1)
src/test/recovery/t/055_create_database_walog.pl(plain test, noPG_TEST_EXTRAneeded): builds a template database with populated VM and FSM (insert 5000 rows, delete every 5th,VACUUM (FREEZE)), runsCREATE DATABASE ... STRATEGY WAL_LOG, waits for standby replay, then comparespg_visibility_map_summary()andpg_freespace()for the copied table on primary vs standby, with sanity checks that the primary's VM/FSM are non-trivial so the comparison can't pass vacuously.pg_visibility/pg_freespacemapadded to the recoveryEXTRA_INSTALL.Kill-test record (fix reverted via
git apply -Rof the bufmgr.c hunk, rebuild, refresh tmp_install, rerun):With the fix restored:
recovery/055_create_database_walog OK, 4 subtests passed.[PATCH 2/2]
77d654c— Pass fork number to rm_mask so wal_consistency_checking covers VM pages (master only)The blindness:
verifyBackupPageConsistency()calledrm_mask(page, blkno)with no fork number, soheap_mask()treated VM pages as heap pages; itsmask_unused_space()memsets[pd_lower, pd_upper)— the entire VM bitmap — so every VM-page comparison passed vacuously. The stale-VM-bit corruption class addressed by ed62d26 was undetectable.API change:
Updated
RmgrData,verifyBackupPageConsistency()(passes the decoded block's fork), the custom-rmgr docs, and all nine in-core mask routines:heap_mask(Heap+Heap2),btree_mask,hash_mask,gin_mask,gist_mask,seq_mask,spg_mask,brin_mask,generic_mask. Heap/Heap2 are the only rmgrs that register VM blocks (verified: all VM buffer registration goes throughheapam.c/pruneheap.c).heap_mask()onVISIBILITYMAP_FORKNUMmasks only page LSN + checksum, preserving the bitmap; assertsMAIN_FORKNUMotherwise.Discovery narrative: with patch 2 alone (pre-fix),
027_stream_regressunderPG_TEST_EXTRA=wal_consistency_checkingFATALed seconds into the suite on the first heap insert into a copied catalog:The XLOG/FPI record that had copied that VM page was 73 bytes total (a full image is ~8 kB) — empirical proof the bitmap was elided as a hole. That finding is what patch 1 fixes; the new coverage detects real VM divergence on first contact.
PageClearAllVisible verdict — keep, documented: after ed62d26 every PD_ALL_VISIBLE transition is WAL-logged, so the mask could in principle go. But
log_heap_prune_and_freeze()intentionally skips the heap-page FPI and LSN stamp when the only change is setting PD_ALL_VISIBLE with checksums/wal_log_hintsoff (stamping the LSN without an FPI could make recovery skip an earlier FPI needed to repair a torn page). The flag's on-disk state is therefore not strictly LSN-ordered and can transiently diverge around recovery restarts without indicating corruption. Conservative call: keep the masking;mask_page_hint_bits()now documents exactly why, retaining the XXX for future tightening.Provenance and validation
d374280(master, v19devel). Build:meson setup build --buildtype=debug -Dcassert=true -Dtap_tests=enabled, gcc, Linux x86_64.git diff --checkclean.recovery/055_create_database_walogOK (4 subtests, ~3 s)PG_TEST_EXTRA=wal_consistency_checking recovery/027_stream_regressOK (11 subtests, ~88–118 s) — full regression suite replayed through a standby withwal_consistency_checking = all, zero inconsistent pageswal_consistency_checkingfails with the VM inconsistency above when patch 2's detection is present without patch 1's fix.Closes #72
🤖 Generated with Claude Code
https://claude.ai/code/session_01PtxA6B4d47ietk5dGsseuP