RE: Race conditions in logical decoding

Hi,

On Saturday, August 22, 2026 3:16 AM Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>
> 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?

Just sharing a few thoughts.

I think the cache might be better placed in the SnapBuild struct (at least on
HEAD) rather than in static variables. As currently written, it persists across
decoding sessions in the same backend - a session could build a snapshot, drop
the slot, and later create a new slot and build another snapshot, potentially
consulting stale entries from the first builder. For example, it has a wraparound
concern: after XID wrap, a cached value could refer to a different transaction,
causing us to skip the CLOG wait and reintroduce the inconsistency this patch
aims to fix.

Besides, just to confirm one note: IIUC, for exported snapshots by logicalrep, a
transaction could be treated as committed while still in PGPROC, while
concurrent MVCC snapshots still see it as in progress which looks inconsistent.
I understand that waiting for ProcArray removal in the general case could
deadlock against synchronous replication, so it's probably acceptable to leave
it unchanged for internal usage in active replication processes.

However, for cases where the snapshot is exported, would it be possible to
additionally wait for it in SnapBuildInitialSnapshot() (which is used only by
CREATE_REPLICATION_SLOT and REPACK)? Since that runs before START_REPLICATION,
the process isn't streaming or feeding any subscriber, so I believe the deadlock
wouldn't occur there. (I think that the walsender executing
CREATE_REPLICATION_SLOT shouldn't be added to sync_standby_names, otherwise
building the initial snapshot itself would already have a deadlock risk via
SnapBuildWaitSnapshot->XactLockTableWait.)

Best Regards,
Zhijie Hou