Re: Preventing indirection for IndexPageGetOpaque for known-size page special areas

On Thu, 31 Mar 2022 at 09:32, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Mar 28, 2022 at 05:09:10PM +0200, Matthias van de Meent wrote:
> > Not all clusters have checksums enabled (boo!, but we can't
> > realistically fix that), so on-disk corruption could reasonably
> > propagate to the rest of such system. Additionally, checksums are only
> > checked on read, and updated when the page is written to disk (in
> > PageSetChecksumCopy called from FlushBuffer), but this does not check
> > for signs of page corruption. As such, a memory bug resulting in
> > in-memory corruption of pd_special would persist and would currently
> > have the potential to further corrupt neighbouring buffers.
>
> Well, if it comes to corruption then pd_special is not the only
> problem, just one part of it. A broken pd_lower or pd_lower or even
> pd_lsn could also cause AMs to point at areas they should not. There
> are many reasons that could make things go wrong depending on the
> compiled page size.
>
> >>> A second reason would be less indirection to get to the opaque
> >>> pointer. This should improve performance a bit in those cases where we
> >>> (initially) only want to access the [Index]PageOpaque struct.
> >>
> >> We don't have many cases that do that, do we?
> >
> > Indeed not many, but I know that at least the nbtree-code has some
> > cases where it uses the opaque before other fields in the header are
> > accessed (sometimes even without accessing the header for other
> > reasons): in _bt_moveright and _bt_walk_left. There might be more; but
> > these are cases I know of.
>
> Even these are marginal as the page is already loaded in the shared
> buffers..

I can't really disagree there.

> While looking at the page, the pieces in 0002 and 0003 that I really
> liked are the introduction of the macros to grab the special area for
> btree and hash. This brings the code of those APIs closer to GiST,
> SpGiST and GIN. And it is possible to move to a possible change in
> the checks and/or the shape of all the *GetOpaque macros at once after
> the switch to this part is done. So I don't really mind introducing
> this part.
>
> PageGetSpecialOpaque() would not have saved the day with the recent
> pageinspect changes, and that's just moving the responsability of the
> page header to something else. I am not sure if it is a good idea to
> have a mention of the opaque page type in bufpage.h actually, as this
> is optional. Having an AssertMacro() checking that pd_special is in
> line with MAXALIGN(sizeof(OpaqueData)) is attractive, but I'd rather
> keep that in each AM's headers per its optional nature, and an index
> AM may handle things differently depending on a page type, as well.

PageInit MAXALIGNs the size of the special area that it receives as an
argument; so any changes to the page header that would misalign the
value would be AM-specific; in which case it is quite unlikely that
this is the right accessor for your page's special area.

- Matthias