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: <202603201543.t6gxppyyk66p@alvherre.pgsql>
References: <202603201543.t6gxppyyk66p@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, 20 Mar 2026 16:55:52 +0100."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Mon, 23 Mar 2026 10:58:00 +0100
Message-ID: <10225.1774259880@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/10225.1774259880%40localhost>
Precedence: bulk

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

> I realized that I hadn't posted the patch version I described somewhere
> downthread.  Here it is, as 0001.
>=20
> While thinking about it for posting just now, I wondered if it would
> work to consider that any transaction whose commit record has been
> decoded but which nevertheless gets a true return from
> TransactionIdIsInProgress(), just is not committed yet and so should be
> omitted from the xip array of the snapshot.  That is, if it's
> still-running, then we just don't copy it into the output snapshot.
> That's implemented as 0002 here.  This seems somehow less controversial,
> as we don't have to test TransactionIdDidCommit() for a transaction that
> we haven't seen as not-running per PGPROC; and it should also be faster,
> because we don't have to wait for anybody to commit.However it gives
> me pause that perhaps the snapshot would not be fully correct.  (Indeed
> there are a few failing tests in the subscription suite).

I recall that in some of the patches for REPACK enhancements (snapshot
switching, MVCC-safety, etc.) for v20 I had a problem with
TransactionIdIsInProgress(). In particular, I tried to add a flag like
PROC_IN_VACUUM, to limit the REPACK's impact on VACUUM xmin horizon.

The problem was that TransactionIdIsInProgress() compares the xid to
RecentXmin before accessing CLOG. IIRC VACUUM's RecentXmin skipped the xid =
of
REPACK and considered its xid not in progress anymore, but since the
transaction wasn't committed yet per CLOG, VACCUM incorrectly considered it
aborted (per HeapTupleSatisfiesVacuumHorizon).

Thus I imagine that with 0002, transaction having PROC_IN_SAFE_IC set might=
 be
added to the snapshot's list of committed transactions although its still in
progress.

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

Mihail already told me that I should consider adding this patch to the CF. I
said that I'm aware of its importance (because REPACK probably exposes the =
bug
more than logical replication does) and that I'll remind you if you happen =
to
forget about it. However I thought that fixes of existing bugs are not subj=
ect
to feature freeze, so I did not bring it up yet.

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