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