Re: Low hanging fruit in lazy-XID-assignment patch?

"Florian G. Pflug" <fgp@phlo.org> writes:
> Tom Lane wrote:
>> The lazy-XID patch, as committed, doesn't help that situation at all,

> I think the comment is correct in principle - If we remove the oldest
> xmin without locking, then two concurrent OldestXmin calculations
> will get two different results. The question is if that has any
> negative effects, though.

My point is that there's a difference between what you compute (and
publish) as your own xmin, and what you compute as the RecentGlobalXmin.
I don't think there's any need for a guarantee that two concurrent
processes get the same estimate of RecentGlobalXmin, as long as
they do not get an estimate less than reality, ie, that someone cannot
later compute and publish a smaller xmin.

There are reasons why we want two concurrent GetSnapshotDatas to compute
the same xmin, but I think in the end it just comes down to being a
prerequisite for the above constraint --- without that you're not sure
that someone might not be about to publish an xmin less than what you
obtained as RecentGlobalXmin.

Dropping a live xid is a whole different issue.  There, you have the
problem that you need everyone to see a consistent commit order, which
is what the example in GetSnapshotData is about.  But I don't think that
xmin enters into that.  xmin is only about "is it safe to drop this
tuple because no one can see it?".  There, we don't have to be exactly
correct, we only have to err in the conservative direction.

> It was this comment in GetSnapshotData that made me keep the locking
> in the first place:

>   * It is sufficient to get shared lock on ProcArrayLock, even if we are
>   * computing a serializable snapshot and therefore will be setting
>   * MyProc->xmin. This is because any two backends that have overlapping
>   * shared holds on ProcArrayLock will certainly compute the same xmin

If I recall correctly, that text was written to justify downgrading
GetSnapshotData's hold on ProcArrayLock from exclusive to shared --- it
was merely arguing that the results wouldn't change if we did that.
I don't see an argument there that this condition is really *necessary*.
We do have to think carefully about whether GetOldestXmin can compute a
value that's too large, but right at the moment I see no problem there.

> So I believe you're right, and we can skip taking the lock in the no
> xid case - I actually think with just a little bit of more work, we
> can go even further, and get rid of the ReadNewTransactionId() call
> completely during snapshotting.

[ squint... ]  This goes a bit far for me.  In particular, I think this
will fail in the edge case when there are no live XIDs visible in
ProcArray.  You cannot go back and do ReadNewTransactionId afterward,
at least not without re-scanning the ProcArray a second time, which
makes it at best a questionable win.
        regards, tom lane