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-23 20:23:43 |
Message-ID: | CAD21AoD8wJ0OiQbq=pc-TUCkEm1ohu6Et8G-Ntc8TFmPk3TuTA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andrey M. Borodin | 2024-10-23 20:27:04 | Re: Using read_stream in index vacuum |
Previous Message | Andrey M. Borodin | 2024-10-23 17:57:28 | Re: Using read_stream in index vacuum |