Hi,
visibilitymap_set() arguably breaks a few of the coding rules for
modifying and WAL logging buffers set out in
src/backend/access/transam/README.
In 4 of visibilitymap_set()'s 5 non-recovery callers, we set
PD_ALL_VISIBLE and mark the heap buffer dirty outside of the critical
section in which we make the VM changes and emit WAL.
In at least two of its callers, before visibilitymap_set() is called,
MarkBufferDirty() is used when MarkBufferDirtyHint() would be
appropriate (when checksums/wal_log_hints are disabled) -- because we
are not making other changes to the heap page.
And in at least one of its callers (see lazy_scan_prune()), where
PD_ALL_VISIBLE is already set and we don't mark the buffer dirty and
checksums/wal_log_hints are enabled, visibilitymap_set() will still
set the heap page LSN -- even though we didn't set the buffer dirty.
It may be uncommon for the page to be set PD_ALL_VISIBLE and the VM
bit to be clear because of a crash or error after modifying the heap
page but before setting the VM. But it seems easy for us to be only
setting the all-frozen bit in the VM and thus not need to set
PD_ALL_VISIBLE or mark the heap buffer dirty. In that case, we'll
incorrectly set the page LSN without having marked the buffer dirty
(when wal_log_hints/checksums on).
Besides all of these issues, having these operations open-coded all
over the place is error-prone. It's easy to forget to always
PageSetAllVisible() before visibilitymap_set().
I've attached a patch to add a heap-specific wrapper for
visibilitymap_set() that attempts to handle all cases.
One thing I noticed is that log_heap_visible() will do
XLogRegisterBuffer() for the heap page -- even if we made no changes
to the heap page. It seems like we shouldn't do that. The most common
case would be when setting all-visible pages all-frozen when checksums
are not enabled. I don't actually know how we handle replay when we
are not sure which blocks might be registered, though.
Besides feeling like principled cleanup, this patch is a prerequisite
for a larger project I am working on [1] (targeting 19) to combine VM
updates in the same WAL record as the heap page changes and eliminate
xl_heap_visible.
- Melanie
[1]
https://www.postgresql.org/message-id/flat/CAAKRu_ZMw6Npd_qm2KM%2BFwQ3cMOMx1Dh3VMhp8-V7SOLxdK9-g%40mail.gmail.com
From 4a654c720fcea84a320d5e9378976d9bafd76dd2 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Tue, 24 Jun 2025 15:05:41 -0400
Subject: [PATCH v1] Introduce heap-specific wrapper for visibilitymap_set()
visibilitymap_set(), which sets bits in the visibility map corresponding
to the heap block of the table passed in, arguably breaks a few of the
coding rules for modifying and WAL logging buffers set out in
access/transam/README.
In several of the places where visibilitymap_set() is called, setting
the heap page PD_ALL_VISIBLE and marking the buffer dirty are done
outside of a critical section.
In some places before visibilitymap_set() is called, MarkBufferDirty()
is used when MarkBufferDirtyHint() would be appropriate.
And in some places where PD_ALL_VISIBLE may already be set and we don't
mark the buffer dirty, when checksums/wal_log_hints are enabled
visibilitymap_set() will still set the heap page LSN -- even though it
was correct not to set the buffer dirty.
Besides all of these issues, having these operations open-coded all over
the place is error-prone. This commit introduces a wrapper that does the
correct operations to the heap page itself and invokes
visibilitymap_set() to make changes to the VM page.
---
src/backend/access/heap/heapam.c | 92 ++++++++++++++++++++-----
src/backend/access/heap/heapam_xlog.c | 2 +-
src/backend/access/heap/vacuumlazy.c | 66 +++++-------------
src/backend/access/heap/visibilitymap.c | 58 ++++++----------
src/include/access/heapam.h | 3 +
src/include/access/visibilitymap.h | 2 +-
6 files changed, 117 insertions(+), 106 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0dcd6ee817e..112f946dab0 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2505,8 +2505,6 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
BufferGetBlockNumber(buffer),
vmbuffer, VISIBILITYMAP_VALID_BITS);
}
- else if (all_frozen_set)
- PageSetAllVisible(page);
/*
* XXX Should we set PageSetPrunable on this page ? See heap_insert()
@@ -2632,23 +2630,16 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
/*
* If we've frozen everything on the page, update the visibilitymap.
- * We're already holding pin on the vmbuffer.
+ * We're already holding pin on the vmbuffer. It's fine to use
+ * InvalidTransactionId as the cutoff_xid here - this is only used
+ * when HEAP_INSERT_FROZEN is specified, which intentionally violates
+ * visibility rules.
*/
if (all_frozen_set)
- {
- Assert(PageIsAllVisible(page));
- Assert(visibilitymap_pin_ok(BufferGetBlockNumber(buffer), vmbuffer));
-
- /*
- * It's fine to use InvalidTransactionId here - this is only used
- * when HEAP_INSERT_FROZEN is specified, which intentionally
- * violates visibility rules.
- */
- visibilitymap_set(relation, BufferGetBlockNumber(buffer), buffer,
- InvalidXLogRecPtr, vmbuffer,
- InvalidTransactionId,
- VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
- }
+ heap_page_set_vm_and_log(relation, BufferGetBlockNumber(buffer), buffer,
+ vmbuffer,
+ InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
UnlockReleaseBuffer(buffer);
ndone += nthispage;
@@ -7840,6 +7831,73 @@ heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)
return false;
}
+/*
+ * Make the heap and VM page changes needed to set a page all-visible.
+ * Do not call in recovery.
+ */
+uint8
+heap_page_set_vm_and_log(Relation rel, BlockNumber heap_blk, Buffer heap_buf,
+ Buffer vmbuf, TransactionId cutoff_xid,
+ uint8 vmflags)
+{
+ Page heap_page = BufferGetPage(heap_buf);
+ bool set_heap_lsn = false;
+ XLogRecPtr recptr = InvalidXLogRecPtr;
+ uint8 old_vmbits = 0;
+
+ Assert(BufferIsValid(heap_buf));
+
+ START_CRIT_SECTION();
+
+ /* Check that we have the right heap page pinned, if present */
+ if (BufferGetBlockNumber(heap_buf) != heap_blk)
+ elog(ERROR, "wrong heap buffer passed to heap_page_set_vm_and_log");
+
+ /*
+ * We must never end up with the VM bit set and the page-level
+ * PD_ALL_VISIBLE bit clear. If that were to occur, a subsequent page
+ * modification would fail to clear the VM bit. Though it is possible for
+ * the page-level bit to be set and the VM bit to be clear if checksums
+ * and wal_log_hints are not enabled.
+ */
+ if (!PageIsAllVisible(heap_page))
+ {
+ PageSetAllVisible(heap_page);
+
+ /*
+ * Buffer will usually be dirty from other changes, so it is worth the
+ * extra check
+ */
+ if (!BufferIsDirty(heap_buf))
+ {
+ if (XLogHintBitIsNeeded())
+ MarkBufferDirty(heap_buf);
+ else
+ MarkBufferDirtyHint(heap_buf, true);
+ }
+
+ set_heap_lsn = XLogHintBitIsNeeded();
+ }
+
+ old_vmbits = visibilitymap_set(rel, heap_blk, heap_buf,
+ &recptr, vmbuf, cutoff_xid, vmflags);
+
+ /*
+ * If we modified the heap page and data checksums are enabled (or
+ * wal_log_hints=on), we need to protect the heap page from being torn.
+ *
+ * If not, then we must *not* update the heap page's LSN. In this case,
+ * the FPI for the heap page was omitted from the WAL record inserted in
+ * the VM record, so it would be incorrect to update the heap page's LSN.
+ */
+ if (set_heap_lsn)
+ PageSetLSN(heap_page, recptr);
+
+ END_CRIT_SECTION();
+
+ return old_vmbits;
+}
+
/*
* heap_tuple_should_freeze
*
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index fa94e104f1c..d9a4d6af2f8 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -297,7 +297,7 @@ heap_xlog_visible(XLogReaderState *record)
reln = CreateFakeRelcacheEntry(rlocator);
visibilitymap_pin(reln, blkno, &vmbuffer);
- visibilitymap_set(reln, blkno, InvalidBuffer, lsn, vmbuffer,
+ visibilitymap_set(reln, blkno, InvalidBuffer, &lsn, vmbuffer,
xlrec->snapshotConflictHorizon, vmbits);
ReleaseBuffer(vmbuffer);
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af9..a9d0c570361 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1876,9 +1876,6 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
START_CRIT_SECTION();
- /* mark buffer dirty before writing a WAL record */
- MarkBufferDirty(buf);
-
/*
* It's possible that another backend has extended the heap,
* initialized the page, and then failed to WAL-log the page due
@@ -1890,14 +1887,15 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
*/
if (RelationNeedsWAL(vacrel->rel) &&
PageGetLSN(page) == InvalidXLogRecPtr)
+ {
+ MarkBufferDirty(buf);
log_newpage_buffer(buf, true);
+ }
- PageSetAllVisible(page);
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
- InvalidXLogRecPtr,
- vmbuffer, InvalidTransactionId,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN);
+ old_vmbits = heap_page_set_vm_and_log(vacrel->rel, blkno, buf,
+ vmbuffer, InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN);
END_CRIT_SECTION();
/*
@@ -2079,25 +2077,9 @@ lazy_scan_prune(LVRelState *vacrel,
flags |= VISIBILITYMAP_ALL_FROZEN;
}
- /*
- * It should never be the case that the visibility map page is set
- * while the page-level bit is clear, but the reverse is allowed (if
- * checksums are not enabled). Regardless, set both bits so that we
- * get back in sync.
- *
- * NB: If the heap page is all-visible but the VM bit is not set, we
- * don't need to dirty the heap page. However, if checksums are
- * enabled, we do need to make sure that the heap page is dirtied
- * before passing it to visibilitymap_set(), because it may be logged.
- * Given that this situation should only happen in rare cases after a
- * crash, it is not worth optimizing.
- */
- PageSetAllVisible(page);
- MarkBufferDirty(buf);
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
- InvalidXLogRecPtr,
- vmbuffer, presult.vm_conflict_horizon,
- flags);
+ old_vmbits = heap_page_set_vm_and_log(vacrel->rel, blkno, buf,
+ vmbuffer, presult.vm_conflict_horizon,
+ flags);
/*
* If the page wasn't already set all-visible and/or all-frozen in the
@@ -2169,17 +2151,6 @@ lazy_scan_prune(LVRelState *vacrel,
{
uint8 old_vmbits;
- /*
- * Avoid relying on all_visible_according_to_vm as a proxy for the
- * page-level PD_ALL_VISIBLE bit being set, since it might have become
- * stale -- even when all_visible is set
- */
- if (!PageIsAllVisible(page))
- {
- PageSetAllVisible(page);
- MarkBufferDirty(buf);
- }
-
/*
* Set the page all-frozen (and all-visible) in the VM.
*
@@ -2188,11 +2159,10 @@ lazy_scan_prune(LVRelState *vacrel,
* was logged when the page's tuples were frozen.
*/
Assert(!TransactionIdIsValid(presult.vm_conflict_horizon));
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
- InvalidXLogRecPtr,
- vmbuffer, InvalidTransactionId,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN);
+ old_vmbits = heap_page_set_vm_and_log(vacrel->rel, blkno, buf,
+ vmbuffer, InvalidTransactionId,
+ VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN);
/*
* The page was likely already set all-visible in the VM. However,
@@ -2924,11 +2894,9 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
flags |= VISIBILITYMAP_ALL_FROZEN;
}
- PageSetAllVisible(page);
- old_vmbits = visibilitymap_set(vacrel->rel, blkno, buffer,
- InvalidXLogRecPtr,
- vmbuffer, visibility_cutoff_xid,
- flags);
+ old_vmbits = heap_page_set_vm_and_log(vacrel->rel, blkno, buffer,
+ vmbuffer, visibility_cutoff_xid,
+ flags);
/*
* If the page wasn't already set all-visible and/or all-frozen in the
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 745a04ef26e..c57632168c7 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -222,29 +222,31 @@ visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf)
/*
* visibilitymap_set - set bit(s) on a previously pinned page
*
- * recptr is the LSN of the XLOG record we're replaying, if we're in recovery,
- * or InvalidXLogRecPtr in normal running. The VM page LSN is advanced to the
- * one provided; in normal running, we generate a new XLOG record and set the
- * page LSN to that value (though the heap page's LSN may *not* be updated;
- * see below). cutoff_xid is the largest xmin on the page being marked
- * all-visible; it is needed for Hot Standby, and can be InvalidTransactionId
- * if the page contains no tuples. It can also be set to InvalidTransactionId
- * when a page that is already all-visible is being marked all-frozen.
- *
* Caller is expected to set the heap page's PD_ALL_VISIBLE bit before calling
- * this function. Except in recovery, caller should also pass the heap
- * buffer. When checksums are enabled and we're not in recovery, we must add
- * the heap buffer to the WAL chain to protect it from being torn.
+ * this function. Except in recovery, caller should also pass the heap buffer.
+ * When checksums are enabled and we're not in recovery, we must add the heap
+ * buffer to the WAL chain to protect it from being torn.
*
* 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 the state of the page's VM bits before setting flags.
+ * cutoff_xid is the largest xmin on the page being marked all-visible; it is
+ * needed for Hot Standby, and can be InvalidTransactionId if the page
+ * contains no tuples. It can also be set to InvalidTransactionId when a page
+ * that is already all-visible is being marked all-frozen.
+ *
+ * If we're in recovery, recptr points to the LSN of the XLOG record we're
+ * replaying and the VM page LSN is advanced to this LSN. During normal
+ * running, we'll generate a new XLOG record for the changes to the VM and set
+ * the VM page LSN. We will return this LSN in recptr, and the caller may use
+ * this to set the heap page LSN.
+ *
+ * Returns the state of the page's VM bits before setting flags and sets.
*/
uint8
visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
- XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid,
+ XLogRecPtr *recptr, Buffer vmBuf, TransactionId cutoff_xid,
uint8 flags)
{
BlockNumber mapBlock = HEAPBLK_TO_MAPBLOCK(heapBlk);
@@ -258,17 +260,13 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
elog(DEBUG1, "vm_set %s %d", RelationGetRelationName(rel), heapBlk);
#endif
- Assert(InRecovery || XLogRecPtrIsInvalid(recptr));
+ Assert(InRecovery || XLogRecPtrIsInvalid(*recptr));
Assert(InRecovery || PageIsAllVisible((Page) BufferGetPage(heapBuf)));
Assert((flags & VISIBILITYMAP_VALID_BITS) == flags);
/* Must never set all_frozen bit without also setting all_visible bit */
Assert(flags != VISIBILITYMAP_ALL_FROZEN);
- /* Check that we have the right heap page pinned, if present */
- if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk)
- elog(ERROR, "wrong heap buffer passed to visibilitymap_set");
-
/* Check that we have the right VM page pinned */
if (!BufferIsValid(vmBuf) || BufferGetBlockNumber(vmBuf) != mapBlock)
elog(ERROR, "wrong VM buffer passed to visibilitymap_set");
@@ -287,28 +285,12 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf,
if (RelationNeedsWAL(rel))
{
- if (XLogRecPtrIsInvalid(recptr))
+ if (XLogRecPtrIsInvalid(*recptr))
{
Assert(!InRecovery);
- recptr = log_heap_visible(rel, heapBuf, vmBuf, cutoff_xid, flags);
-
- /*
- * If data checksums are enabled (or wal_log_hints=on), we
- * need to protect the heap page from being torn.
- *
- * If not, then we must *not* update the heap page's LSN. In
- * this case, the FPI for the heap page was omitted from the
- * WAL record inserted above, so it would be incorrect to
- * update the heap page's LSN.
- */
- if (XLogHintBitIsNeeded())
- {
- Page heapPage = BufferGetPage(heapBuf);
-
- PageSetLSN(heapPage, recptr);
- }
+ *recptr = log_heap_visible(rel, heapBuf, vmBuf, cutoff_xid, flags);
}
- PageSetLSN(page, recptr);
+ PageSetLSN(page, *recptr);
}
END_CRIT_SECTION();
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 3a9424c19c9..9375296062f 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -360,6 +360,9 @@ extern bool heap_tuple_should_freeze(HeapTupleHeader tuple,
TransactionId *NoFreezePageRelfrozenXid,
MultiXactId *NoFreezePageRelminMxid);
extern bool heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple);
+extern uint8 heap_page_set_vm_and_log(Relation rel, BlockNumber heap_blk, Buffer heap_buf,
+ Buffer vmbuf, TransactionId cutoff_xid,
+ uint8 vmflags);
extern void simple_heap_insert(Relation relation, HeapTuple tup);
extern void simple_heap_delete(Relation relation, ItemPointer tid);
diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h
index be21c6dd1a3..4c7472e0b51 100644
--- a/src/include/access/visibilitymap.h
+++ b/src/include/access/visibilitymap.h
@@ -33,7 +33,7 @@ extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk,
extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf);
extern uint8 visibilitymap_set(Relation rel,
BlockNumber heapBlk, Buffer heapBuf,
- XLogRecPtr recptr,
+ XLogRecPtr *recptr,
Buffer vmBuf,
TransactionId cutoff_xid,
uint8 flags);
--
2.34.1
