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)
>From 45252d5989dc0e06e0027142a1bfd4cfd744e48e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <[email protected]> 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 <[email protected]> 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
