Re: HOT chain validation in verify_heapam()

Hi,

On 2022-11-30 16:09:19 +0530, Himanshu Upadhyaya wrote:
> has been updated to handle cases of sub-transaction.

Thanks!


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


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


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



Greetings,

Andres Freund