Re: [HACKERS] Macro nesting hell
... btw, why don't we convert c.h's Max(), Min(), and Abs() to inlines? They've all got multi-eval hazards. It might also be interesting to research whether inline would allow simplifying the MemSetFoo family. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macro nesting hell
On 2015-08-12 10:18:12 -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote: Tom Lane wrote: I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. I think it'd be good to convert the macros in bufpage.h and bufmgr.h that either currently have multiple evaluation hazards, or have a AssertMacro() in them. The former for obvious reasons, the latter because that immediately makes them rather large (on and it implies multiple evaluation hazards anyway). That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(), PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(), PageIsPrunable(), PageSetPrunable(), PageClearPrunable(), BufferIsValid(), BufferGetBlock(), BufferGetPageSize(). Sounds reasonable to me. If you do this, I'll see whether pademelon can be adjusted to build using the minimum macro expansion buffer size specified by the C standard. Here's the patch attached. There's two more macros on the list that I had missed: PageXLogRecPtrSet(), PageXLogRecPtrGet(). Unfortunately *Set() requires to pas a pointer to the PageXLogRecPtr - there's only two callers tho. We could instead just leave these, or add an indirecting macro. Seems fine to me though. With it applied pg compiles without the warning I saw before: /home/andres/src/postgresql/src/backend/access/brin/brin_pageops.c:778:5: warning: string literal of length 7732 exceeds maximum length 4095 that ISO C99 compilers are required to support [-Woverlength-strings] Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf))); ^~ We could obviously be more aggressive and convert all the applicable defines, but they are already readable and have no multiple eval hazards, so I'm not inclined to that. Andres From 8203fd286c276051060a6fe8c98c8b576c644f14 Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Thu, 13 Aug 2015 01:37:44 +0200 Subject: [PATCH] Change some buffer and page related macros to inline functions. Firstly some of these macros could recursively expand to string literals larger than the minimum required to be supported by an environment by the C standard. That's primarily due to included assertions. Secondly some macros had multiple evaluation hazards. This required one minor API change in that PageXLogRecPtrSet now takes a pointer to a PageXLogRecPtr instead the value directly. There shouldn't be callers to it out there... Discussion: 4407.1435763...@sss.pgh.pa.us --- src/include/access/gist.h | 2 +- src/include/storage/bufmgr.h | 29 +- src/include/storage/bufpage.h | 124 ++ 3 files changed, 95 insertions(+), 60 deletions(-) diff --git a/src/include/access/gist.h b/src/include/access/gist.h index 81e559b..33eb9d3 100644 --- a/src/include/access/gist.h +++ b/src/include/access/gist.h @@ -142,7 +142,7 @@ typedef struct GISTENTRY #define GistClearFollowRight(page) ( GistPageGetOpaque(page)-flags = ~F_FOLLOW_RIGHT) #define GistPageGetNSN(page) ( PageXLogRecPtrGet(GistPageGetOpaque(page)-nsn)) -#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)-nsn, val)) +#define GistPageSetNSN(page, val) ( PageXLogRecPtrSet(GistPageGetOpaque(page)-nsn, val)) /* * Vector of GISTENTRY structs; user-defined methods union and picksplit diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h index ec0a254..235264c 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -96,11 +96,12 @@ extern PGDLLIMPORT int32 *LocalRefCount; * even in non-assert-enabled builds can be significant. Thus, we've * now demoted the range checks to assertions within the macro itself. */ -#define BufferIsValid(bufnum) \ -( \ - AssertMacro((bufnum) = NBuffers (bufnum) = -NLocBuffer), \ - (bufnum) != InvalidBuffer \ -) +static inline bool +BufferIsValid(Buffer bufnum) +{ + AssertArg(bufnum = NBuffers bufnum = -NLocBuffer); + return bufnum != InvalidBuffer; +} /* * BufferGetBlock @@ -109,14 +110,16 @@ extern PGDLLIMPORT int32 *LocalRefCount; * Note: * Assumes buffer is valid. */ -#define BufferGetBlock(buffer) \ -( \ - AssertMacro(BufferIsValid(buffer)), \ - BufferIsLocal(buffer) ? \ - LocalBufferBlockPointers[-(buffer) - 1] \ - : \ - (Block) (BufferBlocks + ((Size) ((buffer) - 1)) * BLCKSZ) \ -) +static inline Block +BufferGetBlock(Buffer buffer) +{ + AssertArg(BufferIsValid(buffer)); + + if (BufferIsLocal(buffer)) + return LocalBufferBlockPointers[-buffer - 1]; + else + return (Block) (BufferBlocks + ((Size) (buffer - 1)) * BLCKSZ); +} /* * BufferGetPageSize diff --git a/src/include/storage/bufpage.h
Re: [HACKERS] Macro nesting hell
Andres Freund and...@anarazel.de writes: On 2015-08-12 10:18:12 -0400, Tom Lane wrote: Sounds reasonable to me. If you do this, I'll see whether pademelon can be adjusted to build using the minimum macro expansion buffer size specified by the C standard. Here's the patch attached. Looks like you need to pay more attention to the surrounding comments: some of them still refer to the code as a macro, and I see at least one place that explicitly mentions double-eval hazards that this presumably removes. (I think your previous patch re fastgetattr was also a bit weak on the comments, btw.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macro nesting hell
On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote: Tom Lane wrote: Last night my ancient HP compiler spit up on HEAD: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelondt=2015-07-01%2001%3A30%3A18 complaining thus: cpp: brin_pageops.c, line 626: error 4018: Macro param too large after substitution - use -H option. I was able to revive pademelon by adding a new compiler flag as suggested, but after looking at what the preprocessor is emitting, I can't say that I blame it for being unhappy. This simple-looking line Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf))); is expanding to this: I just hit this in clang which also warns about too long literals unless you silence it... Wow, that's kind of amazing. I think this particular case boils down to just PageGetSpecialPointer (bufpage.h) and BufferGetBlock (bufmgr.h). Inlining just BufferGetBlock already helps sufficiently to press it below 4k (the standard's limit IIRC), but that doesn't mean we shouldn't go a bit further. I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. I think it'd be good to convert the macros in bufpage.h and bufmgr.h that either currently have multiple evaluation hazards, or have a AssertMacro() in them. The former for obvious reasons, the latter because that immediately makes them rather large (on and it implies multiple evaluation hazards anyway). That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(), PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(), PageIsPrunable(), PageSetPrunable(), PageClearPrunable(), BufferIsValid(), BufferGetBlock(), BufferGetPageSize(). Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macro nesting hell
Andres Freund and...@anarazel.de writes: On 2015-07-01 12:55:48 -0300, Alvaro Herrera wrote: Tom Lane wrote: I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. I think it'd be good to convert the macros in bufpage.h and bufmgr.h that either currently have multiple evaluation hazards, or have a AssertMacro() in them. The former for obvious reasons, the latter because that immediately makes them rather large (on and it implies multiple evaluation hazards anyway). That'd mean inlining PageSetPageSizeAndVersion(), PageGetSpecialSize(), PageGetSpecialPointer(), PageGetItem(), PageGetMaxOffsetNumber(), PageIsPrunable(), PageSetPrunable(), PageClearPrunable(), BufferIsValid(), BufferGetBlock(), BufferGetPageSize(). Sounds reasonable to me. If you do this, I'll see whether pademelon can be adjusted to build using the minimum macro expansion buffer size specified by the C standard. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Macro nesting hell
Tom Lane wrote: Last night my ancient HP compiler spit up on HEAD: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelondt=2015-07-01%2001%3A30%3A18 complaining thus: cpp: brin_pageops.c, line 626: error 4018: Macro param too large after substitution - use -H option. I was able to revive pademelon by adding a new compiler flag as suggested, but after looking at what the preprocessor is emitting, I can't say that I blame it for being unhappy. This simple-looking line Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf))); is expanding to this: Wow, that's kind of amazing. I think this particular case boils down to just PageGetSpecialPointer (bufpage.h) and BufferGetBlock (bufmgr.h). I'm thinking we really ought to mount a campaign to replace some of these macros with inlined-if-possible functions. My guess is that changing a very small amount of them will do a large enough portion of the job. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Macro nesting hell
Last night my ancient HP compiler spit up on HEAD: http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=pademelondt=2015-07-01%2001%3A30%3A18 complaining thus: cpp: brin_pageops.c, line 626: error 4018: Macro param too large after substitution - use -H option. I was able to revive pademelon by adding a new compiler flag as suggested, but after looking at what the preprocessor is emitting, I can't say that I blame it for being unhappy. This simple-looking line Assert(BRIN_IS_REGULAR_PAGE(BufferGetPage(oldbuf))); is expanding to this: do { if (!(BrinSpecialSpace *) ( ((void) ((bool) (! (!(((const void*)(((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer), (FailedAssertion), brin_pageops.c, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 )), (FailedAssertion), brin_pageops.c, 626), 0, ((oldbuf) 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))) != ((void *)0 || (ExceptionalCondition(!(((const void*)(((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) ! = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(\!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\\\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\\\, (\\\FailedAssertion\\\), \\\brin_pageops.c\\\, 626), 0, (oldbuf) != 0 ))\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, ((oldbuf) 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) ))) != ((void *)0))), (FailedAssertion), brin_pageops.c, 626), 0, ((void) ((bool) (! (!(((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer), (FailedAssertion), brin_pageops.c, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) ! = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 )), (FailedAssertion), brin_pageops.c, 626), 0, ((oldbuf) 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) -pd_special = 8192)) || (ExceptionalCondition(!(((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(\!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\\\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\\\, (\\\FailedAssertion\\\), \\\brin_pageops.c\\\, 626), 0, (oldbuf) != 0 ))\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, ((oldbuf) 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Bl! ock) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) -pd_special = 8192), (FailedAssertion), brin_pageops.c, 626), 0, ((void) ((bool) (! (!(((PageHeader) (((Page)( ((void) ((bool) (! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer), (FailedAssertion), brin_pageops.c, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 )), (FailedAssertion), brin_pageops.c, 626), 0, ((oldbuf) 0) ? LocalBufferBlockPointers[-(oldbuf) - 1] : (Block) (BufferBlocks + ((Size) ((oldbuf) - 1)) * 8192) -pd_special = (__builtin_offsetof (PageHeaderData, pd_linp || (ExceptionalCondition(!(((PageHeader) (((Page)( ((void) ((bool) (!! (!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\, (\FailedAssertion\), \brin_pageops.c\, 626), 0, (oldbuf) != 0 ))) || (ExceptionalCondition(\!(( ((void) ((bool) (! (!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)) || (ExceptionalCondition(\\\!((oldbuf) = NBuffers (oldbuf) = -NLocBuffer)\\\, (\\\FailedAssertion\\\), \\\brin_pageops.c\\\, 626), 0, (oldbuf) != 0 ))\, (\FailedAssertion\), \brin_pageops.c\,