Re: PageGetItemIdCareful - should we MAXALIGN sizeof(BTPageOpaqueData)?

On Thu, Apr 22, 2021 at 11:36 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> 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)).

Thanks for taking a look at it. I added a CF entry
https://commitfest.postgresql.org/33/3089/ so that we don't lose track
of it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com