Hi Fujii-san, Thank you very much for the review and for the suggested comment.
You are right: if the transaction already has a top-level XID and the next XID is already at least the target, consume_xids_until() returns the next XID without assigning it, so the old comment was not accurate. The attached v3 updates the comment of consume_xids_until() as you suggested. This is the only change from v2: /* * Advance XID allocation so that the next XID is at least the target. * This may consume XIDs beyond the target. * * Ensures that the current transaction has a top-level XID. * Returns the last XID assigned by this call, or the next XID observed * if no XID was assigned. */ The code is unchanged. I applied v3 to current master (8d87367b39e), built it with assertions, and compared it with the module from master, each call in a new transaction: - consume_xids(1), next XID 666: v3 returns 666. With the master module (next XID 682) it returns 683, so two XIDs were consumed. - consume_xids(10), next XID 668: v3 returns 677 (10 XIDs). Master (next XID 685) returns 695 (11 XIDs). - consume_xids(0): v3 raises "invalid nxids argument: 0"; master returns an XID. - The case from your mail, consume_xids_until(pg_current_xact_id()) in a transaction that already has XID 678: it returns 679, the next XID. Master behaves the same, which matches the new comment. I did not run the xid_wraparound TAP tests (PG_TEST_EXTRA) for this version. Thanks, João Em qua., 11 de nov. de 2026 às 05:52, Fujii Masao <[email protected]> escreveu: > > On Wed, Nov 11, 2026 at 7:53 AM João Rito <[email protected]> wrote: > > Following Michael's request in January, I picked this one up since the > > thread has been idle for a while. I rebased the work onto current master > > and addressed the review comments Sawada-san posted on 2024-10-31. > > Thanks for updating the patch! LGTM, except for one comment. > > > I kept the "Returns the last XID assigned by this function" comment on > > consume_xids() and consume_xids_until(), as Fujii-san asked. > > If the transaction already has a top-level XID and the next XID is > at least the target, consume_xids_until() returns that next XID > without assigning it. So isn't it better to update the comment for > consume_xids_until(), for example, as follows? > > /* > * Advance XID allocation so that the next XID is at least the target. > * This may consume XIDs beyond the target. > * > * Ensures that the current transaction has a top-level XID. > * Returns the last XID assigned by this call, or the next XID observed > * if no XID was assigned. > */ > > Regards, > > -- > Fujii Masao
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Rito?= <[email protected]> Date: Tue, 10 Nov 2026 17:31:18 -0300 Subject: [PATCH v3] xid_wraparound: Fix consume_xids() consuming one XID too many MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit consume_xids() and consume_xids_until() called GetTopTransactionId() without counting the XID that this may assign, so when the transaction had no XID yet, they consumed one more XID than requested and returned a XID that was not the last one they assigned. Count the XID obtained here and remember it in lastxid, so that the result is correct even for consume_xids(1). Since consume_xids(0) cannot return an XID assigned by the function, make it raise an error instead of returning the next XID. Also, replace the hard-coded shortcut threshold of 2000 with XID_SHORTCUT_THRESHOLD, set to 4000 so that the shortcut cannot skip more XIDs than requested even with 32kB blocks, where XidSkip() can return up to 3276. Add an assertion to XidSkip() to verify this. Document what the two functions return. Author: Yushi Ogiwara <[email protected]> Author: João Rito <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/[email protected] --- .../modules/xid_wraparound/xid_wraparound.c | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/src/test/modules/xid_wraparound/xid_wraparound.c b/src/test/modules/xid_wraparound/xid_wraparound.c index ca25d7e..8a1cba7 100644 --- a/src/test/modules/xid_wraparound/xid_wraparound.c +++ b/src/test/modules/xid_wraparound/xid_wraparound.c @@ -26,6 +26,8 @@ static FullTransactionId consume_xids_common(FullTransactionId untilxid, uint64 /* * Consume the specified number of XIDs. + * + * Returns the last XID assigned by this function. */ PG_FUNCTION_INFO_V1(consume_xids); Datum @@ -34,19 +36,21 @@ consume_xids(PG_FUNCTION_ARGS) int64 nxids = PG_GETARG_INT64(0); FullTransactionId lastxid; - if (nxids < 0) + if (nxids <= 0) elog(ERROR, "invalid nxids argument: %" PRId64, nxids); - if (nxids == 0) - lastxid = ReadNextFullTransactionId(); - else - lastxid = consume_xids_common(InvalidFullTransactionId, (uint64) nxids); + lastxid = consume_xids_common(InvalidFullTransactionId, (uint64) nxids); PG_RETURN_FULLTRANSACTIONID(lastxid); } /* - * Consume XIDs, up to the given XID. + * Advance XID allocation so that the next XID is at least the target. + * This may consume XIDs beyond the target. + * + * Ensures that the current transaction has a top-level XID. + * Returns the last XID assigned by this call, or the next XID observed + * if no XID was assigned. */ PG_FUNCTION_INFO_V1(consume_xids_until); Datum @@ -64,6 +68,23 @@ consume_xids_until(PG_FUNCTION_ARGS) PG_RETURN_FULLTRANSACTIONID(lastxid); } +/* + * These constants copied from .c files, because they're private. + */ +#define COMMIT_TS_XACTS_PER_PAGE (BLCKSZ / 10) +#define SUBTRANS_XACTS_PER_PAGE (BLCKSZ / sizeof(TransactionId)) +#define CLOG_XACTS_PER_BYTE 4 +#define CLOG_XACTS_PER_PAGE (BLCKSZ * CLOG_XACTS_PER_BYTE) + +/* + * Take the shortcut in consume_xids_common() only if we have more than this + * many XIDs left to consume. XidSkip() never skips more than + * COMMIT_TS_XACTS_PER_PAGE XIDs, which is 3276 with the largest supported + * block size (32kB), so this is enough to make sure that the shortcut never + * consumes more XIDs than requested. + */ +#define XID_SHORTCUT_THRESHOLD 4000 + /* * Common functionality between the two public functions. */ @@ -89,7 +110,11 @@ consume_xids_common(FullTransactionId untilxid, uint64 nxids) * the cache overflows, but beyond that, we don't keep track of the * consumed XIDs. */ - (void) GetTopTransactionId(); + if (!FullTransactionIdIsValid(GetTopFullTransactionIdIfAny())) + { + lastxid = GetTopFullTransactionId(); + consumed++; + } for (;;) { @@ -115,7 +140,7 @@ consume_xids_common(FullTransactionId untilxid, uint64 nxids) * If we still have plenty of XIDs to consume, try to take a shortcut * and bump up the nextXid counter directly. */ - if (xids_left > 2000 && + if (xids_left > XID_SHORTCUT_THRESHOLD && consumed - last_reported_at < REPORT_INTERVAL && MyProc->subxidStatus.overflowed) { @@ -153,14 +178,6 @@ consume_xids_common(FullTransactionId untilxid, uint64 nxids) return lastxid; } -/* - * These constants copied from .c files, because they're private. - */ -#define COMMIT_TS_XACTS_PER_PAGE (BLCKSZ / 10) -#define SUBTRANS_XACTS_PER_PAGE (BLCKSZ / sizeof(TransactionId)) -#define CLOG_XACTS_PER_BYTE 4 -#define CLOG_XACTS_PER_PAGE (BLCKSZ * CLOG_XACTS_PER_BYTE) - /* * All the interesting action in GetNewTransactionId happens when we extend * the SLRUs, or at the uint32 wraparound. If the nextXid counter is not close @@ -193,6 +210,9 @@ XidSkip(FullTransactionId fullxid) return 0; distance = Min(distance, CLOG_XACTS_PER_PAGE - rem); + /* The shortcut threshold in consume_xids_common() relies on this */ + Assert(distance <= XID_SHORTCUT_THRESHOLD); + return distance; } -- 2.53.0.windows.1
