Re: More heap tuple header fixes

Manfred Koizar <mkoi-pg@aon.at> writes:

> +    tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
>      HeapTupleHeaderSetXmin(tup->t_data, GetCurrentTransactionId());
>      HeapTupleHeaderSetCmin(tup->t_data, cid);
>      HeapTupleHeaderSetXmaxInvalid(tup->t_data);
> -    HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId);
> -    tup->t_data->t_infomask &= ~(HEAP_XACT_MASK);
> +    /*
> +     * Do *not* set Cmax!  This would overwrite Cmin.
> +     */
> +    /* HeapTupleHeaderSetCmax(tup->t_data, FirstCommandId); */
>      tup->t_data->t_infomask |= HEAP_XMAX_INVALID;

This sort of thing crystallizes the vague unease I had about those
HeapTupleHeader macros.

I'd recommend redesigning the HeapTupleHeaderSet macros so that they
do not do any setting of t_infomask bits, or even take any conditional
action based on them, but solely Assert() that the bits are already
in the appropriate state to allow storing of the value to be stored.
Then, all uses have to be checked to ensure that t_infomask is coerced
into the right state *before* doing HeapTupleHeaderSetFoo.  Anything
else is subject to order-of-operations mistakes that were not errors
before, and cannot be detected by the macros as now defined.  The
cmax-set-is-not-okay bug illustrated above is a perfect example of
what I'm talking about.

            regards, tom lane