Re: plpython vs _POSIX_C_SOURCE

Hi,

On 2023-01-24 12:55:15 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > The background for the undefines is that _POSIX_C_SOURCE needs to be defined
> > the same for the whole compilation, not change in the middle, and Python.h
> > defines it. To protect "our" parts a11cf433413 instituted the rule that all
> > postgres headers have to be included first. But then that promptly got broken
> > in 147c2482542.
>
> > But apparently the breakage in 147c2482542 was partial enough that we didn't
> > run into obvious trouble so far (although I wonder if some of the linkage
> > issues we had in the past with plpython could be related).
>
> I found the discussion thread that led up to a11cf433413:
>
> https://www.postgresql.org/message-id/flat/4DB3B546.9080508%40dunslane.net
>
> What we originally set out to fix, AFAICS, was compiler warnings about
> _POSIX_C_SOURCE getting redefined with a different value. I think that'd
> only happen if pyconfig.h had originally been constructed on a machine
> where _POSIX_C_SOURCE was different from what prevails in a Postgres
> build.

Python's _POSIX_C_SOURCE value is set to a specific value in their configure
script:

if test $define_xopen_source = yes
then
# X/Open 7, incorporating POSIX.1-2008
AC_DEFINE(_XOPEN_SOURCE, 700,
Define to the level of X/Open that your system supports)

# On Tru64 Unix 4.0F, defining _XOPEN_SOURCE also requires
# definition of _XOPEN_SOURCE_EXTENDED and _POSIX_C_SOURCE, or else
# several APIs are not declared. Since this is also needed in some
# cases for HP-UX, we define it globally.
AC_DEFINE(_XOPEN_SOURCE_EXTENDED, 1,
Define to activate Unix95-and-earlier features)

AC_DEFINE(_POSIX_C_SOURCE, 200809L, Define to activate features from IEEE Stds 1003.1-2008)
fi

So the concrete values don't depend on the environment (but whether they are
set does, sunos, hpux as well as a bunch of obsolete OS versions don't). But I
somehow doubt we'll see a different _POSIX_C_SOURCE value coming up.

> So I wouldn't see this warning, and I venture that you'd never see
> it on any other Linux/glibc platform either.

Yea, it works just fine on linux without it, I tried that already.

In fact, removing the _POSIX_C_SOURCE stuff build without additional warnings (*)
on freebsd, netbsd, linux (centos 7, fedora rawhide, debian bullseye, sid),
macOS, openbsd, windows with msvc and mingw.

Those I can test automatedly with the extended set of CI OSs:
https://cirrus-ci.com/build/4853456020701184

Some of the OSs are still running tests, but I doubt there's a runtime issue.

Solaris and AIX are the ones missing.

I guess I'll test them manually. It seems promising not to need this stuff
anymore?

(*) I see one existing plpython related warning on netbsd 9:
[18:49:12.710] ld: warning: libintl.so.1, needed by /usr/pkg/lib/libpython3.9.so, may conflict with libintl.so.8

But that's not related to this change. I assume it's an issue with one side
using libintl from /usr/lib and the other from /usr/pkg/lib.

> The 2011 thread started with concerns about Windows, where it's a lot easier
> to believe that there might be mismatched build environments. But maybe
> nobody's actually set up a Windows box with that particular problem since
> 2011.

The native python doesn't have the issue, the windows pyconfig.h doesn't
define _POSIX_SOURCE et al. It's possible that there's a problem with older
mingw versions though - but I don't really care about old mingw versions, tbh.

> Whether inconsistency in _POSIX_C_SOURCE could lead to worse problems
> than a compiler warning isn't entirely clear to me, but it certainly
> seems possible.

I'm sure it can lead to compiler errors, there's IIRC some struct members only
defined for certain values.

> Anyway, I'm still of the opinion that what a11cf433413 tried to do
> was the best available fix, and we need to do whatever we have to do
> to plpython's headers to reinstate that coding rule.

You think it's not a viable path to just remove the _POSIX_C_SOURCE,
_XOPEN_SOURCE undefines?

> > The most minimal fix I can see is to institute the rule that no plpy_*.h
> > header can include a postgres header other than plpython.h.
>
> Doesn't sound *too* awful.

It's not too bad to make the change, I'm less hopeful about it staying
fixed. I can't think of a reasonable way to make violations of the rule error
out.

> > Or we could see what breaks if we just don't care about _POSIX_C_SOURCE -
> > evidently we haven't succeeded in making this work for a long time.
>
> Well, hoverfly is broken right now ...

What I mean is that we haven't handled the _POSIX_C_SOURCE stuff correctly for
a long time and the only problem that became apparent is hoverfly's issue, and
that's a problem of undefining _POSIX_C_SOURCE, rather than it being
redefined.

> > * Sometimes python carefully scribbles on our *printf macros.
> > * So we undefine them here and redefine them after it's done its dirty deed.
>
> > I didn't find code in recent-ish python to do that. Perhaps we should try to
> > get away with not doing that?
>
> That would be nice. This old code was certainly mostly concerned with
> python 2, maybe python 3 no longer does that?

There's currently no non-comment references to *printf in their headers. The
only past reference was removed in:

commit e822e37946f27c09953bb5733acf3b07c2db690f
Author: Victor Stinner <vstinner(at)python(dot)org>
Date: 2020-06-15 21:59:47 +0200

bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)
...

with the following relevant hunk:

@@ -307,21 +309,6 @@ PyAPI_FUNC(int) PyUnicodeTranslateError_SetReason(
const char *reason /* UTF-8 encoded string */
);

-/* These APIs aren't really part of the error implementation, but
- often needed to format error messages; the native C lib APIs are
- not available on all platforms, which is why we provide emulations
- for those platforms in Python/mysnprintf.c,
- WARNING: The return value of snprintf varies across platforms; do
- not rely on any particular behavior; eventually the C99 defn may
- be reliable.
-*/
-#if defined(MS_WIN32) && !defined(HAVE_SNPRINTF)
-# define HAVE_SNPRINTF
-# define snprintf _snprintf
-# define vsnprintf _vsnprintf
-#endif
-
-#include <stdarg.h>
PyAPI_FUNC(int) PyOS_snprintf(char *str, size_t size, const char *format, ...)
Py_GCC_ATTRIBUTE((format(printf, 3, 4)));
PyAPI_FUNC(int) PyOS_vsnprintf(char *str, size_t size, const char *format, va_list va)

Which suggests an easier fix would be to just to do

/*
* Python versions <= 3.8 otherwise define a replacement, causing
* macro redefinition warnings.
*/
#define HAVE_SNPRINTF

And have that be enough for all python versions?

Greetings,

Andres Freund