Feedback-ID: ie3de48e3:Fastmail
Date: Fri, 21 Aug 2026 20:16:02 +0200
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de>
To: Andres Freund <andres@anarazel.de>
Cc: Antonin Houska <ah@cybertec.at>, pgsql-hackers@lists.postgresql.org,
	Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Subject: Re: Race conditions in logical decoding
Message-ID: <aoiRAEAAzDnXfkDN@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ic4mjwhsvm7ysii6"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202603201543.t6gxppyyk66p@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/aoiRAEAAzDnXfkDN%40alvherre.pgsql>
Precedence: bulk


--ic4mjwhsvm7ysii6
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2026-Mar-20, Álvaro Herrera wrote:

> Failing other ideas, I think we should just go with 0001.  We'd need more
> commentary on why is TransactionIdDidCommit() OK, when we haven't
> scanned PGPROC for that xid, though.

I spent some more time stepping through the motions here.  In the test I
saw, the problem is caused by the check for latestCompletedXid.  The
transaction we saw as committed in WAL has not yet been removed from
ProcArray, which is what updates latestCompletedXid.  So that makes
TransactionIdIsInProgress() report that yes, the transaction is in
progress, therefore we continue to wait in a loop forever, at least in
synchronous replication.

To recap: the problem was that returned a snapshot with a transaction
recorded as committed, but which was not yet marked as such in CLOG, so
when we did things like HeapTupleSatisfiesMVCC() with the snapshot so
obtained, it would run TransactionIdDidCommit(), get false from it, and
conclude that the transaction "must have aborted or crashed", therefore
marking the tuple as HEAP_XMIN_INVALID.  So what we do here is ensure
that TransactionIdDidCommit() will return the correct value before
giving the snapshot back.


The other problem with this patch in the back of my mind was that we may
be doing TransactionIdDidCommit() potentially for a lot of transactions.
Instrumenting these code paths I saw that some tests in the suite would
call the transam.c routine several thousand times, and some XIDs would
repeat over and over.  This may not sound like much, but we don't
actually know what happens in production systems; and every transam.c
call has the potential to do I/O to get the relevant CLOG page.  And
because we do this snapshot building in places like
SnapBuildProcessChange(), it has the potential to do nasty.  So I added
a quick and dirty process-local cache: the list of transactions we
tested on the previous cycle.  We don't test nor wait for any
transaction that's on that list, since evidently we must have tested it
already and it cannot become uncommitted after that.  All in all, we
test for each potentially in-progress transaction just once per backend.

So, what do you think of the attached?


(On second thought, it may be a good idea to plant some of my
explanation above in the new comment in SnapBuildBuildSnapshot.  No time
for that right now though.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

--ic4mjwhsvm7ysii6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename=v3-0001-Fix-race-conditions-during-the-setup-of-logical-d.patch

From 45252d5989dc0e06e0027142a1bfd4cfd744e48e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@kurilemu.de>
Date: Fri, 21 Aug 2026 19:52:05 +0200
Subject: [PATCH v3] Fix race conditions during the setup of logical decoding.

Although it's rather unlikely, it can happen that the snapshot builder
considers transaction committed (according to WAL) before the commit could be
recorded in CLOG. In an extreme case, snapshot can even be created and used in
between. Since both snapshot and CLOG are needed for visibility checks, this
inconsistency can make them work incorrectly.

The typical symptom is that a transaction that the snapshot considers not
running anymore is (per CLOG) considered aborted instead of committed. Thus a
new tuple version can be evaluated as invisible (if xmin is incorrectly
considered aborted) or a deleted tuple version can be evaluated as visible (if
xmax is incorrectly considered aborted).

This patch fixes the problem by checking if all the XIDs that the new snapshot
considers committed are really committed per CLOG. If at least one is not, the
check is repeated after a short delay. However, a single check is sufficient
in almost all cases, so the performance impact should be minimal.

Author: Antonin Houska <ah@cybertec.at>
Discussion: https://postgr.es/m/85833.1768840165@localhost
---
 src/backend/replication/logical/snapbuild.c   | 74 ++++++++++++++++++-
 .../utils/activity/wait_event_names.txt       |  1 +
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index f60bcf09605..31608c186dc 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -130,6 +130,7 @@
 #include "access/xact.h"
 #include "common/file_utils.h"
 #include "miscadmin.h"
+#include "nodes/pg_list.h"
 #include "pgstat.h"
 #include "replication/logical.h"
 #include "replication/reorderbuffer.h"
@@ -365,6 +366,8 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
 {
 	Snapshot	snapshot;
 	Size		ssize;
+	static List *xids_already_tested = NIL;
+	static List *new_xids_already_tested = NIL;
 
 	Assert(builder->state >= SNAPBUILD_FULL_SNAPSHOT);
 
@@ -378,7 +381,7 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
 
 	/*
 	 * We misuse the original meaning of SnapshotData's xip and subxip fields
-	 * to make the more fitting for our needs.
+	 * to make them more fitting for our needs.
 	 *
 	 * In the 'xip' array we store transactions that have to be treated as
 	 * committed. Since we will only ever look at tuples from transactions
@@ -403,6 +406,75 @@ SnapBuildBuildSnapshot(SnapBuild *builder)
 	snapshot->xmin = builder->xmin;
 	snapshot->xmax = builder->xmax;
 
+	/*
+	 * Although very unlikely, it's possible that a commit WAL record was
+	 * decoded but CLOG is not aware of the commit yet. Should the CLOG update
+	 * be delayed even more, visibility checks that use this snapshot could
+	 * work incorrectly.  Therefore we check the CLOG status here.
+	 *
+	 * We must not do this using TransactionIdIsInProgress()!  The check there
+	 * for latestCompletedXid would wreak havoc because the transaction we're
+	 * interested in may not be out of ProcArray yet, but we must not wait for
+	 * that.  Doing the transam.c check directly is correct, though unusual.
+	 *
+	 * We don't want to repeatedly read the CLOG status for the same
+	 * transaction, and it's easy to keep track of which ones we've already
+	 * checked.  Keep a list of the ones we test on each cycle, and use the
+	 * list from the previous cycle to skip testing them now.
+	 */
+	for (int i = 0; i < builder->committed.xcnt; i++)
+	{
+		for (;;)
+		{
+			if (list_member_xid(xids_already_tested, builder->committed.xip[i]))
+			{
+				MemoryContext oldcxt;
+
+				oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+				new_xids_already_tested = lappend_xid(new_xids_already_tested,
+													  builder->committed.xip[i]);
+				MemoryContextSwitchTo(oldcxt);
+				break;
+			}
+			else if (TransactionIdDidCommit(builder->committed.xip[i]))
+			{
+				MemoryContext oldcxt;
+
+				oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+				new_xids_already_tested = lappend_xid(new_xids_already_tested,
+													  builder->committed.xip[i]);
+				MemoryContextSwitchTo(oldcxt);
+				break;
+			}
+			else
+			{
+				/*
+				 * Note that the other process doesn't know we're waiting for
+				 * them, so nothing is going to signal us out of this latch.
+				 * Therefore use a short timeout.
+				 */
+				(void) WaitLatch(MyLatch,
+								 WL_LATCH_SET | WL_TIMEOUT |
+								 WL_EXIT_ON_PM_DEATH,
+								 2L,
+								 WAIT_EVENT_SNAPBUILD_CLOG);
+				ResetLatch(MyLatch);
+			}
+			CHECK_FOR_INTERRUPTS();
+		}
+	}
+
+	/* Swap these lists for next time */
+	if (xids_already_tested != NIL)
+		list_free(xids_already_tested);
+	if (new_xids_already_tested != NIL)
+	{
+		xids_already_tested = new_xids_already_tested;
+		new_xids_already_tested = NIL;
+	}
+	else
+		xids_already_tested = NIL;
+
 	/* store all transactions to be treated as committed by this snapshot */
 	snapshot->xip =
 		(TransactionId *) ((char *) snapshot + sizeof(SnapshotData));
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index 256b3a3c02e..55a9c8296b5 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -185,6 +185,7 @@ PG_SLEEP	"Waiting due to a call to <function>pg_sleep</function> or a sibling fu
 RECOVERY_APPLY_DELAY	"Waiting to apply WAL during recovery because of a delay setting."
 RECOVERY_RETRIEVE_RETRY_INTERVAL	"Waiting during recovery when WAL data is not available from any source (<filename>pg_wal</filename>, archive or stream)."
 REGISTER_SYNC_REQUEST	"Waiting while sending synchronization requests to the checkpointer, because the request queue is full."
+SNAPBUILD_CLOG	"Waiting for CLOG update before building snapshot."
 SPIN_DELAY	"Waiting while acquiring a contended spinlock."
 VACUUM_DELAY	"Waiting in a cost-based vacuum delay point."
 VACUUM_TRUNCATE	"Waiting to acquire an exclusive lock to truncate off any empty pages at the end of a table vacuumed."
-- 
2.47.3


--ic4mjwhsvm7ysii6--