Обсуждение: plpython vs _POSIX_C_SOURCE

Hi,

A recent commit of mine [1] broke compilation of plpython on AIX [2]. But my
commit turns out to only be very tangentially related - it only causes a
failure because it references clock_gettime() in an inline function instead of
a macro and, as it turns out, plpython currently breaks references to
clock_gettime() and lots of other things.

From [2]
> There's nice bit in plpython.h:
>
> /*
>  * Include order should be: postgres.h, other postgres headers, plpython.h,
>  * other plpython headers.  (In practice, other plpython headers will also
>  * include this file, so that they can compile standalone.)
>  */
> #ifndef POSTGRES_H
> #error postgres.h must be included before plpython.h
> #endif
>
> /*
>  * Undefine some things that get (re)defined in the Python headers. They aren't
>  * used by the PL/Python code, and all PostgreSQL headers should be included
>  * earlier, so this should be pretty safe.
>  */
> #undef _POSIX_C_SOURCE
> #undef _XOPEN_SOURCE
>
>
> the relevant stuff in time.h is indeed guarded by
> #if _XOPEN_SOURCE>=500
>
>
> I don't think the plpython actually code follows the rule about including all
> postgres headers earlier.
>
> plpy_typeio.h:
>
> #include "access/htup.h"
> #include "fmgr.h"
> #include "plpython.h"
> #include "utils/typcache.h"
> [...]
>
> The include order aspect was perhaps feasible when there just was plpython.c,
> but with the split into many different C files and many headers, it seems hard
> to maintain. There's a lot of violations afaics.
> 
> The undefines were added in a11cf433413, the split in 147c2482542.


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 don't see a good solution here. I don't think the include order can
trivially be repaired, as long as plpy_*.h headers include postgres headers,
because there's no way to order two plpy_*.h includes in a .c file and have
all postgres headers come first.


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.


A completely different approach would be to for our build to acquire the value
of _POSIX_C_SOURCE _XOPEN_SOURCE from Python.h and define them when compiling
plpython .c files. That has some dangers of incompatibilities with the rest of
the build though.  But it'd allow us to get rid of an obviously hard to
enforce rule.

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.


Some other semi-related things:

An old comments:
/* Also hide away errcode, since we load Python.h before postgres.h */
#define errcode __msvc_errcode

but we don't do include Python.h before postgres.h...

We try to force linking to a non-debug python:
#if defined(_MSC_VER) && defined(_DEBUG)
/* Python uses #pragma to bring in a non-default libpython on VC++ if
 * _DEBUG is defined */
#undef _DEBUG

Which seems ill-advised? That's from d8f75d41315 in 2006.


python scribbling over our macros:
/*
 * 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?


Greetings,

Andres Freund

[1] https://postgr.es/m/E1pJ6NT-004jOH-DF@gemulon.postgresql.org
[2] https://postgr.es/m/20230121190303.7xjiwdg3gvb62lu3@awork3.anarazel.de

Andres Freund <andres@anarazel.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.  On my RHEL8 box, I see that /usr/include/python3.6m/pyconfig-64.h
unconditionally does

#define _POSIX_C_SOURCE 200809L

while /usr/include/features.h can set a few different values, but the
one that would always prevail for us is

#ifdef _GNU_SOURCE
...
# undef  _POSIX_C_SOURCE
# define _POSIX_C_SOURCE    200809L

So I wouldn't see this warning, and I venture that you'd never see
it on any other Linux/glibc platform either.  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.

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.

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.

> 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.

> 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 ...

>  * 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?  (Unfortunately, the
_POSIX_C_SOURCE business is clearly still there in current python.)

            regards, tom lane

Hi,

On 2023-01-24 12:55:15 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.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@python.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

Andres Freund <andres@anarazel.de> writes:
> Python's _POSIX_C_SOURCE value is set to a specific value in their configure
> script:

> if test $define_xopen_source = yes
> then
>   ...
>   AC_DEFINE(_POSIX_C_SOURCE, 200809L, Define to activate features from IEEE Stds 1003.1-2008)
> fi

Hm.  I looked into Python 3.2 (the oldest release we still support)
and it has similar code but

  AC_DEFINE(_POSIX_C_SOURCE, 200112L, Define to activate features from IEEE Stds 1003.1-2001)

So yeah it's fixed (or else not defined) for any particular Python
release, but could vary across releases.

> Solaris and AIX are the ones missing.
> I guess I'll test them manually. It seems promising not to need this stuff
> anymore?

Given that hoverfly is AIX, I'm betting there's an issue there.

>> 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?

I think at the least that will result in warnings on some platforms,
and at the worst in actual build problems.  Maybe there are no more
of the latter a dozen years after the fact, but ...

>> 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@python.org>
> Date:   2020-06-15 21:59:47 +0200
>     bpo-36020: Remove snprintf macro in pyerrors.h (GH-20889)

Oh, interesting.

> 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?

Nice idea.  We did not have that option while we were using HAVE_SNPRINTF
ourselves, but now that we are not I concur that this should work.
(I confirmed that their code looks the same in Python 3.2.)
Note that you'd better make it

#define HAVE_SNPRINTF 1

or you risk macro-redefinition warnings.

            regards, tom lane

Hi,

On 2023-01-24 16:16:06 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Python's _POSIX_C_SOURCE value is set to a specific value in their configure
> > script:
>
> > if test $define_xopen_source = yes
> > then
> >   ...
> >   AC_DEFINE(_POSIX_C_SOURCE, 200809L, Define to activate features from IEEE Stds 1003.1-2008)
> > fi
>
> Hm.  I looked into Python 3.2 (the oldest release we still support)
> and it has similar code but
>
>   AC_DEFINE(_POSIX_C_SOURCE, 200112L, Define to activate features from IEEE Stds 1003.1-2001)
>
> So yeah it's fixed (or else not defined) for any particular Python
> release, but could vary across releases.

Looks like it changed in 3.3:

$ git grep -E 'AC_DEFINE.*_POSIX_C_SOURCE' v3.2 v3.3.0
v3.2:configure.in:  AC_DEFINE(_POSIX_C_SOURCE, 200112L, Define to activate features from IEEE Stds 1003.1-2001)
v3.3.0:configure.ac:  AC_DEFINE(_POSIX_C_SOURCE, 200809L, Define to activate features from IEEE Stds 1003.1-2008)

I'm not sure we need to care a lot about a build with python 3.3 triggering a
bunch of warnings.

Personally I'd just bump the python requirements to well above it - the last
3.2 release was Oct. 12, 2014.

Official EOL date:
Ver    Last Release    EOL Date
3.2    2014-10-11    2016-02-20
3.3    2017-09-19    2017-09-29
From 3.4 on there's just an official last release:
3.4    2019-03-18
3.5    2020-09-05
3.6    2021-09-04
3.7    2023-06-??



> > Solaris and AIX are the ones missing.
> > I guess I'll test them manually. It seems promising not to need this stuff
> > anymore?
>
> Given that hoverfly is AIX, I'm betting there's an issue there.

Doesn't look that way.

I found plenty problems on AIX, but all independent of _POSIX_C_SOURCE.


Both autoconf and meson builds seem to need externally specified
-D_LARGE_FILES=1 to build successfully when using plpython, otherwise we end
up with conflicting signatures with lseek. I see that Noah has that in his
buildfarm config.  ISTM that we should just move that into our build specs.




To get 64 bit autoconf to link plpython3.so correctly, I needed to add to
manually add -lpthread:
ld: 0711-317 ERROR: Undefined symbol: .pthread_init
...

I suspect Noah might not hit this, because one of the dependencies he has
enabled already adds it to the backend LDFLAGS.


Also for autoconf, I needed to link
$prefix/lib/python3.11/config-3.11/libpython3.11.a
to
$prefix/lib/libpython3.11.a
That might be a python version difference or be related to building python
with --enable-shared - but I see saw other problems without --enable-shared.



I ran out of energy to test on aix with xlc, I spent way more time on this
than I have already. I'll pick it up later.



I also tested 64bit solaris. No relevant warnings (lots of other warnings
though), tests pass, with both acc and gcc.



> >> 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?
>
> I think at the least that will result in warnings on some platforms,
> and at the worst in actual build problems.  Maybe there are no more
> of the latter a dozen years after the fact, but ...

I think it might be ok. I tested nearly all OSs that we support, with the
exception of DragonFlyBSD and Illumos, which both are very similar to tested
operating systems.


> Nice idea.  We did not have that option while we were using HAVE_SNPRINTF
> ourselves, but now that we are not I concur that this should work.

Cool.


> (I confirmed that their code looks the same in Python 3.2.)
> Note that you'd better make it
>
> #define HAVE_SNPRINTF 1
>
> or you risk macro-redefinition warnings.

Good point.

I guess I'll push that part first, given that we have agreement how it should
look like.

Greetings,

Andres Freund

Hi,

On 2023-01-24 17:48:56 -0800, Andres Freund wrote:
> Also for autoconf, I needed to link
> $prefix/lib/python3.11/config-3.11/libpython3.11.a
> to
> $prefix/lib/libpython3.11.a
> That might be a python version difference or be related to building python
> with --enable-shared - but I see saw other problems without --enable-shared.

That actually doesn't quite work right. One needs to either link to the file
by name (i.e. just $prefix/lib/libpython3.11.so instead of -lpython3.11), or
create a wrapper .a "manually". I.e.

ar crs $prefix/lib/libpython3.11.a $prefix/lib/libpython3.11.so

I tried quite a few things and got confused between the attempts.


> I ran out of energy to test on aix with xlc, I spent way more time on this
> than I have already. I'll pick it up later.

Building in the background now.

Greetings,

Andres Freund

Hi,

On 2023-01-24 18:32:46 -0800, Andres Freund wrote:
> > I ran out of energy to test on aix with xlc, I spent way more time on this
> > than I have already. I'll pick it up later.
> 
> Building in the background now.

Also passes.


Thus I think getting rid of the #undefines is the best plan going
forward. Fewer complicated rules to follow => fewer rule violations.


Patches attached.


Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> Patches attached.

+1 for 0001.  I'm still nervous about 0002.  However, maybe the
cases that we had trouble with are legacy issues that nobody cares
about anymore in 2023.  We can always look for another answer if
we get complaints, I guess.

            regards, tom lane

Hi,

On 2023-01-24 23:37:44 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Patches attached.
> 
> +1 for 0001.

Cool, will push tomorrow.


> I'm still nervous about 0002.  However, maybe the cases that we had trouble
> with are legacy issues that nobody cares about anymore in 2023.  We can
> always look for another answer if we get complaints, I guess.

Yea, it's a patch that should be easily revertable, if it comes to that. I'll
add a note to the commit message about potentially needing to do that if
there's not easily addressed fallout.

Greetings,

Andres Freund

On Tue, Jan 24, 2023 at 11:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Patches attached.
>
> +1 for 0001.  I'm still nervous about 0002.  However, maybe the
> cases that we had trouble with are legacy issues that nobody cares
> about anymore in 2023.  We can always look for another answer if
> we get complaints, I guess.

It feels like things are changing so fast these days that whatever was
happening 12 years ago is not likely to be relevant. Compilers change
enough to cause warnings and even errors in just a few years. A decade
is long enough for an entire platform to become irrelevant.

Plus, the cost of experimentation here seems very low. Sure, something
might break, but if it does, we can just change it back, or change it
again. That's not really a big deal. The thing that would be a big
deal, maybe, is if we released and only found out afterward that this
caused some subtle and horrible problem for which we had no
back-patchable fix, but that seems pretty unlikely.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

Pushed the patches. So far no fallout, and hoverfly recovered.

I just checked a few of the more odd animals (Illumos, Solaris, old OpenBSD,
AIX) that already ran without finding new warnings.

There's a few more animals to run before I'll fully relax though.


On 2023-01-25 08:31:23 -0500, Robert Haas wrote:
> Plus, the cost of experimentation here seems very low. Sure, something
> might break, but if it does, we can just change it back, or change it
> again. That's not really a big deal. The thing that would be a big
> deal, maybe, is if we released and only found out afterward that this
> caused some subtle and horrible problem for which we had no
> back-patchable fix, but that seems pretty unlikely.

Agreed.

Greetings,

Andres Freund