Re: Fix for consume_xids advancing XIDs incorrectly

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
> <btogiwarayuushi(at)oss(dot)nttdata(dot)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,