Re: remove spurious CREATE INDEX CONCURRENTLY wait

On 2020-Nov-23, Tom Lane wrote:

> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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.