Re: Raising our compiler requirements for 9.6

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.h
8 modules mention lwlock.h
7 modules mention LWLock
4 modules mention lock.h
1 module mentions LockAcquire()

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