From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Andres Freund <andres(at)anarazel(dot)de> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Raising our compiler requirements for 9.6 |
Date: | 2015-08-16 07:31:48 |
Message-ID: | 20150816073148.GG2069620@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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.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
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2015-08-16 09:50:22 | Re: Management of simple_eval_estate for plpgsql DO blocks |
Previous Message | Noah Misch | 2015-08-16 06:46:35 | Re: Test code is worth the space |