From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Himanshu Upadhyaya <upadhyaya(dot)himanshu(at)gmail(dot)com> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: HOT chain validation in verify_heapam() |
Date: | 2022-12-02 00:13:21 |
Message-ID: | 20221202001321.igy24zwq7myyfi4o@awork3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Nathan Bossart | 2022-12-02 00:21:30 | Re: wake up logical workers after ALTER SUBSCRIPTION |
Previous Message | Thomas Munro | 2022-12-02 00:02:36 | Using AF_UNIX sockets always for tests on Windows |