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

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

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