Re: apply pragma system_header to python headers - Mailing list pgsql-hackers

I wrote:
> Probably a better way is to invent a separate header "plpython_system.h"
> that just includes the Python headers, to scope the pragma precisely.
> (I guess it could have the fixup #defines we're wrapping those headers
> in, too.)

> The same idea would work in plperl.

After updating one of my test machines to Fedora 39, I'm seeing these
Python warnings too.  So here's a fleshed-out patch proposal for doing
it like that.

            regards, tom lane

diff --git a/src/pl/plperl/GNUmakefile b/src/pl/plperl/GNUmakefile
index 975f540b3e..31fb212bf1 100644
--- a/src/pl/plperl/GNUmakefile
+++ b/src/pl/plperl/GNUmakefile
@@ -108,11 +108,11 @@ uninstall: uninstall-lib uninstall-data

 install-data: installdirs
     $(INSTALL_DATA) $(addprefix $(srcdir)/, $(DATA)) '$(DESTDIR)$(datadir)/extension/'
-    $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/ppport.h '$(DESTDIR)$(includedir_server)'
+    $(INSTALL_DATA) $(srcdir)/plperl.h $(srcdir)/plperl_system.h $(srcdir)/ppport.h '$(DESTDIR)$(includedir_server)'

 uninstall-data:
     rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
-    rm -f $(addprefix '$(DESTDIR)$(includedir_server)'/, plperl.h ppport.h)
+    rm -f $(addprefix '$(DESTDIR)$(includedir_server)'/, plperl.h plperl_system.h ppport.h)

 .PHONY: install-data uninstall-data

diff --git a/src/pl/plperl/meson.build b/src/pl/plperl/meson.build
index 6a28f40a11..182e0fe169 100644
--- a/src/pl/plperl/meson.build
+++ b/src/pl/plperl/meson.build
@@ -71,6 +71,7 @@ install_data(

 install_headers(
   'plperl.h',
+  'plperl_system.h',
   'ppport.h',
   install_dir: dir_include_server,
 )
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index 5d8e51089a..7f338084a9 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -18,200 +18,11 @@
 /* defines free() by way of system headers, so must be included before perl.h */
 #include "mb/pg_wchar.h"

-/* stop perl headers from hijacking stdio and other stuff on Windows */
-#ifdef WIN32
-#define WIN32IO_IS_STDIO
-#endif                            /* WIN32 */
-
 /*
- * Supply a value of PERL_UNUSED_DECL that will satisfy gcc - the one
- * perl itself supplies doesn't seem to.
+ * Pull in Perl headers via a wrapper header, to control the scope of
+ * the system_header pragma therein.
  */
-#define PERL_UNUSED_DECL pg_attribute_unused()
-
-/*
- * Sometimes perl carefully scribbles on our *printf macros.
- * So we undefine them here and redefine them after it's done its dirty deed.
- */
-#undef vsnprintf
-#undef snprintf
-#undef vsprintf
-#undef sprintf
-#undef vfprintf
-#undef fprintf
-#undef vprintf
-#undef printf
-
-/*
- * Perl scribbles on the "_" macro too.
- */
-#undef _
-
-/*
- * ActivePerl 5.18 and later are MinGW-built, and their headers use GCC's
- * __inline__.  Translate to something MSVC recognizes. Also, perl.h sometimes
- * defines isnan, so undefine it here and put back the definition later if
- * perl.h doesn't.
- */
-#ifdef _MSC_VER
-#define __inline__ inline
-#ifdef isnan
-#undef isnan
-#endif
-/* Work around for using MSVC and Strawberry Perl >= 5.30. */
-#define __builtin_expect(expr, val) (expr)
-#endif
-
-/*
- * Regarding bool, both PostgreSQL and Perl might use stdbool.h or not,
- * depending on configuration.  If both agree, things are relatively harmless.
- * If not, things get tricky.  If PostgreSQL does but Perl does not, define
- * HAS_BOOL here so that Perl does not redefine bool; this avoids compiler
- * warnings.  If PostgreSQL does not but Perl does, we need to undefine bool
- * after we include the Perl headers; see below.
- */
-#ifdef PG_USE_STDBOOL
-#define HAS_BOOL 1
-#endif
-
-/*
- * Newer versions of the perl headers trigger a lot of warnings with our
- * compiler flags (at least -Wdeclaration-after-statement,
- * -Wshadow=compatible-local are known to be problematic). The system_header
- * pragma hides warnings from within the rest of this file, if supported.
- */
-#ifdef HAVE_PRAGMA_GCC_SYSTEM_HEADER
-#pragma GCC system_header
-#endif
-
-/*
- * Get the basic Perl API.  We use PERL_NO_GET_CONTEXT mode so that our code
- * can compile against MULTIPLICITY Perl builds without including XSUB.h.
- */
-#define PERL_NO_GET_CONTEXT
-#include "EXTERN.h"
-#include "perl.h"
-
-/*
- * We want to include XSUB.h only within .xs files, because on some platforms
- * it undesirably redefines a lot of libc functions.  But it must appear
- * before ppport.h, so use a #define flag to control inclusion here.
- */
-#ifdef PG_NEED_PERL_XSUB_H
-/*
- * On Windows, win32_port.h defines macros for a lot of these same functions.
- * To avoid compiler warnings when XSUB.h redefines them, #undef our versions.
- */
-#ifdef WIN32
-#undef accept
-#undef bind
-#undef connect
-#undef fopen
-#undef fstat
-#undef kill
-#undef listen
-#undef lstat
-#undef mkdir
-#undef open
-#undef putenv
-#undef recv
-#undef rename
-#undef select
-#undef send
-#undef socket
-#undef stat
-#undef unlink
-#endif
-
-#include "XSUB.h"
-#endif
-
-/* put back our *printf macros ... this must match src/include/port.h */
-#ifdef vsnprintf
-#undef vsnprintf
-#endif
-#ifdef snprintf
-#undef snprintf
-#endif
-#ifdef vsprintf
-#undef vsprintf
-#endif
-#ifdef sprintf
-#undef sprintf
-#endif
-#ifdef vfprintf
-#undef vfprintf
-#endif
-#ifdef fprintf
-#undef fprintf
-#endif
-#ifdef vprintf
-#undef vprintf
-#endif
-#ifdef printf
-#undef printf
-#endif
-
-#define vsnprintf        pg_vsnprintf
-#define snprintf        pg_snprintf
-#define vsprintf        pg_vsprintf
-#define sprintf            pg_sprintf
-#define vfprintf        pg_vfprintf
-#define fprintf            pg_fprintf
-#define vprintf            pg_vprintf
-#define printf(...)        pg_printf(__VA_ARGS__)
-
-/*
- * Put back "_" too; but rather than making it just gettext() as the core
- * code does, make it dgettext() so that the right things will happen in
- * loadable modules (if they've set up TEXTDOMAIN correctly).  Note that
- * we can't just set TEXTDOMAIN here, because this file is used by more
- * extensions than just PL/Perl itself.
- */
-#undef _
-#define _(x) dgettext(TEXTDOMAIN, x)
-
-/* put back the definition of isnan if needed */
-#ifdef _MSC_VER
-#ifndef isnan
-#define isnan(x) _isnan(x)
-#endif
-#endif
-
-/* perl version and platform portability */
-#include "ppport.h"
-
-/*
- * perl might have included stdbool.h.  If we also did that earlier (see c.h),
- * then that's fine.  If not, we probably rejected it for some reason.  In
- * that case, undef bool and proceed with our own bool.  (Note that stdbool.h
- * makes bool a macro, but our own replacement is a typedef, so the undef
- * makes ours visible again).
- */
-#ifndef PG_USE_STDBOOL
-#ifdef bool
-#undef bool
-#endif
-#endif
-
-/* supply HeUTF8 if it's missing - ppport.h doesn't supply it, unfortunately */
-#ifndef HeUTF8
-#define HeUTF8(he)               ((HeKLEN(he) == HEf_SVKEY) ?               \
-                                SvUTF8(HeKEY_sv(he)) :                   \
-                                (U32)HeKUTF8(he))
-#endif
-
-/* supply GvCV_set if it's missing - ppport.h doesn't supply it, unfortunately */
-#ifndef GvCV_set
-#define GvCV_set(gv, cv)        (GvCV(gv) = cv)
-#endif
-
-/* Perl 5.19.4 changed array indices from I32 to SSize_t */
-#if PERL_BCDVERSION >= 0x5019004
-#define AV_SIZE_MAX SSize_t_MAX
-#else
-#define AV_SIZE_MAX I32_MAX
-#endif
+#include "plperl_system.h"

 /* declare routines from plperl.c for access by .xs files */
 HV           *plperl_spi_exec(char *, int);
diff --git a/src/pl/plperl/plperl_system.h b/src/pl/plperl/plperl_system.h
new file mode 100644
index 0000000000..61550db2a5
--- /dev/null
+++ b/src/pl/plperl/plperl_system.h
@@ -0,0 +1,215 @@
+/*-------------------------------------------------------------------------
+ *
+ * plperl_system.h
+ *      Pull in Perl's system header files.
+ *
+ * We break this out as a separate header file to precisely control
+ * the scope of the "system_header" pragma.  No Postgres-specific
+ * declarations should be put here.  However, we do include some stuff
+ * that is meant to prevent conflicts between our code and Perl.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1995, Regents of the University of California
+ *
+ * src/pl/plperl/plperl_system.h
+ */
+
+#ifndef PL_PERL_SYSTEM_H
+#define PL_PERL_SYSTEM_H
+
+/*
+ * Newer versions of the perl headers trigger a lot of warnings with our
+ * preferred compiler flags (at least -Wdeclaration-after-statement,
+ * -Wshadow=compatible-local are known to be problematic). The system_header
+ * pragma hides warnings from within the rest of this file, if supported.
+ */
+#ifdef HAVE_PRAGMA_GCC_SYSTEM_HEADER
+#pragma GCC system_header
+#endif
+
+/* stop perl headers from hijacking stdio and other stuff on Windows */
+#ifdef WIN32
+#define WIN32IO_IS_STDIO
+#endif                            /* WIN32 */
+
+/*
+ * Supply a value of PERL_UNUSED_DECL that will satisfy gcc - the one
+ * perl itself supplies doesn't seem to.
+ */
+#define PERL_UNUSED_DECL pg_attribute_unused()
+
+/*
+ * Sometimes perl carefully scribbles on our *printf macros.
+ * So we undefine them here and redefine them after it's done its dirty deed.
+ */
+#undef vsnprintf
+#undef snprintf
+#undef vsprintf
+#undef sprintf
+#undef vfprintf
+#undef fprintf
+#undef vprintf
+#undef printf
+
+/*
+ * Perl scribbles on the "_" macro too.
+ */
+#undef _
+
+/*
+ * ActivePerl 5.18 and later are MinGW-built, and their headers use GCC's
+ * __inline__.  Translate to something MSVC recognizes. Also, perl.h sometimes
+ * defines isnan, so undefine it here and put back the definition later if
+ * perl.h doesn't.
+ */
+#ifdef _MSC_VER
+#define __inline__ inline
+#ifdef isnan
+#undef isnan
+#endif
+/* Work around for using MSVC and Strawberry Perl >= 5.30. */
+#define __builtin_expect(expr, val) (expr)
+#endif
+
+/*
+ * Regarding bool, both PostgreSQL and Perl might use stdbool.h or not,
+ * depending on configuration.  If both agree, things are relatively harmless.
+ * If not, things get tricky.  If PostgreSQL does but Perl does not, define
+ * HAS_BOOL here so that Perl does not redefine bool; this avoids compiler
+ * warnings.  If PostgreSQL does not but Perl does, we need to undefine bool
+ * after we include the Perl headers; see below.
+ */
+#ifdef PG_USE_STDBOOL
+#define HAS_BOOL 1
+#endif
+
+/*
+ * Get the basic Perl API.  We use PERL_NO_GET_CONTEXT mode so that our code
+ * can compile against MULTIPLICITY Perl builds without including XSUB.h.
+ */
+#define PERL_NO_GET_CONTEXT
+#include "EXTERN.h"
+#include "perl.h"
+
+/*
+ * We want to include XSUB.h only within .xs files, because on some platforms
+ * it undesirably redefines a lot of libc functions.  But it must appear
+ * before ppport.h, so use a #define flag to control inclusion here.
+ */
+#ifdef PG_NEED_PERL_XSUB_H
+/*
+ * On Windows, win32_port.h defines macros for a lot of these same functions.
+ * To avoid compiler warnings when XSUB.h redefines them, #undef our versions.
+ */
+#ifdef WIN32
+#undef accept
+#undef bind
+#undef connect
+#undef fopen
+#undef fstat
+#undef kill
+#undef listen
+#undef lstat
+#undef mkdir
+#undef open
+#undef putenv
+#undef recv
+#undef rename
+#undef select
+#undef send
+#undef socket
+#undef stat
+#undef unlink
+#endif
+
+#include "XSUB.h"
+#endif
+
+/* put back our *printf macros ... this must match src/include/port.h */
+#ifdef vsnprintf
+#undef vsnprintf
+#endif
+#ifdef snprintf
+#undef snprintf
+#endif
+#ifdef vsprintf
+#undef vsprintf
+#endif
+#ifdef sprintf
+#undef sprintf
+#endif
+#ifdef vfprintf
+#undef vfprintf
+#endif
+#ifdef fprintf
+#undef fprintf
+#endif
+#ifdef vprintf
+#undef vprintf
+#endif
+#ifdef printf
+#undef printf
+#endif
+
+#define vsnprintf        pg_vsnprintf
+#define snprintf        pg_snprintf
+#define vsprintf        pg_vsprintf
+#define sprintf            pg_sprintf
+#define vfprintf        pg_vfprintf
+#define fprintf            pg_fprintf
+#define vprintf            pg_vprintf
+#define printf(...)        pg_printf(__VA_ARGS__)
+
+/*
+ * Put back "_" too; but rather than making it just gettext() as the core
+ * code does, make it dgettext() so that the right things will happen in
+ * loadable modules (if they've set up TEXTDOMAIN correctly).  Note that
+ * we can't just set TEXTDOMAIN here, because this file is used by more
+ * extensions than just PL/Perl itself.
+ */
+#undef _
+#define _(x) dgettext(TEXTDOMAIN, x)
+
+/* put back the definition of isnan if needed */
+#ifdef _MSC_VER
+#ifndef isnan
+#define isnan(x) _isnan(x)
+#endif
+#endif
+
+/* perl version and platform portability */
+#include "ppport.h"
+
+/*
+ * perl might have included stdbool.h.  If we also did that earlier (see c.h),
+ * then that's fine.  If not, we probably rejected it for some reason.  In
+ * that case, undef bool and proceed with our own bool.  (Note that stdbool.h
+ * makes bool a macro, but our own replacement is a typedef, so the undef
+ * makes ours visible again).
+ */
+#ifndef PG_USE_STDBOOL
+#ifdef bool
+#undef bool
+#endif
+#endif
+
+/* supply HeUTF8 if it's missing - ppport.h doesn't supply it, unfortunately */
+#ifndef HeUTF8
+#define HeUTF8(he)               ((HeKLEN(he) == HEf_SVKEY) ?               \
+                                SvUTF8(HeKEY_sv(he)) :                   \
+                                (U32)HeKUTF8(he))
+#endif
+
+/* supply GvCV_set if it's missing - ppport.h doesn't supply it, unfortunately */
+#ifndef GvCV_set
+#define GvCV_set(gv, cv)        (GvCV(gv) = cv)
+#endif
+
+/* Perl 5.19.4 changed array indices from I32 to SSize_t */
+#if PERL_BCDVERSION >= 0x5019004
+#define AV_SIZE_MAX SSize_t_MAX
+#else
+#define AV_SIZE_MAX I32_MAX
+#endif
+
+#endif                            /* PL_PERL_SYSTEM_H */
diff --git a/src/pl/plpython/Makefile b/src/pl/plpython/Makefile
index ef0a5905ae..f959083a0b 100644
--- a/src/pl/plpython/Makefile
+++ b/src/pl/plpython/Makefile
@@ -39,6 +39,7 @@ DATA = $(NAME)u.control $(NAME)u--1.0.sql
 # header files to install - it's not clear which of these might be needed
 # so install them all.
 INCS =     plpython.h \
+    plpython_system.h \
     plpy_cursorobject.h \
     plpy_elog.h \
     plpy_exec.h \
@@ -120,7 +121,7 @@ install-data: installdirs

 uninstall-data:
     rm -f $(addprefix '$(DESTDIR)$(datadir)/extension'/, $(notdir $(DATA)))
-    rm -f $(addprefix '$(DESTDIR)$(includedir_server)'/, plpython.h plpy_util.h)
+    rm -f $(addprefix '$(DESTDIR)$(includedir_server)'/, $(INCS))

 .PHONY: install-data uninstall-data

diff --git a/src/pl/plpython/meson.build b/src/pl/plpython/meson.build
index 9f4603a8b5..0071817e44 100644
--- a/src/pl/plpython/meson.build
+++ b/src/pl/plpython/meson.build
@@ -67,6 +67,7 @@ install_headers(
   'plpy_typeio.h',
   'plpy_util.h',
   'plpython.h',
+  'plpython_server.h',
   install_dir: dir_include_server,
 )

diff --git a/src/pl/plpython/plpython.h b/src/pl/plpython/plpython.h
index 91f6f8d860..f33f7b5920 100644
--- a/src/pl/plpython/plpython.h
+++ b/src/pl/plpython/plpython.h
@@ -20,27 +20,10 @@
 #endif

 /*
- * Python versions <= 3.8 otherwise define a replacement, causing macro
- * redefinition warnings.
+ * Pull in Python headers via a wrapper header, to control the scope of
+ * the system_header pragma therein.
  */
-#define HAVE_SNPRINTF 1
-
-#if defined(_MSC_VER) && defined(_DEBUG)
-/* Python uses #pragma to bring in a non-default libpython on VC++ if
- * _DEBUG is defined */
-#undef _DEBUG
-/* Also hide away errcode, since we load Python.h before postgres.h */
-#define errcode __msvc_errcode
-#include <Python.h>
-#undef errcode
-#define _DEBUG
-#elif defined (_MSC_VER)
-#define errcode __msvc_errcode
-#include <Python.h>
-#undef errcode
-#else
-#include <Python.h>
-#endif
+#include "plpython_system.h"

 /* define our text domain for translations */
 #undef TEXTDOMAIN
diff --git a/src/pl/plpython/plpython_system.h b/src/pl/plpython/plpython_system.h
new file mode 100644
index 0000000000..9532789ae2
--- /dev/null
+++ b/src/pl/plpython/plpython_system.h
@@ -0,0 +1,53 @@
+/*-------------------------------------------------------------------------
+ *
+ * plpython_system.h - pull in Python's system header files
+ *
+ * We break this out as a separate header file to precisely control
+ * the scope of the "system_header" pragma.  No Postgres-specific
+ * declarations should be put here.  However, we do include some stuff
+ * that is meant to prevent conflicts between our code and Python.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/pl/plpython/plpython_system.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef PLPYTHON_SYSTEM_H
+#define PLPYTHON_SYSTEM_H
+
+/*
+ * Newer versions of the Python headers trigger a lot of warnings with our
+ * preferred compiler flags (at least -Wdeclaration-after-statement is known
+ * to be problematic). The system_header pragma hides warnings from within
+ * the rest of this file, if supported.
+ */
+#ifdef HAVE_PRAGMA_GCC_SYSTEM_HEADER
+#pragma GCC system_header
+#endif
+
+/*
+ * Python versions <= 3.8 otherwise define a replacement, causing macro
+ * redefinition warnings.
+ */
+#define HAVE_SNPRINTF 1
+
+#if defined(_MSC_VER) && defined(_DEBUG)
+/* Python uses #pragma to bring in a non-default libpython on VC++ if
+ * _DEBUG is defined */
+#undef _DEBUG
+/* Also hide away errcode, since we load Python.h before postgres.h */
+#define errcode __msvc_errcode
+#include <Python.h>
+#undef errcode
+#define _DEBUG
+#elif defined (_MSC_VER)
+#define errcode __msvc_errcode
+#include <Python.h>
+#undef errcode
+#else
+#include <Python.h>
+#endif
+
+#endif                            /* PLPYTHON_SYSTEM_H */