Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Sun, May 22, 2016 at 12:39 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>> As per your latest patch, you are using ReadNewTransactionId() to get the
>> nextXid which then is used to check if any database's frozenxid is already
>> wrapped. Now, isn't the value of nextXID in your patch same as
>> lastSaneFrozenXid in most cases (I mean there is a small window where some
>> new transaction might have started due to which the value of
>> ShmemVariableCache->nextXid has been advanced)? So isn't relying on
>> lastSaneFrozenXid check sufficient?
> Hmm... So, this code already contains comparison with lastSaneFrozenXid.
> Thus, current code compares against both of lastSaneFrozenXid and myXID. I
> have no comment clarifying why this should be so. In my opinion we can
> just remove myXID with its checks. Git shows that Tom Lane
> committed lastSaneFrozenXid and lastSaneMinMulti checks in addition to
> myXID check in 78db307b.
> Tom, what do you think? Could we remove myXID from vac_truncate_clog()?
Well, I do not want to remove the frozenAlreadyWrapped check. In
principle that can't happen anymore ... but if it did, truncating CLOG
would be a really bad response.
I don't see any good reason for this function to assume that
lastSaneFrozenXid is indistinguishably close to the current latest XID.
That would be introducing a not-very-obvious dependency on how the caller
computes that value, which is the same kind of context dependency that
caused this bug in the first place --- I'm pretty sure that this code was
fine when written, because it couldn't be invoked in a transaction without
an XID.
Checking against ReadNewTransactionId() is actually better than what's
there now, since it's guaranteed to be the upper limit of XIDs in use,
which the transaction's own XID certainly isn't.
So I think Alexander's revised patch is good as-is. Will push in a bit.
Also, I notice another problem in vac_truncate_clog() now that I'm looking
at it: it's expecting that the pg_database datfrozenxid and datminmxid
values will hold still while it's looking at them. Since
vac_update_datfrozenxid updates those values in-place, there's a race
condition against VACUUMs happening in other databases. We should fetch
those values into local variables before doing the various tests inside
the scan loop.
regards, tom lane