Hi, On Saturday, August 22, 2026 3:16 AM Álvaro Herrera <[email protected]> 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
