From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | Yushi Ogiwara <btogiwarayuushi(at)oss(dot)nttdata(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fix for consume_xids advancing XIDs incorrectly |
Date: | 2024-10-25 21:31:38 |
Message-ID: | CAD21AoB0uW34gHg23CoS=sQjgf29Fnj_7hF=VaCfcqexeBemMg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Oct 24, 2024 at 7:55 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Jeff Davis | 2024-10-25 22:18:48 | Re: Statistics Import and Export |
Previous Message | Kirill Reshke | 2024-10-25 21:03:32 | Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row |