On Thu, Apr 30, 2026 at 5:44 PM Melanie Plageman <[email protected]> wrote: > > Attached is a patch set to fix the issue based largely on the work you > started on your branch. I attached the version targeting master/19 > which is prefixed with v1_PGMASTER and the version targeting 18, > prefixed v1_PG18. The pg 18 changes aren't a straight cherry-pick to > 17 (the earliest I'll backpatch because that was when incremental > backup was introduced) because the redo functions live in a different > file in 18 than in 17, but I want to avoid discussing three different > versions of this patch set on this thread. > > The backpatched changes are different for a few reasons, but the > biggest difference from a review standpoint is that in pg18, the redo > routines can read WAL in the old format or the new format, so that > people can reasonably upgrade to the new minor version.
There is an interaction between CREATE DATABASE STRATEGY WAL_LOG and this fix that causes data corruption (identified while investigating [1]). The scenario is you have a heap page set PD_ALL_VISIBLE and a corresponding VM page set all-visible, you do a CREATE DATABASE STRATEGY WAL_LOG using that database as a template. The primary has the heap page set PDV (PD_ALL_VISIBLE) set and VM page AV set but the standby does NOT have the page set all-visible in the VM (it does have the heap page set PDV). After failover you have a primary with heap page PDV set and VM page not set. You modify a tuple on the page. Because the VM was already clear, we do not register the VM block in the WAL record. The standby replays this and clears the heap page PD_ALL_VISIBLE but leaves the VM set. This is data corruption leading to wrong results from index scans and worse if that standby is later promoted back to primary. We didn't have this problem before ed62d26caca because it cleared the VM during replay as long as PD_ALL_VISIBLE needed clearing. I mention in the thread [2] that I think we have to fix RelationCopyStorageUsingBuffer() to avoid producing this scenario because tuple locking already caused data corruption before ed62d26caca. However, for update/delete, etc, I'm wondering if we should harden this a bit and go back to registering a vm clear operation whenever PD_ALL_VISIBLE was set on the heap page. If this is a no-op on the primary for the VM, we register it with REBGUF_NO_CHANGE. Vacuum works very hard to never end up with PD_ALL_VISIBLE cleared and the VM set. Perhaps we should do everything we can to avoid this happening with page modifications as well. It's a pretty small change except for annoyingly making log_heap_update() even more complicated. Note that this does not fix tuple locking since that would be prohibitively expensive as described in [2]. AI-generated repro and proposed fix attached. - Melanie [1] https://www.postgresql.org/message-id/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com [2] https://www.postgresql.org/message-id/flat/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com#2b10a62acd511839653ee32693538993
repro-noop-vm-clear-portable.sh
Description: application/shellscript
From c5824d62c4d7cd24452f83a776fbfab8460a883a Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 16 Sep 2026 11:48:05 -0400 Subject: [PATCH v1] Register VM buffer on no-op clears so redo fixes a divergent VM Since ed62d26caca, heap WAL records clearing PD_ALL_VISIBLE (insert, multi_insert, delete, update) only registered the VM buffer when clearing its bits changed the VM page. With CREATE DATABASE using strategy WAL_LOG and copying from a template, it's possible to be in a scenario where the VM is clear on the primary and set on the standby. Skipping clearing the VM during redo would then lead to data corruption. Though the reported CREATE DATABASE issue should be independently fixed, harden the VM clear paths against VM divergence by always registering the VM buffer when having to clear even PD_ALL_VISIBLE. Tuple locking isn't fixed because it is pre-existing and hardening it in this way would negatively impact performance significantly. --- src/backend/access/heap/heapam.c | 59 ++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 10 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 9ebb1b35d37..46815f52305 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -64,6 +64,7 @@ static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf, Buffer vmbuffer_new, HeapTuple oldtup, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared, + bool vmbuffer_old_modified, bool vmbuffer_new_modified, bool walLogical); #ifdef USE_ASSERT_CHECKING static void check_lock_if_inplace_updateable_rel(Relation relation, @@ -2172,8 +2173,14 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); - if (vmbuffer_modified) - XLogRegisterBuffer(HEAP_INSERT_BLKREF_VM, vmbuffer, 0); + /* + * Register the VM buffer even if its bits were already clear, so redo + * clears PD_ALL_VISIBLE's VM bits; the VM can be out-of-sync across a + * cluster. + */ + if (clear_all_visible) + XLogRegisterBuffer(HEAP_INSERT_BLKREF_VM, vmbuffer, + vmbuffer_modified ? 0 : REGBUF_NO_CHANGE); recptr = XLogInsert(RM_HEAP_ID, info); @@ -2611,8 +2618,16 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, XLogRegisterData(xlrec, tupledata - scratch.data); XLogRegisterBuffer(HEAP_MULTI_INSERT_BLKREF_HEAP, buffer, REGBUF_STANDARD | bufflags); - if (all_frozen_set || vmbuffer_modified) - XLogRegisterBuffer(HEAP_MULTI_INSERT_BLKREF_VM, vmbuffer, 0); + + /* + * Register the VM buffer when setting it all-frozen, or so redo + * clears PD_ALL_VISIBLE's VM bits even if they were already clear + * here (the VM can be out-of-sync across a cluster). + */ + if (all_frozen_set || clear_all_visible) + XLogRegisterBuffer(HEAP_MULTI_INSERT_BLKREF_VM, vmbuffer, + (all_frozen_set || vmbuffer_modified) ? + 0 : REGBUF_NO_CHANGE); XLogRegisterBufData(HEAP_MULTI_INSERT_BLKREF_HEAP, tupledata, totaldatalen); @@ -3142,8 +3157,14 @@ heap_delete(Relation relation, const ItemPointerData *tid, /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); - if (vmbuffer_modified) - XLogRegisterBuffer(HEAP_DELETE_BLKREF_VM, vmbuffer, 0); + /* + * Register the VM buffer even if its bits were already clear, so redo + * clears PD_ALL_VISIBLE's VM bits; the VM can be out-of-sync across a + * cluster. + */ + if (clear_all_visible) + XLogRegisterBuffer(HEAP_DELETE_BLKREF_VM, vmbuffer, + vmbuffer_modified ? 0 : REGBUF_NO_CHANGE); recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE); @@ -4274,14 +4295,25 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, log_heap_new_cid(relation, heaptup); } + /* + * When both heap pages are all-visible and share a VM page, that page + * is registered once as VM_NEW; pass the old slot as invalid to avoid + * registering the same buffer twice. + */ recptr = log_heap_update(relation, buffer, - vmbuffer_modified ? vmbuffer : InvalidBuffer, + (clear_all_visible && + !(clear_all_visible_new && + vmbuffer == vmbuffer_new)) ? + vmbuffer : InvalidBuffer, newbuf, - vmbuffer_new_modified ? vmbuffer_new : InvalidBuffer, + clear_all_visible_new ? + vmbuffer_new : InvalidBuffer, &oldtup, heaptup, old_key_tuple, clear_all_visible, clear_all_visible_new, + vmbuffer_modified, + vmbuffer_new_modified, walLogical); if (newbuf != buffer) { @@ -9021,6 +9053,7 @@ log_heap_update(Relation reln, Buffer oldbuf, Buffer vmbuffer_old, HeapTuple oldtup, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared, + bool vmbuffer_old_modified, bool vmbuffer_new_modified, bool walLogical) { xl_heap_update xlrec; @@ -9233,15 +9266,21 @@ log_heap_update(Relation reln, Buffer oldbuf, Buffer vmbuffer_old, * same VM page and both their VM bits were cleared, the caller passes * only vmbuffer_new (mirroring the heap page convention where block 0 = * new is always registered). + * + * A buffer is registered even if its bits were already clear, so redo + * clears PD_ALL_VISIBLE's VM bits (the VM can be out-of-sync across a + * cluster); use REGBUF_NO_CHANGE when the page was not modified. */ Assert((BufferIsInvalid(vmbuffer_old) && BufferIsInvalid(vmbuffer_new)) || (vmbuffer_old != vmbuffer_new)); if (BufferIsValid(vmbuffer_new)) - XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_NEW, vmbuffer_new, 0); + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_NEW, vmbuffer_new, + vmbuffer_new_modified ? 0 : REGBUF_NO_CHANGE); if (BufferIsValid(vmbuffer_old)) - XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_OLD, vmbuffer_old, 0); + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_OLD, vmbuffer_old, + vmbuffer_old_modified ? 0 : REGBUF_NO_CHANGE); /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); -- 2.43.0
