Re: Fix for consume_xids advancing XIDs incorrectly

On Tue, Oct 22, 2024 at 11:47 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)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