Re: pgsql: Fix traversal of half-frozen update chains

On Fri, Oct 6, 2017 at 8:29 AM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Fix traversal of half-frozen update chains

I have a question about this (this is taken from the master branch):

> bool
> HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
> {
> TransactionId xmin = HeapTupleHeaderGetXmin(htup);
>
> /*
> * If the xmax of the old tuple is identical to the xmin of the new one,
> * it's a match.
> */
> if (TransactionIdEquals(xmax, xmin))
> return true;
>
> /*
> * If the Xmin that was in effect prior to a freeze matches the Xmax,
> * it's good too.
> */
> if (HeapTupleHeaderXminFrozen(htup) &&
> TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax))
> return true;
>
> /*
> * When a tuple is frozen, the original Xmin is lost, but we know it's a
> * committed transaction. So unless the Xmax is InvalidXid, we don't know
> * for certain that there is a match, but there may be one; and we must
> * return true so that a HOT chain that is half-frozen can be walked
> * correctly.
> *
> * We no longer freeze tuples this way, but we must keep this in order to
> * interpret pre-pg_upgrade pages correctly.
> */
> if (TransactionIdEquals(xmin, FrozenTransactionId) &&
> TransactionIdIsValid(xmax))
> return true;
>
> return false;
> }

Wouldn't this last "if" test, to cover the pg_upgrade case, be better
targeted by comparing *raw* xmin to FrozenTransactionId? You're using
the potentially distinct xmin value returned by
HeapTupleHeaderGetXmin() for the test here. I think we should be
directly targeting tuples frozen on or before 9.4 (prior to
pg_upgrade) instead.

Have I missed something?

--
Peter Geoghegan