Re: HOT chain validation in verify_heapam() - Mailing list pgsql-hackers

> +             /* 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].

> +                     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.

> +                     {
> +                             /*
> +                              * 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?