Subject: Re: Buffer locking is special (hints, checksums, AIO writes)

From: Andres Freund <[email-masked]>
Date: Mon, 9 Feb 2026 17:16:38 -0500

Hi,

On 2026-02-09 12:42:25 +0100, Antonin Houska wrote:
> Andres Freund <[email-masked]> wrote:
> 
> > On 2026-01-12 12:45:03 -0500, Andres Freund wrote:
> > > I'm doing another pass through 0003 and will push that if I don't find
> > > anything significant.
> > 
> > Done, after adjust two comments in minor ways.
> 
> I suppose this is commit 0b96e734c590.
> 
> While troubleshooting REPACK issue [1], I realized that
> HeapTupleSatisfiesMVCCBatch() can also be called during logical decoding - in
> that case we need to use a historic MVCC snapshot.

Huh. Indeed. That's unintentional - the path should never have been reached,
we are checking that an MVCC snapshot is used. Unfortunately, somebody
(i.e. probably me) at some point defined the relevant macro as

/* This macro encodes the knowledge of which snapshots are MVCC-safe */
#define IsMVCCSnapshot(snapshot)  \
	((snapshot)->snapshot_type == SNAPSHOT_MVCC || \
	 (snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC)

Which makes sense for some places, but not for plenty others.

The reason this didn't cause more widespread issues is that during logical
decoding we mostly don't use sequential scans etc that are affected by the
these paths.

> My proposal to fix the problem is attached.

That's imo not at all the right fix - it'd make visibility during seqscans
checking noticeably slower.

I think we ought to instead restrict the page-at-a-time scans to only happen
with "real" mvcc snapshots. I.e. this:

	/*
	 * Disable page-at-a-time mode if it's not a MVCC-safe snapshot.
	 */
	if (!(snapshot && IsMVCCSnapshot(snapshot)))
		scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;

should trigger for historic snapshots as well.

Does that fix the issue for you?

What's your reproducer?

Greetings,

Andres Freund

---

From: Antonin Houska <[email-masked]>
Date: Tue, 10 Feb 2026 08:46:27 +0100

> checking noticeably slower.

ok

> should trigger for historic snapshots as well.

I suppose you mean changing it to

	if (!(snapshot && IsMVCCSnapshot(snapshot) &&
		  !IsHistoricMVCCSnapshot(snapshot)))
		scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;

> Does that fix the issue for you?

Yes, with this change, I don't hit the problem anymore.

> What's your reproducer?

Check out this branch

https://github.com/michail-nikolaev/postgres/tree/repack_concurrently_repro_22

and run t/008_repack_concurrently.pl in contrib/amcheck. The error we saw in
most cases was "ERROR: cache lookup failed for relation". I noticed that the
related pg_class entries had hint bits set incorrectly, so I added the
following to see when exactly it happens (actually I used lower elevel first,
to find out that the decoding worker is responsible for the problem):

diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 75ae268d753..ebf38460873 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -73,6 +73,7 @@
 #include "access/transam.h"
 #include "access/xact.h"
 #include "access/xlog.h"
+#include "commands/cluster.h"
 #include "storage/bufmgr.h"
 #include "storage/procarray.h"
 #include "utils/builtins.h"
@@ -938,6 +939,8 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
                                                HeapTupleHeaderGetRawXmin(tuple));
                else
                {
+                       if (am_decoding_for_repack())
+                               elog(PANIC, "HEAP_XMIN_INVALID set");
                        /* it must have aborted or crashed */
                        SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
                                                InvalidTransactionId);

The backtrace looked like:

    #3  0x0000000000c367ad errfinish (postgres + 0x8367ad)
    #4  0x0000000000517ee1 HeapTupleSatisfiesMVCC (postgres + 0x117ee1)
    #5  0x0000000000518e14 HeapTupleSatisfiesMVCCBatch (postgres + 0x118e14)
    #6  0x000000000050483e page_collect_tuples (postgres + 0x10483e)
    #7  0x0000000000504a8e heap_prepare_pagescan (postgres + 0x104a8e)
    #8  0x0000000000505344 heapgettup_pagemode (postgres + 0x105344)
    #9  0x0000000000505cff heap_getnextslot (postgres + 0x105cff)
    #10 0x000000000052ca08 table_scan_getnextslot (postgres + 0x12ca08)
    #11 0x000000000052d4ed systable_getnext (postgres + 0x12d4ed)
    #12 0x0000000000c2098e ScanPgRelation (postgres + 0x82098e)
    #13 0x0000000000c23aa2 RelationReloadIndexInfo (postgres + 0x823aa2)
    #14 0x0000000000c24376 RelationRebuildRelation (postgres + 0x824376)
    #15 0x0000000000c23784 RelationIdGetRelation (postgres + 0x823784)
    #16 0x00000000004b558f relation_open (postgres + 0xb558f)
    #17 0x000000000052e073 index_open (postgres + 0x12e073)
    #18 0x000000000052d0c5 systable_beginscan (postgres + 0x12d0c5)
    #19 0x0000000000c2097e ScanPgRelation (postgres + 0x82097e)
    #20 0x0000000000c23dde RelationReloadNailed (postgres + 0x823dde)
    #21 0x0000000000c24399 RelationRebuildRelation (postgres + 0x824399)
    #22 0x0000000000c23784 RelationIdGetRelation (postgres + 0x823784)
    #23 0x00000000004b558f relation_open (postgres + 0xb558f)
    #24 0x0000000000573ab1 table_open (postgres + 0x173ab1)
    #25 0x0000000000c20919 ScanPgRelation (postgres + 0x820919)
    #26 0x0000000000c21c98 RelationBuildDesc (postgres + 0x821c98)
    #27 0x0000000000c237d6 RelationIdGetRelation (postgres + 0x8237d6)
    #28 0x00000000009a028a ReorderBufferProcessTXN (postgres + 0x5a028a)
    #29 0x00000000009a10ed ReorderBufferReplay (postgres + 0x5a10ed)
    #30 0x00000000009a116b ReorderBufferCommit (postgres + 0x5a116b)
    #31 0x000000000098c7fe DecodeCommit (postgres + 0x58c7fe)
    #32 0x000000000098ba67 xact_decode (postgres + 0x58ba67)
    #33 0x000000000098b685 LogicalDecodingProcessRecord (postgres + 0x58b685)
    #34 0x00000000006a3e4b decode_concurrent_changes (postgres + 0x2a3e4b)
    #35 0x00000000006a5ea3 repack_worker_internal (postgres + 0x2a5ea3)
    #36 0x00000000006a5d5e RepackWorkerMain (postgres + 0x2a5d5e)
    #37 0x0000000000961b2b BackgroundWorkerMain (postgres + 0x561b2b)
    #38 0x0000000000964a61 postmaster_child_launch (postgres + 0x564a61)
    #39 0x000000000096b58d StartBackgroundWorker (postgres + 0x56b58d)
    #40 0x000000000096b80a maybe_start_bgworkers (postgres + 0x56b80a)
    #41 0x000000000096a5d9 LaunchMissingBackgroundProcesses (postgres + 0x56a5d9)
    #42 0x00000000009683fc ServerLoop (postgres + 0x5683fc)
    #43 0x0000000000967d37 PostmasterMain (postgres + 0x567d37)
    #44 0x0000000000813421 main (postgres + 0x413421)

Thanks.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

---

From: Andres Freund <[email-masked]>
Date: Tue, 10 Feb 2026 11:49:36 -0500

Hi,

> 		scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;

Yes.

For something committable, I think we should probably split IsMVCCSnapshot
into IsMVCCSnapshot(), just accepting SNAPSHOT_MVCC, and IsMVCCLikeSnapshot()
accepting both SNAPSHOT_MVCC and SNAPSHOT_HISTORIC_MVCC. And then go through
all the existing callers of IsMVCCSnapshot() - only about half should stay
as-is, I think.

> Yes, with this change, I don't hit the problem anymore.

Great!

Greetings,

Andres Freund

---

From: Antonin Houska <[email-masked]>
Date: Thu, 12 Feb 2026 11:36:08 +0100

> all the existing callers of IsMVCCSnapshot() - only about half should stay

The attached patch tries to do that.

>From dcdbaf3095e632a1f7f65f3abc43eccff0249d4c Mon Sep 17 00:00:00 2001
From: Antonin Houska <[email-masked]>
Date: Thu, 12 Feb 2026 11:14:00 +0100
Subject: [PATCH] Refine checking of snapshot type.

It appears to be confusing if IsMVCCSnapshot() evaluates to true for both
"regular" and "historic" MVCC snapshot. This patch restricts the meaning of
the macro to the "regular" MVCC snapshot, and introduces a new macro
IsMVCCLikeSnapshot() to recognize both types.

IsMVCCLikeSnapshot() is only used in functions that can (supposedly) be called
during logical decoding.
---
 src/backend/access/heap/heapam_handler.c | 2 +-
 src/backend/access/index/indexam.c       | 2 +-
 src/backend/access/nbtree/nbtree.c       | 2 +-
 src/include/utils/snapmgr.h              | 6 ++++--
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index cbef73e5d4b..332c788bab2 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -159,7 +159,7 @@ heapam_index_fetch_tuple(struct IndexFetchTableData *scan,
 		 * Only in a non-MVCC snapshot can more than one member of the HOT
 		 * chain be visible.
 		 */
-		*call_again = !IsMVCCSnapshot(snapshot);
+		*call_again = !IsMVCCLikeSnapshot(snapshot);
 
 		slot->tts_tableOid = RelationGetRelid(scan->rel);
 		ExecStoreBufferHeapTuple(&bslot->base.tupdata, slot, hscan->xs_cbuf);
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 4ed0508c605..80623350d6f 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -445,7 +445,7 @@ index_markpos(IndexScanDesc scan)
 void
 index_restrpos(IndexScanDesc scan)
 {
-	Assert(IsMVCCSnapshot(scan->xs_snapshot));
+	Assert(IsMVCCLikeSnapshot(scan->xs_snapshot));
 
 	SCAN_CHECKS;
 	CHECK_SCAN_PROCEDURE(amrestrpos);
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 3dec1ee657d..07ba5997fbc 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -422,7 +422,7 @@ btrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
 	 * Note: so->dropPin should never change across rescans.
 	 */
 	so->dropPin = (!scan->xs_want_itup &&
-				   IsMVCCSnapshot(scan->xs_snapshot) &&
+				   IsMVCCLikeSnapshot(scan->xs_snapshot) &&
 				   RelationNeedsWAL(scan->indexRelation) &&
 				   scan->heapRelation != NULL);
 
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index b8c01a291a1..dd5aaae6953 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -53,12 +53,14 @@ extern PGDLLIMPORT SnapshotData SnapshotToastData;
 
 /* This macro encodes the knowledge of which snapshots are MVCC-safe */
 #define IsMVCCSnapshot(snapshot)  \
-	((snapshot)->snapshot_type == SNAPSHOT_MVCC || \
-	 (snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC)
+	((snapshot)->snapshot_type == SNAPSHOT_MVCC)
 
 #define IsHistoricMVCCSnapshot(snapshot)  \
 	((snapshot)->snapshot_type == SNAPSHOT_HISTORIC_MVCC)
 
+#define IsMVCCLikeSnapshot(snapshot)  \
+	(IsMVCCSnapshot(snapshot) || IsHistoricMVCCSnapshot(snapshot))
+
 extern Snapshot GetTransactionSnapshot(void);
 extern Snapshot GetLatestSnapshot(void);
 extern void SnapshotSetCommandId(CommandId curcid);
-- 
2.47.3

---

From: Andres Freund <[email-masked]>
Date: Wed, 11 Mar 2026 19:09:26 -0400

Hi,

> The attached patch tries to do that.

Thanks!

> IsMVCCLikeSnapshot() is only used in functions that can (supposedly) be called

I think I agree with where you selected IsMVCCSnapshot() and where you
selected IsMVCCLikeSnapshot().

>  extern void SnapshotSetCommandId(CommandId curcid);

Probably need to update the comments a bit.  What about something like

/*
 * Is the snapshot implemented as an MVCC snapshot (i.e. it uses
 * SNAPSHOT_MVCC).  If so, there will be at most be one visible row in a chain
 * of updated tuples, and each visible tuple will be seen exactly once.
 */
#define IsMVCCSnapshot(snapshot)  \
...

/*
 * Is the snapshot either an MVCC snapshot or has equivalent visibility
 * semantics (see IsMVCCSnapshot()).
 */
#define IsMVCCLikeSnapshot(snapshot)  \

Greetings,

Andres Freund