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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
