From 8ef81094e39a34d845b1a40cad7de469c8501bee Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Wed, 29 Apr 2026 12:53:49 -0400 Subject: [PATCH v1_PG18 1/3] Fix WAL logging of VM clears Previously, we failed to register the visibility map buffer when emitting WAL after clearing the VM bits for heap pages (recovery code read the VM page normally). This meant that we couldn't emit FPIs of VM pages when clearing VM bits. While this would not result in a checksum error because the visibility map is read with RBM_ZERO_ON_ERROR, WAL summarizer only includes pages which were registered, so a restore from an incremental backup would not include the modified VM pages -- leading to data corruption. Fix this by registering the VM buffer in the WAL record when clearing VM bits. This also means the VM buffer must be locked throughout the duration of the critical section where we make changes to the VM and heap page and emit WAL. --- contrib/pg_surgery/heap_surgery.c | 10 +- src/backend/access/heap/heapam.c | 233 +++++++++++++++++++---- src/backend/access/heap/heapam_xlog.c | 235 ++++++++++++++++++++---- src/backend/access/heap/visibilitymap.c | 22 ++- src/bin/pg_walsummary/t/002_blocks.pl | 7 +- src/include/access/heapam_xlog.h | 15 ++ src/include/access/visibilitymap.h | 2 + 7 files changed, 438 insertions(+), 86 deletions(-) diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c index 3e86283beb7..d7e4e051fff 100644 --- a/contrib/pg_surgery/heap_surgery.c +++ b/contrib/pg_surgery/heap_surgery.c @@ -266,9 +266,10 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) */ if (PageIsAllVisible(page)) { + LockBuffer(vmbuf, BUFFER_LOCK_EXCLUSIVE); PageClearAllVisible(page); - visibilitymap_clear(rel, blkno, vmbuf, - VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(rel, blkno, vmbuf, + VISIBILITYMAP_VALID_BITS); did_modify_vm = true; } } @@ -331,7 +332,10 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) UnlockReleaseBuffer(buf); - if (vmbuf != InvalidBuffer) + + if (did_modify_vm) + UnlockReleaseBuffer(vmbuf); + else if (BufferIsValid(vmbuf)) ReleaseBuffer(vmbuf); /* Update the current_start_ptr before moving to the next page. */ diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 6203e3d7f8d..38137f5dc8e 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -58,7 +58,8 @@ static HeapTuple heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid, CommandId cid, int options); static XLogRecPtr log_heap_update(Relation reln, Buffer oldbuf, - Buffer newbuf, HeapTuple oldtup, + Buffer vmbuffer_old, Buffer newbuf, + Buffer vmbuffer_new, HeapTuple oldtup, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared); #ifdef USE_ASSERT_CHECKING @@ -2135,10 +2136,12 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, if (PageIsAllVisible(BufferGetPage(buffer))) { all_visible_cleared = true; + + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); PageClearAllVisible(BufferGetPage(buffer)); - visibilitymap_clear(relation, - ItemPointerGetBlockNumber(&(heaptup->t_self)), - vmbuffer, VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(relation, + ItemPointerGetBlockNumber(&(heaptup->t_self)), + vmbuffer, VISIBILITYMAP_VALID_BITS); } /* @@ -2228,13 +2231,22 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); + if (all_visible_cleared) + XLogRegisterBuffer(1, vmbuffer, 0); + recptr = XLogInsert(RM_HEAP_ID, info); PageSetLSN(page, recptr); + + if (all_visible_cleared) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + if (all_visible_cleared) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + UnlockReleaseBuffer(buffer); if (vmbuffer != InvalidBuffer) ReleaseBuffer(vmbuffer); @@ -2503,10 +2515,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN)) { all_visible_cleared = true; + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); PageClearAllVisible(page); - visibilitymap_clear(relation, - BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(relation, + BufferGetBlockNumber(buffer), + vmbuffer, VISIBILITYMAP_VALID_BITS); } else if (all_frozen_set) PageSetAllVisible(page); @@ -2620,6 +2633,8 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, XLogBeginInsert(); XLogRegisterData(xlrec, tupledata - scratch.data); XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags); + if (all_visible_cleared) + XLogRegisterBuffer(1, vmbuffer, 0); XLogRegisterBufData(0, tupledata, totaldatalen); @@ -2629,10 +2644,15 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, recptr = XLogInsert(RM_HEAP2_ID, info); PageSetLSN(page, recptr); + if (all_visible_cleared) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + if (all_visible_cleared) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + /* * If we've frozen everything on the page, update the visibilitymap. * We're already holding pin on the vmbuffer. @@ -3050,9 +3070,10 @@ l1: if (PageIsAllVisible(page)) { all_visible_cleared = true; + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); PageClearAllVisible(page); - visibilitymap_clear(relation, BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(relation, BufferGetBlockNumber(buffer), + vmbuffer, VISIBILITYMAP_VALID_BITS); } /* store transaction information of xact deleting the tuple */ @@ -3133,13 +3154,23 @@ l1: /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); + if (all_visible_cleared) + XLogRegisterBuffer(1, vmbuffer, 0); + recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE); PageSetLSN(page, recptr); + + if (all_visible_cleared) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* release VM lock first, since it covers many heap blocks */ + if (all_visible_cleared) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (vmbuffer != InvalidBuffer) @@ -3257,7 +3288,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, HeapTuple heaptup; HeapTuple old_key_tuple = NULL; bool old_key_copied = false; - Page page; + Page page, + newpage; BlockNumber block; MultiXactStatus mxact_status; Buffer buffer, @@ -3274,6 +3306,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, bool key_intact; bool all_visible_cleared = false; bool all_visible_cleared_new = false; + bool modified_vmbuffer = false; + bool modified_vmbuffer_new = false; bool checked_lockers; bool locker_remains; bool id_has_external = false; @@ -3382,8 +3416,11 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, UnlockReleaseBuffer(buffer); Assert(!have_tuple_lock); - if (vmbuffer != InvalidBuffer) + if (BufferIsValid(vmbuffer)) + { ReleaseBuffer(vmbuffer); + vmbuffer = InvalidBuffer; + } tmfd->ctid = *otid; tmfd->xmax = InvalidTransactionId; tmfd->cmax = InvalidCommandId; @@ -3686,8 +3723,11 @@ l2: UnlockReleaseBuffer(buffer); if (have_tuple_lock) UnlockTupleTuplock(relation, &(oldtup.t_self), *lockmode); - if (vmbuffer != InvalidBuffer) + if (BufferIsValid(vmbuffer)) + { ReleaseBuffer(vmbuffer); + vmbuffer = InvalidBuffer; + } *update_indexes = TU_None; bms_free(hot_attrs); @@ -3870,10 +3910,15 @@ l2: * overhead would be unchanged, that doesn't seem necessarily * worthwhile. */ - if (PageIsAllVisible(page) && - visibilitymap_clear(relation, block, vmbuffer, - VISIBILITYMAP_ALL_FROZEN)) - cleared_all_frozen = true; + if (PageIsAllVisible(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + if (visibilitymap_clear_locked(relation, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + else + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + } MarkBufferDirty(buffer); @@ -3892,12 +3937,23 @@ l2: xlrec.flags = cleared_all_frozen ? XLH_LOCK_ALL_FROZEN_CLEARED : 0; XLogRegisterData(&xlrec, SizeOfHeapLock); + + if (cleared_all_frozen) + XLogRegisterBuffer(1, vmbuffer, 0); + recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK); PageSetLSN(page, recptr); + + if (cleared_all_frozen) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* release VM lock first, since it covers many heap blocks */ + if (cleared_all_frozen) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); /* @@ -3983,6 +4039,8 @@ l2: heaptup = newtup; } + newpage = BufferGetPage(newbuf); + /* * We're about to do the actual update -- check for conflict first, to * avoid possibly having to roll back work we've just done. @@ -4096,20 +4154,59 @@ l2: /* record address of new tuple in t_ctid of old one */ oldtup.t_data->t_ctid = heaptup->t_self; - /* clear PD_ALL_VISIBLE flags, reset all visibilitymap bits */ - if (PageIsAllVisible(BufferGetPage(buffer))) + /* + * Clear PD_ALL_VISIBLE flags and reset visibility map bits for any heap + * pages that were all-visible. If there are two heap pages, we may need + * to clear VM bits for both. + */ + if (PageIsAllVisible(page) && + newbuf != buffer && PageIsAllVisible(newpage) && + vmbuffer_new == vmbuffer) { + /* + * This is the more complicated case: both the new and old heap pages + * are all-visible and both their VM bits are on the same page of the + * VM, so we register a single VM buffer as HEAP_UPDATE_BLKREF_VM_NEW + * in the WAL record. We must be careful to only lock and register one + * buffer, even though we modify it twice -- once for each heap + * block's VM bits. + */ + PageClearAllVisible(page); + PageClearAllVisible(newpage); + LockBuffer(vmbuffer_new, BUFFER_LOCK_EXCLUSIVE); + visibilitymap_clear_locked(relation, block, + vmbuffer_new, VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(relation, BufferGetBlockNumber(newbuf), + vmbuffer_new, VISIBILITYMAP_VALID_BITS); + modified_vmbuffer_new = true; all_visible_cleared = true; - PageClearAllVisible(BufferGetPage(buffer)); - visibilitymap_clear(relation, BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); + all_visible_cleared_new = true; } - if (newbuf != buffer && PageIsAllVisible(BufferGetPage(newbuf))) + else { - all_visible_cleared_new = true; - PageClearAllVisible(BufferGetPage(newbuf)); - visibilitymap_clear(relation, BufferGetBlockNumber(newbuf), - vmbuffer_new, VISIBILITYMAP_VALID_BITS); + /* + * In all the remaining cases, we will clear at most one heap block's + * VM bits per VM page, so we don't have to worry about locking or + * registering the same buffer twice. + */ + if (PageIsAllVisible(page)) + { + PageClearAllVisible(page); + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + visibilitymap_clear_locked(relation, block, + vmbuffer, VISIBILITYMAP_VALID_BITS); + modified_vmbuffer = true; + all_visible_cleared = true; + } + if (newbuf != buffer && PageIsAllVisible(newpage)) + { + PageClearAllVisible(newpage); + LockBuffer(vmbuffer_new, BUFFER_LOCK_EXCLUSIVE); + visibilitymap_clear_locked(relation, BufferGetBlockNumber(newbuf), + vmbuffer_new, VISIBILITYMAP_VALID_BITS); + modified_vmbuffer_new = true; + all_visible_cleared_new = true; + } } if (newbuf != buffer) @@ -4132,7 +4229,10 @@ l2: } recptr = log_heap_update(relation, buffer, - newbuf, &oldtup, heaptup, + modified_vmbuffer ? vmbuffer : InvalidBuffer, + newbuf, + modified_vmbuffer_new ? vmbuffer_new : InvalidBuffer, + &oldtup, heaptup, old_key_tuple, all_visible_cleared, all_visible_cleared_new); @@ -4141,10 +4241,20 @@ l2: PageSetLSN(BufferGetPage(newbuf), recptr); } PageSetLSN(BufferGetPage(buffer), recptr); + + if (modified_vmbuffer) + PageSetLSN(BufferGetPage(vmbuffer), recptr); + if (modified_vmbuffer_new) + PageSetLSN(BufferGetPage(vmbuffer_new), recptr); } END_CRIT_SECTION(); + if (modified_vmbuffer) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + if (modified_vmbuffer_new) + LockBuffer(vmbuffer_new, BUFFER_LOCK_UNLOCK); + if (newbuf != buffer) LockBuffer(newbuf, BUFFER_LOCK_UNLOCK); LockBuffer(buffer, BUFFER_LOCK_UNLOCK); @@ -5193,10 +5303,15 @@ failed: tuple->t_data->t_ctid = *tid; /* Clear only the all-frozen bit on visibility map if needed */ - if (PageIsAllVisible(page) && - visibilitymap_clear(relation, block, vmbuffer, - VISIBILITYMAP_ALL_FROZEN)) - cleared_all_frozen = true; + if (PageIsAllVisible(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + if (visibilitymap_clear_locked(relation, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + else + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + } MarkBufferDirty(*buffer); @@ -5228,15 +5343,25 @@ failed: xlrec.flags = cleared_all_frozen ? XLH_LOCK_ALL_FROZEN_CLEARED : 0; XLogRegisterData(&xlrec, SizeOfHeapLock); + if (cleared_all_frozen) + XLogRegisterBuffer(1, vmbuffer, 0); + /* we don't decode row locks atm, so no need to log the origin */ recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_LOCK); PageSetLSN(page, recptr); + + if (cleared_all_frozen) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* release VM lock first, since it covers many heap blocks */ + if (cleared_all_frozen) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + result = TM_Ok; out_locked: @@ -5947,10 +6072,15 @@ l4: xid, mode, false, &new_xmax, &new_infomask, &new_infomask2); - if (PageIsAllVisible(BufferGetPage(buf)) && - visibilitymap_clear(rel, block, vmbuffer, - VISIBILITYMAP_ALL_FROZEN)) - cleared_all_frozen = true; + if (PageIsAllVisible(BufferGetPage(buf))) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + if (visibilitymap_clear_locked(rel, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + else + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + } START_CRIT_SECTION(); @@ -5981,13 +6111,23 @@ l4: XLogRegisterData(&xlrec, SizeOfHeapLockUpdated); + if (cleared_all_frozen) + XLogRegisterBuffer(1, vmbuffer, 0); + recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_LOCK_UPDATED); PageSetLSN(page, recptr); + + if (cleared_all_frozen) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* release VM lock first, since it covers many heap blocks */ + if (cleared_all_frozen) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + next: /* if we find the end of update chain, we're done. */ if (mytup.t_data->t_infomask & HEAP_XMAX_INVALID || @@ -8844,8 +8984,9 @@ log_heap_visible(Relation rel, Buffer heap_buffer, Buffer vm_buffer, * have modified the buffer(s) and marked them dirty. */ static XLogRecPtr -log_heap_update(Relation reln, Buffer oldbuf, - Buffer newbuf, HeapTuple oldtup, HeapTuple newtup, +log_heap_update(Relation reln, Buffer oldbuf, Buffer vmbuffer_old, + Buffer newbuf, Buffer vmbuffer_new, + HeapTuple oldtup, HeapTuple newtup, HeapTuple old_key_tuple, bool all_visible_cleared, bool new_all_visible_cleared) { @@ -8972,9 +9113,9 @@ log_heap_update(Relation reln, Buffer oldbuf, if (need_tuple_data) bufflags |= REGBUF_KEEP_DATA; - XLogRegisterBuffer(0, newbuf, bufflags); + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_NEW, newbuf, bufflags); if (oldbuf != newbuf) - XLogRegisterBuffer(1, oldbuf, REGBUF_STANDARD); + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_OLD, oldbuf, REGBUF_STANDARD); XLogRegisterData(&xlrec, SizeOfHeapUpdate); @@ -9051,6 +9192,20 @@ log_heap_update(Relation reln, Buffer oldbuf, old_key_tuple->t_len - SizeofHeapTupleHeader); } + /* + * Register VM buffers. If the old and new heap pages' VM bits are on the + * same VM page, the caller passes only vmbuffer_new (mirroring the heap + * page convention where block 0 = new is always registered). + */ + Assert((BufferIsInvalid(vmbuffer_old) && BufferIsInvalid(vmbuffer_new)) || + (vmbuffer_old != vmbuffer_new)); + + if (BufferIsValid(vmbuffer_new)) + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_NEW, vmbuffer_new, 0); + + if (BufferIsValid(vmbuffer_old)) + XLogRegisterBuffer(HEAP_UPDATE_BLKREF_VM_OLD, vmbuffer_old, 0); + /* filtering by origin on a row level is much more efficient */ XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN); diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c index eb4bd3d6ae3..364e51791a5 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -363,9 +363,24 @@ heap_xlog_delete(XLogReaderState *record) Relation reln = CreateFakeRelcacheEntry(target_locator); Buffer vmbuffer = InvalidBuffer; - visibilitymap_pin(reln, blkno, &vmbuffer); - visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); + if (XLogRecHasBlockRef(record, 1)) + { + if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, + blkno, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, blkno, &vmbuffer); + visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); + ReleaseBuffer(vmbuffer); + } FreeFakeRelcacheEntry(reln); } @@ -450,9 +465,24 @@ heap_xlog_insert(XLogReaderState *record) Relation reln = CreateFakeRelcacheEntry(target_locator); Buffer vmbuffer = InvalidBuffer; - visibilitymap_pin(reln, blkno, &vmbuffer); - visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); + if (XLogRecHasBlockRef(record, 1)) + { + if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, + blkno, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, blkno, &vmbuffer); + visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); + ReleaseBuffer(vmbuffer); + } FreeFakeRelcacheEntry(reln); } @@ -566,17 +596,38 @@ heap_xlog_multi_insert(XLogReaderState *record) (xlrec->flags & XLH_INSERT_ALL_FROZEN_SET))); /* - * The visibility map may need to be fixed even if the heap page is - * already up-to-date. + * If required, clear the VM first, to prevent all-visible temporarily + * being set for a heap page that's not all visible anymore. + * + * If it were possible for XLH_INSERT_ALL_VISIBLE_CLEARED and + * XLH_INSERT_ALL_FROZEN_SET to be present in the same record, doing the + * XLogReadBufferForRedo() before the PageSetAllVisible() below would be a + * problem, as it'd violate the rule that a heap page must never be set + * all-visible in the VM while its PD_ALL_VISIBLE is clear. */ if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) { Relation reln = CreateFakeRelcacheEntry(rlocator); Buffer vmbuffer = InvalidBuffer; - visibilitymap_pin(reln, blkno, &vmbuffer); - visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); + if (XLogRecHasBlockRef(record, 1)) + { + if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, + blkno, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, blkno, &vmbuffer); + visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); + ReleaseBuffer(vmbuffer); + } FreeFakeRelcacheEntry(reln); } @@ -691,6 +742,8 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) Buffer obuffer, nbuffer; Page page; + bool new_cleared, + old_cleared; OffsetNumber offnum; ItemId lp = NULL; HeapTupleData oldtup; @@ -728,14 +781,107 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) * The visibility map may need to be fixed even if the heap page is * already up-to-date. */ - if (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) + new_cleared = (xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0; + old_cleared = (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0; + if (new_cleared || old_cleared) { Relation reln = CreateFakeRelcacheEntry(rlocator); - Buffer vmbuffer = InvalidBuffer; + bool has_vm_old = XLogRecHasBlockRef(record, HEAP_UPDATE_BLKREF_VM_OLD); + bool has_vm_new = XLogRecHasBlockRef(record, HEAP_UPDATE_BLKREF_VM_NEW); + + if (has_vm_new || has_vm_old) + { + /* + * If both the old and new heap pages were all-visible and their + * VM bits are on the same VM page, that single VM page is + * registered as HEAP_UPDATE_BLKREF_VM_NEW. Clear both heap + * blocks' VM bits from the single provided VM buffer. + */ + bool same_vm_page = new_cleared && old_cleared && + !has_vm_old && has_vm_new; + + if (same_vm_page) + { + Buffer vmbuffer = InvalidBuffer; + + if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) == + BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, oldblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(reln, newblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + /* + * We can be sure that we need to clear at most one heap + * page's VM bits in each registered VM buffer. + */ + if (new_cleared) + { + Buffer vmbuffer = InvalidBuffer; + + if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) == + BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, newblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + if (old_cleared) + { + Buffer vmbuffer = InvalidBuffer; + + if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, &vmbuffer) == + BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, oldblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + } + } + else + { + /* + * Backwards compatibility path. Previously, the VM buffers were + * not registered in the WAL record. We need this path to replay + * WAL generated by a not-yet-patched primary during upgrade. + */ + if (old_cleared) + { + Buffer vmbuffer = InvalidBuffer; + + visibilitymap_pin(reln, oldblk, &vmbuffer); + visibilitymap_clear(reln, oldblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + ReleaseBuffer(vmbuffer); + } + if (new_cleared) + { + Buffer vmbuffer = InvalidBuffer; + + visibilitymap_pin(reln, newblk, &vmbuffer); + visibilitymap_clear(reln, newblk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + ReleaseBuffer(vmbuffer); + } + } - visibilitymap_pin(reln, oldblk, &vmbuffer); - visibilitymap_clear(reln, oldblk, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); FreeFakeRelcacheEntry(reln); } @@ -808,21 +954,6 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) else newaction = XLogReadBufferForRedo(record, 0, &nbuffer); - /* - * The visibility map may need to be fixed even if the heap page is - * already up-to-date. - */ - if (xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) - { - Relation reln = CreateFakeRelcacheEntry(rlocator); - Buffer vmbuffer = InvalidBuffer; - - visibilitymap_pin(reln, newblk, &vmbuffer); - visibilitymap_clear(reln, newblk, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); - FreeFakeRelcacheEntry(reln); - } - /* Deal with new tuple */ if (newaction == BLK_NEEDS_REDO) { @@ -1018,10 +1149,23 @@ heap_xlog_lock(XLogReaderState *record) XLogRecGetBlockTag(record, 0, &rlocator, NULL, &block); reln = CreateFakeRelcacheEntry(rlocator); - visibilitymap_pin(reln, block, &vmbuffer); - visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); - - ReleaseBuffer(vmbuffer); + if (XLogRecHasBlockRef(record, 1)) + { + if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, block, &vmbuffer); + visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); + ReleaseBuffer(vmbuffer); + } FreeFakeRelcacheEntry(reln); } @@ -1094,10 +1238,23 @@ heap_xlog_lock_updated(XLogReaderState *record) XLogRecGetBlockTag(record, 0, &rlocator, NULL, &block); reln = CreateFakeRelcacheEntry(rlocator); - visibilitymap_pin(reln, block, &vmbuffer); - visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); - - ReleaseBuffer(vmbuffer); + if (XLogRecHasBlockRef(record, 1)) + { + if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, block, &vmbuffer); + visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); + ReleaseBuffer(vmbuffer); + } FreeFakeRelcacheEntry(reln); } diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 1874a3fda37..89079b52a2f 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -135,9 +135,27 @@ static Buffer vm_extend(Relation rel, BlockNumber vm_nblocks); * You must pass a buffer containing the correct map page to this function. * Call visibilitymap_pin first to pin the right one. This function doesn't do * any I/O. Returns true if any bits have been cleared and false otherwise. + * + * This is retained for backwards compatability, as it is usually necessary to + * register the VM buffer in the WAL record and this necessitates holding the + * lock for longer than it is held here. */ bool visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags) +{ + bool cleared = false; + + LockBuffer(vmbuf, BUFFER_LOCK_EXCLUSIVE); + + cleared = visibilitymap_clear_locked(rel, heapBlk, vmbuf, flags); + + LockBuffer(vmbuf, BUFFER_LOCK_UNLOCK); + + return cleared; +} + +bool +visibilitymap_clear_locked(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags) { BlockNumber mapBlock = HEAPBLK_TO_MAPBLOCK(heapBlk); int mapByte = HEAPBLK_TO_MAPBYTE(heapBlk); @@ -157,7 +175,8 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags if (!BufferIsValid(vmbuf) || BufferGetBlockNumber(vmbuf) != mapBlock) elog(ERROR, "wrong buffer passed to visibilitymap_clear"); - LockBuffer(vmbuf, BUFFER_LOCK_EXCLUSIVE); + Assert(BufferIsExclusiveLocked(vmbuf)); + map = PageGetContents(BufferGetPage(vmbuf)); if (map[mapByte] & mask) @@ -168,7 +187,6 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags cleared = true; } - LockBuffer(vmbuf, BUFFER_LOCK_UNLOCK); return cleared; } diff --git a/src/bin/pg_walsummary/t/002_blocks.pl b/src/bin/pg_walsummary/t/002_blocks.pl index 0f98c7df82e..b545098be60 100644 --- a/src/bin/pg_walsummary/t/002_blocks.pl +++ b/src/bin/pg_walsummary/t/002_blocks.pl @@ -93,13 +93,14 @@ my $filename = sprintf "%s/pg_wal/summaries/%08s%08s%08s%08s%08s.summary", split(m@/@, $end_lsn); ok(-f $filename, "WAL summary file exists"); -# Run pg_walsummary on it. We expect exactly two blocks to be modified, -# block 0 and one other. +# Run pg_walsummary on it. We expect exactly three blocks to be modified, +# block 0 (old tuple), another block (new tuple) and the block for the VM. my ($stdout, $stderr) = run_command([ 'pg_walsummary', '-i', $filename ]); note($stdout); @lines = split(/\n/, $stdout); like($stdout, qr/FORK main: block 0$/m, "stdout shows block 0 modified"); +like($stdout, qr/FORK vm: block 0$/m, "stdout shows VM block 0 modified"); is($stderr, '', 'stderr is empty'); -is(0 + @lines, 2, "UPDATE modified 2 blocks"); +is(0 + @lines, 3, "UPDATE modified 3 blocks"); done_testing(); diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index 277df6b3cf0..0e55ae58b5c 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -214,7 +214,22 @@ typedef struct xl_multi_insert_tuple * included even if a full-page image was taken. * * Backup blk 1: old page, if different. (no data, just a reference to the blk) + * + * Backup blk 2: VM page covering the new heap page. Registered whenever + * XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED is set. Also covers the old + * heap page's VM bits when both heap pages map to the same VM + * page. + * + * Backup blk 3: VM page covering the old heap page. Only registered when + * XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is set and the old heap + * page's VM bits are on a different VM page from the new heap + * page's. */ +#define HEAP_UPDATE_BLKREF_NEW 0 +#define HEAP_UPDATE_BLKREF_OLD 1 +#define HEAP_UPDATE_BLKREF_VM_NEW 2 +#define HEAP_UPDATE_BLKREF_VM_OLD 3 + typedef struct xl_heap_update { TransactionId old_xmax; /* xmax of the old tuple */ diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h index ea889bf9ec7..1bc59c9ac33 100644 --- a/src/include/access/visibilitymap.h +++ b/src/include/access/visibilitymap.h @@ -26,6 +26,8 @@ #define VM_ALL_FROZEN(r, b, v) \ ((visibilitymap_get_status((r), (b), (v)) & VISIBILITYMAP_ALL_FROZEN) != 0) +extern bool visibilitymap_clear_locked(Relation rel, BlockNumber heapBlk, + Buffer vmbuf, uint8 flags); extern bool visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags); extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk, -- 2.43.0