Alvaro Herrera <alvhe...@alvh.no-ip.org> writes:
> PROC_IN_LOGICAL_DECODING:
> Oddly enough, I think the reset of PROC_IN_LOGICAL_DECODING in
> ReplicationSlotRelease might be the most problematic one of the lot.
> That's because a proc's xmin that had been ignored all along by
> ComputeXidHorizons, will now be included in the computation. Adding
> asserts that proc->xmin and proc->xid are InvalidXid by the time we
> reset the flag, I got hits in pg_basebackup, test_decoding and
> subscription tests. I think it's OK for ComputeXidHorizons (since it
> just means that a vacuum that reads a later will remove less rows.) But
> in GetSnapshotData it is just not correct to have the Xmin go backwards.
> Therefore it seems to me that this code has a bug independently of the
> lock level used.
That is only a bug if the flags are *cleared* in a way that's not
atomic with clearing the transaction's xid/xmin, no? I agree that
once set, the flag had better stay set till transaction end, but
that's not what's at stake here.
> 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.
regards, tom lane
