agora inbox for pgsql-hackers@postgresql.org  

help / color / mirror / Atom feed

From: Antonin Houska <ah@cybertec.at>
To: alvherre@kurilemu.de
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Subject: Re: Race conditions in logical decoding
Date: Tue, 25 Aug 2026 19:25:17 +0200
Message-ID: <36316.1787678717@localhost> (raw)
In-Reply-To: <aoiRAEAAzDnXfkDN@alvherre.pgsql>
References: <aoiRAEAAzDnXfkDN@alvherre.pgsql>

Álvaro Herrera <alvherre@kurilemu.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

view thread (38+ messages)  latest in thread

Message-ID: <36316.1787678717@localhost>
Permalink:  ../36316.1787678717@localhost/
Also on:    postgresql.org/message-id/36316.1787678717@localhost

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: ah@cybertec.at, alvherre@kurilemu.de, andres@anarazel.de, mihailnikalayeu@gmail.com
  Subject: Re: Race conditions in logical decoding
  In-Reply-To: <36316.1787678717@localhost>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox