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

On 2015-08-12 13:00:25 -0400, Robert Haas wrote:
> On Tue, Aug 11, 2015 at 10:34 PM, Noah Misch <noah@leadboat.com> wrote:
> >> In my opinion this drastically increases readability and thus should be
> >> applied. Will do so sometime tomorrow unless there's protest.
> >
> > -1 to introducing more inline functions before committable code replaces what
> > you've already pushed for this thread.
>
> This appears to be intended as an insult, but maybe I'm misreading it.

I read it as one.


> I am not thrilled with the rate at which this stuff is getting whacked
> around.  Less-significant changes have been debated for far longer,
> and I really doubt that the rate at which Andres is committing changes
> in this area is healthy.

There was one "feature" patch committed about the actual topic so far
(de6fd1c), and then some fixes to handle the portability fallout and a
admittedly stypid typo. We've debated the inline thing for years now and
seem to have a come to a conclusion about a month ago
(http://archives.postgresql.org/message-id/27127.1435791908%40sss.pgh.pa.us). You
then brought up the thread again
(CA+TgmoaaVfx1KVz5WBkvi1o6oZRxzF0micStTAO7gGUV5a4MwQ@mail.gmail.com)
sometimes after that I started to work on a patch.

Maybe I should have waited bit more to commit the initial, rather
mechanical, conversion to inlines patch, although I don't see what
that'd really have bought us:

This kind of patch (tinkering with autoconf, portability and the like)
normally triggers just about no feedback until it has caused
problems. The buildfarm exists to catch such portability problems.

The issue we're actually arguing about (lockdefs split) was indeed
necessitated by a portability issue (30786.1438831088@sss.pgh.pa.us).
One that actually has existed at least since atomics went in - it just
happens that most (but not all, c.f. a9baeb361d and 7507b19) compilers
that support inlines don't emit references to symbols referenced in a
unused inline function. Since nobody noticed that issue in the 1+ years
atomics was learning to walk on list, and not in the 8 months since it
was committed, it's quite obviously not obvious.

WRT the lockdefs.h split. It wasn't my idea (IIRC Alvaro's; I wanted to
do something closes to what Noah suggested before Tom objected to lock.h
in FRONTEND programs!), and I did wait for a day, while the buildfarm
was red!, after posting it. At least two committers did comment on the
approach before I pushed it.  We've seen far more hot-needled patches to
fix the buildfarm in topics with less portability risks.  I could have
left the buildfarm red for longer, but that might have hidden more
severe portability problems.  It's not like I committed that patch after
somebody had suggested another way: Noah only commented after I had
already pushed the split.

The only actual separate patch since then (fastgetattr as inline
function) was posted 2015-08-05 and I yesterday suggested to push it
today. And it's just replacing two existing macros by inline functions.


Imo there are far more complex patches regularly get committed by
various people, with less discussion and review.


There's afaik nothing broken due to either the inline patch or the
lockdefs split right now. There's one portability bandaid, judged to be
ugly by some, that we're discussing right now, and I'm happy to rejigger
it if the various people with contradicting opinions can come to an
agreement.