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: <aq0sA8aF8q6kTmJM@alvherre.pgsql>
References: <aq0sA8aF8q6kTmJM@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@kurilemu.de>
   message dated "Fri, 18 Sep 2026 14:28:55 +0200."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7387.1789737392.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 18 Sep 2026 15:16:32 +0200
Message-ID: <7388.1789737392@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/7388.1789737392%40localhost>
Precedence: bulk

Alvaro Herrera <alvherre@kurilemu.de> wrote:

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

> > > +		for (int i =3D 0; i < nrunning; i++)
> > > +		{
> > > +			TransactionId running_xid =3D running->xids[i];
> > > +
> > > +			if (bsearch(&running_xid, snap->xip, snap->xcnt,
> > > +						sizeof(TransactionId), xidComparator) !=3D NULL)
> > > +				XactLockTableWait(running_xid, NULL, NULL, XLTW_None);
> > > +		}
> > > +	}
> > =

> > I don't understand why you check all transactions in procarray, instea=
d of
> > only those in snap->xip.
> =

> Hmm, but he does: for all the transactions that are running, only those
> that are found by bsearch() in the snap->xip array are waited for.  Is
> that not what we want?
> =

> I guess we could do it the other way around: iterate for each item on
> snap->xip and search for those in running->xids.  Is that what you
> suggest?
> =

> We don't know offhand which array is largest; it would be better to
> iterate on the smaller one and bsearch the largest.  (Or maybe if both
> are sorted, scan them simultaneously.)  I don't find any reference to
> say that running_xid is sorted.

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 means i=
t
	 * stores committed transactions in ->xip, not ones in progress. Build 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. eac=
h
committed transaction)?

> I don't understand these two paragraphs:
> =

> 	 * A subtransaction is covered by its top-level transaction, which is i=
n
> 	 * snap->xip as well, or was purged from it because it is below xmin an=
d
> 	 * thus finished long ago.

Me neither. AFAIU SnapBuildCommitTxn() adds both top-level transaction and
subtransactions to the builder's array of committed transaction.

> 	 * Historic snapshots do not need this: between xmin and xmax they rely=
 on
> 	 * xip alone, and transactions below xmin had left the procarray by the
> 	 * time the xl_running_xacts record that set xmin was written.

I think this is related to the note that HeapTupleSatisfiesHistoricMVCC() =
does
not really use CLOG in the 3rd paragraph in [1].

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

-- =

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