Re: Raising our compiler requirements for 9.6 - Mailing list pgsql-hackers

On 2015-08-15 12:47:09 -0400, Noah Misch wrote:
> Atomics were a miner's canary for pademelon's trouble with post-de6fd1c
> inlining.  Expect pademelon to break whenever a frontend-included file gains
> an inline function that calls a backend function.  Atomics were the initial
> examples, but this patch adds more:

That's a good point.

> $ make -s PROFILE='-O0 -DPG_FORCE_DISABLE_INLINE=1'
> pg_resetxlog.o: In function `fastgetattr':
> /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:754: undefined
referenceto `nocachegetattr'
 
> pg_resetxlog.o: In function `heap_getattr':
> /data/nmisch/src/pg/postgresql/src/bin/pg_resetxlog/../../../src/include/access/htup_details.h:783: undefined
referenceto `heap_getsysattr'
 
> 
> The htup_details.h case is trickier in a way, because pg_resetxlog has a
> legitimate need for SizeofHeapTupleHeader via TOAST_MAX_CHUNK_SIZE.  Expect
> this class of problem to recur as we add more inline functions.  Our method to
> handle these first two instances will set a precedent.
> 
> That gave me new respect for STATIC_IF_INLINE.  While it does add tedious work
> to the task of introducing a new batch of inline functions, the work is
> completely mechanical.  Anyone can write it; anyone can review it; there's one
> correct way to write it.

What's the advantage of using STATIC_IF_INLINE over just #ifndef
FRONTEND? That doesn't work well for entire headers in my opinion, but
for individual functions it shouldn't be a problem? In fact we've done
it for years for MemoryContextSwitchTo(). And by the problem definition
such functions cannot be required by frontend code.

STATIC_IF_INLINE is really tedious because to know whether it works you
essentially need to recompile postgres with inlines enabled/disabled.

In fact STATIC_IF_INLINE does *not* even help here unless we also detect
compilers that support inlining but balk when inline functions reference
symbols not available. There was an example of that in the buildfarm as
of 2014, c.f. a9baeb361d63 . We'd have to disable inlining for such
compilers.

> Header surgery like
> 0001-Don-t-include-low-level-locking-code-from-frontend-c.patch
> requires expert design from scratch, and it (trivially) breaks builds
> for a sample of non-core code.

I agree that such surgery isn't always a good idea. In my opinion the
removing proc.h from the number of headers in the above and the followon
WIP patch I posted has value completely independently from fixing
problems.

Greetings,

Andres Freund