On Thu, Dec 5, 2024 at 12:16 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> In c.h, I'd put in a very explicit comment in front of pg_config.h.
> Also, I don't like making it look like postgres_ext.h is of the
> same ilk as the config headers, since it isn't. So maybe like
>
> /*
> * These headers must be included before any system headers, because
> * on some platforms they affect the behavior of the system headers
> * (for example, by defining _FILE_OFFSET_BITS).
> */
> #include "pg_config.h"
> #include "pg_config_manual.h" /* must be after pg_config.h */
> #include "pg_config_os.h" /* must be before any system header files */
>
> /* Pull in fundamental symbols that we also expose to applications */
> #include "postgres_ext.h"
>
> /* System header files that should be available everywhere in Postgres */
> #include <inttypes.h>
> ...
>
> The comment for pg_config_os.h is redundant this way, maybe we could
> rewrite it as something more useful?
OK, how about:
+#include "pg_config_os.h" /* config from
include/port/PORTNAME.h */
That demystifies where it's really coming from ("portname" is used in
the meson script, and seems more informative than "template").
> Also, there's probably no reason anymore that postgres_ext.h couldn't
> be placed after those fundamental system headers, and that might be
> clearer. (I think perhaps the main reason for the existing ordering
> was to demonstrate that postgres_ext.h could be included before any
> system headers, and that's no longer a consideration.)
Yeah, that makes sense.
> I don't especially care for your proposed changes to postgres_ext.h.
> That substantially expands the footprint of what gets defined by
> pulling that in, and some users might not care for that. (For
> example, because they have ordering assumptions similar to what we're
> dealing with here.) Now you already snuck the camel's nose under the
> tent by including stdint.h there, and maybe these additional headers
> wouldn't do any further damage. But I don't see a strong argument to
> change long-standing external APIs any more than we absolutely must.
If someone wants to define things like that before potentially
including system headers, they're not going about it the right way if
they're including our headers first (or anything at all not under
their direct control). But OK, I can work with the
not-broken-so-don't-fix-it and
pulling-in-more-stuff-that-maybe-they-don't-want arguments. :-)
From 05f334be9d01e90b0031f1e96ccd4449ae48e617 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Thu, 5 Dec 2024 11:06:28 +1300
Subject: [PATCH v2] Fix header inclusion order in c.h.
Commit 962da900a added #include <stdint.h> to postgres_ext.h, which
broke c.h's header ordering rule.
The system headers on some systems would then lock down off_t's size in
private macros, before they'd had a chance to see our definition of
_FILE_OFFSET_BITS (and presumably other things). This was picked up by
perl's ABI compatibility checks on some 32 bit systems in the build
farm.
Move #include "postgres_ext.h" down below the system header section, and
make the comments clearer (thanks to Tom for the new wording).
Diagnosed-by: Tom Lane <t...@sss.pgh.pa.us>
Discussion: https://postgr.es/m/2397643.1733347237%40sss.pgh.pa.us
---
src/include/c.h | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/include/c.h b/src/include/c.h
index 734626e75a8..13bb39fdef3 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -46,11 +46,14 @@
#ifndef C_H
#define C_H
-#include "postgres_ext.h"
-
+/*
+ * These headers must be included before any system headers, because on some
+ * platforms they affect the behavior of the system headers (for example, by
+ * defining _FILE_OFFSET_BITS).
+ */
#include "pg_config.h"
#include "pg_config_manual.h" /* must be after pg_config.h */
-#include "pg_config_os.h" /* must be before any system header files */
+#include "pg_config_os.h" /* config from include/port/PORTNAME.h */
/* System header files that should be available everywhere in Postgres */
#include <inttypes.h>
@@ -76,6 +79,9 @@
#include <libintl.h>
#endif
+ /* Pull in fundamental symbols that we also expose to applications */
+#include "postgres_ext.h"
+
/* Define before including zlib.h to add const decorations to zlib API. */
#ifdef HAVE_LIBZ
#define ZLIB_CONST
--
2.47.0
