Hi,
This thread is referenced an open item, and we ought to make some
progress on it.
On a more cosmetic note:
On 2019-04-16 09:10:19 -0700, Andres Freund wrote:
> On 2019-04-16 12:01:36 -0400, Tom Lane wrote:
> > (BTW, I don't understand why that code will throw "found xmin %u from
> > before relfrozenxid %u" if HeapTupleHeaderXminFrozen is true? Shouldn't
> > the whole if-branch at lines 6113ff be skipped if xmin_frozen?)
>
> I *think* that just looks odd, but isn't actively wrong. That's because
> TransactionIdIsNormal() won't trigger, as:
>
> #define HeapTupleHeaderGetXmin(tup) \
> ( \
> HeapTupleHeaderXminFrozen(tup) ? \
> FrozenTransactionId : HeapTupleHeaderGetRawXmin(tup) \
> )
>
> which afaict makes
> xmin_frozen = ((xid == FrozenTransactionId) ||
> HeapTupleHeaderXminFrozen(tuple));
> redundant.
>
> Looks like that was introduced relatively recently, in
>
> commit d2599ecfcc74fea9fad1720a70210a740c716730
> Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date: 2018-05-04 15:24:44 -0300
>
> Don't mark pages all-visible spuriously
>
>
> @@ -6814,6 +6815,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
>
> /* Process xmin */
> xid = HeapTupleHeaderGetXmin(tuple);
> + xmin_frozen = ((xid == FrozenTransactionId) ||
> + HeapTupleHeaderXminFrozen(tuple));
> if (TransactionIdIsNormal(xid))
> {
> if (TransactionIdPrecedes(xid, relfrozenxid))
> @@ -6832,9 +6835,8 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple,
>
> frz->t_infomask |= HEAP_XMIN_FROZEN;
> changed = true;
> + xmin_frozen = true;
> }
> - else
> - totally_frozen = false;
> }
Alvaro, could we perhaps clean this up a bit? This is pretty confusing
looking. I think this probably could just be changed to
bool xmin_frozen = false;
xid = HeapTupleHeaderGetXmin(tuple);
if (xid == FrozenTransactionId)
xmin_frozen = true;
else if (TransactionIdIsNormal(xid))
or somesuch. There's no need to check for
HeapTupleHeaderXminFrozen(tuple) etc, because HeapTupleHeaderGetXmin()
already does so - and if it didn't, the issue Tom points out would be
problematic.
Greetings,
Andres Freund