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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Matthias van de Meent <boekewurm+postgres(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Preventing indirection for IndexPageGetOpaque for known-size page special areas
Date: 2022-03-31 07:32:03
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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.

Attachment Content-Type Size
0001-Introduce-macros-for-hash-and-btree-to-grab-their-op.patch text/x-diff 65.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2022-03-31 07:42:37 Re: BUG #17448: In Windows 10, version 1703 and later, huge_pages doesn't work.
Previous Message Dipesh Pandit 2022-03-31 07:19:32 Re: basebackup/lz4 crash