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
v5-0004-Fix-XID-count-in-initial-decoding-snapshots.patch
Description: Binary data
