Re: HOT chain validation in verify_heapam()

On Fri, Dec 2, 2022 at 5:43 AM Andres Freund <andres@anarazel.de> wrote:

>
> > +             /* Loop over offsets and validate the data in the
> predecessor array. */
> > +             for (OffsetNumber currentoffnum = FirstOffsetNumber;
> currentoffnum <= maxoff;
> > +                      currentoffnum = OffsetNumberNext(currentoffnum))
> > +             {
> > +                     HeapTupleHeader pred_htup;
> > +                     HeapTupleHeader curr_htup;
> > +                     TransactionId pred_xmin;
> > +                     TransactionId curr_xmin;
> > +                     ItemId          pred_lp;
> > +                     ItemId          curr_lp;
> > +                     bool            pred_in_progress;
> > +                     XidCommitStatus xid_commit_status;
> > +                     XidBoundsViolation xid_status;
> > +
> > +                     ctx.offnum = predecessor[currentoffnum];
> > +                     ctx.attnum = -1;
> > +                     curr_lp = PageGetItemId(ctx.page, currentoffnum);
> > +                     if (!lp_valid[currentoffnum] ||
> ItemIdIsRedirected(curr_lp))
> > +                             continue;
>
> I don't think we should do PageGetItemId(ctx.page, currentoffnum); if
> !lp_valid[currentoffnum].
>
> Fixed.

>
> > +                     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.
>
> changed all relevant places to use  InvalidOffsetNumber.

>
> > +                     {
> > +                             /*
> > +                              * No harm in overriding value of
> ctx.offnum as we will always
> > +                              * continue if we are here.
> > +                              */
> > +                             ctx.offnum = currentoffnum;
> > +                             if (TransactionIdIsInProgress(curr_xmin)
> || TransactionIdDidCommit(curr_xmin))
>
> Is it actually ok to call TransactionIdDidCommit() here? There's a reason
> get_xid_status() exists after all. And we do have the xid status for xmin
> already, so this could just check xid_commit_status, no?
>
>
> I think it will be good to pass NULL to get_xid_status like
"get_xid_status(curr_xmin, &ctx, NULL);" so that we can only check the xid
status at the time when it is actually required. This way we can avoid
checking xid status in cases when we simply 'continue' due to some check.


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