Re: Race conditions in logical decoding

Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:

> 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?

I appreciate it that you performed the tests. I considered the race condition
pretty rarely, however it does not imply anything about the cost of the
checks: yes they can be quite frequent.

I'm just thinking if the 'xids_already_tested' variable name is
appropriate. Since you only add XIDs known to be committed, how about
something like 'xids_known_committed'?

Besides, that, it occurred to me that a sorted array might be appropriate
instead of a list, so that bsearch() can be used, but I'm not sure about that.

> (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.)

I think it's worth mentioning at least the synchronous replication problem you
mentioned above, so it's easier to understand why we cannot use
TransactionIdIsInProgress():

--
Antonin Houska
Web: https://www.cybertec-postgresql.com