Re: HOT chain validation in verify_heapam()

Hi,

On Fri, Dec 2, 2022 at 5:43 AM Andres Freund <andres(at)anarazel(dot)de> wrote:

>
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmin = HeapTupleHeaderGetXmin(curr_htup);
> > + xid_status = get_xid_status(curr_xmin, &ctx,
> &xid_commit_status);
> > + if (!(xid_status == XID_BOUNDS_OK || xid_status ==
> XID_INVALID))
> > + continue;
>
> Why can we even get here if the xid status isn't XID_BOUNDS_OK?
>
>

@@ -504,9 +516,269 @@ verify_heapam(PG_FUNCTION_ARGS)
/* It should be safe to examine the tuple's header,
at least */
ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page,
ctx.itemid);
ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
+ lp_valid[ctx.offnum] = true;

/* Ok, ready to check this next tuple */
check_tuple(&ctx);

referring above code, check_tuple(&ctx); do have this check but we populate
lp_valid before that call.
Populating lp_valid before check_tuple() is intentional because even if we
do changes to get the return status from check_tuple() to populate that in
lp_valid, it will be hard to validate cases that are dependent on aborted
transaction (like "tuple with aborted xmin %u was updated to produce a
tuple at offset %u with committed xmin %u") because check_tuple_visibility
is also looking for aborted xmin and return false if tuple's xmin is
aborted, in fact we can add one more parameter to check_tuple and get the
status of transaction if it is aborted and accordingly set lp_valid to true
but that will add unnecessary complexity and don't find it convincing
implementation. Alternatively, I found rechecking xid_status is simpler and
straight.

>
> > + if (ctx.offnum == 0)
>
> For one, I think it'd be better to use InvalidOffsetNumber here. But more
> generally, storing the predecessor in ctx.offnum seems quite confusing.
>
> ok, I will change it to InvalidOffsetNumber at all the places, we need
ctx.offnum to have the value of the predecessor array as this will be
internally used by report_corruption function to generate the message(eg.
below), and the format of these message's seems more simple and meaningful
to report corruption.

report_corruption(&ctx,

psprintf("heap-only update produced a non-heap only tuple at offset %u",

(unsigned) currentoffnum));
Here we don't need to mention ctx.offnum explicitly in the above message as
this will be taken care of by the code below.

"report_corruption_internal(Tuplestorestate *tupstore, TupleDesc tupdesc,
BlockNumber blkno,
OffsetNumber offnum,
AttrNumber attnum, char
*msg)
{
Datum values[HEAPCHECK_RELATION_COLS] = {0};
bool nulls[HEAPCHECK_RELATION_COLS] = {0};
HeapTuple tuple;

values[0] = Int64GetDatum(blkno);
values[1] = Int32GetDatum(offnum);"

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com