Re: Fix for consume_xids advancing XIDs incorrectly

Hi,

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. Thank
you to Yushi Ogiwara for the original patch, and to Sawada-san and
Fujii-san for the review; I kept Yushi as the first author in the commit
message.

The attached v2 is a single patch (it merges skip_xid_correctly.diff and
support_blksz_32k.diff). Compared with the previous versions:

1. The XID assigned by GetTopFullTransactionId() is counted in "consumed"
and stored in "lastxid", so consume_xids(n) now consumes exactly n XIDs
and consume_xids(1) returns the XID it actually assigned.
2. consume_xids(0) raises an error, and the unnecessary "else" after the
elog(ERROR) is gone, as Sawada-san suggested.
3. I dropped the extra comment about the consumed counter, as suggested.
I kept the "Returns the last XID assigned by this function" comment on
consume_xids() and consume_xids_until(), as Fujii-san asked.
4. The hard-coded 2000 is replaced by XID_SHORTCUT_THRESHOLD (4000), with a
comment explaining why that is enough (XidSkip() returns at most
COMMIT_TS_XACTS_PER_PAGE, which is 3276 with 32kB blocks), and an
Assert() in XidSkip() checks that. The *_XACTS_PER_PAGE constants moved
above their first use.

Testing: I built with --enable-cassert --enable-debug --enable-tap-tests
(Debian bookworm, no compiler warnings). Using txid_current() followed by
consume_xids(n) in autocommit mode, unpatched master returned one more than
expected for every n I tried (1, 2, 100, 3999, 4000, 4001, 5000, 10000,
100000), for example txid_current() = 672 and consume_xids(100) = 773. With
the patch, the returned value is exactly txid_current() + n in all cases.
Inside a transaction block, the first and second consume_xids(5000) calls
now each advance by exactly 5000 (127777 -> 132777 -> 137777).
consume_xids(0) now fails with "invalid nxids argument: 0", and
consume_xids_until() still returns its target. I did not test with a 32kB
block size. "PG_TEST_EXTRA=xid_wraparound make -C
src/test/modules/xid_wraparound check" passes (4 files, 23 tests). The
patch applies on commit 952bab8b74 and was tested at that commit plus the patch
(the unpatched numbers above came from an older master where the module and
varsup.c are identical).

Comments are welcome.

Regards,
João Rito

Em ter., 10 de nov. de 2026 às 19:45, Yushi Ogiwara
<btogiwarayuushi(at)oss(dot)nttdata(dot)com> escreveu:
>
> Hi,
>
> I found that the consume_xids function incorrectly advances XIDs as
> shown:
>
> postgres=# select txid_current();
> txid_current
> --------------
> 746
> (1 row)
>
> postgres=# select consume_xids('100');
> consume_xids
> --------------
> 847
> (1 row)
>
> In the example, the consume_xids function consumes 100 XIDs when XID =
> 746, so the desired outcome from consume_xids should be 746 + 100 = 846,
> which differs from the actual outcome, 847.
>
> Behavior inside a transaction block:
>
> postgres=# select txid_current();
> txid_current
> --------------
> 1410
> (1 row)
>
> postgres=# begin;
> BEGIN
> postgres=*# select consume_xids('100');
> consume_xids
> --------------
> 1511
> (1 row)
> postgres=*# select consume_xids('100');
> consume_xids
> --------------
> 1521
> (1 row)
>
> Here, the first call inside the transaction block consumes 100+1 XIDs
> (incorrect), while the second call consumes exactly 100 XIDs (as
> expected)
>
> Summary:
>
> The function performs incorrectly when:
> - Outside of a transaction block
> - The first call inside a transaction block
> But works correctly when:
> - After the second call inside a transaction block
>
> The issue arises because consume_xids does not correctly count the
> consumed XIDs when it calls the GetTopTransactionId function, which
> allocates a new XID when none has been assigned.
>
> With the attached patch, the consume_xids works as expected, both inside
> and outside of transaction blocks as shown below:
>
> postgres=# select txid_current();
> txid_current
> --------------
> 1546
> (1 row)
>
> postgres=# select consume_xids('100');
> consume_xids
> --------------
> 1646
> (1 row)
>
> postgres=# begin;
> BEGIN
> postgres=*# select consume_xids('100');
> consume_xids
> --------------
> 1746
> (1 row)
>
> postgres=*# select consume_xids('100');
> consume_xids
> --------------
> 1846
> (1 row)
>
> Regards,
> Yushi