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=fn7jdpd0gyaxgtmcnqb...@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/
