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

On Fri, Aug 14, 2015 at 08:40:13PM +0200, Andres Freund wrote:
> > > On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote:
> > > > > + * lockdefs.h
> > > > > + *       Frontend exposed parts of postgres' low level lock mechanism

> I actually think the split came out to work far better than I'd
> anticipated. Having a slimmed-down version of lock.h for code that just
> needs to declare/pass lockmode parameters seems to improve our layering
> considerably.  I got round to Alvaro's and Roberts position that
> removing lock.h from namespace.h and heapam.h would be a really nice
> improvemen on that front.

I assessed 0001-WIP-Decrease-usage-of-lock.h-further.patch and reassessed
0001-Don-t-include-low-level-locking-code-from-frontend-c.patch (commit
4eda0a6).  I changed the details of my position compared to my last review.

As we see from the patches' need to add #include statements to .c files and
from Robert's report of a broken EDB build, this change creates work for
maintainers of non-core code, both backend code (modules) and frontend code
(undocumented, but folks do it).  That's to be expected and doesn't take a
great deal of justification, but users should get benefits in connection with
the work.  This brings to mind the htup_details.h introduction, which created
so much busy work in non-core code.  I don't expect the lock.h split to be
quite that disruptive.  Statistics on PGXN modules:

29 modules mention htup_details.h8 modules mention lwlock.h7 modules mention LWLock4 modules mention lock.h1 module
mentionsLockAcquire()
 

Four modules (imcs, pg_stat_kcache, query_histogram, query_recorder) mentioned
LWLock without mentioning lwlock.h.  These patches (trivially) break the imcs
build.  The other three failed to build for unrelated reasons, but I suspect
these patches give those modules one more thing to update.  What do users get
out of this?  They'll learn if their code is not portable to pademelon, but
#warning "atomics.h in frontend code is not portable" would communicate the
same without compelling non-core code to care.  Other than that benefit,
making headers #error-on-FRONTEND mostly lets us congratulate ourselves for
having introduced the start of a header layer distinction.  I'd be for that if
PostgreSQL were new, but I can't justify doing it at the user cost already
apparent.  That would be bad business.

Therefore, I urge you to instead add the aforementioned #warning and wrap with
#ifdef FRONTEND the two #include "port/atomics.h" in headers.  That tightly
limits build breakage; it can only break frontend code, which is rare outside
core.  Hackers will surely notice if a patch makes the warning bleat, so
mistakes won't survive long.  Non-core frontend code, if willing to cede a
shred of portability, can experiment with atomics.  Folks could do nifty
things with that.

Thanks,
nm