From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@kurilemu.de>
cc: Rui Zhao <zhaorui126@gmail.com>, 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: <aq1Jy9aVF-lIhKwe@alvherre.pgsql>
References: <aq1Jy9aVF-lIhKwe@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@kurilemu.de>
   message dated "Fri, 18 Sep 2026 16:29:20 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <12833.1789745009.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 18 Sep 2026 17:23:29 +0200
Message-ID: <12834.1789745009@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/12834.1789745009%40localhost>
Precedence: bulk

Alvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2026-Sep-18, Antonin Houska wrote:
> =

> > Maybe I miss the point, but what's wrong about modifying the existing =
loop
> > that inverts the meaning of the ->xip array
> > =

> > 	/*
> > 	 * snapbuild.c builds transactions in an "inverted" manner, which mea=
ns it
> > 	 * stores committed transactions in ->xip, not ones in progress. Buil=
d a
> > 	 * classical snapshot by marking all non-committed transactions as
> > 	 * in-progress. This can be expensive.
> > 	 */
> > 	for (xid =3D snap->xmin; NormalTransactionIdPrecedes(xid, snap->xmax)=
;)
> > 	{
> > 		...
> > 	}
> > =

> > by calling XactLockTableWait() for each XID we find in the array (i.e.=
 each
> > committed transaction)?
> =

> Ah, you mean something like the attached quick POC?  This does pass the
> two tests that Rui wrote, also attached.  (I didn't test Zhijie's, which
> AFAICT is written to pass with the bug and fail without it.)

Yes, I mean checking if those transactions have really ended.

Regarding [1], perhaps the idea is to avoid calling XactLockTableWait() if=
 the
transaction is no longer in procarray. I'm not sure it's a problem to call
that function (possibly many times) for transactions that are no longer
running. (GetRunningTransactionData() is not free either.)

And regarding the deadlock with synchronous replica, my understanding is t=
hat
we avoid it by waiting in SnapBuildInitialSnapshot() instead of in
SnapBuildBuildSnapshot().

[1] https://www.postgresql.org/message-id/CAHWVJhHXyLtS-8mdL9WhEWfsERb%3DF=
N7JdPD0GYAXgTmCnqbYGw%40mail.gmail.com

-- =

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