Re: HOT chain validation in verify_heapam()

Hi,


On Fri, Dec 2, 2022 at 5:43 AM Andres Freund <andres@anarazel.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