Re: Fix for consume_xids advancing XIDs incorrectly

On Wed, Oct 30, 2024 at 12:00 AM Yushi Ogiwara
<btogiwarayuushi(at)oss(dot)nttdata(dot)com> wrote:
>
> I made a new patch (skip_xid_correctly.diff) that incorporates the
> points we discussed:
>
> 1. Fix the issue that consume_xids consumes nxids+1 XIDs.
> 2. Update lastxid when calling GetTopFullTransactionId() to support
> nxids==1 case.
> 3. Forbid consume_xids when nxids==0.
> 4. Add comments explaining the return values of consume_xids and
> consume_xids_until, and the rationale for incrementing consumed++ when
> GetTopFullTransactionId() is called.
>
> Also, I made another patch (support_blksz_32k.diff) that supports the
> block size == 32K case.
>

Thank you for making patches! Here are review comments.

skip_xid_correctly.diff:

- if (nxids < 0)
+ if (nxids <= 0)
elog(ERROR, "invalid nxids argument: %lld", (long long) nxids);
-
- if (nxids == 0)
- lastxid = ReadNextFullTransactionId();
else
lastxid = consume_xids_common(InvalidFullTransactionId, (uint64) nxids);

After calling elog(ERROR) we don't return here, so the "else" is not
necessary. That is, we can rewrite it to:

if (nxids <= 0)
elog(ERROR, "invalid nxids argument: %lld", (long long) nxids);

lastxid = consume_xids_common(InvalidFullTransactionId, (uint64) nxids);

---
+ *
+ * If no top-level XID is assigned, a new one is obtained,
+ * and the consumed XID counter is incremented.

I'm not sure this comment is appropriate since what we do here is
obvious and the comment doesn't explain why we do that. IMO we don't
need to update these comments.

support_blksz_32k.diff:

- if (xids_left > 2000 &&
+ if (xids_left > COMMIT_TS_XACTS_PER_PAGE &&

I think it's better to just replace 2000 with 4000 and explain why
4000 is sufficient. Probably we can define '#define
XID_SHORTCUT_THRESHOLD 4000" and put an assertion to XidSkip() or
consume_xids_common() to check if the number of consumed XIDs doesn't
exceed XID_SHORTCUT_THRESHOLD.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com