Subject: Re: Fix for consume_xids advancing XIDs incorrectly

From: Yushi Ogiwara <[email-masked]>
Date: Wed, 16 Oct 2024 14:06:14 +0900

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
> <[email-masked]> 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 (;;)
 	{

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 16 Oct 2024 10:58:50 -0700

> function returns the (current XID) + 1.

But it's possible that concurrent transactions consume XIDs in meanwhile, no?

> postgres=# select txid_current();

Hmm, I think if we expect this function to return the last XID that
the function actually consumed, calling consume_xids with 0 should
raise an error instead. Even if it returns
ReadNextFullTransactionId().value - 1 as you proposed, other
concurrent transactions might consume XIDs between txid_current() and
consume_xids(0), resulting in consume_xids() appearing to have
consumed XIDs.

Regards,

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

---

From: Yushi Ogiwara <[email-masked]>
Date: Fri, 18 Oct 2024 14:57:36 +0900

Hi,

Thank you for your suggestion. Let me organize the discussion as 
follows:

When I created the patch, I did not account for situations where other 
transactions concurrently consume XIDs. If we establish the rule that 
"consume_xids should not return an XID that is consumed by any other 
transaction," I believe it will simplify the discussion.

Case: consume_xids(1)
Let's consider a scenario where transaction 1 (t1) and transaction 2 
(t2) run concurrently. t1 calls txid_current() and consume_xids(1), 
while t2 calls txid_current().

The schedule above illustates when t1 and t2 run concurrently. The "next 
XID" column shows how the TransamVariables->nextXid global variable 
changes over time. The /**/ blocks shows the value of each variable and 
outcome of a called function.

In my previous patch, the schedule above could occur, violating the rule 
because consume_xids does not update lastxid when GetTopTransactionId is 
called.

If your suggestion is applied, consume_xids would no longer violate the 
rule, as it would update lastxid when generating a new XID.

Case: consume_xids(0)

Now, consider the case where transaction 1 consumes 0 XIDs.

In this example, both t1 and t2 consume XID = 20, which violates the 
rule.

Using ReadNextFullTransactionId() - 1 also does not resolve the issue, 
as demonstrated above. This is because consume_xids(0) does not allocate 
any XIDs.

In conclusion, I agree that:

  	* Update lastxid with GetTopTransactionId().
  	* consume_xids with 0 should raise an error.

I have attached a new patch that incorporates your suggestions.

Regards,
Yushi

diff --git a/src/test/modules/xid_wraparound/xid_wraparound.c b/src/test/modules/xid_wraparound/xid_wraparound.c
index dce81c0c6d..70a748c41d 100644
--- a/src/test/modules/xid_wraparound/xid_wraparound.c
+++ b/src/test/modules/xid_wraparound/xid_wraparound.c
@@ -34,11 +34,8 @@ consume_xids(PG_FUNCTION_ARGS)
 	int64		nxids = PG_GETARG_INT64(0);
 	FullTransactionId lastxid;
 
-	if (nxids < 0)
+	if (nxids <= 0)
-
-	if (nxids == 0)
 
@@ -89,7 +86,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 = GetTopTransactionId();
+		consumed++;
+	}
 
 	for (;;)
 	{