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

On Sat, Apr 24, 2021 at 4:11 AM Peter Geoghegan <pg@bowt.ie> wrote:
> > 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).

If the PageGetPageSize can't be sane within amcheck, does it mean that
the page would have been corrupted somewhere?

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

Should we get rid of all existing PageGetPageSize and directly use
BLCKSZ instead? AFAICS, all the index and heap pages are of BLCKSZ
(PageInit has Assert(pageSize == BLCKSZ);).

Using PageGetPageSize to get the size that's been stored in the page,
we might catch errors early if at all the page is corrupted and the
size is overwritten . That's not the case if we use BLCKSZ which is
not stored in the page. In this case the size stored on the page
becomes redundant and the pd_pagesize_version could just be 2 bytes
storing the page version. While we save 2 bytes per page, I'm not sure
this is acceptable as PageHeader size gets changed.

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

I found this while working on one internal feature but not while using amcheck.

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