public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v4 2/3] New test for timeline-tracking of walsender 5+ messages / 3 participants [nested] [flat]
* [PATCH v4 2/3] New test for timeline-tracking of walsender @ 2020-12-09 08:21 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-12-09 08:21 UTC (permalink / raw) Walsender should track timeline changes while sending a historic timeline. Add a test for it. --- src/test/recovery/t/001_stream_rep.pl | 41 ++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl index 9e31a53de7..e78e98fb43 100644 --- a/src/test/recovery/t/001_stream_rep.pl +++ b/src/test/recovery/t/001_stream_rep.pl @@ -2,8 +2,9 @@ use strict; use warnings; use PostgresNode; +use Time::HiRes qw(usleep); use TestLib; -use Test::More tests => 36; +use Test::More tests => 37; # Initialize primary node my $node_primary = get_new_node('primary'); @@ -409,3 +410,41 @@ ok( ($phys_restart_lsn_pre cmp $phys_restart_lsn_post) == 0, my $primary_data = $node_primary->data_dir; ok(!-f "$primary_data/pg_wal/$segment_removed", "WAL segment $segment_removed recycled after physical slot advancing"); + +# +# Check if timeline-increment works while reading a historic timeline. +my $node_primary_2 = get_new_node('primary_2'); +# archiving is needed to create .paritial segment +$node_primary_2->init(allows_streaming => 1, has_archiving => 1); +$node_primary_2->start; +$node_primary_2->backup($backup_name); +my $node_standby_3 = get_new_node('standby_3'); +$node_standby_3->init_from_backup($node_primary_2, $backup_name, + has_streaming => 1); +$node_primary_2->stop; +$node_primary_2->set_standby_mode; # increment primary timeline +$node_primary_2->start; +$node_primary_2->promote; +my $logstart = $node_standby_3->current_log_position(); +$node_standby_3->start; + +my $success = 0; +for (my $i = 0 ; $i < 1000; $i++) +{ + if ($node_standby_3->find_in_log( + "requested WAL segment [0-9A-F]+ has already been removed", + $logstart)) + { + last; + } + elsif ($node_standby_3->find_in_log( + "End of WAL reached on timeline", + $logstart)) + { + $success = 1; + last; + } + usleep(100_000); +} + +ok($success, 'Timeline increment while reading a historic timeline'); -- 2.27.0 ----Next_Part(Thu_Jan__7_16_32_36_2021_122)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v4-0003-Fix-timeline-tracking-failure-while-sending-a-his.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* plpython vs _POSIX_C_SOURCE @ 2023-01-24 16:58 Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-01-24 16:58 UTC (permalink / raw) To: pgsql-hackers; Tom Lane <[email protected]>; Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]> 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/[email protected] [2] https://postgr.es/m/[email protected] ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: plpython vs _POSIX_C_SOURCE @ 2023-01-24 17:55 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2023-01-24 17:55 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]> Andres Freund <[email protected]> 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: plpython vs _POSIX_C_SOURCE @ 2023-01-24 19:07 Andres Freund <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Andres Freund @ 2023-01-24 19:07 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]> Hi, On 2023-01-24 12:55:15 -0500, Tom Lane wrote: > Andres Freund <[email protected]> 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 <[email protected]> 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: plpython vs _POSIX_C_SOURCE @ 2023-01-24 21:16 Tom Lane <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2023-01-24 21:16 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]> Andres Freund <[email protected]> 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 <[email protected]> > 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 ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-01-24 21:16 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-12-09 08:21 [PATCH v4 2/3] New test for timeline-tracking of walsender Kyotaro Horiguchi <[email protected]> 2023-01-24 16:58 plpython vs _POSIX_C_SOURCE Andres Freund <[email protected]> 2023-01-24 17:55 ` Re: plpython vs _POSIX_C_SOURCE Tom Lane <[email protected]> 2023-01-24 19:07 ` Re: plpython vs _POSIX_C_SOURCE Andres Freund <[email protected]> 2023-01-24 21:16 ` Re: plpython vs _POSIX_C_SOURCE Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox