Thread: Re: Fix for consume_xids advancing XIDs incorrectly
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,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Tue, Oct 15, 2024 at 10:06 PM Yushi Ogiwara <btogiwarayuushi@oss.nttdata.com> wrote: > > 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. But it's possible that concurrent transactions consume XIDs in meanwhile, no? > > 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) 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
On 2024/10/18 14:57, Yushi Ogiwara wrote:
> 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.
One concern in this discussion is that the return value of this function isn't
entirely clear. To address this, how about adding a comment at the beginning of
consume_xids() like: "Returns the last XID assigned by consume_xids()"?
* the cache overflows, but beyond that, we don't keep track of the
* consumed XIDs.
*/
- (void) GetTopTransactionId();
+ if(!FullTransactionIdIsValid(GetTopFullTransactionIdIfAny()))
+ {
+ lastxid = GetTopTransactionId();
+ consumed++;
+ }
How about appending the following to the end of the first paragraph in
the above source comments?
If no top-level XID is assigned, a new one is obtained,
and the consumed XID counter is incremented.
if (xids_left > 2000 &&
consumed - last_reported_at < REPORT_INTERVAL &&
MyProc->subxidStatus.overflowed)
{
int64 consumed_by_shortcut = consume_xids_shortcut();
if (consumed_by_shortcut > 0)
{
consumed += consumed_by_shortcut;
continue;
}
}
By the way, this isn't directly related to the proposed patch, but while reading
the xid_wraparound code, I noticed that consume_xids_common() could potentially
return an unexpected XID if consume_xids_shortcut() returns a value greater
than 2000. Based on the current logic, consume_xids_common() should always return
a value less than 2000, so the issue I'm concerned about shouldn't occur. Still,
would it be worth adding an assertion to ensure that consume_xids_common() never
returns a value greater than 2000?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Tue, Oct 22, 2024 at 11:47 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2024/10/18 14:57, Yushi Ogiwara wrote:
> > 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.
>
> One concern in this discussion is that the return value of this function isn't
> entirely clear. To address this, how about adding a comment at the beginning of
> consume_xids() like: "Returns the last XID assigned by consume_xids()"?
Agreed. That value is what I expected this function to return.
>
>
> * the cache overflows, but beyond that, we don't keep track of the
> * consumed XIDs.
> */
> - (void) GetTopTransactionId();
> + if(!FullTransactionIdIsValid(GetTopFullTransactionIdIfAny()))
> + {
> + lastxid = GetTopTransactionId();
> + consumed++;
> + }
>
> How about appending the following to the end of the first paragraph in
> the above source comments?
>
> If no top-level XID is assigned, a new one is obtained,
> and the consumed XID counter is incremented.
Agreed.
>
>
>
> if (xids_left > 2000 &&
> consumed - last_reported_at < REPORT_INTERVAL &&
> MyProc->subxidStatus.overflowed)
> {
> int64 consumed_by_shortcut = consume_xids_shortcut();
>
> if (consumed_by_shortcut > 0)
> {
> consumed += consumed_by_shortcut;
> continue;
> }
> }
>
> By the way, this isn't directly related to the proposed patch, but while reading
> the xid_wraparound code, I noticed that consume_xids_common() could potentially
> return an unexpected XID if consume_xids_shortcut() returns a value greater
> than 2000. Based on the current logic, consume_xids_common() should always return
> a value less than 2000, so the issue I'm concerned about shouldn't occur.
Good point. Yes, the function doesn't return a value greater than 2000
as long as we do "distance = Min(distance, COMMIT_TS_XACTS_PER_PAGE -
rem);". But it's true with <= 16KB block sizes.
> Still,
> would it be worth adding an assertion to ensure that consume_xids_common() never
> returns a value greater than 2000?
While adding an assertion makes sense to me, another idea is to set
last_xid even in the shortcut path. That way, it works even with 32KB
block size.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On 2024/10/24 5:23, Masahiko Sawada wrote:
>> if (xids_left > 2000 &&
>> consumed - last_reported_at < REPORT_INTERVAL &&
>> MyProc->subxidStatus.overflowed)
>> {
>> int64 consumed_by_shortcut = consume_xids_shortcut();
>>
>> if (consumed_by_shortcut > 0)
>> {
>> consumed += consumed_by_shortcut;
>> continue;
>> }
>> }
>>
>> By the way, this isn't directly related to the proposed patch, but while reading
>> the xid_wraparound code, I noticed that consume_xids_common() could potentially
>> return an unexpected XID if consume_xids_shortcut() returns a value greater
>> than 2000. Based on the current logic, consume_xids_common() should always return
>> a value less than 2000, so the issue I'm concerned about shouldn't occur.
>
> Good point. Yes, the function doesn't return a value greater than 2000
> as long as we do "distance = Min(distance, COMMIT_TS_XACTS_PER_PAGE -
> rem);". But it's true with <= 16KB block sizes.
>
>> Still,
>> would it be worth adding an assertion to ensure that consume_xids_common() never
>> returns a value greater than 2000?
>
> While adding an assertion makes sense to me, another idea is to set
> last_xid even in the shortcut path. That way, it works even with 32KB
> block size.
Agreed on making xid_wraparound compatible with a 32k block size. As you pointed out,
with 32k blocks, XidSkip() can return values greater than 2000. So, the first step is
to adjust the following if-condition by increasing "2000" to a higher value.
Since XidSkip() can return up to 3276 (based on COMMIT_TS_XACTS_PER_PAGE (BLCKSZ / 10) with 32k blocks),
we could, for instance, update "2000" to "4000."
if (xids_left > 2000 &&
consumed - last_reported_at < REPORT_INTERVAL &&
MyProc->subxidStatus.overflowed)
To ensure lastxid is set even in the shortcut case, what about modifying XidSkip()
so it returns "distance - 1" instead of "distance"? This change would make
consume_xids_common() run at least one more loop cycle,
triggering GetNewTransactionId() and setting lastxid correctly.
consumed = XidSkip(nextXid);
if (consumed > 0)
TransamVariables->nextXid.value += (uint64) consumed;
BTW, the code snippet above in consume_xids_shortcut() could potentially set
the next XID to a value below FirstNormalTransactionId? If yes, we should account for
FirstNormalFullTransactionId when increasing the next XID, similar to
how FullTransactionIdAdvance() handles it.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, Oct 24, 2024 at 7:55 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2024/10/24 5:23, Masahiko Sawada wrote:
> >> if (xids_left > 2000 &&
> >> consumed - last_reported_at < REPORT_INTERVAL &&
> >> MyProc->subxidStatus.overflowed)
> >> {
> >> int64 consumed_by_shortcut = consume_xids_shortcut();
> >>
> >> if (consumed_by_shortcut > 0)
> >> {
> >> consumed += consumed_by_shortcut;
> >> continue;
> >> }
> >> }
> >>
> >> By the way, this isn't directly related to the proposed patch, but while reading
> >> the xid_wraparound code, I noticed that consume_xids_common() could potentially
> >> return an unexpected XID if consume_xids_shortcut() returns a value greater
> >> than 2000. Based on the current logic, consume_xids_common() should always return
> >> a value less than 2000, so the issue I'm concerned about shouldn't occur.
> >
> > Good point. Yes, the function doesn't return a value greater than 2000
> > as long as we do "distance = Min(distance, COMMIT_TS_XACTS_PER_PAGE -
> > rem);". But it's true with <= 16KB block sizes.
> >
> >> Still,
> >> would it be worth adding an assertion to ensure that consume_xids_common() never
> >> returns a value greater than 2000?
> >
> > While adding an assertion makes sense to me, another idea is to set
> > last_xid even in the shortcut path. That way, it works even with 32KB
> > block size.
>
> Agreed on making xid_wraparound compatible with a 32k block size. As you pointed out,
> with 32k blocks, XidSkip() can return values greater than 2000. So, the first step is
> to adjust the following if-condition by increasing "2000" to a higher value.
> Since XidSkip() can return up to 3276 (based on COMMIT_TS_XACTS_PER_PAGE (BLCKSZ / 10) with 32k blocks),
> we could, for instance, update "2000" to "4000."
>
> if (xids_left > 2000 &&
> consumed - last_reported_at < REPORT_INTERVAL &&
> MyProc->subxidStatus.overflowed)
>
>
> To ensure lastxid is set even in the shortcut case, what about modifying XidSkip()
> so it returns "distance - 1" instead of "distance"? This change would make
> consume_xids_common() run at least one more loop cycle,
> triggering GetNewTransactionId() and setting lastxid correctly.
Increasing "2000" to "4000" makes sense to me. I think that with this
change we don't necessarily need to change XidSkip() to return
"distance - 1'. What do you think?
>
>
> consumed = XidSkip(nextXid);
> if (consumed > 0)
> TransamVariables->nextXid.value += (uint64) consumed;
>
> BTW, the code snippet above in consume_xids_shortcut() could potentially set
> the next XID to a value below FirstNormalTransactionId? If yes, we should account for
> FirstNormalFullTransactionId when increasing the next XID, similar to
> how FullTransactionIdAdvance() handles it.
Good catch. I agree with you. We need to do something similar to what
FullTransactionIdAdvance() does so that it does not appear as a
special 32-bit XID.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
>> >> >> consumed = XidSkip(nextXid); >> if (consumed > 0) >> TransamVariables->nextXid.value += (uint64) consumed; >> >> BTW, the code snippet above in consume_xids_shortcut() could >> potentially set >> the next XID to a value below FirstNormalTransactionId? If yes, we >> should account for >> FirstNormalFullTransactionId when increasing the next XID, similar to >> how FullTransactionIdAdvance() handles it. > > Good catch. I agree with you. We need to do something similar to what > FullTransactionIdAdvance() does so that it does not appear as a > special 32-bit XID. > > Regards, I think this is not the case since XidSkip returns min(UINT32_MAX - 5 - low, *), which prevents the wrap-around of nextXid. Regards, Yushi Ogiwara
On 2024/10/29 18:00, Yushi Ogiwara wrote: >>> >>> >>> consumed = XidSkip(nextXid); >>> if (consumed > 0) >>> TransamVariables->nextXid.value += (uint64) consumed; >>> >>> BTW, the code snippet above in consume_xids_shortcut() could potentially set >>> the next XID to a value below FirstNormalTransactionId? If yes, we should account for >>> FirstNormalFullTransactionId when increasing the next XID, similar to >>> how FullTransactionIdAdvance() handles it. >> >> Good catch. I agree with you. We need to do something similar to what >> FullTransactionIdAdvance() does so that it does not appear as a >> special 32-bit XID. >> >> Regards, > > I think this is not the case since XidSkip returns min(UINT32_MAX - 5 - low, *), which prevents the wrap-around of nextXid. Yes, you're right. Thanks for the correction! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION