Re: plpython vs _POSIX_C_SOURCE
От | Andres Freund |
---|---|
Тема | Re: plpython vs _POSIX_C_SOURCE |
Дата | |
Msg-id | 20230124190721.mi7l5c3n2tsrqjno@awork3.anarazel.de обсуждение исходный текст |
Ответ на | Re: plpython vs _POSIX_C_SOURCE (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
Re: plpython vs _POSIX_C_SOURCE
(Tom Lane <tgl@sss.pgh.pa.us>)
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: