On Mon, Jun 29, 2026 at 10:43 AM Robert Haas <[email protected]> wrote:
>
> I have reviewed 0001 and 0002 and in general I think this looks good.
> There is possibly room for some concern about performance regressions
> with 0002 since, as you point out in some of the added comments, we
> will now keep the VM buffer locked for longer. It might be worth
> benchmarking that to see how bad it looks, but I don't know that we
> really have any choice. Correctness bugs are bad.
Thanks for the review!
As for performance regressions, I concocted what I felt was the worst
case (repro script pasted at the bottom of the email) which is a table
with all the pages covered by a single VM page and 32 clients each
trying to clear VM bits from different heap pages. It's not a perfect
benchmark because the only way to get the pages to keep being set
all-visible between updates was to run vacuum separately in a loop and
that wouldn't produce the same pattern of vacuums from run-to-run. I
didn't see any performance degradation for this with mostly default
settings. If I turn synchronous_commit off, I see a 10-15% TPS
performance degradation. This seems due to the extra time holding the
VM buffer lock. If you sample wait events, you see a big increase in
BufferContent lock wait events with the patch. As you mentioned, I
don't think there is anything we can do about it, though, and it
doesn't seem to be the bottleneck in the common configuration.
> One problem is that I think the newly-registered VM blocks will get
> fed to heap_mask() if wal_consistency_checking is turned on, but it's
> really only meant to work with heap pages.
visibilitymap_set() registers VM blocks already and those go through
wal consistency checking like all other HEAP2 records. It isn't great,
but it is basically harmless because heap_mask() loop conditions will
cause it to do nothing anyway. We could perhaps do something better
with non-heap pages in wal consistency checking, but we certainly
shouldn't do that on back branches.
> 0001. I personally don't find this to be an improvement. Even it is, I
> feel like a back-patched commit is not a good place to invent a new
> convention for identifying block reference indexes. However, it's
> ultimately a matter of style, so if I lose this particular argument,
> c'est la vie. I just feel like if we introduce this for every WAL
> record, it's going to be a lot of new #defines that are going to be
> long and clunky, and if we don't, then we have a weird mix of styles.
I agree with you that this is less than ideal on back branches and not
great that we don't have some way of enforcing people continuing to do
it even when done on master (leading to inconsistent usage).
However, I find registering VM buffers untenably confusing without
naming the various block IDs registered. Especially because we don't
replay them in the same order they were registered in. And for
heap_update(), we register up to 4 blocks (after the fix) and the
order is significant because we may be using one VM block for two heap
blocks. I had at some point suggested only macro-izing the update
block refs, but another reviewer preferred aliasing all of them, as it
isn't immediately clear what that block ID is with even two blocks
registered.
> 0002. In heap_insert, the local variable called 'all_visible_cleared'
> seems like it needs a bit of work. You'd expect such a variable to be
> set at the point where we clear the all-visible bit, but it's actually
> set before that, when we lock the buffer. Later, you write in the
> comments "We locked vmbuffer if all_visible_cleared was true
> regardless of whether or not we ended up modifying it," which
> similarly implies that the variable is now misnamed. However,
> PageClearAllVisible() is called if and only if all_visible_cleared is
> true, so that comment doesn't seem to be entirely accurate. My first
> instinct is to rename the variable to clear_all_visible and delete the
> comment, but there might be something better.
I had originally wanted to keep the name all_visible_cleared since it
was an existing variable, but you're right that that is now confusing.
I've changed it as you suggested. I think the comment is clarifying
because we don't have a separate unlock_vmbuffer variable here.
> In heap_update, the comment /* clear PD_ALL_VISIBLE flags, reset all
> visibilitymap bits */ has been lost, and the code has been reordered
> slightly in that area. This is fine, but keeping the existing comment
> and/or code ordering could be worth considering.
>
> In heap_lock_tuple, I think it might be a good idea to set
> unlock_vmbuffer = false when we release the lock, and then just after
> the out_unlocked: label, Assert(!unlock_vmbuffer). That way, if
> someone adds an improvident goto in a future patch, it has a chance of
> tripping an Assert.
I've done these two things.
> I think the comment for heap_xlog_vm_clear could be better: instead of
> saying that it "handles" something, say what it actually does.
> Likewise "use it for redo" inside the function is a little imprecise.
> If I'm understanding the purpose of this machinery correctly, the
> point here is that any FPIs included for the VM block references need
> to be applied, and otherwise (if there's no FPI or the block reference
> is not included at all) we need to just clear the relevant bit. That
> seems correct, but a little more work on the comments would make it
> clearer to future readers, I feel.
I think I've improved this.
> In heap_xlog_multi_insert, you delete the comment "The visibility map
> may need to be fixed even if the heap page is already up-to-date," and
> replace it with a completely new comment that seems to be addresing
> someone different concerns, but the identical comments in other
> functions in the file are retained. Consider adding to the comment
> instead of replacing it. Also, I am not sure I understand the point of
> the new comment. Perhaps try to make it clearer.
I didn't understand the existing comment or even what it was trying to
say, which is why I deleted it. I've restored it now and tried to
clarify my additional point.
> I find the phrasing of the comment addition at the top of
> visibilitymap_clear to be a little harder to understand than
> necessary. Maybe: "Most callers should use visibilitymap_clear_locked
> rather than this function. 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. However, we retain this function for
> backward compatibility, and for a few callers that don't have this
> requirement."
I adopted this phrasing.
> The redo routines ignore the return value of
> visibilitymap_clear_locked, which means they set the page LSN even if
> it isn't modified. Apparently there is other precedent for that
> treatment, but maybe it's best avoided anyway.
Done.
> I think that the commit messages could use some polishing. For 0001,
> if we keep it, I'd suggest that the subject line should use language
> like "introduce macros for" instead of "alias" because nobody's going
> to know what "alias" means without reading the commit itself. For
> 0002, I'd suggest that the subject line should be something like "Fix
> heap WAL records to register VM buffers when clearing VM bits". I
> think your goals here should be (1) to maximize the chance that
> somebody can understand what the commit does if they just read the
> one-line summary, or at most the whole commit message, and (2) to
> minimize the amount of work it will take to turn your commit message
> into a release note entry. I'm not saying your existing commit
> messages are bad, but they read a bit as if they were written to
> summarize work that the reader has already seen.
I've tried to clarify my commit messages.
Your original review didn't mention 0003/0004 (the test commits). I
was hoping to get a review especially on 0003.
For 0004, I feel relatively confident in it, but I am wondering if the
extra validation I added to verify that the updates happen either on a
single page or across pages is overkill or not. It will make sure the
test continues to cover this case, but, it is quite a few extra lines
of test code.
For 0003, I am much less confident. I've never written test
infrastructure in Perl before. I used AI to help me write it, so I
could really use review from someone experienced with adding TAP test
utility functions. A few thoughts on it/questions:
_run_pg_combinebackup_with_debug_log's error handling is basically
copied from system_or_bail -- is that too duplicative? Also, I'm not
sure if the END code block is done correctly or not.
I think the overall behavior seems okay -- make a temp directory for
the logs from each pg_combinebackup run. Store the paths to those
directories in an array. If pg_combinebackup invocation itself fails,
print the verbose logs right away. Otherwise, at the end of the whole
test run, if any tests failed, print all the verbose logs. If no tests
failed, unlink the test files. It doesn't actually remove the temp
dirs though -- I think those are cleaned up in the general test
cleanup.
One annoying thing is we save the logs at the time of the
pg_combinebackup run, so we don't know if the assertions following it
pass or succeed. That means if any of the assertions fail we will
print all logs. This may be unavoidable, as we wouldn't even be sure
which logs to attribute to which assertions.
Also combinebackup_debug_logs_printed hash set is a little
over-complicated -- it was added because we could print the same log
output twice if pg_combinebackup itself failed. Maybe there is a less
complicated way to handle this? I don't think the END block is
guaranteed to run depending on how pg_combinebackup fails, so I don't
know that we can be sure the logs print without doing it right when
pg_combinebackup fails.
- Melanie
repro (set sync commit off to see the regression):
psql -c "ALTER SYSTEM SET shared_buffers = '2GB';" \
-c "ALTER SYSTEM SET max_wal_size = '150 GB';" \
-c "ALTER SYSTEM SET maintenance_work_mem = '1GB';" \
-c "alter system set synchronous_commit = on"
pg_ctl -l $PGLOG restart
psql -f- <<EOF
DROP TABLE IF EXISTS foo;
CREATE TABLE foo (
cid int not null, -- which client owns the row
k int not null, -- key within that client's partition
filler text not null
) WITH (autovacuum_enabled = off);
ALTER TABLE foo ALTER COLUMN filler SET STORAGE PLAIN;
INSERT INTO foo
SELECT (g-1) % 16, (g-1) / 16, repeat('x', 4200)
FROM generate_series(1, 32000) g;
CREATE UNIQUE INDEX foo_cid_k ON foo (cid, k); -- avoid seq scans
VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) foo;
EOF
while :; do psql -c "VACUUM (FREEZE) foo;"; sleep 0.05; done &
pgbench -n -c 32 -j 32 -t 10000 -D rpc=2000 \
-f- <<EOF
\set k random(0, :rpc - 1)
UPDATE foo SET filler = filler WHERE cid = :client_id AND k = :k;
EOF
kill %1
From 8bacdc59c088c19b60c74fc3d5dcb815fe7ff14b Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Mon, 4 May 2026 10:41:13 -0400
Subject: [PATCH v4 1/4] Introduce macros for WAL block reference IDs of some
heap record types
When registering a buffer with the WAL machinery, the caller assigns it a
block reference ID, and replay must read each block back by that same ID.
Today these IDs are bare integers assigned by convention (0, 1, 2, ...),
which is easy to follow when a record registers a single block, or when the
blocks are handled during replay in their registration order.
An upcoming bug fix registers up to two visibility map blocks in addition
to the heap block(s) when clearing the VM, and these are not handled during
replay in a straightforward 1:1, in-registration-order fashion. Relying on
bare integers for the block IDs in that case is error-prone.
Introduce macros naming the block reference IDs for the heap record types
that the upcoming commit extends to register visibility map blocks, so the
registration and replay sites refer to the same block by a meaningful name.
Author: Melanie Plageman <[email protected]>
Reviewed-by: Robert Haas <[email protected]>
Discussion: https://postgr.es/m/66mqpfyti3qhfttcsv6r2lbvqqd32rrmpn6i47ovrsnvguts46%40gou54xc>
Backpatch through: 17
---
src/backend/access/heap/heapam.c | 43 +++++++++++---------
src/backend/access/heap/heapam_xlog.c | 56 +++++++++++++++++----------
src/include/access/heapam_xlog.h | 28 ++++++++++----
3 files changed, 82 insertions(+), 45 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 6203e3d7f8d..96c25197788 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2218,10 +2218,12 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
* write the whole page to the xlog, we don't need to store
* xl_heap_header in the xlog.
*/
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags);
- XLogRegisterBufData(0, &xlhdr, SizeOfHeapHeader);
+ XLogRegisterBuffer(HEAP_INSERT_BLKREF_HEAP, buffer,
+ REGBUF_STANDARD | bufflags);
+ XLogRegisterBufData(HEAP_INSERT_BLKREF_HEAP, &xlhdr,
+ SizeOfHeapHeader);
/* PG73FORMAT: write bitmap [+ padding] [+ oid] + data */
- XLogRegisterBufData(0,
+ XLogRegisterBufData(HEAP_INSERT_BLKREF_HEAP,
(char *) heaptup->t_data + SizeofHeapTupleHeader,
heaptup->t_len - SizeofHeapTupleHeader);
@@ -2619,9 +2621,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
XLogBeginInsert();
XLogRegisterData(xlrec, tupledata - scratch.data);
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD | bufflags);
+ XLogRegisterBuffer(HEAP_MULTI_INSERT_BLKREF_HEAP, buffer,
+ REGBUF_STANDARD | bufflags);
- XLogRegisterBufData(0, tupledata, totaldatalen);
+ XLogRegisterBufData(HEAP_MULTI_INSERT_BLKREF_HEAP, tupledata,
+ totaldatalen);
/* filtering by origin on a row level is much more efficient */
XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
@@ -3112,7 +3116,7 @@ l1:
XLogBeginInsert();
XLogRegisterData(&xlrec, SizeOfHeapDelete);
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+ XLogRegisterBuffer(HEAP_DELETE_BLKREF_HEAP, buffer, REGBUF_STANDARD);
/*
* Log replica identity of the deleted tuple if there is one
@@ -3883,7 +3887,7 @@ l2:
XLogRecPtr recptr;
XLogBeginInsert();
- XLogRegisterBuffer(0, buffer, REGBUF_STANDARD);
+ XLogRegisterBuffer(HEAP_LOCK_BLKREF_HEAP, buffer, REGBUF_STANDARD);
xlrec.offnum = ItemPointerGetOffsetNumber(&oldtup.t_self);
xlrec.xmax = xmax_lock_old_tuple;
@@ -5219,7 +5223,7 @@ failed:
XLogRecPtr recptr;
XLogBeginInsert();
- XLogRegisterBuffer(0, *buffer, REGBUF_STANDARD);
+ XLogRegisterBuffer(HEAP_LOCK_BLKREF_HEAP, *buffer, REGBUF_STANDARD);
xlrec.offnum = ItemPointerGetOffsetNumber(&tuple->t_self);
xlrec.xmax = xid;
@@ -5971,7 +5975,7 @@ l4:
Page page = BufferGetPage(buf);
XLogBeginInsert();
- XLogRegisterBuffer(0, buf, REGBUF_STANDARD);
+ XLogRegisterBuffer(HEAP_LOCK_BLKREF_HEAP, buf, REGBUF_STANDARD);
xlrec.offnum = ItemPointerGetOffsetNumber(&mytup.t_self);
xlrec.xmax = new_xmax;
@@ -8972,9 +8976,9 @@ log_heap_update(Relation reln, Buffer oldbuf,
if (need_tuple_data)
bufflags |= REGBUF_KEEP_DATA;
- XLogRegisterBuffer(0, newbuf, bufflags);
+ XLogRegisterBuffer(HEAP_UPDATE_BLKREF_HEAP_NEW, newbuf, bufflags);
if (oldbuf != newbuf)
- XLogRegisterBuffer(1, oldbuf, REGBUF_STANDARD);
+ XLogRegisterBuffer(HEAP_UPDATE_BLKREF_HEAP_OLD, oldbuf, REGBUF_STANDARD);
XLogRegisterData(&xlrec, SizeOfHeapUpdate);
@@ -8987,15 +8991,18 @@ log_heap_update(Relation reln, Buffer oldbuf,
{
prefix_suffix[0] = prefixlen;
prefix_suffix[1] = suffixlen;
- XLogRegisterBufData(0, &prefix_suffix, sizeof(uint16) * 2);
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW, &prefix_suffix,
+ sizeof(uint16) * 2);
}
else if (prefixlen > 0)
{
- XLogRegisterBufData(0, &prefixlen, sizeof(uint16));
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW, &prefixlen,
+ sizeof(uint16));
}
else
{
- XLogRegisterBufData(0, &suffixlen, sizeof(uint16));
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW, &suffixlen,
+ sizeof(uint16));
}
}
@@ -9009,10 +9016,10 @@ log_heap_update(Relation reln, Buffer oldbuf,
*
* The 'data' doesn't include the common prefix or suffix.
*/
- XLogRegisterBufData(0, &xlhdr, SizeOfHeapHeader);
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW, &xlhdr, SizeOfHeapHeader);
if (prefixlen == 0)
{
- XLogRegisterBufData(0,
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW,
(char *) newtup->t_data + SizeofHeapTupleHeader,
newtup->t_len - SizeofHeapTupleHeader - suffixlen);
}
@@ -9025,13 +9032,13 @@ log_heap_update(Relation reln, Buffer oldbuf,
/* bitmap [+ padding] [+ oid] */
if (newtup->t_data->t_hoff - SizeofHeapTupleHeader > 0)
{
- XLogRegisterBufData(0,
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW,
(char *) newtup->t_data + SizeofHeapTupleHeader,
newtup->t_data->t_hoff - SizeofHeapTupleHeader);
}
/* data after common prefix */
- XLogRegisterBufData(0,
+ XLogRegisterBufData(HEAP_UPDATE_BLKREF_HEAP_NEW,
(char *) newtup->t_data + newtup->t_data->t_hoff + prefixlen,
newtup->t_len - newtup->t_data->t_hoff - prefixlen - suffixlen);
}
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index eb4bd3d6ae3..87b5c44fd46 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -350,7 +350,8 @@ heap_xlog_delete(XLogReaderState *record)
RelFileLocator target_locator;
ItemPointerData target_tid;
- XLogRecGetBlockTag(record, 0, &target_locator, NULL, &blkno);
+ XLogRecGetBlockTag(record, HEAP_DELETE_BLKREF_HEAP, &target_locator, NULL,
+ &blkno);
ItemPointerSetBlockNumber(&target_tid, blkno);
ItemPointerSetOffsetNumber(&target_tid, xlrec->offnum);
@@ -369,7 +370,8 @@ heap_xlog_delete(XLogReaderState *record)
FreeFakeRelcacheEntry(reln);
}
- if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
+ if (XLogReadBufferForRedo(record, HEAP_DELETE_BLKREF_HEAP,
+ &buffer) == BLK_NEEDS_REDO)
{
page = BufferGetPage(buffer);
@@ -434,7 +436,8 @@ heap_xlog_insert(XLogReaderState *record)
ItemPointerData target_tid;
XLogRedoAction action;
- XLogRecGetBlockTag(record, 0, &target_locator, NULL, &blkno);
+ XLogRecGetBlockTag(record, HEAP_INSERT_BLKREF_HEAP, &target_locator, NULL,
+ &blkno);
ItemPointerSetBlockNumber(&target_tid, blkno);
ItemPointerSetOffsetNumber(&target_tid, xlrec->offnum);
@@ -462,13 +465,14 @@ heap_xlog_insert(XLogReaderState *record)
*/
if (XLogRecGetInfo(record) & XLOG_HEAP_INIT_PAGE)
{
- buffer = XLogInitBufferForRedo(record, 0);
+ buffer = XLogInitBufferForRedo(record, HEAP_INSERT_BLKREF_HEAP);
page = BufferGetPage(buffer);
PageInit(page, BufferGetPageSize(buffer), 0);
action = BLK_NEEDS_REDO;
}
else
- action = XLogReadBufferForRedo(record, 0, &buffer);
+ action = XLogReadBufferForRedo(record, HEAP_INSERT_BLKREF_HEAP,
+ &buffer);
if (action == BLK_NEEDS_REDO)
{
Size datalen;
@@ -479,7 +483,7 @@ heap_xlog_insert(XLogReaderState *record)
if (PageGetMaxOffsetNumber(page) + 1 < xlrec->offnum)
elog(PANIC, "invalid max offset number");
- data = XLogRecGetBlockData(record, 0, &datalen);
+ data = XLogRecGetBlockData(record, HEAP_INSERT_BLKREF_HEAP, &datalen);
newlen = datalen - SizeOfHeapHeader;
Assert(datalen > SizeOfHeapHeader && newlen <= MaxHeapTupleSize);
@@ -559,7 +563,8 @@ heap_xlog_multi_insert(XLogReaderState *record)
*/
xlrec = (xl_heap_multi_insert *) XLogRecGetData(record);
- XLogRecGetBlockTag(record, 0, &rlocator, NULL, &blkno);
+ XLogRecGetBlockTag(record, HEAP_MULTI_INSERT_BLKREF_HEAP, &rlocator, NULL,
+ &blkno);
/* check that the mutually exclusive flags are not both set */
Assert(!((xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED) &&
@@ -582,13 +587,14 @@ heap_xlog_multi_insert(XLogReaderState *record)
if (isinit)
{
- buffer = XLogInitBufferForRedo(record, 0);
+ buffer = XLogInitBufferForRedo(record, HEAP_MULTI_INSERT_BLKREF_HEAP);
page = BufferGetPage(buffer);
PageInit(page, BufferGetPageSize(buffer), 0);
action = BLK_NEEDS_REDO;
}
else
- action = XLogReadBufferForRedo(record, 0, &buffer);
+ action = XLogReadBufferForRedo(record, HEAP_MULTI_INSERT_BLKREF_HEAP,
+ &buffer);
if (action == BLK_NEEDS_REDO)
{
char *tupdata;
@@ -596,7 +602,8 @@ heap_xlog_multi_insert(XLogReaderState *record)
Size len;
/* Tuples are stored as block data */
- tupdata = XLogRecGetBlockData(record, 0, &len);
+ tupdata = XLogRecGetBlockData(record, HEAP_MULTI_INSERT_BLKREF_HEAP,
+ &len);
endptr = tupdata + len;
page = (Page) BufferGetPage(buffer);
@@ -713,8 +720,10 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
oldtup.t_data = NULL;
oldtup.t_len = 0;
- XLogRecGetBlockTag(record, 0, &rlocator, NULL, &newblk);
- if (XLogRecGetBlockTagExtended(record, 1, NULL, NULL, &oldblk, NULL))
+ XLogRecGetBlockTag(record, HEAP_UPDATE_BLKREF_HEAP_NEW, &rlocator, NULL,
+ &newblk);
+ if (XLogRecGetBlockTagExtended(record, HEAP_UPDATE_BLKREF_HEAP_OLD, NULL, NULL,
+ &oldblk, NULL))
{
/* HOT updates are never done across pages */
Assert(!hot_update);
@@ -750,7 +759,8 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
*/
/* Deal with old tuple version */
- oldaction = XLogReadBufferForRedo(record, (oldblk == newblk) ? 0 : 1,
+ oldaction = XLogReadBufferForRedo(record, (oldblk == newblk) ?
+ HEAP_UPDATE_BLKREF_HEAP_NEW : HEAP_UPDATE_BLKREF_HEAP_OLD,
&obuffer);
if (oldaction == BLK_NEEDS_REDO)
{
@@ -800,13 +810,14 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
}
else if (XLogRecGetInfo(record) & XLOG_HEAP_INIT_PAGE)
{
- nbuffer = XLogInitBufferForRedo(record, 0);
+ nbuffer = XLogInitBufferForRedo(record, HEAP_UPDATE_BLKREF_HEAP_NEW);
page = (Page) BufferGetPage(nbuffer);
PageInit(page, BufferGetPageSize(nbuffer), 0);
newaction = BLK_NEEDS_REDO;
}
else
- newaction = XLogReadBufferForRedo(record, 0, &nbuffer);
+ newaction = XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_HEAP_NEW,
+ &nbuffer);
/*
* The visibility map may need to be fixed even if the heap page is
@@ -831,7 +842,8 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
Size datalen;
Size tuplen;
- recdata = XLogRecGetBlockData(record, 0, &datalen);
+ recdata = XLogRecGetBlockData(record, HEAP_UPDATE_BLKREF_HEAP_NEW,
+ &datalen);
recdata_end = recdata + datalen;
page = BufferGetPage(nbuffer);
@@ -1015,7 +1027,8 @@ heap_xlog_lock(XLogReaderState *record)
BlockNumber block;
Relation reln;
- XLogRecGetBlockTag(record, 0, &rlocator, NULL, &block);
+ XLogRecGetBlockTag(record, HEAP_LOCK_BLKREF_HEAP, &rlocator, NULL,
+ &block);
reln = CreateFakeRelcacheEntry(rlocator);
visibilitymap_pin(reln, block, &vmbuffer);
@@ -1025,7 +1038,8 @@ heap_xlog_lock(XLogReaderState *record)
FreeFakeRelcacheEntry(reln);
}
- if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
+ if (XLogReadBufferForRedo(record, HEAP_LOCK_BLKREF_HEAP,
+ &buffer) == BLK_NEEDS_REDO)
{
page = (Page) BufferGetPage(buffer);
@@ -1091,7 +1105,8 @@ heap_xlog_lock_updated(XLogReaderState *record)
BlockNumber block;
Relation reln;
- XLogRecGetBlockTag(record, 0, &rlocator, NULL, &block);
+ XLogRecGetBlockTag(record, HEAP_LOCK_BLKREF_HEAP, &rlocator, NULL,
+ &block);
reln = CreateFakeRelcacheEntry(rlocator);
visibilitymap_pin(reln, block, &vmbuffer);
@@ -1101,7 +1116,8 @@ heap_xlog_lock_updated(XLogReaderState *record)
FreeFakeRelcacheEntry(reln);
}
- if (XLogReadBufferForRedo(record, 0, &buffer) == BLK_NEEDS_REDO)
+ if (XLogReadBufferForRedo(record, HEAP_LOCK_BLKREF_HEAP,
+ &buffer) == BLK_NEEDS_REDO)
{
page = BufferGetPage(buffer);
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 277df6b3cf0..92744c53899 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -110,6 +110,8 @@
(XLH_DELETE_CONTAINS_OLD_TUPLE | XLH_DELETE_CONTAINS_OLD_KEY)
/* This is what we need to know about delete */
+#define HEAP_DELETE_BLKREF_HEAP 0
+
typedef struct xl_heap_delete
{
TransactionId xmax; /* xmax of the deleted tuple */
@@ -157,12 +159,14 @@ typedef struct xl_heap_header
#define SizeOfHeapHeader (offsetof(xl_heap_header, t_hoff) + sizeof(uint8))
/* This is what we need to know about insert */
+#define HEAP_INSERT_BLKREF_HEAP 0
+
typedef struct xl_heap_insert
{
OffsetNumber offnum; /* inserted tuple's offset */
uint8 flags;
- /* xl_heap_header & TUPLE DATA in backup block 0 */
+ /* xl_heap_header & TUPLE DATA in HEAP_INSERT_BLKREF_HEAP */
} xl_heap_insert;
#define SizeOfHeapInsert (offsetof(xl_heap_insert, flags) + sizeof(uint8))
@@ -173,10 +177,13 @@ typedef struct xl_heap_insert
* The main data of the record consists of this xl_heap_multi_insert header.
* 'offsets' array is omitted if the whole page is reinitialized
* (XLOG_HEAP_INIT_PAGE).
- *
- * In block 0's data portion, there is an xl_multi_insert_tuple struct,
- * followed by the tuple data for each tuple. There is padding to align
- * each xl_multi_insert_tuple struct.
+ */
+#define HEAP_MULTI_INSERT_BLKREF_HEAP 0
+
+/*
+ * In HEAP_MULTI_INSERT_BLKREF_HEAP's data portion, there is an
+ * xl_multi_insert_tuple struct, followed by the tuple data for each tuple.
+ * There is padding to align each xl_multi_insert_tuple struct.
*/
typedef struct xl_heap_multi_insert
{
@@ -201,7 +208,7 @@ typedef struct xl_multi_insert_tuple
/*
* This is what we need to know about update|hot_update
*
- * Backup blk 0: new page
+ * HEAP_UPDATE_BLKREF_HEAP_NEW: new page
*
* If XLH_UPDATE_PREFIX_FROM_OLD or XLH_UPDATE_SUFFIX_FROM_OLD flags are set,
* the prefix and/or suffix come first, as one or two uint16s.
@@ -213,8 +220,13 @@ typedef struct xl_multi_insert_tuple
* If XLH_UPDATE_CONTAINS_NEW_TUPLE flag is given, the tuple data is
* included even if a full-page image was taken.
*
- * Backup blk 1: old page, if different. (no data, just a reference to the blk)
+ * HEAP_UPDATE_BLKREF_HEAP_OLD: old page, if different. (no data, just a reference
+ * to the block)
*/
+
+#define HEAP_UPDATE_BLKREF_HEAP_NEW 0
+#define HEAP_UPDATE_BLKREF_HEAP_OLD 1
+
typedef struct xl_heap_update
{
TransactionId old_xmax; /* xmax of the old tuple */
@@ -393,6 +405,8 @@ typedef struct xlhp_prune_items
#define XLH_LOCK_ALL_FROZEN_CLEARED 0x01
/* This is what we need to know about lock */
+#define HEAP_LOCK_BLKREF_HEAP 0
+
typedef struct xl_heap_lock
{
TransactionId xmax; /* might be a MultiXactId */
--
2.47.3
From 8ddc25481951f9115fecd6c8880f3bf14657e5eb Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Wed, 29 Apr 2026 12:53:49 -0400
Subject: [PATCH v4 2/4] Fix VM clear WAL logging by registering VM blocks
Heap WAL records that clear bits on the visibility map (like inserts and
deletes) did not register the visibility map blocks they modified.
Because the WAL summarizer only records registered blocks, an
incremental backup taken over such operations would omit the changed VM
pages. On restore, the VM would retain stale all-visible/all-frozen
bits, which can cause wrong results from index-only scans and incorrect
relfrozenxid advancement due to vacuum page skipping.
Not registering the VM buffer also meant we never emitted FPIs of VM
pages when clearing bits. A torn VM page won't raise an error because
the VM is read with ZERO_ON_ERROR; with checksums on, it would be
detected and zeroed, but with checksums off, it is accepted as-is and
can lead to data corruption.
Fix this by registering the VM buffer in the WAL record when clearing VM
bits. The VM buffer must now be locked throughout the critical section
that modifies the VM and heap pages and emits the WAL record. This can
slow down operations that clear the VM, since the VM lock is held longer
and VM FPIs may be emitted, but it is required for correctness.
Note that this fix does not repair existing incremental backups.
Author: Melanie Plageman <[email protected]>
Author: Andres Freund <[email protected]>
Reviewed-by: Robert Haas <[email protected]>
Reviewed-by: Andrey Borodin <[email protected]>
Discussion: https://postgr.es/m/flat/CAAKRu_bn%2Be7F4yPFBgFbnP%2BsyJRKyNK092bjD2LKvZW7O4Svag>
Backpatch-through: 17
---
contrib/pg_surgery/heap_surgery.c | 40 ++-
src/backend/access/heap/heapam.c | 383 ++++++++++++++++++++----
src/backend/access/heap/heapam_xlog.c | 239 ++++++++++-----
src/backend/access/heap/visibilitymap.c | 34 ++-
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, 574 insertions(+), 148 deletions(-)
diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index 602aca66c60..6a38ac577c6 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,29 @@ 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);
+ /* Include the VM page if it was modified. */
+ 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 96c25197788..c3f611a42ed 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 clear_all_visible = 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);
+ clear_all_visible = 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 (clear_all_visible)
{
- 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;
@@ -2185,7 +2197,7 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
xlrec.offnum = ItemPointerGetOffsetNumber(&heaptup->t_self);
xlrec.flags = 0;
- if (all_visible_cleared)
+ if (clear_all_visible)
xlrec.flags |= XLH_INSERT_ALL_VISIBLE_CLEARED;
if (options & HEAP_INSERT_SPECULATIVE)
xlrec.flags |= XLH_INSERT_IS_SPECULATIVE;
@@ -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 clear_all_visible was true regardless of whether
+ * or not we ended up modifying the vmbuffer.
+ */
+ if (clear_all_visible)
+ LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK);
+ if (BufferIsValid(vmbuffer))
ReleaseBuffer(vmbuffer);
/*
@@ -2419,8 +2444,9 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
while (ndone < ntuples)
{
Buffer buffer;
- bool all_visible_cleared = false;
+ bool clear_all_visible = 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);
+ clear_all_visible = true;
+ }
+
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
@@ -2502,13 +2539,15 @@ 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 (clear_all_visible)
{
- 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);
}
else if (all_frozen_set)
PageSetAllVisible(page);
@@ -2554,10 +2593,10 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
tupledata = scratchptr;
/* check that the mutually exclusive flags are not both set */
- Assert(!(all_visible_cleared && all_frozen_set));
+ Assert(!(clear_all_visible && all_frozen_set));
xlrec->flags = 0;
- if (all_visible_cleared)
+ if (clear_all_visible)
xlrec->flags = XLH_INSERT_ALL_VISIBLE_CLEARED;
if (all_frozen_set)
xlrec->flags = XLH_INSERT_ALL_FROZEN_SET;
@@ -2623,6 +2662,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 +2674,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 clear_all_visible was true regardless of
+ * whether or not we ended up modifying the vmbuffer.
+ */
+ if (clear_all_visible)
+ 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,12 +2836,13 @@ 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;
bool have_tuple_lock = false;
bool iscombo;
- bool all_visible_cleared = false;
+ bool clear_all_visible = false;
HeapTuple old_key_tuple = NULL; /* replica identity of the tuple */
bool old_key_copied = false;
@@ -3040,6 +3091,13 @@ l1:
xid, LockTupleExclusive, true,
&new_xmax, &new_infomask, &new_infomask2);
+ /* Lock the VM before entering the critical section */
+ if (PageIsAllVisible(page))
+ {
+ clear_all_visible = true;
+ LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
+ }
+
START_CRIT_SECTION();
/*
@@ -3051,12 +3109,14 @@ l1:
*/
PageSetPrunable(page, xid);
- if (PageIsAllVisible(page))
+ if (clear_all_visible)
{
- 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 */
@@ -3096,7 +3156,7 @@ l1:
log_heap_new_cid(relation, &tp);
xlrec.flags = 0;
- if (all_visible_cleared)
+ if (clear_all_visible)
xlrec.flags |= XLH_DELETE_ALL_VISIBLE_CLEARED;
if (changingPart)
xlrec.flags |= XLH_DELETE_IS_PARTITION_MOVE;
@@ -3137,13 +3197,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 clear_all_visible was true regardless of whether or not we
+ * ended up modifying the vmbuffer.
+ */
+ if (clear_all_visible)
+ LockBuffer(vmbuffer, BUFFER_LOCK_UNLOCK);
+
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
if (vmbuffer != InvalidBuffer)
@@ -3261,13 +3335,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;
@@ -3276,8 +3353,10 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
bool use_hot_update = false;
bool summarized_update = false;
bool key_intact;
- bool all_visible_cleared = false;
- bool all_visible_cleared_new = false;
+ bool clear_all_visible = false;
+ bool clear_all_visible_new = false;
+ bool vmbuffer_modified = false;
+ bool vmbuffer_new_modified = false;
bool checked_lockers;
bool locker_remains;
bool id_has_external = false;
@@ -3852,6 +3931,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 +3959,13 @@ 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))
+ {
+ /* It's possible all-frozen was already clear */
+ if (visibilitymap_clear_locked(relation, block, vmbuffer,
+ VISIBILITYMAP_ALL_FROZEN))
+ cleared_all_frozen = true;
+ }
MarkBufferDirty(buffer);
@@ -3896,12 +3984,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 +4087,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 +4152,69 @@ l2:
id_has_external,
&old_key_copied);
+ clear_all_visible = PageIsAllVisible(page);
+ clear_all_visible_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 (clear_all_visible && clear_all_visible_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] = {
+ clear_all_visible ? vmbuffer : InvalidBuffer,
+ clear_all_visible_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 (clear_all_visible && clear_all_visible_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 (clear_all_visible)
+ unlock_vmbuffer = true;
+ if (clear_all_visible_new)
+ unlock_vmbuffer_new = true;
+ }
+
/* NO EREPORT(ERROR) from here till changes are logged */
START_CRIT_SECTION();
@@ -4086,7 +4251,6 @@ l2:
RelationPutHeapTuple(relation, newbuf, heaptup, false); /* insert new tuple */
-
/* Clear obsolete visibility flags, possibly set by ourselves above... */
oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
@@ -4101,19 +4265,38 @@ l2:
oldtup.t_data->t_ctid = heaptup->t_self;
/* clear PD_ALL_VISIBLE flags, reset all visibilitymap bits */
- if (PageIsAllVisible(BufferGetPage(buffer)))
+ if (clear_all_visible)
{
- all_visible_cleared = true;
- PageClearAllVisible(BufferGetPage(buffer));
- visibilitymap_clear(relation, BufferGetBlockNumber(buffer),
- vmbuffer, VISIBILITYMAP_VALID_BITS);
+ /* It's possible the VM bits were already clear */
+ if (visibilitymap_clear_locked(relation, block,
+ vmbuffer, VISIBILITYMAP_VALID_BITS))
+ {
+ /*
+ * If both heap blocks' 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 (newbuf != buffer && PageIsAllVisible(BufferGetPage(newbuf)))
+ if (clear_all_visible_new)
{
- all_visible_cleared_new = true;
- PageClearAllVisible(BufferGetPage(newbuf));
- visibilitymap_clear(relation, BufferGetBlockNumber(newbuf),
- vmbuffer_new, VISIBILITYMAP_VALID_BITS);
+ /*
+ * If both heap blocks' VM bits are on the same VM buffer, this will
+ * clear the new heap block's VM bits from the shared vmbuffer.
+ */
+ if (visibilitymap_clear_locked(relation, BufferGetBlockNumber(newbuf),
+ vmbuffer_new, VISIBILITYMAP_VALID_BITS))
+ vmbuffer_new_modified = true;
+
+ PageClearAllVisible(newpage);
}
if (newbuf != buffer)
@@ -4136,19 +4319,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);
+ clear_all_visible,
+ clear_all_visible_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 +4780,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 +5361,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 +5399,13 @@ 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))
+ {
+ /* It's possible all-frozen was already clear */
+ if (visibilitymap_clear_locked(relation, block, vmbuffer,
+ VISIBILITYMAP_ALL_FROZEN))
+ cleared_all_frozen = true;
+ }
MarkBufferDirty(*buffer);
@@ -5232,19 +5436,31 @@ 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);
+ unlock_vmbuffer = false;
+
result = TM_Ok;
out_locked:
LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
+ Assert(!unlock_vmbuffer);
out_unlocked:
if (BufferIsValid(vmbuffer))
@@ -5707,6 +5923,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 +5933,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 +5942,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 +5962,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 +5991,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 +6172,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 +6189,19 @@ l4:
MarkBufferDirty(buf);
+ if (PageIsAllVisible(page))
+ {
+ /* It's possible all-frozen was already clear */
+ 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 +6214,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 +9090,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 +9301,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 87b5c44fd46..41bd03e80a0 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -22,6 +22,70 @@
#include "storage/freespace.h"
#include "storage/standby.h"
+/*
+ * Clear visibility map bits for a single heap block during heap redo.
+ *
+ * Used by records that modify one heap block and, at most, its corresponding
+ * VM block (insert, delete, multi_insert, lock). Records that can touch
+ * multiple heap or VM blocks (e.g. updates) replay the VM changes inline
+ * instead.
+ *
+ * 'record' is the WAL record being replayed
+ * 'target_locator' identifies the relation whose VM is being updated
+ * 'heap_blkno' is the heap block whose VM bits should be cleared
+ * 'wal_vm_block_id' is the WAL block reference id of the VM page
+ * 'flags' specifies which visibility map bits to clear
+ */
+static void
+heap_xlog_vm_clear(XLogReaderState *record,
+ RelFileLocator target_locator,
+ BlockNumber heap_blkno,
+ uint8 wal_vm_block_id, uint8 flags)
+{
+ XLogRecPtr lsn = record->EndRecPtr;
+ Relation reln = CreateFakeRelcacheEntry(target_locator);
+ Buffer vmbuffer = InvalidBuffer;
+
+ /*
+ * If the vmbuffer was registered, use the recovery-specific routines to
+ * read it. These will either apply an FPI or indicate that we should
+ * clear the requested bits ourselves.
+ *
+ * Originally, clearing the VM did not register the VM buffers, so since
+ * registering the VM buffer was a bug fix, we keep a fallback path to
+ * support replay of WAL generated from before the fix.
+ */
+ if (XLogRecHasBlockRef(record, wal_vm_block_id))
+ {
+ if (XLogReadBufferForRedo(record, wal_vm_block_id,
+ &vmbuffer) == BLK_NEEDS_REDO)
+ {
+ if (visibilitymap_clear_locked(reln,
+ heap_blkno, vmbuffer,
+ flags))
+ PageSetLSN(BufferGetPage(vmbuffer), lsn);
+ }
+ if (BufferIsValid(vmbuffer))
+ UnlockReleaseBuffer(vmbuffer);
+ }
+ else
+ {
+ /*
+ * This is the backwards compatibility path to clear VM bits for
+ * records predating VM buffer registration. It is also invoked if the
+ * heap page's PD_ALL_VISIBLE was cleared but the VM bits were already
+ * clear. The WAL record flags do not distinguish between these two
+ * situations. Though this is wasted effort, the behavior is
+ * historical and the situation should be rare.
+ */
+ visibilitymap_pin(reln, heap_blkno, &vmbuffer);
+ visibilitymap_clear(reln, heap_blkno, vmbuffer, flags);
+ ReleaseBuffer(vmbuffer);
+ }
+
+ FreeFakeRelcacheEntry(reln);
+}
+
/*
* Replay XLOG_HEAP2_PRUNE_* records.
@@ -360,15 +424,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, target_locator,
+ blkno, HEAP_DELETE_BLKREF_VM,
+ VISIBILITYMAP_VALID_BITS);
if (XLogReadBufferForRedo(record, HEAP_DELETE_BLKREF_HEAP,
&buffer) == BLK_NEEDS_REDO)
@@ -449,15 +507,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, target_locator,
+ blkno, HEAP_INSERT_BLKREF_VM,
+ VISIBILITYMAP_VALID_BITS);
/*
* If we inserted the first and only tuple on the page, re-initialize the
@@ -573,17 +625,15 @@ heap_xlog_multi_insert(XLogReaderState *record)
/*
* The visibility map may need to be fixed even if the heap page is
* already up-to-date.
+ *
+ * Clear the VM (if needed) before clearing the heap page-level visibility
+ * flag (PD_ALL_VISIBLE) to prevent the heap page from being marked
+ * 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, rlocator,
+ blkno, HEAP_MULTI_INSERT_BLKREF_VM,
+ VISIBILITYMAP_VALID_BITS);
if (isinit)
{
@@ -698,6 +748,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 +789,92 @@ 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;
+ bool vmbuffer_new_modified = false;
+
+ 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))
+ {
+ if (visibilitymap_clear_locked(reln, oldblk, vmbuffer_new,
+ VISIBILITYMAP_VALID_BITS))
+ vmbuffer_new_modified = true;
+ }
+ if (visibilitymap_clear_locked(reln, newblk, vmbuffer_new,
+ VISIBILITYMAP_VALID_BITS))
+ vmbuffer_new_modified = true;
+
+ if (vmbuffer_new_modified)
+ 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)
+ {
+ if (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 +923,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 +949,6 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
newaction = XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_HEAP_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 +1045,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 +1137,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, rlocator,
+ block, HEAP_LOCK_BLKREF_VM,
+ VISIBILITYMAP_ALL_FROZEN);
}
if (XLogReadBufferForRedo(record, HEAP_LOCK_BLKREF_HEAP,
@@ -1100,20 +1210,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, 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..d44620eca7f 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -135,9 +135,38 @@ 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.
+ *
+ * Most callers should use visibilitymap_clear_locked rather than this
+ * function. 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. However, we retain this function for behavioral compatibility on the
+ * back branches (out-of-tree callers may expect it to take the lock). And,
+ * since we have it, we use it for in-tree callers that don't have to manage
+ * the VM buffer lock themselves.
*/
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;
+}
+
+/*
+ * Clear specified bits, caller holds VM lock
+ *
+ * Like visibilitymap_clear(), except the caller must already hold the VM
+ * buffer exclusive lock and is responsible for unlocking it.
+ * Returns true if any bits were actually cleared, false otherwise.
+ */
+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 +186,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 +198,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 92744c53899..c4c3c3f15fa 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -111,6 +111,7 @@
/* This is what we need to know about delete */
#define HEAP_DELETE_BLKREF_HEAP 0
+#define HEAP_DELETE_BLKREF_VM 1
typedef struct xl_heap_delete
{
@@ -160,6 +161,7 @@ typedef struct xl_heap_header
/* This is what we need to know about insert */
#define HEAP_INSERT_BLKREF_HEAP 0
+#define HEAP_INSERT_BLKREF_VM 1
typedef struct xl_heap_insert
{
@@ -179,6 +181,7 @@ typedef struct xl_heap_insert
* (XLOG_HEAP_INIT_PAGE).
*/
#define HEAP_MULTI_INSERT_BLKREF_HEAP 0
+#define HEAP_MULTI_INSERT_BLKREF_VM 1
/*
* In HEAP_MULTI_INSERT_BLKREF_HEAP's data portion, there is an
@@ -222,10 +225,21 @@ typedef struct xl_multi_insert_tuple
*
* HEAP_UPDATE_BLKREF_HEAP_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_HEAP_NEW 0
#define HEAP_UPDATE_BLKREF_HEAP_OLD 1
+#define HEAP_UPDATE_BLKREF_VM_NEW 2
+#define HEAP_UPDATE_BLKREF_VM_OLD 3
typedef struct xl_heap_update
{
@@ -406,6 +420,7 @@ typedef struct xlhp_prune_items
/* This is what we need to know about lock */
#define HEAP_LOCK_BLKREF_HEAP 0
+#define HEAP_LOCK_BLKREF_VM 1
typedef struct xl_heap_lock
{
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.47.3
From a171432d65b3dd4d68dc1eba3e91c008ab98798e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Thu, 30 Apr 2026 16:06:06 -0400
Subject: [PATCH v4 3/4] Put pg_combinebackup debug test output in temp file
Having --debug as the default option to pg_combinebackup in
init_from_backup() makes the logs extremely verbose. It is used for
debugging failures, so put the output in a temp file and only keep it
around if tests fail.
Author: Melanie Plageman <[email protected]>
Disussion: https://postgr.es/m/y73coty2smkxgvelh4e32kqrt5pyn2pb7cz7x4mrz6yaxitqd5%40nzhkeazqf4bn
Backpatch-through: 17
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 83 +++++++++++++++++++++++-
1 file changed, 81 insertions(+), 2 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 2e31c23a6ac..3388e45b726 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -125,6 +125,8 @@ our $min_compat = 12;
# list of file reservations made by get_free_port
my @port_reservation_files;
+my @combinebackup_debug_logs;
+my %combinebackup_debug_logs_printed;
# We want to choose a server port above the range that servers typically use
# on Unix systems and below the range those systems typically use for ephemeral
@@ -898,7 +900,9 @@ pathnames that should be used for the target cluster.
To restore from an incremental backup, pass the parameter combine_with_prior
as a reference to an array of prior backup names with which this backup
-is to be combined using pg_combinebackup.
+is to be combined using pg_combinebackup. pg_combinebackup is run with
+--debug, but the output is captured in a temporary file and only copied to the
+test log if pg_combinebackup fails or if a later test fails.
Streaming replication can be enabled on this node by passing the keyword
parameter has_streaming => 1. This is disabled by default.
@@ -964,7 +968,7 @@ sub init_from_backup
}
push @combineargs, @prior_backup_path, $backup_path,
'--output' => $data_path;
- PostgreSQL::Test::Utils::system_or_bail(@combineargs);
+ _run_pg_combinebackup_with_debug_log(@combineargs);
}
elsif (defined $params{tar_program})
{
@@ -1079,6 +1083,81 @@ port = $port
return;
}
+sub _run_pg_combinebackup_with_debug_log
+{
+ my (@cmd) = @_;
+ my $debug_log = PostgreSQL::Test::Utils::tempdir('pg_combinebackup') .
+ '/debug.log';
+
+ push @combinebackup_debug_logs, $debug_log;
+
+ print("# Running: " . join(" ", @cmd) . "\n");
+ # If this pg_combinebackup invocation itself fails, print its captured
+ # output immediately. Successful invocations may still be printed later if
+ # some subsequent TAP assertion in this test script fails.
+ if (!IPC::Run::run(\@cmd, '&>' => $debug_log))
+ {
+ _print_pg_combinebackup_debug_log($debug_log);
+
+ if ($? == -1)
+ {
+ BAIL_OUT(
+ sprintf(
+ "failed to execute command \"%s\": $!", join(" ", @cmd)));
+ }
+ elsif ($? & 127)
+ {
+ BAIL_OUT(
+ sprintf(
+ "command \"%s\" died with signal %d",
+ join(" ", @cmd), $? & 127));
+ }
+ else
+ {
+ BAIL_OUT(
+ sprintf(
+ "command \"%s\" exited with value %d",
+ join(" ", @cmd), $? >> 8));
+ }
+ }
+}
+
+sub _print_pg_combinebackup_debug_log
+{
+ my ($debug_log) = @_;
+
+ return unless -e $debug_log;
+ return if $combinebackup_debug_logs_printed{$debug_log}++;
+
+ diag("pg_combinebackup debug output from $debug_log:\n" .
+ PostgreSQL::Test::Utils::slurp_file($debug_log));
+}
+
+# Keep pg_combinebackup debug logs until test exit. If pg_combinebackup itself
+# fails, its log is printed at the failure site above. If pg_combinebackup
+# succeeds but a later TAP assertion fails, print the retained logs here so the
+# regress output includes the combine step that produced the restored state.
+# Remove the logs after a clean run.
+END
+{
+ # take care not to change the script's exit value
+ my $exit_code = $?;
+
+ if ($exit_code == 0 && PostgreSQL::Test::Utils::all_tests_passing())
+ {
+ unlink @combinebackup_debug_logs;
+ }
+ else
+ {
+ foreach my $debug_log (@combinebackup_debug_logs)
+ {
+ _print_pg_combinebackup_debug_log($debug_log);
+ }
+ }
+
+ $? = $exit_code;
+}
+
=pod
=item $node->rotate_logfile()
--
2.47.3
From 7dba64d8dd71d2c1e30bf978781a399f99b1676e Mon Sep 17 00:00:00 2001
From: Melanie Plageman <[email protected]>
Date: Thu, 23 Apr 2026 11:13:23 -0400
Subject: [PATCH v4 4/4] Test that VM clear registers VM buffers
The WAL summarizer only tracks registered buffers, so unregistered VM
clears are ommitted from incremental backups, corrupting the restored
visibility map. Test those cases are now fixed.
Author: Melanie Plageman <[email protected]>
Reviewed-by: Andrey Borodin <[email protected]>
Discussion: https://postgr.es/m/oqcsevg35xjan2327x5kdfth6q4fgeqboxfo3v3imeyih2uiny%406sez5dzxl6nt
Backpatch-through: 17
---
src/bin/pg_combinebackup/Makefile | 2 +
src/bin/pg_combinebackup/meson.build | 1 +
.../pg_combinebackup/t/012_vm_consistency.pl | 258 ++++++++++++++++++
3 files changed, 261 insertions(+)
create mode 100644 src/bin/pg_combinebackup/t/012_vm_consistency.pl
diff --git a/src/bin/pg_combinebackup/Makefile b/src/bin/pg_combinebackup/Makefile
index 33a1f4483bf..bdba0353412 100644
--- a/src/bin/pg_combinebackup/Makefile
+++ b/src/bin/pg_combinebackup/Makefile
@@ -12,6 +12,8 @@
PGFILEDESC = "pg_combinebackup - combine incremental backups"
PGAPPICON=win32
+EXTRA_INSTALL=contrib/pg_visibility
+
subdir = src/bin/pg_combinebackup
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global
diff --git a/src/bin/pg_combinebackup/meson.build b/src/bin/pg_combinebackup/meson.build
index bbc4c5735ba..757b78e2fe8 100644
--- a/src/bin/pg_combinebackup/meson.build
+++ b/src/bin/pg_combinebackup/meson.build
@@ -39,6 +39,7 @@ tests += {
't/009_no_full_file.pl',
't/010_hardlink.pl',
't/011_ib_truncation.pl',
+ 't/012_vm_consistency.pl',
],
}
}
diff --git a/src/bin/pg_combinebackup/t/012_vm_consistency.pl b/src/bin/pg_combinebackup/t/012_vm_consistency.pl
new file mode 100644
index 00000000000..b0810f8022a
--- /dev/null
+++ b/src/bin/pg_combinebackup/t/012_vm_consistency.pl
@@ -0,0 +1,258 @@
+# Copyright (c) 2026, PostgreSQL Global Development Group
+#
+# Test that heap operations clearing visibility map bits (INSERT, UPDATE,
+# DELETE, SELECT FOR UPDATE, COPY) correctly register visibility map buffers,
+# since incremental backups rely on the WAL summarizer, which only tracks
+# registered buffers.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $tempdir = PostgreSQL::Test::Utils::tempdir_short();
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+# Set up primary with WAL summarization enabled.
+my $primary = PostgreSQL::Test::Cluster->new('primary');
+$primary->init(allows_streaming => 1);
+$primary->append_conf('postgresql.conf', <<EOF);
+summarize_wal = on
+autovacuum = off
+EOF
+$primary->start;
+
+$primary->safe_psql('postgres', q{CREATE EXTENSION pg_visibility});
+
+my @tests = (
+ {
+ label => 'INSERT',
+ table => 'vm_insert_test',
+ setup => q{CREATE TABLE vm_insert_test (id int);
+ INSERT INTO vm_insert_test DEFAULT VALUES;},
+ modify => q{INSERT INTO vm_insert_test VALUES (1)},
+ visible_op => '<',
+ frozen_op => '<',
+ },
+ {
+ label => 'DELETE',
+ table => 'vm_delete_test',
+ setup => q{CREATE TABLE vm_delete_test (id int);
+ INSERT INTO vm_delete_test VALUES (1), (2);},
+ modify => q{DELETE FROM vm_delete_test WHERE id = 1},
+ visible_op => '<',
+ frozen_op => '<',
+ },
+ {
+ label => 'UPDATE',
+ table => 'vm_update_test',
+ # Include both same-page and cross-page updates. val is stored PLAIN
+ # so the large update stays inline.
+ setup => q{CREATE TABLE vm_update_test (id INT, val TEXT);
+ ALTER TABLE vm_update_test ALTER COLUMN val SET STORAGE PLAIN;
+ INSERT INTO vm_update_test VALUES (1, 'same page'), (2, 'cross page');
+ INSERT INTO vm_update_test SELECT i, repeat('a', 200)
+ FROM generate_series(3, 70) i;},
+ modify => q{UPDATE vm_update_test SET id = 0 WHERE id = 1;
+ UPDATE vm_update_test SET val = repeat('b', 4000) WHERE id = 2;},
+ # Confirm the small update stays on the same heap page while the large
+ # update relocates the tuple to a different heap page.
+ ctid_checks => [
+ {
+ before => 'id = 1',
+ after => 'id = 0',
+ same => 1,
+ desc => 'small update stays on the same heap page',
+ },
+ {
+ before => 'id = 2',
+ after => 'id = 2',
+ same => 0,
+ desc => 'large update moves the tuple to a different heap page',
+ },
+ ],
+ visible_op => '<',
+ frozen_op => '<',
+ },
+ {
+ label => 'LOCK',
+ table => 'vm_lock_test',
+ setup => q{CREATE TABLE vm_lock_test (id int);
+ INSERT INTO vm_lock_test VALUES (1), (2);},
+ modify => q{SELECT * FROM vm_lock_test WHERE id = 1 FOR UPDATE},
+ visible_op => '==',
+ frozen_op => '<',
+ },
+ {
+ label => 'COPY',
+ table => 'vm_copy_test',
+ setup => q{CREATE TABLE vm_copy_test (id int);
+ INSERT INTO vm_copy_test DEFAULT VALUES;},
+ modify => q{COPY vm_copy_test FROM PROGRAM 'echo 42'},
+ visible_op => '<',
+ frozen_op => '<',
+ },
+);
+
+sub get_vm_summary
+{
+ my ($node, $table) = @_;
+ my $result = $node->safe_psql('postgres',
+ "SELECT all_visible, all_frozen FROM pg_visibility_map_summary('$table')");
+ my @vals = split(/\|/, $result);
+ return @vals;
+}
+
+# Return the heap block number of the (single) row matching $where. The ctid
+# is "(block,offset)"; casting it through point lets us pull out the block.
+sub heap_block
+{
+ my ($node, $table, $where) = @_;
+ return $node->safe_psql('postgres',
+ "SELECT (ctid::text::point)[0]::int FROM $table WHERE $where");
+}
+
+# Confirm VACUUM (FREEZE) set VM bits before testing whether later heap
+# modifications clear those bits and are captured by incremental backup. We
+# could perhaps be more exact than > 0, but the coarseness attempts to avoid
+# tset flakes.
+sub check_vacuumed_vm
+{
+ my ($node, $test) = @_;
+ my ($all_visible, $all_frozen) = get_vm_summary($node, $test->{table});
+
+ cmp_ok($all_visible, '>', 0,
+ "$test->{label} test: pages are all-visible after vacuum");
+ cmp_ok($all_frozen, '>', 0,
+ "$test->{label} test: pages are all-frozen after vacuum");
+
+ return ($all_visible, $all_frozen);
+}
+
+# Check the VM bit counts after a heap modification against the post-vacuum
+# baseline. Most operations clear both bits; tuple locking clears all-frozen
+# without clearing all-visible.
+sub check_modified_vm
+{
+ my ($node, $test) = @_;
+ my ($post_visible, $post_frozen) = get_vm_summary($node, $test->{table});
+
+ cmp_ok($post_visible, $test->{visible_op}, $test->{pre_visible},
+ "$test->{label} test: all-visible state after modification");
+ cmp_ok($post_frozen, $test->{frozen_op}, $test->{pre_frozen},
+ "$test->{label} test: all-frozen state after modification");
+
+ return ($post_visible, $post_frozen);
+}
+
+# Verify the combined backup restored VM state exactly as it exists on the
+# primary, and ask pg_visibility to check that visible tuples are consistent.
+sub validate_restored_vm
+{
+ my ($restored, $test) = @_;
+
+ my ($primary_visible, $primary_frozen) =
+ get_vm_summary($primary, $test->{table});
+ my ($restored_visible, $restored_frozen) =
+ get_vm_summary($restored, $test->{table});
+
+ is($restored_visible, $primary_visible,
+ "$test->{label} test: restored all_visible count matches primary");
+ is($restored_frozen, $primary_frozen,
+ "$test->{label} test: restored all_frozen count matches primary");
+
+ my $corrupt_tids = $restored->safe_psql('postgres',
+ "SELECT count(*) FROM pg_check_visible('$test->{table}')");
+ is($corrupt_tids, '0',
+ "$test->{label} test: no VM corruption detected by pg_check_visible");
+}
+
+# Create and populate the tables, then vacuum freeze them to set the VM bits
+foreach my $test (@tests)
+{
+ $primary->safe_psql('postgres', $test->{setup});
+ $primary->safe_psql('postgres', "VACUUM (FREEZE) $test->{table}");
+ ($test->{pre_visible}, $test->{pre_frozen}) =
+ check_vacuumed_vm($primary, $test);
+}
+
+# Take a full backup
+my $full_name = 'full';
+my $full_path = $primary->backup_dir . "/$full_name";
+$primary->command_ok(
+ [
+ 'pg_basebackup', '--no-sync',
+ '--pgdata' => $full_path,
+ '--checkpoint' => 'fast',
+ ],
+ 'full backup');
+
+# Modify the tables and check that the VM bits are as expected for that test
+# after the specified modification.
+foreach my $test (@tests)
+{
+ # Record the heap block of any rows whose same-/cross-page movement we
+ # want to verify, before the modification relocates them.
+ foreach my $check (@{ $test->{ctid_checks} // [] })
+ {
+ $check->{before_blk} =
+ heap_block($primary, $test->{table}, $check->{before});
+ }
+
+ $primary->safe_psql('postgres', $test->{modify});
+ ($test->{post_visible}, $test->{post_frozen}) =
+ check_modified_vm($primary, $test);
+
+ # Confirm each checked row did (or did not) move to a different heap page.
+ foreach my $check (@{ $test->{ctid_checks} // [] })
+ {
+ my $after_blk = heap_block($primary, $test->{table}, $check->{after});
+ if ($check->{same})
+ {
+ is($after_blk, $check->{before_blk},
+ "$test->{label} test: $check->{desc}");
+ }
+ else
+ {
+ isnt($after_blk, $check->{before_blk},
+ "$test->{label} test: $check->{desc}");
+ }
+ }
+}
+
+# Take an incremental backup. This will have the changes made in the
+# modification step.
+my $incr_name = 'incr';
+my $incr_path = $primary->backup_dir . "/$incr_name";
+$primary->command_ok(
+ [
+ 'pg_basebackup', '--no-sync',
+ '--pgdata' => $incr_path,
+ '--checkpoint' => 'fast',
+ '--incremental' => $full_path . '/backup_manifest',
+ ],
+ 'incremental backup');
+
+# Start a server from a combined backup composed of the incremental and full
+# backup.
+my $restored = PostgreSQL::Test::Cluster->new('restored');
+$restored->init_from_backup($primary, $incr_name,
+ combine_with_prior => [$full_name],
+ combine_mode => $mode);
+$restored->append_conf('postgresql.conf', <<EOF);
+autovacuum = off
+EOF
+$restored->start;
+$restored->safe_psql('postgres', q{CREATE EXTENSION IF NOT EXISTS pg_visibility});
+
+# Confirm that the restored server's visibility map matches the original server
+foreach my $test (@tests)
+{
+ validate_restored_vm($restored, $test);
+}
+
+$restored->stop;
+$primary->stop;
+
+done_testing();
--
2.47.3
