Hi,
Thank you for your comment.
Regarding the first patch, I believe it works correctly when
consume_xids(1) is called. This is because the lastxid variable in the
consume_xids_common function is initialized as lastxid =
ReadNextFullTransactionId(), where the ReadNextFullTransactionId
function returns the (current XID) + 1.
Separately, I found that consume_xids(0) does not behave as expected.
Below is an example:
postgres=# select txid_current();
txid_current
--------------
45496
(1 row)
postgres=# select consume_xids(0);
consume_xids
--------------
45497
(1 row)
postgres=# select consume_xids(0);
consume_xids
--------------
45497
(1 row)
In the example, the argument to consume_xids is 0, meaning it should not
consume any XIDs. However, the first invocation of consume_xids(0) looks
like unexpectedly consuming 1 XID though it's not consuming actually.
This happens because consume_xids(0) returns the value from
ReadNextFullTransactionId.
I have updated the patch (skip.diff, attached to this e-mail) to address
this issue. Now, when consume_xids(0) is called, it returns
ReadNextFullTransactionId().value - 1, ensuring no XID is consumed as
shown below:
postgres=# select txid_current();
txid_current
--------------
45498
(1 row)
postgres=# select consume_xids(0);
consume_xids
--------------
45498
(1 row)
Regards,
Yushi
On 2024-10-16 07:26, Masahiko Sawada wrote:
Hi,
Thank you for the report.
On Mon, Oct 14, 2024 at 6:51 PM Yushi Ogiwara
<btogiwarayuu...@oss.nttdata.com> wrote:
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.
I agree with your analysis.
I have one comment on the patch:
- (void) GetTopTransactionId();
+ if(!FullTransactionIdIsValid(GetTopFullTransactionIdIfAny()))
+ {
+ (void) GetTopTransactionId();
+ consumed++;
+ }
If we count this case as consumed too, I think we need to set the
returned value of GetTopTranasctionId() to lastxid. Because otherwise,
the return value when passing 1 to the function would be the latest
XID at the time but not the last XID consumed by the function. Passing
1 to this function is very unrealistic case, though.
Regards,
diff --git a/src/test/modules/xid_wraparound/xid_wraparound.c b/src/test/modules/xid_wraparound/xid_wraparound.c
index dce81c0c6d..4b9788d2d9 100644
--- a/src/test/modules/xid_wraparound/xid_wraparound.c
+++ b/src/test/modules/xid_wraparound/xid_wraparound.c
@@ -38,7 +38,7 @@ consume_xids(PG_FUNCTION_ARGS)
elog(ERROR, "invalid nxids argument: %lld", (long long) nxids);
if (nxids == 0)
- lastxid = ReadNextFullTransactionId();
+ lastxid = FullTransactionIdFromU64(ReadNextFullTransactionId().value - 1);
else
lastxid = consume_xids_common(InvalidFullTransactionId, (uint64) nxids);
@@ -89,7 +89,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()))
+ {
+ (void) GetTopTransactionId();
+ consumed++;
+ }
for (;;)
{
