Re: Race conditions in logical decoding

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.

The attached v5-0004 applies on top of v5-0001 through v5-0003. It
puts the increment back with the assignment in 0001 and adds a
snapshot XID check to 0003's test.

With 0003's setup, after slot creation finishes, I ran this on the
connection holding the slot's snapshot:

SELECT pg_current_snapshot();
-- v5: 669:670:0
-- with the correction: 669:670:

The original REPACK and TAP tests pass with v5. The added check
requires each snapshot XID to lie in [xmin, xmax): it fails with v5
and passes with the correction. Both original tests still pass.

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.

On 2026-Sep-18 at 15:23 UTC, Antonin Houska wrote:
> I'm not sure it's a problem to call that function (possibly many times)
> for transactions that are no longer running.

The case I had in mind was a long-running transaction holding xmin
back while many other transactions commit. snap->xip can then be
much larger than the running set. GetSnapshotData() uses the same
ProcArrayLock and array, so my concern was the extra traffic on
shared state used for taking snapshots, not just slot creation time.

On 2026-Sep-12 at 17:35 UTC, I wrote:
> The two callers, CREATE_REPLICATION_SLOT before START_REPLICATION and
> the REPACK worker, stream to nobody, so the synchronous replication
> deadlock of the streaming walsender does not apply to them.

On 2026-Sep-18 at 15:23 UTC, Antonin Houska wrote:
> And regarding the deadlock with synchronous replica, my understanding is
> that we avoid it by waiting in SnapBuildInitialSnapshot() instead of in
> SnapBuildBuildSnapshot().

Yes, that's right. It is the placement of the wait, not the
procarray filtering, that avoids that deadlock.

On 2026-Sep-18 at 12:28 UTC, Alvaro Herrera wrote:
> I don't understand these two paragraphs:
>
> * 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.
> *
> * 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.

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.

If the parent was purged from snap->xip because it is below xmin,
it has already finished, so no wait is needed.

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.

The wait is needed when converting to a normal MVCC snapshot, whose
visibility checks can consult CLOG for transactions in [xmin, xmax)
as well. That is why I put it in SnapBuildInitialSnapshot().

Regards,
Rui