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

On Wed, Apr 21, 2021 at 10:10 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> 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.

Fair point. I pushed a commit to fix this to HEAD just now. Thanks.

> 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).

I didn't replace BLCKSZ with PageGetPageSize() in the commit, though.
We definitely don't want to rely on that being sane in amcheck (this
is also why we don't use PageGetSpecialPointer(), which is the usual
approach).

Actually, even if this wasn't amcheck code I might make the same call.
I personally don't think that most existing calls to PageGetPageSize()
make very much sense.

> Attaching a small patch. Thoughts?

I'm curious: Was this just something that you noticed randomly, while
looking at the code? Or do you have a specific practical reason to
care about it? (I always like hearing about the ways in which people
use amcheck.)

--
Peter Geoghegan