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 <[email protected]> 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
v2-0001-xid_wraparound-Fix-consume_xids-consuming-one-XID.patch
Description: Binary data
