On 2020-Nov-23, Tom Lane wrote: > Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> > GetCurrentVirtualXIDs, ComputeXidHorizons, GetSnapshotData: > > > In these cases, what we need is that the code computes some xmin (or > > equivalent computation) based on a set of transactions that exclude > > those marked with the flags. The behavior we want is that if some > > transaction is marked as vacuum, we ignore the Xid/Xmin *if there is > > one*. In other words, if there's no Xid/Xmin, then the flag is not > > important. So if we can ensure that the flag is set first, and the > > Xid/xmin is installed later, that's sufficient correctness and we don't > > need to hold exclusive lock. But if we can't ensure that, then we must > > use exclusive lock, because otherwise we risk another process seeing our > > Xid first and not our flag, which would be bad. > > I don't buy this either. You get the same result if someone looks just > before you take the ProcArrayLock to set the flag. So if there's a > problem, it's inherent in the way that the flags are defined or used; > the strength of lock used in this stanza won't affect it. The problem is that the writes could be reordered in a way that makes the Xid appear set to an onlooker before PROC_IN_VACUUM appears set. Vacuum always sets the bit first, and *then* the xid. If the reader always reads it like that then it's not a problem. But in order to guarantee that, we would have to have a read barrier for each pass through the loop. With the LW_EXCLUSIVE lock, we block the readers so that the bit is known set by the time they examine it. As I understand, getting the lock is itself a barrier, so there's no danger that we'll set the bit and they won't see it. ... at least, that how I *imagine* the argument to be. In practice, vacuum_rel() calls GetSnapshotData() before installing the PROC_IN_VACUUM bit, and therefore there *is* a risk that reader 1 will get MyProc->xmin included in their snapshot (because bit not yet set), and reader 2 won't. If my understanding is correct, then we should move the PushActiveSnapshot(GetTransactionSnapshot()) call to after we have the PROC_IN_VACUUM bit set.