On Fri, 1 Apr 2022 at 10:50, Matthias van de Meent <boekewurm+postg...@gmail.com> wrote: > > On Fri, 1 Apr 2022 at 07:38, Michael Paquier <mich...@paquier.xyz> wrote: > > > > On Thu, Mar 31, 2022 at 12:09:35PM +0200, Matthias van de Meent wrote: > > > 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. > > > > Right. I'd still be tempted to keep that per-AM rather than making > > the checks deeper with one extra macro layer in the page header or > > with a different macro that would depend on the opaque type, though. > > Like in the attached, for example.
I noticed that you committed the equivalent of 0002 and 0003, thank you. Here's a new 0001 to keep CFBot happy. > I see. I still would like it better if the access could use this > statically determined offset: your opaque-macros.patch doesn't fix the > out-of-bound read/write scenariofor non-assert builds, nor does it > remove the unneeded indirection through the page header that I was > trying to remove. > > Even in assert-enabled builds; with my proposed changes the code paths > for checking the header value and the use of the special area can be > executed independently, which allows for parallel (pre-)fetching of > the page header and special area, as opposed to the current sequential > load order due to the required use of pd_special. As an extra argument for this; it removes the need for a temporary variable on the stack; as now all accesses into the special area can be based on offsets off the base page pointer (which is also used often). This would allow the compiler to utilize the available registers more effectively. -Matthias PS. I noticed that between PageGetSpecialPointer and PageGetSpecialOpaque the binary size was bigger by ~1kb for the *Opaque variant when compiled with `CFLAGS="-o2" ./configure --enable-debug`; but that varied a lot between builds and likely related to binary layouts. For similar builds with `--enable-cassert`, the *Opaque build was slimmer by ~ 50kB, which is a suprisingly large amount.
From d9e6afdfbc021a699e68529cf37d66b8ff999dbc Mon Sep 17 00:00:00 2001 From: Matthias van de Meent <boekewurm+postgres@gmail.com> Date: Mon, 28 Mar 2022 14:53:43 +0200 Subject: [PATCH v3] Add known-size pre-aligned special area pointer macro This removes 1 layer of indirection for special areas of which we know the type (=size) and location. Special area access through the page header is an extra cache line that needs to be fetched. If might only want to look at the special area, it is much less effort to calculate the offset to the special area directly instead of first checking the page header - saving one cache line to fetch. Assertions are added to check that the page has a correctly sized special area, and that the page is of the expected size. This detects data corruption, instead of doing random reads/writes into the page or data allocated next to the page being accessed. Additionally, updates the GIN, [SP-]GIST, Hash, and BTree [Index]PageGetOpaque macros with the new pre-aligned special area accessor. --- src/include/access/ginblock.h | 3 ++- src/include/access/gist.h | 3 ++- src/include/access/hash.h | 3 ++- src/include/access/nbtree.h | 3 ++- src/include/access/spgist_private.h | 3 ++- src/include/storage/bufpage.h | 14 ++++++++++++++ 6 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/include/access/ginblock.h b/src/include/access/ginblock.h index 9347f464f3..3680098c98 100644 --- a/src/include/access/ginblock.h +++ b/src/include/access/ginblock.h @@ -108,7 +108,8 @@ typedef struct GinMetaPageData /* * Macros for accessing a GIN index page's opaque data */ -#define GinPageGetOpaque(page) ( (GinPageOpaque) PageGetSpecialPointer(page) ) +#define GinPageGetOpaque(page) \ + ((GinPageOpaque) PageGetSpecialOpaque(page, GinPageOpaqueData)) #define GinPageIsLeaf(page) ( (GinPageGetOpaque(page)->flags & GIN_LEAF) != 0 ) #define GinPageSetLeaf(page) ( GinPageGetOpaque(page)->flags |= GIN_LEAF ) diff --git a/src/include/access/gist.h b/src/include/access/gist.h index a3337627b8..51223e9051 100644 --- a/src/include/access/gist.h +++ b/src/include/access/gist.h @@ -164,7 +164,8 @@ typedef struct GISTENTRY bool leafkey; } GISTENTRY; -#define GistPageGetOpaque(page) ( (GISTPageOpaque) PageGetSpecialPointer(page) ) +#define GistPageGetOpaque(page) \ + ((GISTPageOpaque) PageGetSpecialOpaque(page, GISTPageOpaqueData)) #define GistPageIsLeaf(page) ( GistPageGetOpaque(page)->flags & F_LEAF) #define GIST_LEAF(entry) (GistPageIsLeaf((entry)->page)) diff --git a/src/include/access/hash.h b/src/include/access/hash.h index da372841c4..0ed6ccc40b 100644 --- a/src/include/access/hash.h +++ b/src/include/access/hash.h @@ -85,7 +85,8 @@ typedef struct HashPageOpaqueData typedef HashPageOpaqueData *HashPageOpaque; -#define HashPageGetOpaque(page) ((HashPageOpaque) PageGetSpecialPointer(page)) +#define HashPageGetOpaque(page) \ + ((HashPageOpaque) PageGetSpecialOpaque(page, HashPageOpaqueData)) #define H_NEEDS_SPLIT_CLEANUP(opaque) (((opaque)->hasho_flag & LH_BUCKET_NEEDS_SPLIT_CLEANUP) != 0) #define H_BUCKET_BEING_SPLIT(opaque) (((opaque)->hasho_flag & LH_BUCKET_BEING_SPLIT) != 0) diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h index 93f8267b48..2d87c8a90f 100644 --- a/src/include/access/nbtree.h +++ b/src/include/access/nbtree.h @@ -70,7 +70,8 @@ typedef struct BTPageOpaqueData typedef BTPageOpaqueData *BTPageOpaque; -#define BTPageGetOpaque(page) ((BTPageOpaque) PageGetSpecialPointer(page)) +#define BTPageGetOpaque(page) \ + ((BTPageOpaque) PageGetSpecialOpaque(page, BTPageOpaqueData)) /* Bits defined in btpo_flags */ #define BTP_LEAF (1 << 0) /* leaf page, i.e. not internal page */ diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h index eb56b1c6b8..b0bdc7df3e 100644 --- a/src/include/access/spgist_private.h +++ b/src/include/access/spgist_private.h @@ -75,7 +75,8 @@ typedef SpGistPageOpaqueData *SpGistPageOpaque; #define SPGIST_LEAF (1<<2) #define SPGIST_NULLS (1<<3) -#define SpGistPageGetOpaque(page) ((SpGistPageOpaque) PageGetSpecialPointer(page)) +#define SpGistPageGetOpaque(page) \ + ((SpGistPageOpaque) PageGetSpecialOpaque(page, SpGistPageOpaqueData)) #define SpGistPageIsMeta(page) (SpGistPageGetOpaque(page)->flags & SPGIST_META) #define SpGistPageIsDeleted(page) (SpGistPageGetOpaque(page)->flags & SPGIST_DELETED) #define SpGistPageIsLeaf(page) (SpGistPageGetOpaque(page)->flags & SPGIST_LEAF) diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h index e9f253f2c8..e96595f338 100644 --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -329,6 +329,20 @@ PageValidateSpecialPointer(Page page) (char *) ((char *) (page) + ((PageHeader) (page))->pd_special) \ ) +/* + * PageGetSpecialOpaque + * Returns pointer to special space on a page, of the given type. + * This removes the data dependency on the page header in builds + * without asserts enabled. + */ +#define PageGetSpecialOpaque(page, OpaqueDataType) \ +( \ + AssertMacro(PageGetPageSize(page) == BLCKSZ && \ + PageGetSpecialSize(page) == MAXALIGN(sizeof(OpaqueDataType))), \ + (OpaqueDataType *) ((char *) (page) + \ + (BLCKSZ - MAXALIGN(sizeof(OpaqueDataType)))) \ +) + /* * PageGetItem * Retrieves an item on the given page. -- 2.30.2
