pgsql: Fix for globals.c- c.h must come first

On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote:

Fix for globals.c- c.h must come first

Commit da9b580 mistakenly put a system header before postgres.h (which
includes c.h). That can cause portability issues and broke (at least)
builds with older Windows compilers.

I assume there is no way to add defined and checks to globals.c and c.h
to cause a compile error when this happens.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 2018-06-19 13:58:34 -0400, Bruce Momjian wrote:

On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote:

Fix for globals.c- c.h must come first

Commit da9b580 mistakenly put a system header before postgres.h (which
includes c.h). That can cause portability issues and broke (at least)
builds with older Windows compilers.

I assume there is no way to add defined and checks to globals.c and c.h
to cause a compile error when this happens.

I don't see how to do so in a form that's even halfway portable.

Just to be clear: There's nothing globals.c specific about the rule to
always include postgres.h first.

Greetings,

Andres Freund

Bruce Momjian <bruce@momjian.us> writes:

On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote:

Commit da9b580 mistakenly put a system header before postgres.h (which
includes c.h). That can cause portability issues and broke (at least)
builds with older Windows compilers.

I assume there is no way to add defined and checks to globals.c and c.h
to cause a compile error when this happens.

Hm. You could imagine adding something like

#ifdef some-relevant-macro
#error include ordering problem, c.h must come before system headers
#endif

near the head of c.h. But I'm not sure how we'd get full coverage.
The case presumably would only occur for off-the-beaten-path headers
(this particular mistake was <sys/stat.h>), and there are lots of those.

regards, tom lane

On 2018-06-19 14:09:15 -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Sat, May 19, 2018 at 01:20:47AM +0000, Stephen Frost wrote:

Commit da9b580 mistakenly put a system header before postgres.h (which
includes c.h). That can cause portability issues and broke (at least)
builds with older Windows compilers.

I assume there is no way to add defined and checks to globals.c and c.h
to cause a compile error when this happens.

Hm. You could imagine adding something like

#ifdef some-relevant-macro
#error include ordering problem, c.h must come before system headers
#endif

near the head of c.h. But I'm not sure how we'd get full coverage.
The case presumably would only occur for off-the-beaten-path headers
(this particular mistake was <sys/stat.h>), and there are lots of those.

If we were ok with doing this in a very platform specific manner, we
probably could make the dependency fairly generic. I.e. glibc IIRC will
pretty much always end up including features.h, so we could check for
that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar
things.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-06-19 14:09:15 -0400, Tom Lane wrote:

Hm. You could imagine adding something like

#ifdef some-relevant-macro
#error include ordering problem, c.h must come before system headers
#endif

near the head of c.h. But I'm not sure how we'd get full coverage.
The case presumably would only occur for off-the-beaten-path headers
(this particular mistake was <sys/stat.h>), and there are lots of those.

If we were ok with doing this in a very platform specific manner, we
probably could make the dependency fairly generic. I.e. glibc IIRC will
pretty much always end up including features.h, so we could check for
that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar
things.

It'd probably be good enough if the error detection worked with glibc's
headers. Even if nobody noticed before a particular patch went in,
the buildfarm would surely find it.

regards, tom lane

I wrote:

It'd probably be good enough if the error detection worked with glibc's
headers. Even if nobody noticed before a particular patch went in,
the buildfarm would surely find it.

On second thought, we'd also need a solution for Windows, because we
have some files that only get compiled for Windows. But even two
separate tests of this sort seems reasonable.

regards, tom lane

Hi,

On 2018-06-19 14:19:56 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2018-06-19 14:09:15 -0400, Tom Lane wrote:

Hm. You could imagine adding something like

#ifdef some-relevant-macro
#error include ordering problem, c.h must come before system headers
#endif

near the head of c.h. But I'm not sure how we'd get full coverage.
The case presumably would only occur for off-the-beaten-path headers
(this particular mistake was <sys/stat.h>), and there are lots of those.

If we were ok with doing this in a very platform specific manner, we
probably could make the dependency fairly generic. I.e. glibc IIRC will
pretty much always end up including features.h, so we could check for
that (#ifdef _FEATURES_H). I'd suspect other C libraries have similar
things.

It'd probably be good enough if the error detection worked with glibc's
headers. Even if nobody noticed before a particular patch went in,
the buildfarm would surely find it.

Then the _FEATURES_H check would work - just tested it locally, and it
triggers appropriately. Does tie us a bit to specific glibc versions,
but... Obvoiusly would need some comments explaining it

Are there arguments for doing this in postgres.h rather than c.h? I
can't immediately think of a case where it'd be ok for postgres_fe.h or
such to be included later. libpq-fe would be different, but that
doesn't include backend headers.

I think we shouldn't do this for v11, as it seems likely that some
extensions would be broken. While not hard to fix, that seems
unnecessary after beta1? Seems unlikely we'll introduce further cases
before new features are merged.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

Are there arguments for doing this in postgres.h rather than c.h?

No, c.h is the correct place. The core problem is that we have to include
pg_config.h before <stdio.h> et al in order to have consistent libc APIs
(on platforms where this matters), and c.h is what does that.

I think we shouldn't do this for v11, as it seems likely that some
extensions would be broken. While not hard to fix, that seems
unnecessary after beta1? Seems unlikely we'll introduce further cases
before new features are merged.

Yeah, waiting for v12 seems fine.

regards, tom lane

Hi,

On 2018-06-19 14:22:25 -0400, Tom Lane wrote:

I wrote:

It'd probably be good enough if the error detection worked with glibc's
headers. Even if nobody noticed before a particular patch went in,
the buildfarm would surely find it.

On second thought, we'd also need a solution for Windows, because we
have some files that only get compiled for Windows. But even two
separate tests of this sort seems reasonable.

Could anybody working on windows (Amit, Michael?) check whether there's
an equivalent to my _FEATURES_H trick?

Greetings,

Andres Freund