From: Antonin Houska <ah@cybertec.at>
To: alvherre@kurilemu.de
cc: Andres Freund <andres@anarazel.de>,
    pgsql-hackers@lists.postgresql.org,
    Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Subject: Re: Race conditions in logical decoding
In-reply-to: <aoiRAEAAzDnXfkDN@alvherre.pgsql>
References: <aoiRAEAAzDnXfkDN@alvherre.pgsql>
Comments: In-reply-to =?us-ascii?Q?=3D=3Futf-8=3FQ=3F=3DC3=3D81lvaro=3F=3D?=
 Herrera <alvherre@kurilemu.de>
   message dated "Fri, 21 Aug 2026 20:16:02 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Tue, 25 Aug 2026 19:25:17 +0200
Message-ID: <36316.1787678717@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/36316.1787678717%40localhost>
Precedence: bulk

=C3=81lvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2026-Mar-20, =C3=81lvaro Herrera wrote:
>=20
> > Failing other ideas, I think we should just go with 0001.  We'd need mo=
re
> > commentary on why is TransactionIdDidCommit() OK, when we haven't
> > scanned PGPROC for that xid, though.
>=20
> 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.
>=20
>=20
> 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.
>=20
> So, what do you think of the attached?

I appreciate it that you performed the tests. I considered the race conditi=
on
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 th=
at.

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

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