From 9358530be17b2e6d84bb1956dc09afadd817e5df Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Wed, 29 Apr 2026 12:53:49 -0400 Subject: [PATCH v3 2/4] 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 ZERO_ON_ERROR, the 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. Author: Melanie Plageman Author: Andres Freund Discussion: https://postgr.es/m/flat/CAAKRu_bn%2Be7F4yPFBgFbnP%2BsyJRKyNK092bjD2LKvZW7O4Svag> Backpatch-through: 17 --- contrib/pg_surgery/heap_surgery.c | 39 ++- src/backend/access/heap/heapam.c | 358 ++++++++++++++++++++---- src/backend/access/heap/heapam_xlog.c | 219 ++++++++++----- src/backend/access/heap/visibilitymap.c | 23 +- src/bin/pg_walsummary/t/002_blocks.pl | 7 +- src/include/access/heapam_xlog.h | 17 +- src/include/access/visibilitymap.h | 2 + 7 files changed, 520 insertions(+), 145 deletions(-) diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c index 602aca66c60..2f89fc1159f 100644 --- a/contrib/pg_surgery/heap_surgery.c +++ b/contrib/pg_surgery/heap_surgery.c @@ -17,6 +17,7 @@ #include "access/visibilitymap.h" #include "access/xloginsert.h" #include "catalog/pg_am_d.h" +#include "catalog/pg_control.h" #include "miscadmin.h" #include "storage/bufmgr.h" #include "utils/acl.h" @@ -146,6 +147,7 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) { Buffer buf; Buffer vmbuf = InvalidBuffer; + bool unlock_vmbuf = false; Page page; BlockNumber blkno; OffsetNumber curoff; @@ -233,11 +235,15 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) } /* - * Before entering the critical section, pin the visibility map page - * if it appears to be necessary. + * Before entering the critical section, pin and lock the visibility + * map page if it appears to be necessary. */ if (heap_force_opt == HEAP_FORCE_KILL && PageIsAllVisible(page)) + { visibilitymap_pin(rel, blkno, &vmbuf); + LockBuffer(vmbuf, BUFFER_LOCK_EXCLUSIVE); + unlock_vmbuf = true; + } /* No ereport(ERROR) from here until all the changes are logged. */ START_CRIT_SECTION(); @@ -266,10 +272,11 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) */ if (PageIsAllVisible(page)) { + if (visibilitymap_clear_locked(rel, blkno, vmbuf, + VISIBILITYMAP_VALID_BITS)) + did_modify_vm = true; + PageClearAllVisible(page); - visibilitymap_clear(rel, blkno, vmbuf, - VISIBILITYMAP_VALID_BITS); - did_modify_vm = true; } } else @@ -320,18 +327,28 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt) /* XLOG stuff */ if (RelationNeedsWAL(rel)) - log_newpage_buffer(buf, true); + { + XLogRecPtr recptr; + + XLogBeginInsert(); + XLogRegisterBuffer(0, buf, REGBUF_STANDARD | REGBUF_FORCE_IMAGE); + if (did_modify_vm) + XLogRegisterBuffer(1, vmbuf, REGBUF_FORCE_IMAGE); + recptr = XLogInsert(RM_XLOG_ID, XLOG_FPI); + if (did_modify_vm) + PageSetLSN(BufferGetPage(vmbuf), recptr); + PageSetLSN(BufferGetPage(buf), recptr); + } } - /* WAL log the VM page if it was modified. */ - if (did_modify_vm && RelationNeedsWAL(rel)) - log_newpage_buffer(vmbuf, false); - END_CRIT_SECTION(); UnlockReleaseBuffer(buf); - if (vmbuf != InvalidBuffer) + if (unlock_vmbuf) + LockBuffer(vmbuf, BUFFER_LOCK_UNLOCK); + + 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 3c3b65b3cbf..7dfa3221652 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 @@ -2083,8 +2084,10 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, TransactionId xid = GetCurrentTransactionId(); HeapTuple heaptup; Buffer buffer; + Page page; Buffer vmbuffer = InvalidBuffer; bool all_visible_cleared = false; + bool vmbuffer_modified = false; /* Cheap, simplistic check that the tuple matches the rel's rowtype. */ Assert(HeapTupleHeaderGetNatts(tup->t_data) <= @@ -2108,6 +2111,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, InvalidBuffer, options, bistate, &vmbuffer, NULL, 0); + page = BufferGetPage(buffer); /* * We're about to do the actual insert -- but check for conflict first, to @@ -2126,19 +2130,28 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, */ CheckForSerializableConflictIn(relation, NULL, InvalidBlockNumber); + /* Lock the vmbuffer before the critical section */ + if (PageIsAllVisible(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + all_visible_cleared = true; + } + /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); RelationPutHeapTuple(relation, buffer, heaptup, (options & HEAP_INSERT_SPECULATIVE) != 0); - if (PageIsAllVisible(BufferGetPage(buffer))) + if (all_visible_cleared) { - all_visible_cleared = true; - PageClearAllVisible(BufferGetPage(buffer)); - visibilitymap_clear(relation, - ItemPointerGetBlockNumber(&(heaptup->t_self)), - vmbuffer, VISIBILITYMAP_VALID_BITS); + /* It's possible the VM bits were already clear */ + if (visibilitymap_clear_locked(relation, + ItemPointerGetBlockNumber(&(heaptup->t_self)), + vmbuffer, VISIBILITYMAP_VALID_BITS)) + vmbuffer_modified = true; + + PageClearAllVisible(page); } /* @@ -2160,7 +2173,6 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid, xl_heap_insert xlrec; xl_heap_header xlhdr; XLogRecPtr recptr; - Page page = BufferGetPage(buffer); uint8 info = XLOG_HEAP_INSERT; int bufflags = 0; @@ -2230,15 +2242,28 @@ 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); + recptr = XLogInsert(RM_HEAP_ID, info); PageSetLSN(page, recptr); + + if (vmbuffer_modified) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); UnlockReleaseBuffer(buffer); - if (vmbuffer != InvalidBuffer) + + /* + * We locked vmbuffer if all_visible_cleared was true regardless of + * whether or not we ended up modifying it. + */ + if (all_visible_cleared) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + if (BufferIsValid(vmbuffer)) ReleaseBuffer(vmbuffer); /* @@ -2421,6 +2446,7 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, Buffer buffer; bool all_visible_cleared = false; bool all_frozen_set = false; + bool vmbuffer_modified = false; int nthispage; CHECK_FOR_INTERRUPTS(); @@ -2462,6 +2488,17 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, if (starting_with_empty_page && (options & HEAP_INSERT_FROZEN)) all_frozen_set = true; + /* + * If clearing all-visible, take the VM buffer lock before entering + * the critical section where that action will be WAL-logged. Setting + * the VM all-frozen is done and WAL-logged separately. + */ + if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + all_visible_cleared = true; + } + /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); @@ -2502,13 +2539,14 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, * If we're only adding already frozen rows to a previously empty * page, mark it as all-visible. */ - if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN)) + if (all_visible_cleared) { - all_visible_cleared = true; + if (visibilitymap_clear_locked(relation, + BufferGetBlockNumber(buffer), + vmbuffer, VISIBILITYMAP_VALID_BITS)) + vmbuffer_modified = true; + PageClearAllVisible(page); - visibilitymap_clear(relation, - BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); } else if (all_frozen_set) PageSetAllVisible(page); @@ -2623,6 +2661,8 @@ 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 (vmbuffer_modified) + XLogRegisterBuffer(HEAP_MULTI_INSERT_BLKREF_VM, vmbuffer, 0); XLogRegisterBufData(HEAP_MULTI_INSERT_BLKREF_HEAP, tupledata, totaldatalen); @@ -2633,10 +2673,19 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples, recptr = XLogInsert(RM_HEAP2_ID, info); PageSetLSN(page, recptr); + if (vmbuffer_modified) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* + * We locked vmbuffer if all_visible_cleared was true regardless of + * whether or not we ended up modifying it. + */ + 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. @@ -2786,6 +2835,7 @@ heap_delete(Relation relation, ItemPointer tid, BlockNumber block; Buffer buffer; Buffer vmbuffer = InvalidBuffer; + bool vmbuffer_modified = false; TransactionId new_xmax; uint16 new_infomask, new_infomask2; @@ -3040,6 +3090,13 @@ l1: xid, LockTupleExclusive, true, &new_xmax, &new_infomask, &new_infomask2); + /* Lock the VM before entering the critical section */ + if (PageIsAllVisible(page)) + { + all_visible_cleared = true; + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + } + START_CRIT_SECTION(); /* @@ -3051,12 +3108,14 @@ l1: */ PageSetPrunable(page, xid); - if (PageIsAllVisible(page)) + if (all_visible_cleared) { - all_visible_cleared = true; + /* It's possible the VM bits were already clear */ + if (visibilitymap_clear_locked(relation, BufferGetBlockNumber(buffer), + vmbuffer, VISIBILITYMAP_VALID_BITS)) + vmbuffer_modified = true; + PageClearAllVisible(page); - visibilitymap_clear(relation, BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); } /* store transaction information of xact deleting the tuple */ @@ -3137,13 +3196,27 @@ l1: /* 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); + recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE); PageSetLSN(page, recptr); + + if (vmbuffer_modified) + PageSetLSN(BufferGetPage(vmbuffer), recptr); } END_CRIT_SECTION(); + /* + * Release VM lock first, since it covers many heap blocks. We locked + * vmbuffer if all_visible_cleared was true regardless of whether or not + * we ended up modifying it. + */ + if (all_visible_cleared) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); if (vmbuffer != InvalidBuffer) @@ -3261,13 +3334,16 @@ 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, newbuf, vmbuffer = InvalidBuffer, vmbuffer_new = InvalidBuffer; + bool unlock_vmbuffer = false; + bool unlock_vmbuffer_new = false; bool need_toast; Size newtupsize, pagefree; @@ -3278,6 +3354,8 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup, bool key_intact; bool all_visible_cleared = false; bool all_visible_cleared_new = false; + bool vmbuffer_modified = false; + bool vmbuffer_new_modified = false; bool checked_lockers; bool locker_remains; bool id_has_external = false; @@ -3852,6 +3930,12 @@ l2: Assert(HEAP_XMAX_IS_LOCKED_ONLY(infomask_lock_old_tuple)); + if (PageIsAllVisible(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + unlock_vmbuffer = true; + } + START_CRIT_SECTION(); /* Clear obsolete visibility flags ... */ @@ -3874,10 +3958,12 @@ 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)) + { + if (visibilitymap_clear_locked(relation, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + } MarkBufferDirty(buffer); @@ -3896,12 +3982,24 @@ l2: xlrec.flags = cleared_all_frozen ? XLH_LOCK_ALL_FROZEN_CLEARED : 0; XLogRegisterData(&xlrec, SizeOfHeapLock); + + if (cleared_all_frozen) + XLogRegisterBuffer(HEAP_LOCK_BLKREF_VM, 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 (unlock_vmbuffer) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + unlock_vmbuffer = false; + LockBuffer(buffer, BUFFER_LOCK_UNLOCK); /* @@ -3987,6 +4085,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. @@ -4050,6 +4150,69 @@ l2: id_has_external, &old_key_copied); + all_visible_cleared = PageIsAllVisible(page); + all_visible_cleared_new = newbuf != buffer && PageIsAllVisible(newpage); + + /* + * 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 (all_visible_cleared && all_visible_cleared_new && + 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. + */ + LockBuffer(vmbuffer_new, BUFFER_LOCK_EXCLUSIVE); + unlock_vmbuffer_new = true; + + /* We will not lock or attempt to modify old VM buffer */ + } + else + { + /* + * In all the remaining cases, we will clear at most one heap block's + * VM bits per VM page. + */ + Buffer vmbuffers[2] = { + all_visible_cleared ? vmbuffer : InvalidBuffer, + all_visible_cleared_new ? vmbuffer_new : InvalidBuffer + }; + + /* + * When both pages need different VM pages cleared, acquire the VM + * buffer locks in VM block order to avoid deadlocks between backends + * updating tuples in opposite directions across VM pages. + */ + if (all_visible_cleared && all_visible_cleared_new && + BufferGetBlockNumber(vmbuffers[0]) > BufferGetBlockNumber(vmbuffers[1])) + { + Buffer swap = vmbuffers[0]; + + vmbuffers[0] = vmbuffers[1]; + vmbuffers[1] = swap; + } + + Assert((!BufferIsValid(vmbuffers[0]) && !BufferIsValid(vmbuffers[1])) || + vmbuffers[0] != vmbuffers[1]); + + if (BufferIsValid(vmbuffers[0])) + LockBuffer(vmbuffers[0], BUFFER_LOCK_EXCLUSIVE); + if (BufferIsValid(vmbuffers[1])) + LockBuffer(vmbuffers[1], BUFFER_LOCK_EXCLUSIVE); + + if (all_visible_cleared) + unlock_vmbuffer = true; + if (all_visible_cleared_new) + unlock_vmbuffer_new = true; + } + /* NO EREPORT(ERROR) from here till changes are logged */ START_CRIT_SECTION(); @@ -4086,6 +4249,34 @@ l2: RelationPutHeapTuple(relation, newbuf, heaptup, false); /* insert new tuple */ + if (all_visible_cleared) + { + if (visibilitymap_clear_locked(relation, block, + vmbuffer, VISIBILITYMAP_VALID_BITS)) + { + /* + * If both heap block's VM bits are on the same VM buffer, + * vmbuffer_new and vmbuffer will be equal. In that case only + * vmbuffer_new is registered in the WAL record (see + * log_heap_update), so attribute any change to the old heap + * block's VM bits to vmbuffer_new as well. + */ + if (vmbuffer == vmbuffer_new) + vmbuffer_new_modified = true; + else + vmbuffer_modified = true; + } + + PageClearAllVisible(page); + } + if (all_visible_cleared_new) + { + if (visibilitymap_clear_locked(relation, BufferGetBlockNumber(newbuf), + vmbuffer_new, VISIBILITYMAP_VALID_BITS)) + vmbuffer_new_modified = true; + + PageClearAllVisible(newpage); + } /* Clear obsolete visibility flags, possibly set by ourselves above... */ oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED); @@ -4100,21 +4291,6 @@ 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))) - { - all_visible_cleared = true; - PageClearAllVisible(BufferGetPage(buffer)); - visibilitymap_clear(relation, BufferGetBlockNumber(buffer), - vmbuffer, VISIBILITYMAP_VALID_BITS); - } - if (newbuf != buffer && PageIsAllVisible(BufferGetPage(newbuf))) - { - all_visible_cleared_new = true; - PageClearAllVisible(BufferGetPage(newbuf)); - visibilitymap_clear(relation, BufferGetBlockNumber(newbuf), - vmbuffer_new, VISIBILITYMAP_VALID_BITS); - } if (newbuf != buffer) MarkBufferDirty(newbuf); @@ -4136,19 +4312,30 @@ l2: } recptr = log_heap_update(relation, buffer, - newbuf, &oldtup, heaptup, + vmbuffer_modified ? vmbuffer : InvalidBuffer, + newbuf, + vmbuffer_new_modified ? vmbuffer_new : InvalidBuffer, + &oldtup, heaptup, old_key_tuple, all_visible_cleared, all_visible_cleared_new); if (newbuf != buffer) - { - PageSetLSN(BufferGetPage(newbuf), recptr); - } + PageSetLSN(newpage, recptr); PageSetLSN(BufferGetPage(buffer), recptr); + + if (vmbuffer_modified) + PageSetLSN(BufferGetPage(vmbuffer), recptr); + if (vmbuffer_new_modified) + PageSetLSN(BufferGetPage(vmbuffer_new), recptr); } END_CRIT_SECTION(); + if (unlock_vmbuffer) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + if (unlock_vmbuffer_new) + LockBuffer(vmbuffer_new, BUFFER_LOCK_UNLOCK); + if (newbuf != buffer) LockBuffer(newbuf, BUFFER_LOCK_UNLOCK); LockBuffer(buffer, BUFFER_LOCK_UNLOCK); @@ -4586,6 +4773,7 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, ItemId lp; Page page; Buffer vmbuffer = InvalidBuffer; + bool unlock_vmbuffer = false; BlockNumber block; TransactionId xid, xmax; @@ -5166,6 +5354,13 @@ failed: GetCurrentTransactionId(), mode, false, &xid, &new_infomask, &new_infomask2); + /* Lock VM buffer before entering critical section */ + if (PageIsAllVisible(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + unlock_vmbuffer = true; + } + START_CRIT_SECTION(); /* @@ -5197,11 +5392,12 @@ 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)) + { + if (visibilitymap_clear_locked(relation, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + } MarkBufferDirty(*buffer); @@ -5232,15 +5428,25 @@ failed: xlrec.flags = cleared_all_frozen ? XLH_LOCK_ALL_FROZEN_CLEARED : 0; XLogRegisterData(&xlrec, SizeOfHeapLock); + if (cleared_all_frozen) + XLogRegisterBuffer(HEAP_LOCK_BLKREF_VM, 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 (unlock_vmbuffer) + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + result = TM_Ok; out_locked: @@ -5707,6 +5913,7 @@ heap_lock_updated_tuple_rec(Relation rel, TransactionId priorXmax, ItemPointerData tupid; HeapTupleData mytup; Buffer buf; + Page page; uint16 new_infomask, new_infomask2, old_infomask, @@ -5716,6 +5923,7 @@ heap_lock_updated_tuple_rec(Relation rel, TransactionId priorXmax, bool cleared_all_frozen = false; bool pinned_desired_page; Buffer vmbuffer = InvalidBuffer; + bool unlock_vmbuffer = false; BlockNumber block; ItemPointerCopy(tid, &tupid); @@ -5724,6 +5932,7 @@ heap_lock_updated_tuple_rec(Relation rel, TransactionId priorXmax, { new_infomask = 0; new_xmax = InvalidTransactionId; + cleared_all_frozen = false; block = ItemPointerGetBlockNumber(&tupid); ItemPointerCopy(&tupid, &(mytup.t_self)); @@ -5743,13 +5952,15 @@ heap_lock_updated_tuple_rec(Relation rel, TransactionId priorXmax, l4: CHECK_FOR_INTERRUPTS(); + page = BufferGetPage(buf); + /* * Before locking the buffer, pin the visibility map page if it * appears to be necessary. Since we haven't got the lock yet, * someone else might be in the middle of changing this, so we'll need * to recheck after we have the lock. */ - if (PageIsAllVisible(BufferGetPage(buf))) + if (PageIsAllVisible(page)) { visibilitymap_pin(rel, block, &vmbuffer); pinned_desired_page = true; @@ -5770,7 +5981,7 @@ l4: * this page. If this page isn't all-visible, we won't use the vm * page, but we hold onto such a pin till the end of the function. */ - if (!pinned_desired_page && PageIsAllVisible(BufferGetPage(buf))) + if (!pinned_desired_page && PageIsAllVisible(page)) { LockBuffer(buf, BUFFER_LOCK_UNLOCK); visibilitymap_pin(rel, block, &vmbuffer); @@ -5951,10 +6162,11 @@ 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(page)) + { + LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE); + unlock_vmbuffer = true; + } START_CRIT_SECTION(); @@ -5967,12 +6179,18 @@ l4: MarkBufferDirty(buf); + if (PageIsAllVisible(page)) + { + if (visibilitymap_clear_locked(rel, block, vmbuffer, + VISIBILITYMAP_ALL_FROZEN)) + cleared_all_frozen = true; + } + /* XLOG stuff */ if (RelationNeedsWAL(rel)) { xl_heap_lock_updated xlrec; XLogRecPtr recptr; - Page page = BufferGetPage(buf); XLogBeginInsert(); XLogRegisterBuffer(HEAP_LOCK_BLKREF_HEAP, buf, REGBUF_STANDARD); @@ -5985,13 +6203,26 @@ l4: XLogRegisterData(&xlrec, SizeOfHeapLockUpdated); + if (cleared_all_frozen) + XLogRegisterBuffer(HEAP_LOCK_BLKREF_VM, 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 (unlock_vmbuffer) + { + LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK); + unlock_vmbuffer = false; + } + next: /* if we find the end of update chain, we're done. */ if (mytup.t_data->t_infomask & HEAP_XMAX_INVALID || @@ -8848,8 +9079,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) { @@ -9058,6 +9290,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 6cc3bc991c3..1b546268a20 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -22,6 +22,51 @@ #include "storage/freespace.h" #include "storage/standby.h" +/* + * Helper to clear visibility map bits during heap redo. + * + * This handles records that modify one heap block and its corresponding VM + * block. More complex cases, such as update records that can touch multiple + * heap or VM blocks, must handle VM replay directly. 'flags' specifies which + * visibility map bits to clear. + */ +static void +heap_xlog_vm_clear(XLogReaderState *record, + XLogRecPtr lsn, RelFileLocator target_locator, + BlockNumber heap_blkno, + int vm_block_id, uint8 flags) +{ + Relation reln = CreateFakeRelcacheEntry(target_locator); + Buffer vmbuffer = InvalidBuffer; + + /* + * If this record includes the VM block, use it for redo. Older releases + * did not register the VM buffer when clearing the VM, so keep the + * fallback path to support replay of WAL generated before that fix. + */ + if (XLogRecHasBlockRef(record, vm_block_id)) + { + if (XLogReadBufferForRedo(record, vm_block_id, + &vmbuffer) == BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, + heap_blkno, vmbuffer, + flags); + PageSetLSN(BufferGetPage(vmbuffer), lsn); + } + if (BufferIsValid(vmbuffer)) + UnlockReleaseBuffer(vmbuffer); + } + else + { + visibilitymap_pin(reln, heap_blkno, &vmbuffer); + visibilitymap_clear(reln, heap_blkno, vmbuffer, flags); + ReleaseBuffer(vmbuffer); + } + + FreeFakeRelcacheEntry(reln); +} + /* * Replay XLOG_HEAP2_PRUNE_* records. @@ -360,15 +405,9 @@ heap_xlog_delete(XLogReaderState *record) * already up-to-date. */ if (xlrec->flags & XLH_DELETE_ALL_VISIBLE_CLEARED) - { - Relation reln = CreateFakeRelcacheEntry(target_locator); - Buffer vmbuffer = InvalidBuffer; - - visibilitymap_pin(reln, blkno, &vmbuffer); - visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); - FreeFakeRelcacheEntry(reln); - } + heap_xlog_vm_clear(record, lsn, target_locator, + blkno, HEAP_DELETE_BLKREF_VM, + VISIBILITYMAP_VALID_BITS); if (XLogReadBufferForRedo(record, HEAP_DELETE_BLKREF_HEAP, &buffer) == BLK_NEEDS_REDO) @@ -449,15 +488,9 @@ heap_xlog_insert(XLogReaderState *record) * already up-to-date. */ if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) - { - Relation reln = CreateFakeRelcacheEntry(target_locator); - Buffer vmbuffer = InvalidBuffer; - - visibilitymap_pin(reln, blkno, &vmbuffer); - visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS); - ReleaseBuffer(vmbuffer); - FreeFakeRelcacheEntry(reln); - } + heap_xlog_vm_clear(record, lsn, target_locator, + blkno, HEAP_INSERT_BLKREF_VM, + VISIBILITYMAP_VALID_BITS); /* * If we inserted the first and only tuple on the page, re-initialize the @@ -571,19 +604,19 @@ 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); - FreeFakeRelcacheEntry(reln); - } + heap_xlog_vm_clear(record, lsn, rlocator, + blkno, HEAP_MULTI_INSERT_BLKREF_VM, + VISIBILITYMAP_VALID_BITS); if (isinit) { @@ -698,6 +731,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; @@ -737,14 +772,85 @@ 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) + { + Buffer vmbuffer_new = InvalidBuffer; + + Assert(new_cleared); + if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer_new) == + BLK_NEEDS_REDO) + { + /* + * 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. + * + * We must verify that oldblk's VM bits really are on this VM + * page, rather than relying on the absence of a separate + * VM_OLD block reference: VM_OLD is also omitted when oldblk + * is on a different VM page but its bit was already clear. + */ + if (old_cleared && visibilitymap_pin_ok(oldblk, vmbuffer_new)) + visibilitymap_clear_locked(reln, oldblk, vmbuffer_new, + VISIBILITYMAP_VALID_BITS); + visibilitymap_clear_locked(reln, newblk, vmbuffer_new, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer_new), lsn); + } + if (BufferIsValid(vmbuffer_new)) + UnlockReleaseBuffer(vmbuffer_new); + } + if (has_vm_old) + { + Buffer vmbuffer_old = InvalidBuffer; + + Assert(old_cleared); + if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, &vmbuffer_old) == + BLK_NEEDS_REDO) + { + visibilitymap_clear_locked(reln, oldblk, vmbuffer_old, + VISIBILITYMAP_VALID_BITS); + PageSetLSN(BufferGetPage(vmbuffer_old), lsn); + } + if (BufferIsValid(vmbuffer_old)) + UnlockReleaseBuffer(vmbuffer_old); + } + if (!has_vm_old && !has_vm_new) + { + /* + * 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); } @@ -793,7 +899,7 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) /* Mark the page as a candidate for pruning */ PageSetPrunable(page, XLogRecGetXid(record)); - if (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) + if (old_cleared) PageClearAllVisible(page); PageSetLSN(page, lsn); @@ -819,21 +925,6 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) newaction = XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_NEW, &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) { @@ -930,7 +1021,7 @@ heap_xlog_update(XLogReaderState *record, bool hot_update) if (offnum == InvalidOffsetNumber) elog(PANIC, "failed to add tuple"); - if (xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) + if (new_cleared) PageClearAllVisible(page); freespace = PageGetHeapFreeSpace(page); /* needed to update FSM below */ @@ -1022,20 +1113,15 @@ heap_xlog_lock(XLogReaderState *record) */ if (xlrec->flags & XLH_LOCK_ALL_FROZEN_CLEARED) { - RelFileLocator rlocator; - Buffer vmbuffer = InvalidBuffer; BlockNumber block; - Relation reln; + RelFileLocator rlocator; XLogRecGetBlockTag(record, HEAP_LOCK_BLKREF_HEAP, &rlocator, NULL, &block); - reln = CreateFakeRelcacheEntry(rlocator); - - visibilitymap_pin(reln, block, &vmbuffer); - visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); - ReleaseBuffer(vmbuffer); - FreeFakeRelcacheEntry(reln); + heap_xlog_vm_clear(record, lsn, rlocator, + block, HEAP_LOCK_BLKREF_VM, + VISIBILITYMAP_ALL_FROZEN); } if (XLogReadBufferForRedo(record, HEAP_LOCK_BLKREF_HEAP, @@ -1100,20 +1186,15 @@ heap_xlog_lock_updated(XLogReaderState *record) */ if (xlrec->flags & XLH_LOCK_ALL_FROZEN_CLEARED) { - RelFileLocator rlocator; - Buffer vmbuffer = InvalidBuffer; BlockNumber block; - Relation reln; + RelFileLocator rlocator; XLogRecGetBlockTag(record, HEAP_LOCK_BLKREF_HEAP, &rlocator, NULL, &block); - reln = CreateFakeRelcacheEntry(rlocator); - - visibilitymap_pin(reln, block, &vmbuffer); - visibilitymap_clear(reln, block, vmbuffer, VISIBILITYMAP_ALL_FROZEN); - ReleaseBuffer(vmbuffer); - FreeFakeRelcacheEntry(reln); + heap_xlog_vm_clear(record, lsn, rlocator, + block, HEAP_LOCK_BLKREF_VM, + VISIBILITYMAP_ALL_FROZEN); } if (XLogReadBufferForRedo(record, HEAP_LOCK_BLKREF_HEAP, diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 1874a3fda37..f02778ba58f 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,8 +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..fcb701e1b98 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 55e3c7b0015..63062424dad 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -225,10 +225,21 @@ typedef struct xl_multi_insert_tuple * * HEAP_UPDATE_BLKREF_OLD: old page, if different. (no data, just a reference * to the block) + * + * HEAP_UPDATE_BLKREF_VM_NEW: VM page covering the new heap page. Registered + * when XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED is set and the new heap page's VM + * bit was actually cleared. Also covers the old heap page's VM bits when both + * heap pages map to the same VM page. + * + * HEAP_UPDATE_BLKREF_VM_OLD: VM page covering the old heap page. Only + * registered when XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED is set, the old heap + * page's VM bits are on a different VM page from the new heap page's, and the + * old heap page's VM bit was actually cleared. */ - -#define HEAP_UPDATE_BLKREF_NEW 0 -#define HEAP_UPDATE_BLKREF_OLD 1 +#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 { 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