MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_de5e7dc1857686863ca2968ec1e5f832"
Content-Transfer-Encoding: 8bit
Date: Wed, 16 Oct 2024 14:06:14 +0900
From: Yushi Ogiwara <btogiwarayuushi@oss.nttdata.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Pgsql Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Fix for consume_xids advancing XIDs incorrectly
In-Reply-To: <CAD21AoATuDuqSr=V3vCSbHbOg6g9zKL61EEbe4ZPG3xOUa0Pcw@mail.gmail.com>
References: <2a62f138c9dcb6fcf5889b076d123aa2@oss.nttdata.com>
 <CAD21AoATuDuqSr=V3vCSbHbOg6g9zKL61EEbe4ZPG3xOUa0Pcw@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <50ee82a96ac0935de88f9a5ef021f04d@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/50ee82a96ac0935de88f9a5ef021f04d%40oss.nttdata.com>
Precedence: bulk

--=_de5e7dc1857686863ca2968ec1e5f832
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=UTF-8;
 format=flowed

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@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,

--=_de5e7dc1857686863ca2968ec1e5f832
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=skip.diff
Content-Disposition: attachment;
 filename=skip.diff;
 size=946

ZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMveGlkX3dyYXBhcm91bmQveGlkX3dyYXBhcm91
bmQuYyBiL3NyYy90ZXN0L21vZHVsZXMveGlkX3dyYXBhcm91bmQveGlkX3dyYXBhcm91bmQuYwpp
bmRleCBkY2U4MWMwYzZkLi40Yjk3ODhkMmQ5IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVz
L3hpZF93cmFwYXJvdW5kL3hpZF93cmFwYXJvdW5kLmMKKysrIGIvc3JjL3Rlc3QvbW9kdWxlcy94
aWRfd3JhcGFyb3VuZC94aWRfd3JhcGFyb3VuZC5jCkBAIC0zOCw3ICszOCw3IEBAIGNvbnN1bWVf
eGlkcyhQR19GVU5DVElPTl9BUkdTKQogCQllbG9nKEVSUk9SLCAiaW52YWxpZCBueGlkcyBhcmd1
bWVudDogJWxsZCIsIChsb25nIGxvbmcpIG54aWRzKTsKIAogCWlmIChueGlkcyA9PSAwKQotCQls
YXN0eGlkID0gUmVhZE5leHRGdWxsVHJhbnNhY3Rpb25JZCgpOworCQlsYXN0eGlkID0gRnVsbFRy
YW5zYWN0aW9uSWRGcm9tVTY0KFJlYWROZXh0RnVsbFRyYW5zYWN0aW9uSWQoKS52YWx1ZSAtIDEp
OwogCWVsc2UKIAkJbGFzdHhpZCA9IGNvbnN1bWVfeGlkc19jb21tb24oSW52YWxpZEZ1bGxUcmFu
c2FjdGlvbklkLCAodWludDY0KSBueGlkcyk7CiAKQEAgLTg5LDcgKzg5LDExIEBAIGNvbnN1bWVf
eGlkc19jb21tb24oRnVsbFRyYW5zYWN0aW9uSWQgdW50aWx4aWQsIHVpbnQ2NCBueGlkcykKIAkg
KiB0aGUgY2FjaGUgb3ZlcmZsb3dzLCBidXQgYmV5b25kIHRoYXQsIHdlIGRvbid0IGtlZXAgdHJh
Y2sgb2YgdGhlCiAJICogY29uc3VtZWQgWElEcy4KIAkgKi8KLQkodm9pZCkgR2V0VG9wVHJhbnNh
Y3Rpb25JZCgpOworCWlmKCFGdWxsVHJhbnNhY3Rpb25JZElzVmFsaWQoR2V0VG9wRnVsbFRyYW5z
YWN0aW9uSWRJZkFueSgpKSkKKwl7CisJCSh2b2lkKSBHZXRUb3BUcmFuc2FjdGlvbklkKCk7CisJ
CWNvbnN1bWVkKys7CisJfQogCiAJZm9yICg7OykKIAl7Cg==
--=_de5e7dc1857686863ca2968ec1e5f832--