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

On Wed, Feb 16, 2022 at 10:24:58PM +0100, Matthias van de Meent wrote:
> A first good reason to do this is preventing further damage when a
> page is corrupted: if I can somehow overwrite pd_special,
> non-assert-enabled builds would start reading and writing at arbitrary
> offsets from the page pointer, quite possibly in subsequent buffers
> (or worse, on the stack, in case of stack-allocated blocks).

Well, pageinspect has proved to be rather careless in this area for a
couple of years, but this just came from the fact that we called
PageGetSpecialPointer() before checking that PageGetSpecialSize() maps
with the MAXALIGN'd size of the opaque area, if the related AM has
one.

I am not sure that we need to worry about corruptions, as data
checksums would offer protection for most cases users usually need to
worry about, at the exception of page corruptions because of memory
for pages already read in the PG shared buffers from disk or even the
OS cache.

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

> Lastly, 0002 and 0003 remove some repetitive tasks of dealing with the
> [nbtree, hash] opaques by providing a typed accessor macro similar to
> what is used in the GIN and (SP-)GIST index methods; improving the
> legibility of the code and decreasing the churn.

+#define PageGetSpecialOpaque(page, OpaqueDataTyp) \
+( \
+ AssertMacro(PageGetPageSize(page) == BLCKSZ && \
+ PageGetSpecialSize(page) == MAXALIGN(sizeof(OpaqueDataTyp))), \
+ (OpaqueDataTyp *) ((char *) (page) + (BLCKSZ - MAXALIGN(sizeof(OpaqueDataTyp)))) \
+)

Did you notice PageValidateSpecialPointer() that mentions MSVC? Could
this stuff a problem if not an inline function.

Perhaps you should do a s/OpaqueDataTyp/OpaqueDataType/.

As a whole, this patch set looks like an improvement in terms of
checks and consistency regarding the special area handling across the
different AMs for the backend code, while reducing the MAXALIGN-ism on
all those sizes, and this tends to be missed. Any other opinions?
--
Michael