Re: Race conditions in logical decoding

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)