Hi,
On 2022-11-20 11:58:12 -0800, Peter Geoghegan wrote:
> There is code in heap_prepare_freeze_tuple() that treats a raw xmax as
> "xmax_already_frozen = true", even when the raw xmax value isn't
> already set to InvalidTransactionId. I'm referring to this code:
>
> if ( ... ) // process raw xmax
> ....
> else if (TransactionIdIsNormal(xid))
> ....
> else if ((tuple->t_infomask & HEAP_XMAX_INVALID) ||
> !TransactionIdIsValid(HeapTupleHeaderGetRawXmax(tuple)))
> {
> freeze_xmax = false;
> xmax_already_frozen = true;
> /* No need for relfrozenxid_out handling for already-frozen xmax */
> }
> else
> ereport(ERROR,
> (errcode(ERRCODE_DATA_CORRUPTED),
> errmsg_internal("found xmax %u (infomask 0x%04x) not
> frozen, not multi, not normal",
> xid, tuple->t_infomask)));
>
> Why should it be okay to not process xmax during this call (by setting
> "xmax_already_frozen = true"), just because HEAP_XMAX_INVALID happens
> to be set? Isn't HEAP_XMAX_INVALID purely a hint? (HEAP_XMIN_FROZEN is
> *not* a hint, but we're dealing with xmax here.)
Hm. But to get to that point we already need to have decided that xmax
is not a normal xid. Unhelpfully we reuse the 'xid' variable for xmax as
well:
xid = HeapTupleHeaderGetRawXmax(tuple);
I don't really know the HEAP_XMAX_INVALID branch is trying to do. For
one, xid already is set to HeapTupleHeaderGetRawXmax(), why is it
refetching the value?
So it looks to me like this path should just test !TransactionIdIsValid(xid)?
> It seems to me that there should be clear rules on what
> special transaction IDs can appear in xmax. Namely: the only special
> transaction ID that can ever appear in xmax is InvalidTransactionId.
> (Also, it's not okay to see *any* other XID in the
> "xmax_already_frozen = true" path, nor would it be okay to leave any
> other XID behind in xmax in the nearby "freeze_xmax = true" path.)
Yea.
Greetings,
Andres Freund
