Re: PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)? - Mailing list pgsql-hackers

From Dilip Kumar
Subject Re: PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?
Date
Msg-id CAFiTN-vf6NbH44ipXGntJoVJi5y9mJM8dbYZaqgKunrcLVbcUg@mail.gmail.com
Whole thread Raw
In response to PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
Responses Re: PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?  (Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>)
List pgsql-hackers
On Thu, Apr 22, 2021 at 10:40 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> In the PageGetItemIdCareful() introduced by commit a9ce839a, it seems
> like we are using btree page pd_special structure BTPageOpaqueData for
> error case without max aligning it.
>     if (ItemIdGetOffset(itemid) + ItemIdGetLength(itemid) >
>         BLCKSZ - sizeof(BTPageOpaqueData))
>         ereport(ERROR,
>
> I'm not sure if it is intentional. ISTM that this was actually not a
> problem because the BTPageOpaqueData already has all-aligned(???)
> members (3 uint32, 2 uint16). But it might be a problem if we add
> unaligned bytes. PageInit always max aligns this structure, when we
> initialize the btree page in _bt_pageini and in all other places we
> max align it before use. Since this is an error throwing path, I think
> we should max align it  just to be on the safer side. While on it, I
> think we can also replace BLCKSZ with PageGetPageSize(page).
>
> Attaching a small patch. Thoughts?

+1 for changing to MAXALIGN(sizeof(BTPageOpaqueData)).

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Replication slot stats misgivings
Next
From: Fujii Masao
Date:
Subject: Re: TRUNCATE on foreign table