On Tue, Sep 22, 2026 at 2:54 AM Andrey Borodin <[email protected]> wrote: > > On 21 Sep 2026, Melanie Plageman wrote: > > That wouldn't help in this case because it was already clear on the > > primary. > > Yes, my suggestion was too vague. I meant a check during redo that > would fail an assert-enabled buildfarm run and preserve enough state > to investigate. A WARNING can go unnoticed in a passing TAP test, as > David recently pointed out [0]. Could we make this fatal in assert > builds, once we have a condition that excludes pages legitimately > ahead of replay?
I don't think it makes sense to implement that here. There are no other places in the code base where we error out in an assert build and warn in a non-assert build. So, we'd be starting a new precedent. And having it error out in non-assert builds for VM corruption will probably make people mad. I do think it makes sense to do something like what David is suggesting in general, though. One thing we could do here is make sure there is test coverage. I committed a patch yesterday 5d84c76021c that changes verify_heapam() to report corruption when PD_ALL_VISIBLE is clear and the VM is set. We could add a test that uses pg_amcheck to detect this kind of corruption. AI drafted one for me that used a lot of fancy perl that I didn't yet evaluate, but I could look into it more. While revisiting the warning I wrote in the patch you reviewed, I realized I don't think it's the right thing to do. The warning as I wrote it would warn whenever a WAL record is clearing the VM and the primary's PD_ALL_VISIBLE was set and its visibility map was already clear and the standby's visibility map is set. This doesn't seem right because it will warn even if the standby doesn't actually have data corruption. That is, if the standby has both PD_ALL_VISIBLE set and visibility map set, it would still warn even though that is not corruption. The warning is then basically about the primary being out of sync with the standby. But there are many other combinations of the primary and standby being out of sync (e.g. we try to set the visibility map on the standby and it is already set). And, I don't think we want to warn in all of these cases. Every combination of PD_ALL_VISIBLE and VM set/clear differing between primary and standby is a lot to warn on. And if you lost the whole VM on one node, for example, you would get this warning for every page. And it's not obvious why just this one case of divergence between primary and standby is most important to warn on. Instead, I suggest that, starting on master, we warn whenever there is corruption on either node. On the primary it already prints a warning when PD_ALL_VISIBLE is clear and the VM is set. Then it fixes it. We should expand this on master and also wal-log it. We should also then add a warning to the standby when it has PD_ALL_VISIBLE clear and the VM set. Once we WAL-log fixing corruption on the primary, it should only warn during recovery when the standby is corrupt and out of sync with the primary. These changes are larger, so I am planning to only propose it in a separate thread to master only. I would couple it with hardening that makes it harder to get into these situations so the warnings are less likely to be produced. I plan to start a new thread with the patches proposed to master only that adds a lot more corruption protection to the VM. > In heap_xlog_vm_clear_unregistered(), couldn't the VM page already have > a newer LSN? Consider a heap record R whose VM clear was a no-op, > followed by VACUUM setting that bit at S. If the VM page at S reaches > disk and crash recovery starts before R, the new helper would call this > corruption and move the page LSN backwards to R. Unlike > XLogReadBufferForRedo(), it does not check the page LSN. I did not > reproduce this, just seems possible. The ironic thing about this is that in the patch version you reviewed, this behavior leaves things in a more correct state than prior to it. However, I know it's wrong to decrease the LSN. Also it is wrong to stamp a page with an LSN after making changes that weren't registered in that WAL record. That's why we don't set the page LSN when clearing the VM when the VM block is not registered in backbranches. Just for fun, I'll describe why, in this particular case, my patch you reviewed leaves things in a more correct state, though. VM page LSN starts at 0, replay R and it does nothing to it. Replay S and it advances the LSN from 0 -> 3. Then the VM page is persisted. Then we crash. We replay R. It clears the VM and sets the LSN from 3 -> 2. We replay S. It sets the VM and advances the LSN from 2 -> 3. On versions 18 and lower after the crash, R will clear the VM and not set the LSN. S will see the LSN is already 3 and not do anything so the VM ends up clear. That's not wrong, but it does create divergent state on primary and standby. (19 without this patch wouldn't clear the VM at all, which is wrong). That being said, attached v2 is the correct approach for 19 and master (for now). It does not stamp the LSN on the page when clearing the VM when the VM block was not registered. > Could we retain the original VM bits and page LSN in the diagnostic? > The WAL redo CONTEXT already identifies the record. I've removed the warning for now, but in the new more invasive patch set I plan for master which will warn, I can include more details like these. We can do things on master like have visibilitymap_clear() return the original bits instead of just a boolean. That will make it much easier to include details like this in the log message. > A page-LSN check > also needs care: another heap block's bit can advance the same VM page's > LSN without repairing the bit we are interested in. Yea, we don't want to gate the VM clear on the page LSN. This made me think about how the VM can end up in an inconsistent state temporarily during crash recovery. So, when I do add the warning to master, I'll make sure it is only when we've reached a consistent state (I think you mention that in your earlier comment). > Also, if we accept a possible torn page from this repair, what happens > on the next recovery? The new reader uses RBM_NORMAL_NO_LOG, not the > VM's usual RBM_ZERO_ON_ERROR, so a checksum failure could stop recovery > before it gets to the repair. Yea, I did this on purpose because since ed62d26caca, I didn't read the VM with ZERO_ON_ERROR, which I thought was okay since we were always registering the VM blocks in the wal record for both setting and clearing. However, this isn't a good idea when 17 and 18 have a fallback path that lets us read the VM in recovery (when clearing it) in a way that could result in a torn page. Then we've created a way for the VM to get corrupt and made it error out reading it during recovery. > For this repair, shouldn't we either use RBM_ZERO_ON_ERROR on every > relevant recovery read path, or require an FPI for the VM page since > the last checkpoint's redo point before allowing the modification? Yea, I've thought about this a lot the last few days. On back branches (and as a stop-gap in master), I think we have to use RBM_ZERO_ON_ERROR everywhere and be okay with tearing pages when reading VM pages during recovery. On master, going forward, we should register the VM whenever PD_ALL_VISIBLE is being cleared. And then we should not use RBM_ZERO_ON_ERROR for setting or clearing. For setting, that means adding a new mode that will extend the VM if the page doesn't exist but error out if the page is corrupt (RBM_ZERO_ON_MISSING). Then we can make the zero_damaged_pages GUC per fork. I have four or five ideas for making master more robust to VM corruption. And there are still some open questions, like what to do about tuple locking, but that can be discussed in more depth there. But I think as long as we have a code path that we know leads to torn pages, we have to zero those pages on error when reading them. > There are quite a few moving parts in VM, and I don't yet have a clear > picture of how they all fit together. Sorry if some of these questions > are a distraction. I have a handful of open tickets about VM corruption, > and I hope we can track down and fix all possible issues. I certainly think we can harden master by making some bigger changes. I have a draft of the changes and hope to post by mid next week. For now, I've attached four patches that are much narrower fixes and are backpatchable: v2-0001 is 19-only and fixes the divergent VM bits on the standby by reading the VM when the VM block wasn't registered master-v2-0001 is the stop-gap for master that does the same thing but in a different way because it doesn't have a fake relcache entry v2-0002 fixes VM clear to read the VM with RBM_ZERO_ON_ERROR. This should apply master -> 17 v2-0003 is a fix for 19 and master for an issue I found while working on this where the VM wasn't guaranteed to be dirty by the time we logged setting it in heap_page_prune_and_freeze(). This can happen if it was already set all-frozen and all-visible but it contained any non-frozen tuples (i.e. VM is corrupt with incorrect all-frozen bit). The VM set operation would be a no-op and no one would fix the corrupt all-frozen bit. This corruption was never fixed, but in 19 the vm-setting code was rearranged to expect that if we try and set the VM, we will dirty it. For that to be true, we have to fix this corruption. And, since the VM corruption detection function is new in 19, it is easiest to just backpatch it that far. - Melanie
From 3bd4475e117a30219b28239709741ac60bf7aec2 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 23 Sep 2026 10:53:09 -0400 Subject: [PATCH v2 1/3] Clear divergent visibility map bits during heap redo When replaying a heap record that clears PD_ALL_VISIBLE but did not register a VM buffer because the VM was already clear on the primary, clear the VM bit anyway. The VM can diverge across a cluster, e.g. via CREATE DATABASE STRATEGY WAL_LOG, and a standby must not keep a set VM bit over a page whose PD_ALL_VISIBLE is clear. You can still get torn pages because you didn't log an FPI on the primary, however the VM is usually read with RBM_ZERO_ON_ERROR, so it is considered sufficient. Only backpatch to 19 because <= 18 already have a fallback. Backpatch-through: 19 --- src/backend/access/heap/heapam_xlog.c | 49 ++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c index fae3b477c09..c65804a6256 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -22,6 +22,33 @@ #include "storage/freespace.h" #include "storage/standby.h" +/* + * Clear visibility map bits for a heap block when the WAL record clearing it + * did not register the VM block. This handles cases where the VM is + * out-of-sync between the primary and standby (for instance, CREATE DATABASE + * STRATEGY WAL_LOG historically could cause this). + * + * This is not fully resilient: the VM page is modified without a full-page + * image, so a torn write during a crash could leave it inconsistent until the + * page is next repaired. That is considered acceptable since the VM is zeroed + * on error when reading it. + */ +static void +heap_xlog_vm_clear_unregistered(Relation reln, BlockNumber heap_blkno, + uint8 flags) +{ + Buffer vmbuffer = InvalidBuffer; + + if (visibilitymap_get_status(reln, heap_blkno, &vmbuffer) & flags) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + visibilitymap_clear(reln, heap_blkno, vmbuffer, flags); + UnlockReleaseBuffer(vmbuffer); + } + else if (BufferIsValid(vmbuffer)) + ReleaseBuffer(vmbuffer); +} + /* * Clear visibility map bits for a single heap block during heap redo. * @@ -46,22 +73,26 @@ heap_xlog_vm_clear(XLogReaderState *record, Relation reln = CreateFakeRelcacheEntry(target_locator); Buffer vmbuffer = InvalidBuffer; + if (!XLogRecHasBlockRef(record, wal_vm_block_id)) + { + heap_xlog_vm_clear_unregistered(reln, heap_blkno, flags); + FreeFakeRelcacheEntry(reln); + return; + } + /* * If the vmbuffer was registered, use the recovery-specific routines to * read it. These will either apply an FPI or indicate that we should * clear the requested bits ourselves. */ - if (XLogRecHasBlockRef(record, wal_vm_block_id)) + if (XLogReadBufferForRedo(record, wal_vm_block_id, + &vmbuffer) == BLK_NEEDS_REDO) { - if (XLogReadBufferForRedo(record, wal_vm_block_id, - &vmbuffer) == BLK_NEEDS_REDO) - { - if (visibilitymap_clear(reln, heap_blkno, vmbuffer, flags)) - PageSetLSN(BufferGetPage(vmbuffer), lsn); - } - if (BufferIsValid(vmbuffer)) - UnlockReleaseBuffer(vmbuffer); + if (visibilitymap_clear(reln, heap_blkno, vmbuffer, flags)) + PageSetLSN(BufferGetPage(vmbuffer), lsn); } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); FreeFakeRelcacheEntry(reln); } -- 2.43.0
From db07f4d74e0632ddd39ea80fa73a719aa8d9a659 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 23 Sep 2026 16:58:54 -0400 Subject: [PATCH vmaster2 1/3] Clear divergent visibility map bits during heap redo When replaying a heap record that clears PD_ALL_VISIBLE but did not register a VM buffer because the VM was already clear on the primary, clear the VM bit anyway. The VM can diverge across a cluster, e.g. via CREATE DATABASE STRATEGY WAL_LOG, and a standby must not keep a set VM bit over a page whose PD_ALL_VISIBLE is clear. You can still get torn pages because you didn't log an FPI on the primary, however the VM is usually read with RBM_ZERO_ON_ERROR, so it is considered sufficient. Only backpatch to 19 because <= 18 already have a fallback. The fix differs on master because it no longer makes a fake relcache entry in recovery, so it needed a dedicated function to read the unregistered VM page. This is a stopgap so that the issue is fixed across branches. In the future, master should implement a more robust fix. Backpatch-through: 19 --- src/backend/access/heap/heapam_xlog.c | 30 ++++++++++++++++++++++++ src/backend/access/heap/visibilitymap.c | 31 +++++++++++++++++++++++++ src/include/access/visibilitymap.h | 2 ++ 3 files changed, 63 insertions(+) diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c index 5fa1de09cfb..e1c68be698d 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -22,6 +22,33 @@ #include "storage/freespace.h" #include "storage/standby.h" +/* + * Clear visibility map bits for a heap block when the WAL record clearing it + * did not register the VM block. This handles cases where the VM is + * out-of-sync between the primary and standby (for instance, CREATE DATABASE + * STRATEGY WAL_LOG historically could cause this). + * + * This is not fully resilient: the VM page is modified without a full-page + * image, so a torn write during a crash could leave it inconsistent until the + * page is next repaired. That is considered acceptable since the VM is zeroed + * on error when reading it. + */ +static void +heap_xlog_vm_clear_unregistered(RelFileLocator rlocator, BlockNumber heap_blkno, + uint8 flags) +{ + Buffer vmbuffer = InvalidBuffer; + + if (xlog_visibilitymap_get_status(rlocator, heap_blkno, &vmbuffer) & flags) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + visibilitymap_clear(rlocator, heap_blkno, vmbuffer, flags); + UnlockReleaseBuffer(vmbuffer); + } + else if (BufferIsValid(vmbuffer)) + ReleaseBuffer(vmbuffer); +} + /* * Clear visibility map bits for a single heap block during heap redo. * @@ -46,7 +73,10 @@ heap_xlog_vm_clear(XLogReaderState *record, Buffer vmbuffer = InvalidBuffer; if (!XLogRecHasBlockRef(record, wal_vm_block_id)) + { + heap_xlog_vm_clear_unregistered(target_locator, heap_blkno, flags); return; + } /* * If the vmbuffer was registered, use the recovery-specific routines to diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index fe5ce437e1b..d7ba6b8ca20 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -12,6 +12,7 @@ * * INTERFACE ROUTINES * visibilitymap_clear - clear bits for one page in the visibility map + * xlog_visibilitymap_get_status - get status of bits during WAL replay * visibilitymap_pin - pin a map page for setting a bit * visibilitymap_pin_ok - check whether correct map page is already pinned * visibilitymap_set - set bit(s) in a previously pinned page @@ -189,6 +190,36 @@ visibilitymap_clear(RelFileLocator rlocator, BlockNumber heapBlk, return cleared; } +/* + * Like visibilitymap_get_status(), but uses a RelFileLocator instead of a + * Relation, so it needs no relcache entry and can be used by redo routines. + * + * On return *vmbuf holds the pinned (but unlocked) map page; the caller is + * responsible for releasing it. A caller that goes on to clear bits must lock + * it first. + */ +uint8 +xlog_visibilitymap_get_status(RelFileLocator rlocator, BlockNumber heapBlk, + Buffer *vmbuf) +{ + BlockNumber mapBlock = HEAPBLK_TO_MAPBLOCK(heapBlk); + uint32 mapByte = HEAPBLK_TO_MAPBYTE(heapBlk); + uint8 mapOffset = HEAPBLK_TO_OFFSET(heapBlk); + char *map; + + Assert(InRecovery); + + *vmbuf = XLogReadBufferExtended(rlocator, VISIBILITYMAP_FORKNUM, mapBlock, + RBM_ZERO_ON_ERROR, InvalidBuffer); + if (!BufferIsValid(*vmbuf)) + return 0; + + map = PageGetContents(BufferGetPage(*vmbuf)); + + /* A single byte read is atomic (see visibilitymap_get_status()). */ + return ((map[mapByte] >> mapOffset) & VISIBILITYMAP_VALID_BITS); +} + /* * visibilitymap_pin - pin a map page for setting a bit * diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h index 165efd1c00e..47b28f311f4 100644 --- a/src/include/access/visibilitymap.h +++ b/src/include/access/visibilitymap.h @@ -28,6 +28,8 @@ extern bool visibilitymap_clear(RelFileLocator rlocator, BlockNumber heapBlk, Buffer vmbuf, uint8 flags); +extern uint8 xlog_visibilitymap_get_status(RelFileLocator rlocator, + BlockNumber heapBlk, Buffer *vmbuf); extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk, Buffer *vmbuf); extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf); -- 2.43.0
From 66e86a8bedaff7c9d47f05b7e025c12f8a9747ac Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 23 Sep 2026 11:59:53 -0400 Subject: [PATCH v2 2/3] Read visibility map pages with RBM_ZERO_ON_ERROR in VM clear redo ed62d26caca started registering VM blocks when clearing the VM which is required for protection against torn pages as well as for correct incremental backups. However, it read the VM pages in recovery with RBM_NORMAL which errors out when it encounters a corrupt page. This is usually desirable, however, we still retain code paths that modify the VM in recovery without the block having been registered. A crash while modifying the VM page could lead to a corrupt page and no FPI to recover it. As long as we can trivially produce corrupt pages during recovery through our own redo mechanism, we shouldn't error out when reading a corrupt VM page. Make clearing the VM read the page with RBM_ZERO_ON_ERROR. This is consistent with the VM's other redo paths which set the VM bit (heap_xlog_prune_freeze() and heap_xlog_multi_insert()). Backpatch-through: 17 --- src/backend/access/heap/heapam_xlog.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c index c65804a6256..4d99d99080b 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -85,8 +85,9 @@ heap_xlog_vm_clear(XLogReaderState *record, * read it. These will either apply an FPI or indicate that we should * clear the requested bits ourselves. */ - if (XLogReadBufferForRedo(record, wal_vm_block_id, - &vmbuffer) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedoExtended(record, wal_vm_block_id, + RBM_ZERO_ON_ERROR, false, + &vmbuffer) == BLK_NEEDS_REDO) { if (visibilitymap_clear(reln, heap_blkno, vmbuffer, flags)) PageSetLSN(BufferGetPage(vmbuffer), lsn); @@ -850,8 +851,9 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) Assert(xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED); - if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, - &vmbuffer_new) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedoExtended(record, HEAP_UPDATE_BLKREF_VM_NEW, + RBM_ZERO_ON_ERROR, false, + &vmbuffer_new) == BLK_NEEDS_REDO) { /* * If both the old and new heap pages were all-visible and their @@ -888,8 +890,9 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) Assert(xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED); - if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, - &vmbuffer_old) == BLK_NEEDS_REDO) + if (XLogReadBufferForRedoExtended(record, HEAP_UPDATE_BLKREF_VM_OLD, + RBM_ZERO_ON_ERROR, false, + &vmbuffer_old) == BLK_NEEDS_REDO) { if (visibilitymap_clear(reln, oldblk, vmbuffer_old, VISIBILITYMAP_VALID_BITS)) -- 2.43.0
From 0bda92f7fe32f24b7b9b0b9caaac111c79c3b434 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Thu, 24 Sep 2026 16:42:36 -0400 Subject: [PATCH v2 3/3] Detect and repair a stale all-frozen visibility map bit visibilitymap_set() does not clear any bits, so if vacuum finds that every tuple on the page is visible but not every tuple is frozen and yet the page is marked all-frozen in the VM, it wouldn't repair it. Add a VM_CORRUPT_STALE_ALL_FROZEN case to heap_page_fix_vm_corruption() that will clear the VM bits. The normal path will then set the all-visible bit separately. After this change, we can be sure that visibilitymap_set() will modify the VM buffer and mark it dirty. Backpatch-through: 19 --- src/backend/access/heap/pruneheap.c | 33 +++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 833f6f0395c..2ddcbecff3a 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -196,6 +196,8 @@ typedef enum VMCorruptionType VM_CORRUPT_LPDEAD, /* Tuple not visible to all transactions on a page marked all-visible */ VM_CORRUPT_TUPLE_VISIBILITY, + /* Page marked all-frozen in the VM but not actually all-frozen */ + VM_CORRUPT_STALE_ALL_FROZEN, } VMCorruptionType; /* Local functions */ @@ -944,6 +946,23 @@ heap_page_fix_vm_corruption(PruneState *prstate, OffsetNumber offnum, relname, prstate->block))); do_clear_vm = true; break; + + case VM_CORRUPT_STALE_ALL_FROZEN: + + /* + * We examined every tuple on the page and found that the page is + * all-visible but not all-frozen, yet the VM marks it all-frozen. + * The page-level PD_ALL_VISIBLE flag is still correct, so only + * the VM is wrong. Clear both the VM bits. All-visible will be + * set again through the normal path. + */ + ereport(WARNING, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("page marked all-frozen in the visibility map is not all-frozen"), + errcontext("relation \"%s\", page %u", + relname, prstate->block))); + do_clear_vm = true; + break; } Assert(do_clear_heap || do_clear_vm); @@ -1259,6 +1278,20 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, Assert(!prstate.set_all_visible || prstate.attempt_set_vm); Assert(!prstate.set_all_visible || (prstate.lpdead_items == 0)); + /* + * If we examined every tuple on the page and found that the page is + * all-visible but not all-frozen, yet the VM marks it all-frozen, that + * all-frozen bit is corrupt. Repair the VM. We will set it back to + * all-visible later along with the other changes. Note that this must be + * done after set_all_visible and set_all_frozen are finalized above to + * account for dead items and unfrozen tuples. + */ + if (prstate.attempt_freeze && prstate.set_all_visible && + !prstate.set_all_frozen && + (prstate.old_vmbits & VISIBILITYMAP_ALL_FROZEN)) + heap_page_fix_vm_corruption(&prstate, InvalidOffsetNumber, + VM_CORRUPT_STALE_ALL_FROZEN); + do_set_vm = heap_page_will_set_vm(&prstate, params->reason, do_prune, do_freeze); /* -- 2.43.0
