Feedback-ID: ie3de48e3:Fastmail
Date: Mon, 21 Sep 2026 17:35:41 +0200
From: Alvaro Herrera <alvherre@kurilemu.de>
To: Rui Zhao <zhaorui126@gmail.com>
Cc: Antonin Houska <ah@cybertec.at>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers@lists.postgresql.org,
 Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Subject: Re: Race conditions in logical decoding
Message-ID: <arFHEuyAnw3S0FAl@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAHWVJhHwV4g-QCWDMaXPT8m-1hQ5=ZpwmNQDD3JKqmLvo7-8nA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/arFHEuyAnw3S0FAl%40alvherre.pgsql>
Precedence: bulk

On 2026-Sep-20, Rui Zhao wrote:

> On 2026-Sep-18 at 14:29 UTC, Alvaro Herrera wrote:
> > This does pass the two tests that Rui wrote, also attached.
> 
> In v5-0001, newxcnt++ needs to stay in the test == NULL branch.
> Otherwise committed XIDs increase the count without filling an entry
> in newxip.

Eh, yeah.

> On 2026-Sep-17 at 08:37 UTC, Antonin Houska wrote:
> > I don't understand why you check all transactions in procarray, instead of
> > only those in snap->xip.
> 
> I first tried calling XactLockTableWait() for every XID in snap->xip,
> the same per-XID waiting approach as v5. Even for an already finished
> XID, that goes through the lock manager and calls
> TransactionIdIsInProgress(). Unless its RecentXmin or cached-XID
> checks suffice, that takes ProcArrayLock and scans procarray.
> 
> In the patch attached to my original mail, I instead read the
> running-XID list once and used bsearch to wait only for XIDs also in
> snap->xip. That was to avoid repeating this work for transactions
> that had already finished.

Yeah, maybe this approach isn't great after all.  We could turn that
around and search for each loop around the snap->xmin..snap->xmax loop
that is found in snap->xip in the running->xids array.  That reduces the
number of times we go through XactLockTableWait() to only running
transactions (same as in Rui's original patch [1]).  However, the
running->xids array is not sorted, so we would have to qsort() it, or do
a plain array walk for each element.  In the end, I think the code in
your (Rui's) first patch is the simplest approach.

It's possible that there's a slight performance difference between
scanning the running->xids array with bsearch() on snap->xip, versus
scanning the snap->xip array with bsearch on running->xids.  However,
given the amount of code involved in the XactLockTableWait() that we
have to do on each item we find still running, I expect the difference
to be negligible.  And doing it certainly beats ending up with corrupt
data anyway.  So I'm going to take the code mostly from Rui's original
patch[1].

[1] https://postgr.es/m/CAHWVJhHXyLtS-8mdL9WhEWfsERb=FN7JdPD0GYAXgTmCnqbYGw@mail.gmail.com

However, the situation with comments is not completely settled for me.
I asked:

> On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> > I don't understand [this comment]:
> >
> >  * A subtransaction is covered by its top-level transaction, which is in
> >  * snap->xip as well, or was purged from it because it is below xmin and
> >  * thus finished long ago.

and you said:

> The first was meant to explain why we don't have to find every
> subxid in the running-XID list. If any backend's subxid cache has
> overflowed, GetRunningTransactionData() returns top-level XIDs but no
> subxids. We still wait for the parent, which covers its children.

However, the code scans running->xids with a limit of

+       nrunning = running->xcnt + running->subxcnt;

which means we scan both main Xids as well as subxids, which seems to
contradict what you said.  I think we should just go up to running->xcnt
only; if any subxids are in there, we can ignore that, because we'd
still do the XactLockTableWait with the parent xact.  (We know, by
construction, that the array has the top-level XIDs first, followed by
subxids.  This doesn't seem documented anywhere though.  Perhaps if this
is ever broken, SnapBuildWaitSnapshot would be trouble.  Maybe worth
adding a comment somewhere.)

I also asked:

> On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> > I don't understand [this other comment]:
> >
> >  * 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.

and you replied:

> The second was a different question: why wait only in
> SnapBuildInitialSnapshot(), rather than in SnapBuildBuildSnapshot(),
> which is also used to build historic snapshots? Here "this" meant
> waiting for transactions to finish, not handling subtransactions.
> 
> Historic snapshots use xip for committed-XID checks in [xmin, xmax).
> They can consult CLOG below xmin, but those transactions had already
> finished when the running-xacts record supplying xmin was written.
> So they need no extra wait.

Ah, I see.  It makes sense when explained like that, but I find it
difficult to understand in the broader context of the comment being
added.  I don't disagree that this is worth commenting about, but I'm
not sure this is the best place to do it.  Rather, maybe we should add
something in SnapBuildBuildSnapshot() to explain why we don't do this
there.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/