public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v17 6/8] Row pattern recognition patch (docs). 20+ messages / 7 participants [nested] [flat]
* [PATCH v17 6/8] Row pattern recognition patch (docs). @ 2024-04-28 11:00 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Tatsuo Ishii @ 2024-04-28 11:00 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 82 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 172 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..b0b1d1c51e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,88 @@ WHERE pos < 3; <literal>rank</literal> less than 3. </para> + <para> + Row pattern common syntax can be used to perform row pattern recognition + in a query. The row pattern common syntax includes two sub + clauses: <literal>DEFINE</literal> + and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines + definition variables along with an expression. The expression must be a + logical expression, which means it must + return <literal>TRUE</literal>, <literal>FALSE</literal> + or <literal>NULL</literal>. The expression may comprise column references + and functions. Window functions, aggregate functions and subqueries are + not allowed. An example of <literal>DEFINE</literal> is as follows. + +<programlisting> +DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +</programlisting> + + Note that <function>PREV</function> returns the price column in the + previous row if it's called in a context of row pattern recognition. Thus in + the second line the definition variable "UP" is <literal>TRUE</literal> + when the price column in the current row is greater than the price column + in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when + the price column in the current row is lower than the price column in the + previous row. + </para> + <para> + Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be + used. <literal>PATTERN</literal> defines a sequence of rows that satisfies + certain conditions. For example following <literal>PATTERN</literal> + defines that a row starts with the condition "LOWPRICE", then one or more + rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that + "+" means one or more matches. Also you can use "*", which means zero or + more matches. If a sequence of rows which satisfies the PATTERN is found, + in the starting row of the sequence of rows all window functions and + aggregates are shown in the target list. Note that aggregations only look + into the matched rows, rather than whole frame. On the second or + subsequent rows all window functions are NULL. Aggregates are NULL or 0 + (count case) depending on its aggregation definition. For rows that do not + match on the PATTERN, all window functions and aggregates are shown AS + NULL too, except count showing 0. This is because the rows do not match, + thus they are in an empty frame. Example of a <literal>SELECT</literal> + using the <literal>DEFINE</literal> and <literal>PATTERN</literal> clause + is as follows. + +<programlisting> +SELECT company, tdate, price, + first_value(price) OVER w, + max(price) OVER w, + count(price) OVER w +FROM stock + WINDOW w AS ( + PARTITION BY company + ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + AFTER MATCH SKIP PAST LAST ROW + INITIAL + PATTERN (LOWPRICE UP+ DOWN+) + DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +); +</programlisting> +<screen> + company | tdate | price | first_value | max | count +----------+------------+-------+-------------+-----+------- + company1 | 2023-07-01 | 100 | 100 | 200 | 4 + company1 | 2023-07-02 | 200 | | | 0 + company1 | 2023-07-03 | 150 | | | 0 + company1 | 2023-07-04 | 140 | | | 0 + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | 0 + company1 | 2023-07-08 | 130 | | | 0 + company1 | 2023-07-09 | 120 | | | 0 + company1 | 2023-07-10 | 130 | | | 0 +(10 rows) +</screen> + </para> + <para> When a query involves multiple window functions, it is possible to write out each one with a separate <literal>OVER</literal> clause, but this is diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 1928de5762..adbcb1f279 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -23124,6 +23124,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -23163,6 +23164,59 @@ SELECT count(*) FROM sometable; Other frame specifications can be used to obtain other effects. </para> + <para> + Row pattern recognition navigation functions are listed in + <xref linkend="functions-rpr-navigation-table"/>. These functions + can be used to describe DEFINE clause of Row pattern recognition. + </para> + + <table id="functions-rpr-navigation-table"> + <title>Row Pattern Navigation Functions</title> + <tgroup cols="1"> + <thead> + <row> + <entry role="func_table_entry"><para role="func_signature"> + Function + </para> + <para> + Description + </para></entry> + </row> + </thead> + + <tbody> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>prev</primary> + </indexterm> + <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the previous row; + returns NULL if there is no previous row in the window frame. + </para></entry> + </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>next</primary> + </indexterm> + <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the next row; + returns NULL if there is no next row in the window frame. + </para></entry> + </row> + + </tbody> + </tgroup> + </table> + <note> <para> The SQL standard defines a <literal>RESPECT NULLS</literal> or diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 066aed44e6..8f18718d58 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl The <replaceable class="parameter">frame_clause</replaceable> can be one of <synopsis> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] </synopsis> where <replaceable>frame_start</replaceable> @@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS a given peer group will be in the frame or excluded from it. </para> + <para> + The + optional <replaceable class="parameter">row_pattern_common_syntax</replaceable> + defines the <firstterm>row pattern recognition condition</firstterm> for + this + window. <replaceable class="parameter">row_pattern_common_syntax</replaceable> + includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST + ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls + how to proceed to next row position after a match + found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the + default) next row position is next to the last row of previous match. On + the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next + row position is always next to the last row of previous + match. <literal>DEFINE</literal> defines definition variables along with a + boolean expression. <literal>PATTERN</literal> defines a sequence of rows + that satisfies certain conditions using variables defined + in <literal>DEFINE</literal> clause. If the variable is not defined in + the <literal>DEFINE</literal> clause, it is implicitly assumed + following is defined in the <literal>DEFINE</literal> clause. + +<synopsis> +<literal>variable_name</literal> AS TRUE +</synopsis> + + Note that the maximu number of variables defined + in <literal>DEFINE</literal> clause is 26. + +<synopsis> +[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ] +PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...] +DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...] +</synopsis> + </para> + <para> The purpose of a <literal>WINDOW</literal> clause is to specify the behavior of <firstterm>window functions</firstterm> appearing in the query's -- 2.25.1 ----Next_Part(Sun_Apr_28_20_28_26_2024_444)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v17-0007-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-20 09:00 Thomas Munro <[email protected]> 1 sibling, 2 replies; 20+ messages in thread From: Thomas Munro @ 2024-11-20 09:00 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Fri, Nov 15, 2024 at 1:53 AM Peter Eisentraut <[email protected]> wrote: > On 14.11.24 08:48, Thomas Munro wrote: > > The three MinGW environments we test today are using ucrt, and > > configure detects the symbol on all. Namely: fairwren > > (msys2/mingw64), the CI mingw64 task and the mingw cross-build that > > runs on Linux in the CI CompilerWarnings task. As far as I know these > > are the reasons for, and mechanism by which, we keep MinGW support > > working. We have no policy requiring arbitrary old MinGW systems > > work, and we wouldn't know anyway. > > Right. So I think we could unwind this in steps. First, remove the > configure test for _configthreadlocale() and all the associated #ifdefs > in the existing ecpg code. This seems totally safe, it would just leave > behind MinGW older than 2016 and MSVC older than 2015, the latter of > which is already the current threshold. > > Then the question whether we want to re-enable the error checking on > _configthreadlocale() that was reverted by 2cf91ccb, or at least > something similar. This should also be okay based on your description > of the different Windows runtimes. I think it would also be good to do > this to make sure this works before we employ _configthreadlocale() in > higher-stakes situations. > > I suggest doing these two steps as separate patches, so this doesn't get > confused between the various thread-related threads that want to > variously add or remove uses of this function. OK, do you think these three patches tell the _configthreadlocale() story properly? (Then after that we can get back to getting rid of it...) Attachments: [application/octet-stream] 0001-Remove-configure-check-for-_configthreadlocale.patch (5.0K, ../../CA+hUKGLbEhVb029AQJDCZ4+QgcqVmERJGZefxS=+b+SBBSGW8A@mail.gmail.com/2-0001-Remove-configure-check-for-_configthreadlocale.patch) download | inline diff: From 4fbeb868b1acf6c760a5ad23b3be3cc70c6afdb6 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 20 Nov 2024 15:22:23 +1300 Subject: [PATCH 1/3] Remove configure check for _configthreadlocale(). All modern Windows systems have _configthreadlocale(). It was introduced in msvcrt80.dll as used by Visual Studio 2005. Historically, MinGW was stuck on even older msvcrt.dll, but added its own dummy implementation years ago anyway, rendering the configure test useless. We can handle MinGW's dummy implementation, but in practice we don't encounter it anymore because MinGW uses ucrt by default. Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech --- configure | 11 ----------- configure.ac | 1 - meson.build | 1 - src/include/pg_config.h.in | 3 --- src/interfaces/ecpg/ecpglib/descriptor.c | 4 ++-- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +- src/interfaces/ecpg/ecpglib/execute.c | 4 ++-- 7 files changed, 5 insertions(+), 21 deletions(-) diff --git a/configure b/configure index f58eae1baa8..728deeb30ff 100755 --- a/configure +++ b/configure @@ -15812,17 +15812,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - for ac_func in _configthreadlocale -do : - ac_fn_c_check_func "$LINENO" "_configthreadlocale" "ac_cv_func__configthreadlocale" -if test "x$ac_cv_func__configthreadlocale" = xyes; then : - cat >>confdefs.h <<_ACEOF -#define HAVE__CONFIGTHREADLOCALE 1 -_ACEOF - -fi -done - case " $LIBOBJS " in *" dirmod.$ac_objext "* ) ;; *) LIBOBJS="$LIBOBJS dirmod.$ac_objext" diff --git a/configure.ac b/configure.ac index 82c5009e3e8..b7aef8ac11e 100644 --- a/configure.ac +++ b/configure.ac @@ -1816,7 +1816,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - AC_CHECK_FUNCS(_configthreadlocale) AC_LIBOBJ(dirmod) AC_LIBOBJ(kill) AC_LIBOBJ(open) diff --git a/meson.build b/meson.build index 5b0510cef78..2accff2c992 100644 --- a/meson.build +++ b/meson.build @@ -2614,7 +2614,6 @@ endif # XXX: Might be worth conditioning some checks on the OS, to avoid doing # unnecessary checks over and over, particularly on windows. func_checks = [ - ['_configthreadlocale', {'skip': host_system != 'windows'}], ['backtrace_symbols', {'dependencies': [execinfo_dep]}], ['clock_gettime', {'dependencies': [rt_dep], 'define': false}], ['copyfile'], diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index cdd9a6e9355..130d0216d7d 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -529,9 +529,6 @@ /* Define to 1 if your compiler understands __builtin_unreachable. */ #undef HAVE__BUILTIN_UNREACHABLE -/* Define to 1 if you have the `_configthreadlocale' function. */ -#undef HAVE__CONFIGTHREADLOCALE - /* Define to 1 if you have __cpuid. */ #undef HAVE__CPUID diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index ad279e245c4..56e2bc41531 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -490,7 +490,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) Assert(ecpg_clocale); stmt.oldlocale = uselocale(ecpg_clocale); #else -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); #endif stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); @@ -510,7 +510,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) setlocale(LC_NUMERIC, stmt.oldlocale); ecpg_free(stmt.oldlocale); } -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 if (stmt.oldthreadlocale != -1) (void) _configthreadlocale(stmt.oldthreadlocale); #endif diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index bad3cd9920b..75cc68275bd 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h @@ -77,7 +77,7 @@ struct statement locale_t oldlocale; #else char *oldlocale; -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 int oldthreadlocale; #endif #endif diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index c578c21cf66..466d5600f9b 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -1995,7 +1995,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, return false; } #else -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); #endif stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); @@ -2219,7 +2219,7 @@ ecpg_do_epilogue(struct statement *stmt) #else if (stmt->oldlocale) setlocale(LC_NUMERIC, stmt->oldlocale); -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 /* * This is a bit trickier than it looks: if we failed partway through -- 2.39.5 (Apple Git-154) [application/octet-stream] 0002-Formally-require-ucrt-on-Windows.patch (3.0K, ../../CA+hUKGLbEhVb029AQJDCZ4+QgcqVmERJGZefxS=+b+SBBSGW8A@mail.gmail.com/3-0002-Formally-require-ucrt-on-Windows.patch) download | inline diff: From 330767ee9aa7081b9b4fc6ca82c337bb0c2a89fc Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 20 Nov 2024 15:56:46 +1300 Subject: [PATCH 2/3] Formally require ucrt on Windows. Historically we tolerated the absence of various Windows features when using the MinGW tool chain, because it used ancient msvcrt.dll for a long period of time. It now uses ucrt by default (like Windows 10, Visual Studio 2015). It was difficult or impossible to use MinGW with the various msvcrXXX.dll versions that came between the original msvcrt.dll and ucrt, so this represents a sudden jump in API from about 1996 to about 2015. In fact we already effectively required ucrt in PostgreSQL 17: commit 8d9a9f03 required _create_locale and friends, which arrived in msvcr120.dll (Visual Studio 2013, the last of the pre-ucrt series of runtimes), and for MinGW users that practically meant ucrt; meanwhile for Visual Studio we currently require at least 2015, which uses ucrt. That may not have been the first such case, but msvrt.dll and Visual Studio 2013 builds had already dropped off our testing radar so we weren't paying attention if so. This commit formalizes the requirement. It removes obsolete comments that discussed msvcrt.dll limitations, and some tests of !defined(_MSC_VER) to imply msvcrt.dll. Later commits will remove more anachronisms resulting from this policy decision. --- src/backend/utils/adt/pg_locale.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index d4e89663ec1..d1b392c248c 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1017,13 +1017,6 @@ cache_locale_time(void) * get ISO Locale name directly by using GetLocaleInfoEx() with LCType as * LOCALE_SNAME. * - * MinGW headers declare _create_locale(), but msvcrt.dll lacks that symbol in - * releases before Windows 8. IsoLocaleName() always fails in a MinGW-built - * postgres.exe, so only Unix-style values of the lc_messages GUC can elicit - * localized messages. In particular, every lc_messages setting that initdb - * can select automatically will yield only C-locale messages. XXX This could - * be fixed by running the fully-qualified locale name through a lookup table. - * * This function returns a pointer to a static buffer bearing the converted * name or NULL if conversion fails. * @@ -1031,8 +1024,6 @@ cache_locale_time(void) * [2] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names */ -#if defined(_MSC_VER) - /* * Callback function for EnumSystemLocalesEx() in get_iso_localename(). * @@ -1201,16 +1192,6 @@ IsoLocaleName(const char *winlocname) return get_iso_localename(winlocname); } -#else /* !defined(_MSC_VER) */ - -static char * -IsoLocaleName(const char *winlocname) -{ - return NULL; /* Not supported on MinGW */ -} - -#endif /* defined(_MSC_VER) */ - #endif /* WIN32 && LC_MESSAGES */ -- 2.39.5 (Apple Git-154) [application/octet-stream] 0003-Revert-Blind-attempt-to-fix-_configthreadlocale-fail.patch (3.4K, ../../CA+hUKGLbEhVb029AQJDCZ4+QgcqVmERJGZefxS=+b+SBBSGW8A@mail.gmail.com/4-0003-Revert-Blind-attempt-to-fix-_configthreadlocale-fail.patch) download | inline diff: From 9e4ee5b625992fa2e0c2a9778942c0ec1e02bfbc Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 20 Nov 2024 17:45:45 +1300 Subject: [PATCH 3/3] Revert "Blind attempt to fix _configthreadlocale() failures on MinGW." This reverts commit 2cf91ccb73ce888c44e3751548fb7c77e87335f2. When using the old msvcrt.dll, MinGW would supply its own dummy version of _configthreadlocale() that just returns -1 if you try to use it. For a time we tolerated that to shut the build farm up. We would fall back to code that was enough for the tests to pass. It would certainly crash a multithreaded program though, so it was bogus. We don't need that kludge anymore, because we require ucrt (the modern Windows C runtime). We expect the real _configthreadlocale() to be present, and the ECPG tests will now fail if it isn't. The workaround was dead code and it's time to revert it. Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org --- src/interfaces/ecpg/ecpglib/descriptor.c | 2 +- src/interfaces/ecpg/ecpglib/execute.c | 20 +++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index 56e2bc41531..aee888432f0 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -512,7 +512,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) } #ifdef WIN32 if (stmt.oldthreadlocale != -1) - (void) _configthreadlocale(stmt.oldthreadlocale); + _configthreadlocale(stmt.oldthreadlocale); #endif #endif } diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index 466d5600f9b..b5089eac787 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -1977,9 +1977,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, * Make sure we do NOT honor the locale for numeric input/output since the * database wants the standard decimal point. If available, use * uselocale() for this because it's thread-safe. Windows doesn't have - * that, but it usually does have _configthreadlocale(). In some versions - * of MinGW, _configthreadlocale() exists but always returns -1 --- so - * treat that situation as if the function doesn't exist. + * that, but it does have _configthreadlocale(). */ #ifdef HAVE_USELOCALE @@ -1997,6 +1995,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, #else #ifdef WIN32 stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); + if (stmt->oldthreadlocale == -1) + { + ecpg_do_epilogue(stmt); + return false; + } #endif stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); if (stmt->oldlocale == NULL) @@ -2218,17 +2221,12 @@ ecpg_do_epilogue(struct statement *stmt) uselocale(stmt->oldlocale); #else if (stmt->oldlocale) + { setlocale(LC_NUMERIC, stmt->oldlocale); #ifdef WIN32 - - /* - * This is a bit trickier than it looks: if we failed partway through - * statement initialization, oldthreadlocale could still be 0. But that's - * okay because a call with 0 is defined to be a no-op. - */ - if (stmt->oldthreadlocale != -1) - (void) _configthreadlocale(stmt->oldthreadlocale); + _configthreadlocale(stmt->oldthreadlocale); #endif + } #endif free_statement(stmt); -- 2.39.5 (Apple Git-154) ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-20 23:30 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Thomas Munro @ 2024-11-20 23:30 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Wed, Nov 20, 2024 at 10:00 PM Thomas Munro <[email protected]> wrote: > OK, do you think these three patches tell the _configthreadlocale() > story properly? (Then after that we can get back to getting rid of > it...) Just by the way, here's another interesting thing I have learned about the msvcrt->ucrt evolution: ucrt introduced UTF-8 support (ie locales can use UTF-8 encoding, and all the standard library char functions work with it just fine like on Unix), but PostgreSQL still believes that Windows can't do that and has a lot of special code paths to work with wchar_t and perform extra conversions. I started nibbling at that[1], but at the time I was still a bit fuzzy on whether we could really rip all that old stuff out yet and harmonise with Unix, as I didn't understand the MinGW/runtime/history situation. It seems the coast is now clear, and that would be a satisfying cleanup. (There's still non-trivial work to do for that though: we allowed weird mismatched encoding scenarios just on that OS, and would need to stop that, which might create some upgrade path problems, needs some thought, see that thread.) [1] https://www.postgresql.org/message-id/CA%2BhUKGJ%3Dca39Cg%3Dy%3DS89EaCYvvCF8NrZRO%3Duog-cnz0VzC6Kfg%... ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-21 07:38 Peter Eisentraut <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: Peter Eisentraut @ 2024-11-21 07:38 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On 20.11.24 10:00, Thomas Munro wrote: > On Fri, Nov 15, 2024 at 1:53 AM Peter Eisentraut <[email protected]> wrote: >> On 14.11.24 08:48, Thomas Munro wrote: >>> The three MinGW environments we test today are using ucrt, and >>> configure detects the symbol on all. Namely: fairwren >>> (msys2/mingw64), the CI mingw64 task and the mingw cross-build that >>> runs on Linux in the CI CompilerWarnings task. As far as I know these >>> are the reasons for, and mechanism by which, we keep MinGW support >>> working. We have no policy requiring arbitrary old MinGW systems >>> work, and we wouldn't know anyway. >> >> Right. So I think we could unwind this in steps. First, remove the >> configure test for _configthreadlocale() and all the associated #ifdefs >> in the existing ecpg code. This seems totally safe, it would just leave >> behind MinGW older than 2016 and MSVC older than 2015, the latter of >> which is already the current threshold. >> >> Then the question whether we want to re-enable the error checking on >> _configthreadlocale() that was reverted by 2cf91ccb, or at least >> something similar. This should also be okay based on your description >> of the different Windows runtimes. I think it would also be good to do >> this to make sure this works before we employ _configthreadlocale() in >> higher-stakes situations. >> >> I suggest doing these two steps as separate patches, so this doesn't get >> confused between the various thread-related threads that want to >> variously add or remove uses of this function. > > OK, do you think these three patches tell the _configthreadlocale() > story properly? (Then after that we can get back to getting rid of > it...) Yes, this is very clear and helpful. Thanks. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-22 21:32 Thomas Munro <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Thomas Munro @ 2024-11-22 21:32 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Thu, Nov 21, 2024 at 8:38 PM Peter Eisentraut <[email protected]> wrote: > On 20.11.24 10:00, Thomas Munro wrote: > > OK, do you think these three patches tell the _configthreadlocale() > > story properly? (Then after that we can get back to getting rid of > > it...) > > Yes, this is very clear and helpful. Thanks. I realised that there is another aspect to this: it must be impossible to build PostgreSQL with the original MinGW/MSYS project by now. I don't understand the history of the MinGW/MinGW-w64 fork, but if they're both still live projects out there adding to the general confusion about the frankenwindows multiverse, we should clarify our situation. As far as I know, we're only testing the second thing, and only the second thing can use UCRT, and only the second thing is a viable alternative toolchain for software that is primarily targeting current Visual Studio, which I think is something we can say about our project. Right? I was looking at the installation.sgml file and wondering where to write "UCRT required" in the 0002 patch. Perhaps we don't even need to mention it if it's the default on MinGW-w64, but I think we do need to remove the discussion of the original MinGW. It's even pointing to a cybersquatted URL. I also suspect that MinGW-w64 shouldn't be described as for "building 64 bit binaries" (maybe it started that way but by now it's has other goals, modern libraries/APIs etc keeping up with VC; and AFAICS it can build 32 bit binaries too, if anyone still cares about that; wow, the naming of all this stuff is so confusing to a non-Windows person, 32 and 64 get thrown around all over the place with apparently no real meaning, but I digress...). Andrew, would you be interested in updating this section to talk only about the modern installation and build steps for MinGW-w64/MSYS2 et al? There are several confusingly related projects and I'm not sure I'd get it right if I tried, I've never used any of it... Attachments: [text/x-patch] v2-0001-Remove-configure-check-for-_configthreadlocale.patch (5.0K, ../../CA+hUKG+iJtpun-sFqRYgJQt7eWEZnrsG_i=d_PRyHBqGrahCQw@mail.gmail.com/2-v2-0001-Remove-configure-check-for-_configthreadlocale.patch) download | inline diff: From fbe7be0c0c1d4e2b047d2dcfb0ba5ccd9ce0994a Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sat, 23 Nov 2024 08:44:02 +1300 Subject: [PATCH v2 1/3] Remove configure check for _configthreadlocale(). All modern Windows systems have _configthreadlocale(). It was first introduced in msvcr80.dll from Visual Studio 2005. Historically, MinGW was stuck on even older msvcrt.dll, but added its own dummy implementation of the function when using msvcrt.dll years ago anyway, effectively rendering the configure test useless. In practice we don't encounter the dummy anymore because modern MinGW uses ucrt. Reviewed-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech --- configure | 11 ----------- configure.ac | 1 - meson.build | 1 - src/include/pg_config.h.in | 3 --- src/interfaces/ecpg/ecpglib/descriptor.c | 4 ++-- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +- src/interfaces/ecpg/ecpglib/execute.c | 4 ++-- 7 files changed, 5 insertions(+), 21 deletions(-) diff --git a/configure b/configure index 28719ed30c..07dd2dec1b 100755 --- a/configure +++ b/configure @@ -15917,17 +15917,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - for ac_func in _configthreadlocale -do : - ac_fn_c_check_func "$LINENO" "_configthreadlocale" "ac_cv_func__configthreadlocale" -if test "x$ac_cv_func__configthreadlocale" = xyes; then : - cat >>confdefs.h <<_ACEOF -#define HAVE__CONFIGTHREADLOCALE 1 -_ACEOF - -fi -done - case " $LIBOBJS " in *" dirmod.$ac_objext "* ) ;; *) LIBOBJS="$LIBOBJS dirmod.$ac_objext" diff --git a/configure.ac b/configure.ac index 533f4ab78a..184b920e3e 100644 --- a/configure.ac +++ b/configure.ac @@ -1825,7 +1825,6 @@ fi # Win32 (really MinGW) support if test "$PORTNAME" = "win32"; then - AC_CHECK_FUNCS(_configthreadlocale) AC_LIBOBJ(dirmod) AC_LIBOBJ(kill) AC_LIBOBJ(open) diff --git a/meson.build b/meson.build index b64d253fe4..a56f3311ea 100644 --- a/meson.build +++ b/meson.build @@ -2614,7 +2614,6 @@ endif # XXX: Might be worth conditioning some checks on the OS, to avoid doing # unnecessary checks over and over, particularly on windows. func_checks = [ - ['_configthreadlocale', {'skip': host_system != 'windows'}], ['backtrace_symbols', {'dependencies': [execinfo_dep]}], ['clock_gettime', {'dependencies': [rt_dep], 'define': false}], ['copyfile'], diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index a903c60a3a..e53560c87f 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -535,9 +535,6 @@ /* Define to 1 if your compiler understands __builtin_unreachable. */ #undef HAVE__BUILTIN_UNREACHABLE -/* Define to 1 if you have the `_configthreadlocale' function. */ -#undef HAVE__CONFIGTHREADLOCALE - /* Define to 1 if you have __cpuid. */ #undef HAVE__CPUID diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index ad279e245c..56e2bc4153 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -490,7 +490,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) Assert(ecpg_clocale); stmt.oldlocale = uselocale(ecpg_clocale); #else -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); #endif stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); @@ -510,7 +510,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) setlocale(LC_NUMERIC, stmt.oldlocale); ecpg_free(stmt.oldlocale); } -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 if (stmt.oldthreadlocale != -1) (void) _configthreadlocale(stmt.oldthreadlocale); #endif diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index bad3cd9920..75cc68275b 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h @@ -77,7 +77,7 @@ struct statement locale_t oldlocale; #else char *oldlocale; -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 int oldthreadlocale; #endif #endif diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index c578c21cf6..466d5600f9 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -1995,7 +1995,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, return false; } #else -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); #endif stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); @@ -2219,7 +2219,7 @@ ecpg_do_epilogue(struct statement *stmt) #else if (stmt->oldlocale) setlocale(LC_NUMERIC, stmt->oldlocale); -#ifdef HAVE__CONFIGTHREADLOCALE +#ifdef WIN32 /* * This is a bit trickier than it looks: if we failed partway through -- 2.47.0 [text/x-patch] v2-0002-Formally-require-ucrt-on-Windows.patch (3.3K, ../../CA+hUKG+iJtpun-sFqRYgJQt7eWEZnrsG_i=d_PRyHBqGrahCQw@mail.gmail.com/3-v2-0002-Formally-require-ucrt-on-Windows.patch) download | inline diff: From a5e4b17a10be22e20d81252783094753ff0afda3 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sat, 23 Nov 2024 09:14:48 +1300 Subject: [PATCH v2 2/3] Formally require ucrt on Windows. Historically we tolerated the absence of various C runtime library features for the benefit of the MinGW tool chain, because it used ancient msvcrt.dll for a long period of time. It now uses ucrt by default (like Windows 10+, Visual Studio 2015+), and that's the only configuration we're testing. In practice, we effectively required ucrt already in PostgreSQL 17, when commit 8d9a9f03 required _create_locale etc, first available in msvcr120.dll (Visual Studio 2013, the last of the pre-ucrt series of runtimes), and for MinGW users that practically meant ucrt because it was difficult or impossible to use msvcr120.dll. That may even not have been the first such case, but old MinGW configurations had already dropped off our testing radar so we weren't paying much attention. This commit formalizes the requirement. It also removes a couple of obsolete comments that discussed msvcrt.dll limitations, and some tests of !defined(_MSC_VER) to imply msvcrt.dll. There are many more anachronisms, but it'll take some time to figure out how to remove them all. XXX Help needed to rewrite doc/src/sgml/installation.sgml, to clarify that we now only support https://en.wikipedia.org/wiki/Mingw-w64 and MSYS2, not https://en.wikipedia.org/wiki/MinGW and MSYS (no one is testing the latter, and it probably can't build PostgreSQL 17, since it can't use UCRT?) Reviewed-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org --- src/backend/utils/adt/pg_locale.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index d4e89663ec..d1b392c248 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1017,13 +1017,6 @@ cache_locale_time(void) * get ISO Locale name directly by using GetLocaleInfoEx() with LCType as * LOCALE_SNAME. * - * MinGW headers declare _create_locale(), but msvcrt.dll lacks that symbol in - * releases before Windows 8. IsoLocaleName() always fails in a MinGW-built - * postgres.exe, so only Unix-style values of the lc_messages GUC can elicit - * localized messages. In particular, every lc_messages setting that initdb - * can select automatically will yield only C-locale messages. XXX This could - * be fixed by running the fully-qualified locale name through a lookup table. - * * This function returns a pointer to a static buffer bearing the converted * name or NULL if conversion fails. * @@ -1031,8 +1024,6 @@ cache_locale_time(void) * [2] https://docs.microsoft.com/en-us/windows/win32/intl/locale-names */ -#if defined(_MSC_VER) - /* * Callback function for EnumSystemLocalesEx() in get_iso_localename(). * @@ -1201,16 +1192,6 @@ IsoLocaleName(const char *winlocname) return get_iso_localename(winlocname); } -#else /* !defined(_MSC_VER) */ - -static char * -IsoLocaleName(const char *winlocname) -{ - return NULL; /* Not supported on MinGW */ -} - -#endif /* defined(_MSC_VER) */ - #endif /* WIN32 && LC_MESSAGES */ -- 2.47.0 [text/x-patch] v2-0003-Revert-Blind-attempt-to-fix-_configthreadlocale-f.patch (3.4K, ../../CA+hUKG+iJtpun-sFqRYgJQt7eWEZnrsG_i=d_PRyHBqGrahCQw@mail.gmail.com/4-v2-0003-Revert-Blind-attempt-to-fix-_configthreadlocale-f.patch) download | inline diff: From 4c94af123a38c8c68178497f7b59e1d948f2d9ca Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sat, 23 Nov 2024 09:43:16 +1300 Subject: [PATCH v2 3/3] Revert "Blind attempt to fix _configthreadlocale() failures on MinGW." This reverts commit 2cf91ccb73ce888c44e3751548fb7c77e87335f2. When using the old msvcrt.dll, MinGW would supply its own dummy version of _configthreadlocale() that just returns -1 if you try to use it. For a time we tolerated that to shut the build farm up. We would fall back to code that was enough for the tests to pass, but it would have crashed a real multithreaded program though, so it was bogus. We don't need that kludge anymore, because we require ucrt (the modern Windows C runtime). We expect the real _configthreadlocale() to be present, and the ECPG tests will now fail if it isn't. The workaround was dead code and it's time to revert it. Reviewed-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/d9e7731c-ca1b-477c-9298-fa51e135574a%40eisentraut.org --- src/interfaces/ecpg/ecpglib/descriptor.c | 2 +- src/interfaces/ecpg/ecpglib/execute.c | 20 +++++++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index 56e2bc4153..aee888432f 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -512,7 +512,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) } #ifdef WIN32 if (stmt.oldthreadlocale != -1) - (void) _configthreadlocale(stmt.oldthreadlocale); + _configthreadlocale(stmt.oldthreadlocale); #endif #endif } diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index 466d5600f9..b5089eac78 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -1977,9 +1977,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, * Make sure we do NOT honor the locale for numeric input/output since the * database wants the standard decimal point. If available, use * uselocale() for this because it's thread-safe. Windows doesn't have - * that, but it usually does have _configthreadlocale(). In some versions - * of MinGW, _configthreadlocale() exists but always returns -1 --- so - * treat that situation as if the function doesn't exist. + * that, but it does have _configthreadlocale(). */ #ifdef HAVE_USELOCALE @@ -1997,6 +1995,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, #else #ifdef WIN32 stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); + if (stmt->oldthreadlocale == -1) + { + ecpg_do_epilogue(stmt); + return false; + } #endif stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); if (stmt->oldlocale == NULL) @@ -2218,17 +2221,12 @@ ecpg_do_epilogue(struct statement *stmt) uselocale(stmt->oldlocale); #else if (stmt->oldlocale) + { setlocale(LC_NUMERIC, stmt->oldlocale); #ifdef WIN32 - - /* - * This is a bit trickier than it looks: if we failed partway through - * statement initialization, oldthreadlocale could still be 0. But that's - * okay because a call with 0 is defined to be a no-op. - */ - if (stmt->oldthreadlocale != -1) - (void) _configthreadlocale(stmt->oldthreadlocale); + _configthreadlocale(stmt->oldthreadlocale); #endif + } #endif free_statement(stmt); -- 2.47.0 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-25 00:42 Michael Paquier <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Michael Paquier @ 2024-11-25 00:42 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Sat, Nov 23, 2024 at 10:32:31AM +1300, Thomas Munro wrote: > I realised that there is another aspect to this: it must be impossible > to build PostgreSQL with the original MinGW/MSYS project by now. I > don't understand the history of the MinGW/MinGW-w64 fork, but if > they're both still live projects out there adding to the general > confusion about the frankenwindows multiverse, we should clarify our > situation. As far as I know, we're only testing the second thing, and > only the second thing can use UCRT, and only the second thing is a > viable alternative toolchain for software that is primarily targeting > current Visual Studio, which I think is something we can say about our > project. Right? FWIW, I am not seeing any advantage in mentioning MinGW at all at this stage, just extra maintenance burden. As far as I know, MinGW is a gcc port that has only a 32b implementation. MinGW-w64 is built on top of it and it includes *both* 32b and 64b implementations, as you say, with more WIN32 APIs than the former. So +1 to simplify a bit that stuff. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-25 00:57 Thomas Munro <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Thomas Munro @ 2024-11-25 00:57 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Mon, Nov 25, 2024 at 1:43 PM Michael Paquier <[email protected]> wrote: > On Sat, Nov 23, 2024 at 10:32:31AM +1300, Thomas Munro wrote: > > I realised that there is another aspect to this: it must be impossible > > to build PostgreSQL with the original MinGW/MSYS project by now. I > > don't understand the history of the MinGW/MinGW-w64 fork, but if > > they're both still live projects out there adding to the general > > confusion about the frankenwindows multiverse, we should clarify our > > situation. As far as I know, we're only testing the second thing, and > > only the second thing can use UCRT, and only the second thing is a > > viable alternative toolchain for software that is primarily targeting > > current Visual Studio, which I think is something we can say about our > > project. Right? > > FWIW, I am not seeing any advantage in mentioning MinGW at all at this > stage, just extra maintenance burden. As far as I know, MinGW is a > gcc port that has only a 32b implementation. MinGW-w64 is built on > top of it and it includes *both* 32b and 64b implementations, as you > say, with more WIN32 APIs than the former. > > So +1 to simplify a bit that stuff. Thanks. I'm going to have a go at adjusting the docs myself so I can get this committed. Invitation remains open for someone closer to the topic to rewrite in a later commit as required for maximum utility to the reader (I'm never going to install MSYS2, or Windows, I just want to blow away as much dead code as possible here as it's in the way of multithreading and other modernisation projects). ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-26 16:23 Peter Eisentraut <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Peter Eisentraut @ 2024-11-26 16:23 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On 25.11.24 01:57, Thomas Munro wrote: > Thanks. I'm going to have a go at adjusting the docs myself so I can > get this committed. Invitation remains open for someone closer to the > topic to rewrite in a later commit as required for maximum utility to > the reader (I'm never going to install MSYS2, or Windows, I just want > to blow away as much dead code as possible here as it's in the way of > multithreading and other modernisation projects). Attached is a simple proposal. The section about MinGW can be replaced mostly by "use MSYS2". That's also what CI and the buildfarm uses. Anyone who strays from that can figure it out themselves. In the Visual Studio section, there was some text that recommended getting flex and bison via MinGW, which seemed clearly outdated. I put in the URL that the CI images use. I don't know if there are other common sources, but that one seems good enough. From 939a94b524b498cab3a86f6a2e001f61a2e520cd Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Tue, 26 Nov 2024 17:18:36 +0100 Subject: [PATCH] doc: Update some outdated installation info related to MinGW --- doc/src/sgml/installation.sgml | 60 +++------------------------------- 1 file changed, 5 insertions(+), 55 deletions(-) diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index 5621606f59a..d6986dbd885 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -3582,33 +3582,9 @@ <title>MinGW</title> <para> PostgreSQL for Windows can be built using MinGW, a Unix-like build - environment for Microsoft operating systems. - The MinGW build procedure uses the normal build system described in - this chapter. - </para> - - <para> - MinGW, the Unix-like build tools, and MSYS, a collection - of Unix tools required to run shell scripts - like <command>configure</command>, can be downloaded - from <ulink url="http://www.mingw.org/"></ulink;. Neither is - required to run the resulting binaries; they are needed only for - creating the binaries. - </para> - - <para> - To build 64 bit binaries using MinGW, install the 64 bit tool set - from <ulink url="https://mingw-w64.org/"></ulink;, put its bin - directory in the <envar>PATH</envar>, and run - <command>configure</command> with the - <command>--host=x86_64-w64-mingw32</command> option. - </para> - - <para> - After you have everything installed, it is suggested that you - run <application>psql</application> - under <command>CMD.EXE</command>, as the MSYS console has - buffering issues. + environment for Windows. It is recommended to use the <ulink + url="https://www.msys2.org/">MSYS2</ulink; environment for this and also + use that to install any prerequisite packages. </para> <sect3 id="mingw-crash-dumps"> @@ -3838,35 +3814,9 @@ <title>Requirements</title> <productname>Bison</productname> and <productname>Flex</productname> are required. Only <productname>Bison</productname> versions 2.3 and later will work. <productname>Flex</productname> must be version 2.5.35 or later. + Binaries can be downloaded from <ulink + url="https://github.com/lexxmark/winflexbison"></ulink;. </para> - - <para> - Both <productname>Bison</productname> and <productname>Flex</productname> - are included in the <productname>msys</productname> tool suite, available - from <ulink url="http://www.mingw.org/wiki/MSYS"></ulink; as part of the - <productname>MinGW</productname> compiler suite. - </para> - - <para> - You will need to add the directory containing - <filename>flex.exe</filename> and <filename>bison.exe</filename> to the - PATH environment variable. In the case of MinGW, the directory is the - <filename>\msys\1.0\bin</filename> subdirectory of your MinGW - installation directory. - </para> - - <note> - <para> - The Bison distribution from GnuWin32 appears to have a bug that - causes Bison to malfunction when installed in a directory with - spaces in the name, such as the default location on English - installations <filename>C:\Program Files\GnuWin32</filename>. - Consider installing into <filename>C:\GnuWin32</filename> or use the - NTFS short name path to GnuWin32 in your PATH environment setting - (e.g., <filename>C:\PROGRA~1\GnuWin32</filename>). - </para> - </note> - </listitem> </varlistentry> </variablelist> -- 2.47.0 Attachments: [text/plain] 0001-doc-Update-some-outdated-installation-info-related-t.patch (3.7K, ../../[email protected]/2-0001-doc-Update-some-outdated-installation-info-related-t.patch) download | inline diff: From 939a94b524b498cab3a86f6a2e001f61a2e520cd Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Tue, 26 Nov 2024 17:18:36 +0100 Subject: [PATCH] doc: Update some outdated installation info related to MinGW --- doc/src/sgml/installation.sgml | 60 +++------------------------------- 1 file changed, 5 insertions(+), 55 deletions(-) diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index 5621606f59a..d6986dbd885 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -3582,33 +3582,9 @@ <title>MinGW</title> <para> PostgreSQL for Windows can be built using MinGW, a Unix-like build - environment for Microsoft operating systems. - The MinGW build procedure uses the normal build system described in - this chapter. - </para> - - <para> - MinGW, the Unix-like build tools, and MSYS, a collection - of Unix tools required to run shell scripts - like <command>configure</command>, can be downloaded - from <ulink url="http://www.mingw.org/"></ulink>. Neither is - required to run the resulting binaries; they are needed only for - creating the binaries. - </para> - - <para> - To build 64 bit binaries using MinGW, install the 64 bit tool set - from <ulink url="https://mingw-w64.org/"></ulink>, put its bin - directory in the <envar>PATH</envar>, and run - <command>configure</command> with the - <command>--host=x86_64-w64-mingw32</command> option. - </para> - - <para> - After you have everything installed, it is suggested that you - run <application>psql</application> - under <command>CMD.EXE</command>, as the MSYS console has - buffering issues. + environment for Windows. It is recommended to use the <ulink + url="https://www.msys2.org/">MSYS2</ulink> environment for this and also + use that to install any prerequisite packages. </para> <sect3 id="mingw-crash-dumps"> @@ -3838,35 +3814,9 @@ <title>Requirements</title> <productname>Bison</productname> and <productname>Flex</productname> are required. Only <productname>Bison</productname> versions 2.3 and later will work. <productname>Flex</productname> must be version 2.5.35 or later. + Binaries can be downloaded from <ulink + url="https://github.com/lexxmark/winflexbison"></ulink>. </para> - - <para> - Both <productname>Bison</productname> and <productname>Flex</productname> - are included in the <productname>msys</productname> tool suite, available - from <ulink url="http://www.mingw.org/wiki/MSYS"></ulink> as part of the - <productname>MinGW</productname> compiler suite. - </para> - - <para> - You will need to add the directory containing - <filename>flex.exe</filename> and <filename>bison.exe</filename> to the - PATH environment variable. In the case of MinGW, the directory is the - <filename>\msys\1.0\bin</filename> subdirectory of your MinGW - installation directory. - </para> - - <note> - <para> - The Bison distribution from GnuWin32 appears to have a bug that - causes Bison to malfunction when installed in a directory with - spaces in the name, such as the default location on English - installations <filename>C:\Program Files\GnuWin32</filename>. - Consider installing into <filename>C:\GnuWin32</filename> or use the - NTFS short name path to GnuWin32 in your PATH environment setting - (e.g., <filename>C:\PROGRA~1\GnuWin32</filename>). - </para> - </note> - </listitem> </varlistentry> </variablelist> -- 2.47.0 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2024-11-26 20:24 Thomas Munro <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Thomas Munro @ 2024-11-26 20:24 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Wed, Nov 27, 2024 at 5:23 AM Peter Eisentraut <[email protected]> wrote: > Attached is a simple proposal. The section about MinGW can be replaced > mostly by "use MSYS2". That's also what CI and the buildfarm uses. > Anyone who strays from that can figure it out themselves. Thanks! I'll take it. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-02-09 07:32 Peter Eisentraut <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Peter Eisentraut @ 2025-02-09 07:32 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers Checking the status of this thread ... The patches that removed the configure checks for _configthreadlocale(), and related cleanup, have been committed. The original patch to "Tidy up locale thread safety in ECPG library" is still outstanding. Attached is a rebased version, based on the posted v6, with a couple of small fixups from me. I haven't re-reviewed it yet, but from scanning the discussion, it looks close to done. From 842a5837898a4a47c8c79d07a2d14f3a82514a36 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sat, 10 Aug 2024 11:01:21 +1200 Subject: [PATCH v7 1/3] Tidy up locale thread safety in ECPG library. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove setlocale() and _configthreadlocal() as fallback strategy on systems that don't have uselocale(), where ECPG tries to control LC_NUMERIC formatting on input and output of floating point numbers. It was probably broken on some systems (NetBSD), and the code was also quite messy and complicated, with obsolete configure tests (Windows). It was also arguably broken, or at least had unstated environmental requirements, if pgtypeslib code was called directly. Instead, introduce PG_C_LOCALE to refer to the "C" locale as a locale_t value. It maps to the special constant LC_C_LOCALE when defined by libc (macOS, NetBSD), or otherwise uses a process-lifetime locale_t that is allocated on first use, just as ECPG previously did itself. The new replacement might be more widely useful. Then change the float parsing and printing code to pass that to _l() functions where appropriate. Unfortunately the portability of those functions is a bit complicated. First, many obvious and useful _l() functions are missing from POSIX, though most standard libraries define some of them anyway. Second, although the thread-safe save/restore technique can be used to replace the missing ones, Windows and NetBSD refused to implement standard uselocale(). They might have a point: "wide scope" uselocale() is hard to combine with other code and error-prone, especially in library code. Luckily they have the _l() functions we want so far anyway. So we have to be prepared for both ways of doing things: 1. In ECPG, use strtod_l() for parsing, and supply a port.h replacement using uselocale() over a limited scope if missing. 2. Inside our own snprintf.c, use three different approaches to format floats. For frontend code, call libc's snprintf_l(), or wrap libc's snprintf() in uselocale() if it's missing. For backend code, snprintf.c can keep assuming that the global locale's LC_NUMERIC is "C" and call libc's snprintf() without change, for now. (It might eventually be possible to call our in-tree Ryū routines to display floats in snprintf.c, given the C-locale-always remit of our in-tree snprintf(), but this patch doesn't risk changing anything that complicated.) Reviewed-by: Peter Eisentraut <[email protected]> Reviewed-by: Tristan Partin <[email protected]> Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech --- configure | 2 +- configure.ac | 2 + meson.build | 2 + src/include/pg_config.h.in | 6 ++ src/include/port.h | 31 ++++++++ src/include/port/win32_port.h | 1 + src/interfaces/ecpg/ecpglib/connect.c | 39 +--------- src/interfaces/ecpg/ecpglib/data.c | 2 +- src/interfaces/ecpg/ecpglib/descriptor.c | 37 --------- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 12 --- src/interfaces/ecpg/ecpglib/execute.c | 53 ------------- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- src/interfaces/ecpg/pgtypeslib/interval.c | 4 +- src/interfaces/ecpg/pgtypeslib/numeric.c | 2 +- src/port/Makefile | 1 + src/port/locale.c | 80 ++++++++++++++++++++ src/port/meson.build | 1 + src/port/snprintf.c | 55 ++++++++++++++ 18 files changed, 188 insertions(+), 148 deletions(-) create mode 100644 src/port/locale.c diff --git a/configure b/configure index 0ffcaeb4367..017e63598c1 100755 --- a/configure +++ b/configure @@ -14934,7 +14934,7 @@ fi LIBS_including_readline="$LIBS" LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'` -for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l +for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strtod_l strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.ac b/configure.ac index f56681e0d91..b06a9cd1080 100644 --- a/configure.ac +++ b/configure.ac @@ -1718,6 +1718,8 @@ AC_CHECK_FUNCS(m4_normalize([ pthread_is_threaded_np setproctitle setproctitle_fast + snprintf_l + strtod_l strchrnul strsignal syncfs diff --git a/meson.build b/meson.build index 1ceadb9a830..e03b5083f27 100644 --- a/meson.build +++ b/meson.build @@ -2648,6 +2648,8 @@ func_checks = [ ['shm_open', {'dependencies': [rt_dep], 'define': false}], ['shm_unlink', {'dependencies': [rt_dep], 'define': false}], ['shmget', {'dependencies': [cygipc_dep], 'define': false}], + ['snprintf_l'], + ['strtod_l'], ['socket', {'dependencies': [socket_dep], 'define': false}], ['strchrnul'], ['strerror_r', {'dependencies': [thread_dep]}], diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 07b2f798abd..ae530506190 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -349,6 +349,9 @@ /* Define to 1 if you have the `setproctitle_fast' function. */ #undef HAVE_SETPROCTITLE_FAST +/* Define to 1 if you have the `snprintf_l' function. */ +#undef HAVE_SNPRINTF_L + /* Define to 1 if the system has the type `socklen_t'. */ #undef HAVE_SOCKLEN_T @@ -394,6 +397,9 @@ /* Define to 1 if you have the `strsignal' function. */ #undef HAVE_STRSIGNAL +/* Define to 1 if you have the `strtod_l' function. */ +#undef HAVE_STRTOD_L + /* Define to 1 if the system has the type `struct option'. */ #undef HAVE_STRUCT_OPTION diff --git a/src/include/port.h b/src/include/port.h index 703cad868ba..30109bfbc95 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -218,6 +218,37 @@ extern int pg_fprintf(FILE *stream, const char *fmt,...) pg_attribute_printf(2, extern int pg_vprintf(const char *fmt, va_list args) pg_attribute_printf(1, 0); extern int pg_printf(const char *fmt,...) pg_attribute_printf(1, 2); +/* + * A couple of systems offer a fast constant locale_t value representing the + * "C" locale. We use that if possible, but fall back to creating a singleton + * object otherwise. To check that it is available, call pg_ensure_c_locale() + * and assume out of memory if it returns false. + */ +#ifdef LC_C_LOCALE +#define PG_C_LOCALE LC_C_LOCALE +#define pg_ensure_c_locale() true +#else +extern locale_t pg_get_c_locale(void); +#define PG_C_LOCALE pg_get_c_locale() +#define pg_ensure_c_locale() (PG_C_LOCALE != 0) +#endif + +#if !defined(HAVE_STRTOD_L) && !defined(WIN32) +/* + * POSIX doesn't define this function, but we can implement it with thread-safe + * save-and-restore. + */ +static inline double +strtod_l(const char *nptr, char **endptr, locale_t loc) +{ + locale_t save = uselocale(loc); + double result = strtod(nptr, endptr); + + uselocale(save); + return result; +} +#endif + #ifndef WIN32 /* * We add a pg_ prefix as a warning that the Windows implementations have the diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index ff7028bdc81..ff0b7a0de18 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -453,6 +453,7 @@ extern int _pglstat64(const char *name, struct stat *buf); #define isspace_l _isspace_l #define iswspace_l _iswspace_l #define strcoll_l _strcoll_l +#define strtod_l _strtod_l #define strxfrm_l _strxfrm_l #define wcscoll_l _wcscoll_l diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c index 2bbb70333dc..475f0215603 100644 --- a/src/interfaces/ecpg/ecpglib/connect.c +++ b/src/interfaces/ecpg/ecpglib/connect.c @@ -10,10 +10,6 @@ #include "ecpgtype.h" #include "sqlca.h" -#ifdef HAVE_USELOCALE -locale_t ecpg_clocale = (locale_t) 0; -#endif - static pthread_mutex_t connections_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_key_t actual_connection_key; static pthread_once_t actual_connection_key_once = PTHREAD_ONCE_INIT; @@ -268,7 +264,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p const char **conn_keywords; const char **conn_values; - if (sqlca == NULL) + if (sqlca == NULL || !pg_ensure_c_locale()) { ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL); @@ -483,39 +479,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p /* add connection to our list */ pthread_mutex_lock(&connections_mutex); - /* - * ... but first, make certain we have created ecpg_clocale. Rely on - * holding connections_mutex to ensure this is done by only one thread. - */ -#ifdef HAVE_USELOCALE - if (!ecpg_clocale) - { - ecpg_clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0); - if (!ecpg_clocale) - { - pthread_mutex_unlock(&connections_mutex); - ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, - ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL); - if (host) - ecpg_free(host); - if (port) - ecpg_free(port); - if (options) - ecpg_free(options); - if (realname) - ecpg_free(realname); - if (dbname) - ecpg_free(dbname); - if (conn_keywords) - ecpg_free(conn_keywords); - if (conn_values) - ecpg_free(conn_values); - free(this); - return false; - } - } -#endif - if (connection_name != NULL) this->name = ecpg_strdup(connection_name, lineno); else diff --git a/src/interfaces/ecpg/ecpglib/data.c b/src/interfaces/ecpg/ecpglib/data.c index fa562767585..856f4c9472d 100644 --- a/src/interfaces/ecpg/ecpglib/data.c +++ b/src/interfaces/ecpg/ecpglib/data.c @@ -466,7 +466,7 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno, pval++; if (!check_special_value(pval, &dres, &scan_length)) - dres = strtod(pval, &scan_length); + dres = strtod_l(pval, &scan_length, PG_C_LOCALE); if (isarray && *scan_length == '"') scan_length++; diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index 8525a6812f2..fc5e1c9dd89 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -475,46 +475,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) memset(&stmt, 0, sizeof stmt); stmt.lineno = lineno; - /* Make sure we do NOT honor the locale for numeric input */ - /* since the database gives the standard decimal point */ - /* (see comments in execute.c) */ -#ifdef HAVE_USELOCALE - - /* - * To get here, the above PQnfields() test must have found nonzero - * fields. One needs a connection to create such a descriptor. (EXEC - * SQL SET DESCRIPTOR can populate the descriptor's "items", but it - * can't change the descriptor's PQnfields().) Any successful - * connection initializes ecpg_clocale. - */ - Assert(ecpg_clocale); - stmt.oldlocale = uselocale(ecpg_clocale); -#else -#ifdef WIN32 - stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); -#endif - stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); - setlocale(LC_NUMERIC, "C"); -#endif - /* desperate try to guess something sensible */ stmt.connection = ecpg_get_connection(NULL); ecpg_store_result(ECPGresult, index, &stmt, &data_var); - -#ifdef HAVE_USELOCALE - if (stmt.oldlocale != (locale_t) 0) - uselocale(stmt.oldlocale); -#else - if (stmt.oldlocale) - { - setlocale(LC_NUMERIC, stmt.oldlocale); - ecpg_free(stmt.oldlocale); - } -#ifdef WIN32 - if (stmt.oldthreadlocale != -1) - _configthreadlocale(stmt.oldthreadlocale); -#endif -#endif } else if (data_var.ind_type != ECPGt_NO_INDICATOR && data_var.ind_pointer != NULL) diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index 75cc68275bd..40988d53575 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h @@ -56,10 +56,6 @@ struct ECPGtype_information_cache enum ARRAY_TYPE isarray; }; -#ifdef HAVE_USELOCALE -extern locale_t ecpg_clocale; /* LC_NUMERIC=C */ -#endif - /* structure to store one statement */ struct statement { @@ -73,14 +69,6 @@ struct statement bool questionmarks; struct variable *inlist; struct variable *outlist; -#ifdef HAVE_USELOCALE - locale_t oldlocale; -#else - char *oldlocale; -#ifdef WIN32 - int oldthreadlocale; -#endif -#endif int nparams; char **paramvalues; int *paramlengths; diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index b5089eac787..ec939d0f63a 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -101,9 +101,6 @@ free_statement(struct statement *stmt) free_variable(stmt->outlist); ecpg_free(stmt->command); ecpg_free(stmt->name); -#ifndef HAVE_USELOCALE - ecpg_free(stmt->oldlocale); -#endif ecpg_free(stmt); } @@ -1973,43 +1970,6 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, if (stmt == NULL) return false; - /* - * Make sure we do NOT honor the locale for numeric input/output since the - * database wants the standard decimal point. If available, use - * uselocale() for this because it's thread-safe. Windows doesn't have - * that, but it does have _configthreadlocale(). - */ -#ifdef HAVE_USELOCALE - - /* - * Since ecpg_init() succeeded, we have a connection. Any successful - * connection initializes ecpg_clocale. - */ - Assert(ecpg_clocale); - stmt->oldlocale = uselocale(ecpg_clocale); - if (stmt->oldlocale == (locale_t) 0) - { - ecpg_do_epilogue(stmt); - return false; - } -#else -#ifdef WIN32 - stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); - if (stmt->oldthreadlocale == -1) - { - ecpg_do_epilogue(stmt); - return false; - } -#endif - stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); - if (stmt->oldlocale == NULL) - { - ecpg_do_epilogue(stmt); - return false; - } - setlocale(LC_NUMERIC, "C"); -#endif - /* * If statement type is ECPGst_prepnormal we are supposed to prepare the * statement before executing them @@ -2216,19 +2176,6 @@ ecpg_do_epilogue(struct statement *stmt) if (stmt == NULL) return; -#ifdef HAVE_USELOCALE - if (stmt->oldlocale != (locale_t) 0) - uselocale(stmt->oldlocale); -#else - if (stmt->oldlocale) - { - setlocale(LC_NUMERIC, stmt->oldlocale); -#ifdef WIN32 - _configthreadlocale(stmt->oldthreadlocale); -#endif - } -#endif - free_statement(stmt); } diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c index c4119ab7932..f8349b66ce9 100644 --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c @@ -1218,7 +1218,7 @@ DecodeNumber(int flen, char *str, int fmask, return DecodeNumberField(flen, str, (fmask | DTK_DATE_M), tmask, tm, fsec, is2digits); - *fsec = strtod(cp, &cp); + *fsec = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; } @@ -2030,7 +2030,7 @@ DecodeDateTime(char **field, int *ftype, int nf, { double frac; - frac = strtod(cp, &cp); + frac = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; *fsec = frac * 1000000; @@ -2054,7 +2054,7 @@ DecodeDateTime(char **field, int *ftype, int nf, { double time; - time = strtod(cp, &cp); + time = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; diff --git a/src/interfaces/ecpg/pgtypeslib/interval.c b/src/interfaces/ecpg/pgtypeslib/interval.c index 936a6883816..155c6cc7770 100644 --- a/src/interfaces/ecpg/pgtypeslib/interval.c +++ b/src/interfaces/ecpg/pgtypeslib/interval.c @@ -60,7 +60,7 @@ ParseISO8601Number(const char *str, char **endptr, int *ipart, double *fpart) if (!(isdigit((unsigned char) *str) || *str == '-' || *str == '.')) return DTERR_BAD_FORMAT; errno = 0; - val = strtod(str, endptr); + val = strtod_l(str, endptr, PG_C_LOCALE); /* did we not see anything that looks like a double? */ if (*endptr == str || errno != 0) return DTERR_BAD_FORMAT; @@ -455,7 +455,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */ else if (*cp == '.') { errno = 0; - fval = strtod(cp, &cp); + fval = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0' || errno != 0) return DTERR_BAD_FORMAT; diff --git a/src/interfaces/ecpg/pgtypeslib/numeric.c b/src/interfaces/ecpg/pgtypeslib/numeric.c index 35e7b92da40..49938543d03 100644 --- a/src/interfaces/ecpg/pgtypeslib/numeric.c +++ b/src/interfaces/ecpg/pgtypeslib/numeric.c @@ -1455,7 +1455,7 @@ numericvar_to_double(numeric *var, double *dp) * strtod does not reset errno to 0 in case of success. */ errno = 0; - val = strtod(tmp, &endptr); + val = strtod_l(tmp, &endptr, PG_C_LOCALE); if (errno == ERANGE) { free(tmp); diff --git a/src/port/Makefile b/src/port/Makefile index 4c224319512..8b864cccce3 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -41,6 +41,7 @@ OBJS = \ bsearch_arg.o \ chklocale.o \ inet_net_ntop.o \ + locale.o \ noblock.o \ path.o \ pg_bitutils.o \ diff --git a/src/port/locale.c b/src/port/locale.c new file mode 100644 index 00000000000..387fb4a107a --- /dev/null +++ b/src/port/locale.c @@ -0,0 +1,80 @@ +/*------------------------------------------------------------------------- + * + * locale.c + * Helper routines for thread-safe system locale usage. + * + * + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/port/locale.c + * + *------------------------------------------------------------------------- + */ + +#include "c.h" + +#ifndef WIN32 +#include <pthread.h> +#else +#include <synchapi.h> +#endif + +/* A process-lifetime singleton, allocated on first need. */ +static locale_t c_locale; + +#ifndef WIN32 +static void +init_c_locale_once(void) +{ + c_locale = newlocale(LC_ALL, "C", NULL); +} +#else +static BOOL +init_c_locale_once(PINIT_ONCE once, PVOID parameter, PVOID *context) +{ + c_locale = _create_locale(LC_ALL, "C"); + return true; +} +#endif + +/* + * Access a process-lifetime singleton locale_t object. Use the macro + * PG_C_LOCALE instead of calling this directly, as it can skip the function + * call on some systems. + */ +locale_t +pg_get_c_locale(void) +{ + /* + * Fast path if already initialized. This assumes that we can read a + * locale_t (in practice, a pointer) without tearing in a multi-threaded + * program. + */ + if (c_locale != (locale_t) 0) + return c_locale; + + /* Make a locale_t. It will live until process exit. */ + { +#ifndef WIN32 + static pthread_once_t once = PTHREAD_ONCE_INIT; + + pthread_once(&once, init_c_locale_once); +#else + static INIT_ONCE once; + InitOnceExecuteOnce(&once, init_c_locale_once, NULL, NULL); +#endif + } + + /* + * It's possible that the allocation of the locale failed due to low + * memory, and then (locale_t) 0 will be returned. Users of PG_C_LOCALE + * should defend against that by checking pg_ensure_c_locale() at a + * convenient time, so that they can treat it as a simple constant after + * that. + */ + + return c_locale; +} diff --git a/src/port/meson.build b/src/port/meson.build index 7fcfa728d43..4ba8f63323e 100644 --- a/src/port/meson.build +++ b/src/port/meson.build @@ -5,6 +5,7 @@ pgport_sources = [ 'chklocale.c', 'inet_net_ntop.c', 'noblock.c', + 'locale.c', 'path.c', 'pg_bitutils.c', 'pg_popcount_avx512.c', diff --git a/src/port/snprintf.c b/src/port/snprintf.c index f8f2018ea0c..376d5c906a0 100644 --- a/src/port/snprintf.c +++ b/src/port/snprintf.c @@ -109,6 +109,36 @@ #undef vprintf #undef printf +#if defined(FRONTEND) +/* + * Frontend code might be multi-threaded. When calling the system snprintf() + * for floats, we have to use either the non-standard snprintf_l() variant, or + * save-and-restore the calling thread's locale using uselocale(), depending on + * availability. + */ +#if defined(HAVE_SNPRINTF_L) +/* At least macOS and the BSDs have the snprintf_l() extension. */ +#define USE_SNPRINTF_L +#elif defined(WIN32) +/* Windows has a version with a different name and argument order. */ +#define snprintf_l(str, size, loc, format, ...) _snprintf_l(str, size, format, loc, __VA_ARGS__) +#define USE_SNPRINTF_L +#else +/* We have to do a thread-safe save-and-restore around snprintf(). */ +#define NEED_USE_LOCALE +#endif +#else +/* + * Backend code doesn't need to worry about locales here, because LC_NUMERIC is + * set to "C" in main() and doesn't change. Plain old snprintf() is always OK + * without uselocale(). + * + * XXX If the backend were multithreaded, we would have to be 100% certain that + * no one is calling setlocale() concurrently, even transiently, to be able to + * keep this small optimization. + */ +#endif + /* * Info about where the formatted output is going. * @@ -1220,6 +1250,9 @@ fmtfloat(double value, char type, int forcesign, int leftjust, * according to == but not according to memcmp. */ static const double dzero = 0.0; +#ifdef NEED_USE_LOCALE + locale_t save_locale = uselocale(PG_C_LOCALE); +#endif if (adjust_sign((value < 0.0 || (value == 0.0 && @@ -1241,7 +1274,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust, fmt[2] = '*'; fmt[3] = type; fmt[4] = '\0'; +#ifdef USE_SNPRINTF_L + vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, prec, value); +#else vallen = snprintf(convert, sizeof(convert), fmt, prec, value); +#endif } else { @@ -1250,6 +1287,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust, fmt[2] = '\0'; vallen = snprintf(convert, sizeof(convert), fmt, value); } + +#ifdef NEED_USE_LOCALE + uselocale(save_locale); +#endif + if (vallen < 0) goto fail; @@ -1372,12 +1414,25 @@ pg_strfromd(char *str, size_t count, int precision, double value) } else { +#ifdef NEED_USE_LOCALE + locale_t save_locale = uselocale(PG_C_LOCALE); +#endif + fmt[0] = '%'; fmt[1] = '.'; fmt[2] = '*'; fmt[3] = 'g'; fmt[4] = '\0'; +#ifdef USE_SNPRINTF_L + vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, precision, value); +#else vallen = snprintf(convert, sizeof(convert), fmt, precision, value); +#endif + +#ifdef NEED_USE_LOCALE + uselocale(save_locale); +#endif + if (vallen < 0) { target.failed = true; base-commit: b92c03342dcd69d259262b06b5c290e27249cb11 -- 2.48.1 From d61d358997ce6c15a891588953b76e174e3041ac Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 9 Feb 2025 07:49:00 +0100 Subject: [PATCH v7 2/3] Fixup: fix compiler warnings if LC_C_LOCALE exists --- src/port/locale.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/port/locale.c b/src/port/locale.c index 387fb4a107a..b3d48628915 100644 --- a/src/port/locale.c +++ b/src/port/locale.c @@ -16,6 +16,8 @@ #include "c.h" +#ifndef LC_C_LOCALE + #ifndef WIN32 #include <pthread.h> #else @@ -78,3 +80,5 @@ pg_get_c_locale(void) return c_locale; } + +#endif /* not LC_C_LOCALE */ -- 2.48.1 From edf9e527e82f1997396cd3fde873e82c25aade37 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 9 Feb 2025 07:52:45 +0100 Subject: [PATCH v7 3/3] Fixup: sort some lists better --- configure | 2 +- configure.ac | 2 +- meson.build | 2 +- src/port/meson.build | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configure b/configure index 017e63598c1..bbe4731bcf5 100755 --- a/configure +++ b/configure @@ -14934,7 +14934,7 @@ fi LIBS_including_readline="$LIBS" LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'` -for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strtod_l strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l +for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strchrnul strsignal strtod_l syncfs sync_file_range uselocale wcstombs_l do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.ac b/configure.ac index b06a9cd1080..d636d4678b7 100644 --- a/configure.ac +++ b/configure.ac @@ -1719,9 +1719,9 @@ AC_CHECK_FUNCS(m4_normalize([ setproctitle setproctitle_fast snprintf_l - strtod_l strchrnul strsignal + strtod_l syncfs sync_file_range uselocale diff --git a/meson.build b/meson.build index e03b5083f27..73ccbd4cee2 100644 --- a/meson.build +++ b/meson.build @@ -2649,7 +2649,6 @@ func_checks = [ ['shm_unlink', {'dependencies': [rt_dep], 'define': false}], ['shmget', {'dependencies': [cygipc_dep], 'define': false}], ['snprintf_l'], - ['strtod_l'], ['socket', {'dependencies': [socket_dep], 'define': false}], ['strchrnul'], ['strerror_r', {'dependencies': [thread_dep]}], @@ -2658,6 +2657,7 @@ func_checks = [ ['strnlen'], ['strsep'], ['strsignal'], + ['strtod_l'], ['sync_file_range'], ['syncfs'], ['uselocale'], diff --git a/src/port/meson.build b/src/port/meson.build index 4ba8f63323e..1656328f210 100644 --- a/src/port/meson.build +++ b/src/port/meson.build @@ -4,8 +4,8 @@ pgport_sources = [ 'bsearch_arg.c', 'chklocale.c', 'inet_net_ntop.c', - 'noblock.c', 'locale.c', + 'noblock.c', 'path.c', 'pg_bitutils.c', 'pg_popcount_avx512.c', -- 2.48.1 Attachments: [text/plain] v7-0001-Tidy-up-locale-thread-safety-in-ECPG-library.patch (22.8K, ../../[email protected]/2-v7-0001-Tidy-up-locale-thread-safety-in-ECPG-library.patch) download | inline diff: From 842a5837898a4a47c8c79d07a2d14f3a82514a36 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Sat, 10 Aug 2024 11:01:21 +1200 Subject: [PATCH v7 1/3] Tidy up locale thread safety in ECPG library. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove setlocale() and _configthreadlocal() as fallback strategy on systems that don't have uselocale(), where ECPG tries to control LC_NUMERIC formatting on input and output of floating point numbers. It was probably broken on some systems (NetBSD), and the code was also quite messy and complicated, with obsolete configure tests (Windows). It was also arguably broken, or at least had unstated environmental requirements, if pgtypeslib code was called directly. Instead, introduce PG_C_LOCALE to refer to the "C" locale as a locale_t value. It maps to the special constant LC_C_LOCALE when defined by libc (macOS, NetBSD), or otherwise uses a process-lifetime locale_t that is allocated on first use, just as ECPG previously did itself. The new replacement might be more widely useful. Then change the float parsing and printing code to pass that to _l() functions where appropriate. Unfortunately the portability of those functions is a bit complicated. First, many obvious and useful _l() functions are missing from POSIX, though most standard libraries define some of them anyway. Second, although the thread-safe save/restore technique can be used to replace the missing ones, Windows and NetBSD refused to implement standard uselocale(). They might have a point: "wide scope" uselocale() is hard to combine with other code and error-prone, especially in library code. Luckily they have the _l() functions we want so far anyway. So we have to be prepared for both ways of doing things: 1. In ECPG, use strtod_l() for parsing, and supply a port.h replacement using uselocale() over a limited scope if missing. 2. Inside our own snprintf.c, use three different approaches to format floats. For frontend code, call libc's snprintf_l(), or wrap libc's snprintf() in uselocale() if it's missing. For backend code, snprintf.c can keep assuming that the global locale's LC_NUMERIC is "C" and call libc's snprintf() without change, for now. (It might eventually be possible to call our in-tree Ryū routines to display floats in snprintf.c, given the C-locale-always remit of our in-tree snprintf(), but this patch doesn't risk changing anything that complicated.) Reviewed-by: Peter Eisentraut <[email protected]> Reviewed-by: Tristan Partin <[email protected]> Discussion: https://postgr.es/m/CWZBBRR6YA8D.8EHMDRGLCKCD%40neon.tech --- configure | 2 +- configure.ac | 2 + meson.build | 2 + src/include/pg_config.h.in | 6 ++ src/include/port.h | 31 ++++++++ src/include/port/win32_port.h | 1 + src/interfaces/ecpg/ecpglib/connect.c | 39 +--------- src/interfaces/ecpg/ecpglib/data.c | 2 +- src/interfaces/ecpg/ecpglib/descriptor.c | 37 --------- src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 12 --- src/interfaces/ecpg/ecpglib/execute.c | 53 ------------- src/interfaces/ecpg/pgtypeslib/dt_common.c | 6 +- src/interfaces/ecpg/pgtypeslib/interval.c | 4 +- src/interfaces/ecpg/pgtypeslib/numeric.c | 2 +- src/port/Makefile | 1 + src/port/locale.c | 80 ++++++++++++++++++++ src/port/meson.build | 1 + src/port/snprintf.c | 55 ++++++++++++++ 18 files changed, 188 insertions(+), 148 deletions(-) create mode 100644 src/port/locale.c diff --git a/configure b/configure index 0ffcaeb4367..017e63598c1 100755 --- a/configure +++ b/configure @@ -14934,7 +14934,7 @@ fi LIBS_including_readline="$LIBS" LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'` -for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l +for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strtod_l strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.ac b/configure.ac index f56681e0d91..b06a9cd1080 100644 --- a/configure.ac +++ b/configure.ac @@ -1718,6 +1718,8 @@ AC_CHECK_FUNCS(m4_normalize([ pthread_is_threaded_np setproctitle setproctitle_fast + snprintf_l + strtod_l strchrnul strsignal syncfs diff --git a/meson.build b/meson.build index 1ceadb9a830..e03b5083f27 100644 --- a/meson.build +++ b/meson.build @@ -2648,6 +2648,8 @@ func_checks = [ ['shm_open', {'dependencies': [rt_dep], 'define': false}], ['shm_unlink', {'dependencies': [rt_dep], 'define': false}], ['shmget', {'dependencies': [cygipc_dep], 'define': false}], + ['snprintf_l'], + ['strtod_l'], ['socket', {'dependencies': [socket_dep], 'define': false}], ['strchrnul'], ['strerror_r', {'dependencies': [thread_dep]}], diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index 07b2f798abd..ae530506190 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -349,6 +349,9 @@ /* Define to 1 if you have the `setproctitle_fast' function. */ #undef HAVE_SETPROCTITLE_FAST +/* Define to 1 if you have the `snprintf_l' function. */ +#undef HAVE_SNPRINTF_L + /* Define to 1 if the system has the type `socklen_t'. */ #undef HAVE_SOCKLEN_T @@ -394,6 +397,9 @@ /* Define to 1 if you have the `strsignal' function. */ #undef HAVE_STRSIGNAL +/* Define to 1 if you have the `strtod_l' function. */ +#undef HAVE_STRTOD_L + /* Define to 1 if the system has the type `struct option'. */ #undef HAVE_STRUCT_OPTION diff --git a/src/include/port.h b/src/include/port.h index 703cad868ba..30109bfbc95 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -218,6 +218,37 @@ extern int pg_fprintf(FILE *stream, const char *fmt,...) pg_attribute_printf(2, extern int pg_vprintf(const char *fmt, va_list args) pg_attribute_printf(1, 0); extern int pg_printf(const char *fmt,...) pg_attribute_printf(1, 2); +/* + * A couple of systems offer a fast constant locale_t value representing the + * "C" locale. We use that if possible, but fall back to creating a singleton + * object otherwise. To check that it is available, call pg_ensure_c_locale() + * and assume out of memory if it returns false. + */ +#ifdef LC_C_LOCALE +#define PG_C_LOCALE LC_C_LOCALE +#define pg_ensure_c_locale() true +#else +extern locale_t pg_get_c_locale(void); +#define PG_C_LOCALE pg_get_c_locale() +#define pg_ensure_c_locale() (PG_C_LOCALE != 0) +#endif + +#if !defined(HAVE_STRTOD_L) && !defined(WIN32) +/* + * POSIX doesn't define this function, but we can implement it with thread-safe + * save-and-restore. + */ +static inline double +strtod_l(const char *nptr, char **endptr, locale_t loc) +{ + locale_t save = uselocale(loc); + double result = strtod(nptr, endptr); + + uselocale(save); + return result; +} +#endif + #ifndef WIN32 /* * We add a pg_ prefix as a warning that the Windows implementations have the diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index ff7028bdc81..ff0b7a0de18 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -453,6 +453,7 @@ extern int _pglstat64(const char *name, struct stat *buf); #define isspace_l _isspace_l #define iswspace_l _iswspace_l #define strcoll_l _strcoll_l +#define strtod_l _strtod_l #define strxfrm_l _strxfrm_l #define wcscoll_l _wcscoll_l diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c index 2bbb70333dc..475f0215603 100644 --- a/src/interfaces/ecpg/ecpglib/connect.c +++ b/src/interfaces/ecpg/ecpglib/connect.c @@ -10,10 +10,6 @@ #include "ecpgtype.h" #include "sqlca.h" -#ifdef HAVE_USELOCALE -locale_t ecpg_clocale = (locale_t) 0; -#endif - static pthread_mutex_t connections_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_key_t actual_connection_key; static pthread_once_t actual_connection_key_once = PTHREAD_ONCE_INIT; @@ -268,7 +264,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p const char **conn_keywords; const char **conn_values; - if (sqlca == NULL) + if (sqlca == NULL || !pg_ensure_c_locale()) { ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL); @@ -483,39 +479,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p /* add connection to our list */ pthread_mutex_lock(&connections_mutex); - /* - * ... but first, make certain we have created ecpg_clocale. Rely on - * holding connections_mutex to ensure this is done by only one thread. - */ -#ifdef HAVE_USELOCALE - if (!ecpg_clocale) - { - ecpg_clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0); - if (!ecpg_clocale) - { - pthread_mutex_unlock(&connections_mutex); - ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, - ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL); - if (host) - ecpg_free(host); - if (port) - ecpg_free(port); - if (options) - ecpg_free(options); - if (realname) - ecpg_free(realname); - if (dbname) - ecpg_free(dbname); - if (conn_keywords) - ecpg_free(conn_keywords); - if (conn_values) - ecpg_free(conn_values); - free(this); - return false; - } - } -#endif - if (connection_name != NULL) this->name = ecpg_strdup(connection_name, lineno); else diff --git a/src/interfaces/ecpg/ecpglib/data.c b/src/interfaces/ecpg/ecpglib/data.c index fa562767585..856f4c9472d 100644 --- a/src/interfaces/ecpg/ecpglib/data.c +++ b/src/interfaces/ecpg/ecpglib/data.c @@ -466,7 +466,7 @@ ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno, pval++; if (!check_special_value(pval, &dres, &scan_length)) - dres = strtod(pval, &scan_length); + dres = strtod_l(pval, &scan_length, PG_C_LOCALE); if (isarray && *scan_length == '"') scan_length++; diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c index 8525a6812f2..fc5e1c9dd89 100644 --- a/src/interfaces/ecpg/ecpglib/descriptor.c +++ b/src/interfaces/ecpg/ecpglib/descriptor.c @@ -475,46 +475,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...) memset(&stmt, 0, sizeof stmt); stmt.lineno = lineno; - /* Make sure we do NOT honor the locale for numeric input */ - /* since the database gives the standard decimal point */ - /* (see comments in execute.c) */ -#ifdef HAVE_USELOCALE - - /* - * To get here, the above PQnfields() test must have found nonzero - * fields. One needs a connection to create such a descriptor. (EXEC - * SQL SET DESCRIPTOR can populate the descriptor's "items", but it - * can't change the descriptor's PQnfields().) Any successful - * connection initializes ecpg_clocale. - */ - Assert(ecpg_clocale); - stmt.oldlocale = uselocale(ecpg_clocale); -#else -#ifdef WIN32 - stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); -#endif - stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); - setlocale(LC_NUMERIC, "C"); -#endif - /* desperate try to guess something sensible */ stmt.connection = ecpg_get_connection(NULL); ecpg_store_result(ECPGresult, index, &stmt, &data_var); - -#ifdef HAVE_USELOCALE - if (stmt.oldlocale != (locale_t) 0) - uselocale(stmt.oldlocale); -#else - if (stmt.oldlocale) - { - setlocale(LC_NUMERIC, stmt.oldlocale); - ecpg_free(stmt.oldlocale); - } -#ifdef WIN32 - if (stmt.oldthreadlocale != -1) - _configthreadlocale(stmt.oldthreadlocale); -#endif -#endif } else if (data_var.ind_type != ECPGt_NO_INDICATOR && data_var.ind_pointer != NULL) diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h index 75cc68275bd..40988d53575 100644 --- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h +++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h @@ -56,10 +56,6 @@ struct ECPGtype_information_cache enum ARRAY_TYPE isarray; }; -#ifdef HAVE_USELOCALE -extern locale_t ecpg_clocale; /* LC_NUMERIC=C */ -#endif - /* structure to store one statement */ struct statement { @@ -73,14 +69,6 @@ struct statement bool questionmarks; struct variable *inlist; struct variable *outlist; -#ifdef HAVE_USELOCALE - locale_t oldlocale; -#else - char *oldlocale; -#ifdef WIN32 - int oldthreadlocale; -#endif -#endif int nparams; char **paramvalues; int *paramlengths; diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index b5089eac787..ec939d0f63a 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -101,9 +101,6 @@ free_statement(struct statement *stmt) free_variable(stmt->outlist); ecpg_free(stmt->command); ecpg_free(stmt->name); -#ifndef HAVE_USELOCALE - ecpg_free(stmt->oldlocale); -#endif ecpg_free(stmt); } @@ -1973,43 +1970,6 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator, if (stmt == NULL) return false; - /* - * Make sure we do NOT honor the locale for numeric input/output since the - * database wants the standard decimal point. If available, use - * uselocale() for this because it's thread-safe. Windows doesn't have - * that, but it does have _configthreadlocale(). - */ -#ifdef HAVE_USELOCALE - - /* - * Since ecpg_init() succeeded, we have a connection. Any successful - * connection initializes ecpg_clocale. - */ - Assert(ecpg_clocale); - stmt->oldlocale = uselocale(ecpg_clocale); - if (stmt->oldlocale == (locale_t) 0) - { - ecpg_do_epilogue(stmt); - return false; - } -#else -#ifdef WIN32 - stmt->oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE); - if (stmt->oldthreadlocale == -1) - { - ecpg_do_epilogue(stmt); - return false; - } -#endif - stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno); - if (stmt->oldlocale == NULL) - { - ecpg_do_epilogue(stmt); - return false; - } - setlocale(LC_NUMERIC, "C"); -#endif - /* * If statement type is ECPGst_prepnormal we are supposed to prepare the * statement before executing them @@ -2216,19 +2176,6 @@ ecpg_do_epilogue(struct statement *stmt) if (stmt == NULL) return; -#ifdef HAVE_USELOCALE - if (stmt->oldlocale != (locale_t) 0) - uselocale(stmt->oldlocale); -#else - if (stmt->oldlocale) - { - setlocale(LC_NUMERIC, stmt->oldlocale); -#ifdef WIN32 - _configthreadlocale(stmt->oldthreadlocale); -#endif - } -#endif - free_statement(stmt); } diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c index c4119ab7932..f8349b66ce9 100644 --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c @@ -1218,7 +1218,7 @@ DecodeNumber(int flen, char *str, int fmask, return DecodeNumberField(flen, str, (fmask | DTK_DATE_M), tmask, tm, fsec, is2digits); - *fsec = strtod(cp, &cp); + *fsec = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; } @@ -2030,7 +2030,7 @@ DecodeDateTime(char **field, int *ftype, int nf, { double frac; - frac = strtod(cp, &cp); + frac = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; *fsec = frac * 1000000; @@ -2054,7 +2054,7 @@ DecodeDateTime(char **field, int *ftype, int nf, { double time; - time = strtod(cp, &cp); + time = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0') return -1; diff --git a/src/interfaces/ecpg/pgtypeslib/interval.c b/src/interfaces/ecpg/pgtypeslib/interval.c index 936a6883816..155c6cc7770 100644 --- a/src/interfaces/ecpg/pgtypeslib/interval.c +++ b/src/interfaces/ecpg/pgtypeslib/interval.c @@ -60,7 +60,7 @@ ParseISO8601Number(const char *str, char **endptr, int *ipart, double *fpart) if (!(isdigit((unsigned char) *str) || *str == '-' || *str == '.')) return DTERR_BAD_FORMAT; errno = 0; - val = strtod(str, endptr); + val = strtod_l(str, endptr, PG_C_LOCALE); /* did we not see anything that looks like a double? */ if (*endptr == str || errno != 0) return DTERR_BAD_FORMAT; @@ -455,7 +455,7 @@ DecodeInterval(char **field, int *ftype, int nf, /* int range, */ else if (*cp == '.') { errno = 0; - fval = strtod(cp, &cp); + fval = strtod_l(cp, &cp, PG_C_LOCALE); if (*cp != '\0' || errno != 0) return DTERR_BAD_FORMAT; diff --git a/src/interfaces/ecpg/pgtypeslib/numeric.c b/src/interfaces/ecpg/pgtypeslib/numeric.c index 35e7b92da40..49938543d03 100644 --- a/src/interfaces/ecpg/pgtypeslib/numeric.c +++ b/src/interfaces/ecpg/pgtypeslib/numeric.c @@ -1455,7 +1455,7 @@ numericvar_to_double(numeric *var, double *dp) * strtod does not reset errno to 0 in case of success. */ errno = 0; - val = strtod(tmp, &endptr); + val = strtod_l(tmp, &endptr, PG_C_LOCALE); if (errno == ERANGE) { free(tmp); diff --git a/src/port/Makefile b/src/port/Makefile index 4c224319512..8b864cccce3 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -41,6 +41,7 @@ OBJS = \ bsearch_arg.o \ chklocale.o \ inet_net_ntop.o \ + locale.o \ noblock.o \ path.o \ pg_bitutils.o \ diff --git a/src/port/locale.c b/src/port/locale.c new file mode 100644 index 00000000000..387fb4a107a --- /dev/null +++ b/src/port/locale.c @@ -0,0 +1,80 @@ +/*------------------------------------------------------------------------- + * + * locale.c + * Helper routines for thread-safe system locale usage. + * + * + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * + * IDENTIFICATION + * src/port/locale.c + * + *------------------------------------------------------------------------- + */ + +#include "c.h" + +#ifndef WIN32 +#include <pthread.h> +#else +#include <synchapi.h> +#endif + +/* A process-lifetime singleton, allocated on first need. */ +static locale_t c_locale; + +#ifndef WIN32 +static void +init_c_locale_once(void) +{ + c_locale = newlocale(LC_ALL, "C", NULL); +} +#else +static BOOL +init_c_locale_once(PINIT_ONCE once, PVOID parameter, PVOID *context) +{ + c_locale = _create_locale(LC_ALL, "C"); + return true; +} +#endif + +/* + * Access a process-lifetime singleton locale_t object. Use the macro + * PG_C_LOCALE instead of calling this directly, as it can skip the function + * call on some systems. + */ +locale_t +pg_get_c_locale(void) +{ + /* + * Fast path if already initialized. This assumes that we can read a + * locale_t (in practice, a pointer) without tearing in a multi-threaded + * program. + */ + if (c_locale != (locale_t) 0) + return c_locale; + + /* Make a locale_t. It will live until process exit. */ + { +#ifndef WIN32 + static pthread_once_t once = PTHREAD_ONCE_INIT; + + pthread_once(&once, init_c_locale_once); +#else + static INIT_ONCE once; + InitOnceExecuteOnce(&once, init_c_locale_once, NULL, NULL); +#endif + } + + /* + * It's possible that the allocation of the locale failed due to low + * memory, and then (locale_t) 0 will be returned. Users of PG_C_LOCALE + * should defend against that by checking pg_ensure_c_locale() at a + * convenient time, so that they can treat it as a simple constant after + * that. + */ + + return c_locale; +} diff --git a/src/port/meson.build b/src/port/meson.build index 7fcfa728d43..4ba8f63323e 100644 --- a/src/port/meson.build +++ b/src/port/meson.build @@ -5,6 +5,7 @@ pgport_sources = [ 'chklocale.c', 'inet_net_ntop.c', 'noblock.c', + 'locale.c', 'path.c', 'pg_bitutils.c', 'pg_popcount_avx512.c', diff --git a/src/port/snprintf.c b/src/port/snprintf.c index f8f2018ea0c..376d5c906a0 100644 --- a/src/port/snprintf.c +++ b/src/port/snprintf.c @@ -109,6 +109,36 @@ #undef vprintf #undef printf +#if defined(FRONTEND) +/* + * Frontend code might be multi-threaded. When calling the system snprintf() + * for floats, we have to use either the non-standard snprintf_l() variant, or + * save-and-restore the calling thread's locale using uselocale(), depending on + * availability. + */ +#if defined(HAVE_SNPRINTF_L) +/* At least macOS and the BSDs have the snprintf_l() extension. */ +#define USE_SNPRINTF_L +#elif defined(WIN32) +/* Windows has a version with a different name and argument order. */ +#define snprintf_l(str, size, loc, format, ...) _snprintf_l(str, size, format, loc, __VA_ARGS__) +#define USE_SNPRINTF_L +#else +/* We have to do a thread-safe save-and-restore around snprintf(). */ +#define NEED_USE_LOCALE +#endif +#else +/* + * Backend code doesn't need to worry about locales here, because LC_NUMERIC is + * set to "C" in main() and doesn't change. Plain old snprintf() is always OK + * without uselocale(). + * + * XXX If the backend were multithreaded, we would have to be 100% certain that + * no one is calling setlocale() concurrently, even transiently, to be able to + * keep this small optimization. + */ +#endif + /* * Info about where the formatted output is going. * @@ -1220,6 +1250,9 @@ fmtfloat(double value, char type, int forcesign, int leftjust, * according to == but not according to memcmp. */ static const double dzero = 0.0; +#ifdef NEED_USE_LOCALE + locale_t save_locale = uselocale(PG_C_LOCALE); +#endif if (adjust_sign((value < 0.0 || (value == 0.0 && @@ -1241,7 +1274,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust, fmt[2] = '*'; fmt[3] = type; fmt[4] = '\0'; +#ifdef USE_SNPRINTF_L + vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, prec, value); +#else vallen = snprintf(convert, sizeof(convert), fmt, prec, value); +#endif } else { @@ -1250,6 +1287,11 @@ fmtfloat(double value, char type, int forcesign, int leftjust, fmt[2] = '\0'; vallen = snprintf(convert, sizeof(convert), fmt, value); } + +#ifdef NEED_USE_LOCALE + uselocale(save_locale); +#endif + if (vallen < 0) goto fail; @@ -1372,12 +1414,25 @@ pg_strfromd(char *str, size_t count, int precision, double value) } else { +#ifdef NEED_USE_LOCALE + locale_t save_locale = uselocale(PG_C_LOCALE); +#endif + fmt[0] = '%'; fmt[1] = '.'; fmt[2] = '*'; fmt[3] = 'g'; fmt[4] = '\0'; +#ifdef USE_SNPRINTF_L + vallen = snprintf_l(convert, sizeof(convert), PG_C_LOCALE, fmt, precision, value); +#else vallen = snprintf(convert, sizeof(convert), fmt, precision, value); +#endif + +#ifdef NEED_USE_LOCALE + uselocale(save_locale); +#endif + if (vallen < 0) { target.failed = true; base-commit: b92c03342dcd69d259262b06b5c290e27249cb11 -- 2.48.1 [text/plain] v7-0002-Fixup-fix-compiler-warnings-if-LC_C_LOCALE-exists.patch (652B, ../../[email protected]/3-v7-0002-Fixup-fix-compiler-warnings-if-LC_C_LOCALE-exists.patch) download | inline diff: From d61d358997ce6c15a891588953b76e174e3041ac Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 9 Feb 2025 07:49:00 +0100 Subject: [PATCH v7 2/3] Fixup: fix compiler warnings if LC_C_LOCALE exists --- src/port/locale.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/port/locale.c b/src/port/locale.c index 387fb4a107a..b3d48628915 100644 --- a/src/port/locale.c +++ b/src/port/locale.c @@ -16,6 +16,8 @@ #include "c.h" +#ifndef LC_C_LOCALE + #ifndef WIN32 #include <pthread.h> #else @@ -78,3 +80,5 @@ pg_get_c_locale(void) return c_locale; } + +#endif /* not LC_C_LOCALE */ -- 2.48.1 [text/plain] v7-0003-Fixup-sort-some-lists-better.patch (2.5K, ../../[email protected]/4-v7-0003-Fixup-sort-some-lists-better.patch) download | inline diff: From edf9e527e82f1997396cd3fde873e82c25aade37 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut <[email protected]> Date: Sun, 9 Feb 2025 07:52:45 +0100 Subject: [PATCH v7 3/3] Fixup: sort some lists better --- configure | 2 +- configure.ac | 2 +- meson.build | 2 +- src/port/meson.build | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/configure b/configure index 017e63598c1..bbe4731bcf5 100755 --- a/configure +++ b/configure @@ -14934,7 +14934,7 @@ fi LIBS_including_readline="$LIBS" LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'` -for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strtod_l strchrnul strsignal syncfs sync_file_range uselocale wcstombs_l +for ac_func in backtrace_symbols copyfile copy_file_range elf_aux_info getauxval getifaddrs getpeerucred inet_pton kqueue mbstowcs_l memset_s posix_fallocate ppoll pthread_is_threaded_np setproctitle setproctitle_fast snprintf_l strchrnul strsignal strtod_l syncfs sync_file_range uselocale wcstombs_l do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.ac b/configure.ac index b06a9cd1080..d636d4678b7 100644 --- a/configure.ac +++ b/configure.ac @@ -1719,9 +1719,9 @@ AC_CHECK_FUNCS(m4_normalize([ setproctitle setproctitle_fast snprintf_l - strtod_l strchrnul strsignal + strtod_l syncfs sync_file_range uselocale diff --git a/meson.build b/meson.build index e03b5083f27..73ccbd4cee2 100644 --- a/meson.build +++ b/meson.build @@ -2649,7 +2649,6 @@ func_checks = [ ['shm_unlink', {'dependencies': [rt_dep], 'define': false}], ['shmget', {'dependencies': [cygipc_dep], 'define': false}], ['snprintf_l'], - ['strtod_l'], ['socket', {'dependencies': [socket_dep], 'define': false}], ['strchrnul'], ['strerror_r', {'dependencies': [thread_dep]}], @@ -2658,6 +2657,7 @@ func_checks = [ ['strnlen'], ['strsep'], ['strsignal'], + ['strtod_l'], ['sync_file_range'], ['syncfs'], ['uselocale'], diff --git a/src/port/meson.build b/src/port/meson.build index 4ba8f63323e..1656328f210 100644 --- a/src/port/meson.build +++ b/src/port/meson.build @@ -4,8 +4,8 @@ pgport_sources = [ 'bsearch_arg.c', 'chklocale.c', 'inet_net_ntop.c', - 'noblock.c', 'locale.c', + 'noblock.c', 'path.c', 'pg_bitutils.c', 'pg_popcount_avx512.c', -- 2.48.1 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-03-28 15:30 Peter Eisentraut <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Peter Eisentraut @ 2025-03-28 15:30 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On 09.02.25 08:32, Peter Eisentraut wrote: > Checking the status of this thread ... > > The patches that removed the configure checks for _configthreadlocale(), > and related cleanup, have been committed. > > The original patch to "Tidy up locale thread safety in ECPG library" is > still outstanding. > > Attached is a rebased version, based on the posted v6, with a couple of > small fixups from me. > > I haven't re-reviewed it yet, but from scanning the discussion, it looks > close to done. After staring at this a few more times, I figured it was ready enough and I committed it. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-03-28 16:14 Masahiko Sawada <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Masahiko Sawada @ 2025-03-28 16:14 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Fri, Mar 28, 2025 at 8:30 AM Peter Eisentraut <[email protected]> wrote: > > On 09.02.25 08:32, Peter Eisentraut wrote: > > Checking the status of this thread ... > > > > The patches that removed the configure checks for _configthreadlocale(), > > and related cleanup, have been committed. > > > > The original patch to "Tidy up locale thread safety in ECPG library" is > > still outstanding. > > > > Attached is a rebased version, based on the posted v6, with a couple of > > small fixups from me. > > > > I haven't re-reviewed it yet, but from scanning the discussion, it looks > > close to done. > > After staring at this a few more times, I figured it was ready enough > and I committed it. It seems that some bf animals such as jackdaw are unhappy with this commit[0][1]. I also got the same 'undefined reference to symbol error' locally when building test_json_parser. Regards, [0] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=snakefly&dt=2025-03-28%2015%3A29%3A04 [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jackdaw&dt=2025-03-28%2015%3A30%3A44 -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-03-28 16:32 Peter Eisentraut <[email protected]> parent: Masahiko Sawada <[email protected]> 0 siblings, 2 replies; 20+ messages in thread From: Peter Eisentraut @ 2025-03-28 16:32 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On 28.03.25 17:14, Masahiko Sawada wrote: > On Fri, Mar 28, 2025 at 8:30 AM Peter Eisentraut <[email protected]> wrote: >> >> On 09.02.25 08:32, Peter Eisentraut wrote: >>> Checking the status of this thread ... >>> >>> The patches that removed the configure checks for _configthreadlocale(), >>> and related cleanup, have been committed. >>> >>> The original patch to "Tidy up locale thread safety in ECPG library" is >>> still outstanding. >>> >>> Attached is a rebased version, based on the posted v6, with a couple of >>> small fixups from me. >>> >>> I haven't re-reviewed it yet, but from scanning the discussion, it looks >>> close to done. >> >> After staring at this a few more times, I figured it was ready enough >> and I committed it. > > It seems that some bf animals such as jackdaw are unhappy with this > commit[0][1]. I also got the same 'undefined reference to symbol > error' locally when building test_json_parser. Yeah, looks like we'll have to revert this for now. But I'm confused, because I don't see any clear pattern for which platforms or configurations it's failing and for which not. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-03-28 20:34 Peter Eisentraut <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Peter Eisentraut @ 2025-03-28 20:34 UTC (permalink / raw) To: Masahiko Sawada <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On 28.03.25 17:32, Peter Eisentraut wrote: > On 28.03.25 17:14, Masahiko Sawada wrote: >> On Fri, Mar 28, 2025 at 8:30 AM Peter Eisentraut >> <[email protected]> wrote: >>> >>> On 09.02.25 08:32, Peter Eisentraut wrote: >>>> Checking the status of this thread ... >>>> >>>> The patches that removed the configure checks for >>>> _configthreadlocale(), >>>> and related cleanup, have been committed. >>>> >>>> The original patch to "Tidy up locale thread safety in ECPG library" is >>>> still outstanding. >>>> >>>> Attached is a rebased version, based on the posted v6, with a couple of >>>> small fixups from me. >>>> >>>> I haven't re-reviewed it yet, but from scanning the discussion, it >>>> looks >>>> close to done. >>> >>> After staring at this a few more times, I figured it was ready enough >>> and I committed it. >> >> It seems that some bf animals such as jackdaw are unhappy with this >> commit[0][1]. I also got the same 'undefined reference to symbol >> error' locally when building test_json_parser. > > Yeah, looks like we'll have to revert this for now. But I'm confused, > because I don't see any clear pattern for which platforms or > configurations it's failing and for which not. reverted ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-03-29 00:01 Masahiko Sawada <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Masahiko Sawada @ 2025-03-29 00:01 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Thomas Munro <[email protected]>; Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Tristan Partin <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers On Fri, Mar 28, 2025 at 9:32 AM Peter Eisentraut <[email protected]> wrote: > > On 28.03.25 17:14, Masahiko Sawada wrote: > > On Fri, Mar 28, 2025 at 8:30 AM Peter Eisentraut <[email protected]> wrote: > >> > >> On 09.02.25 08:32, Peter Eisentraut wrote: > >>> Checking the status of this thread ... > >>> > >>> The patches that removed the configure checks for _configthreadlocale(), > >>> and related cleanup, have been committed. > >>> > >>> The original patch to "Tidy up locale thread safety in ECPG library" is > >>> still outstanding. > >>> > >>> Attached is a rebased version, based on the posted v6, with a couple of > >>> small fixups from me. > >>> > >>> I haven't re-reviewed it yet, but from scanning the discussion, it looks > >>> close to done. > >> > >> After staring at this a few more times, I figured it was ready enough > >> and I committed it. > > > > It seems that some bf animals such as jackdaw are unhappy with this > > commit[0][1]. I also got the same 'undefined reference to symbol > > error' locally when building test_json_parser. > > Yeah, looks like we'll have to revert this for now. But I'm confused, > because I don't see any clear pattern for which platforms or > configurations it's failing and for which not. > Not sure it would help the investigation but I got the linker error when building with 'make' but not with 'meson'. Looking at the build logs, when building test_json_parser with meson, it adds -lpthread as follows: % /home/masahiko/work/gcc/12.2.0/bin/gcc -v -o src/test/modules/test_json_parser/test_json_parser_incremental_shlib src/test/modules/test_json_parser/test_json_parser_incremental_shlib.p/test_json_parser_incremental.c.o -Wl,--as-needed -Wl,--no-undefined '-Wl,-rpath,$ORIGIN/../../../interfaces/libpq:/lib/../lib64' -Wl,-rpath-link,/lib/../lib64 -Wl,-rpath-link,/home/masahiko/pgsql/source/dev_master/build/src/interfaces/libpq -Wl,--start-group src/common/libpgcommon_excluded_shlib.a src/common/libpgcommon_shlib.a src/common/libpgcommon_shlib_config_info.a src/common/libpgcommon_shlib_ryu.a src/port/libpgport_shlib.a src/interfaces/libpq/libpq.so.5.18 -lm -ldl -pthread -lrt /usr/lib64/libz.so /lib/../lib64/libzstd.so /usr/lib64/liblz4.so /usr/lib64/libssl.so /usr/lib64/libcrypto.so -Wl,--end-group whereas with 'make' it doesn't: % gcc -v -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O0 test_json_parser_incremental.o -L../../../../src/port -L../../../../src/common -Wl,--as-needed -Wl,-rpath,'/home/masahiko/pgsql/master/lib',--enable-new-dtags -lpgcommon_excluded_shlib -L../../../../src/common -lpgcommon_shlib -L../../../../src/port -lpgport_shlib -L../../../../src/interfaces/libpq -lpq -o test_json_parser_incremental_shlib FYI the following change fixed the issue in my local env: --- a/src/test/modules/test_json_parser/Makefile +++ b/src/test/modules/test_json_parser/Makefile @@ -27,7 +27,7 @@ test_json_parser_incremental$(X): test_json_parser_incremental.o $(WIN32RES) $(CC) $(CFLAGS) $^ $(PG_LIBS_INTERNAL) $(LDFLAGS) $(LDFLAGS_EX) $(PG_LIBS) $(LIBS) -o $@ test_json_parser_incremental_shlib$(X): test_json_parser_incremental.o $(WIN32RES) - $(CC) $(CFLAGS) $^ $(LDFLAGS) -lpgcommon_excluded_shlib $(libpq_pgport_shlib) $(filter -lintl, $(LIBS)) -o $@ + $(CC) $(CFLAGS) $^ $(LDFLAGS) -lpgcommon_excluded_shlib $(libpq_pgport_shlib) $(filter -lintl, $(LIBS)) $(LIBS) -o $@ test_json_parser_perf$(X): test_json_parser_perf.o $(WIN32RES) $(CC) $(CFLAGS) $^ $(PG_LIBS_INTERNAL) $(LDFLAGS) $(LDFLAGS_EX) $(PG_LIBS) $(LIBS) -o $@ It seems that no MacOS and NetBSD animals failed due to this error because they have LC_C_LOCALE. But I'm not sure why some animals successfully built test_json_parser() even without -lpthread or -pthread[1][2]. Regards, [1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=prion&dt=2025-03-28%2015%3A33%3A03... [2] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=bushmaster&dt=2025-03-28%2015%3A32... -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: On non-Windows, hard depend on uselocale(3) @ 2025-12-12 18:07 Tom Lane <[email protected]> 1 sibling, 0 replies; 20+ messages in thread From: Tom Lane @ 2025-12-12 18:07 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Thomas Munro <[email protected]>; Tristan Partin <[email protected]>; pgsql-hackers Peter Eisentraut <[email protected]> writes: > On 28.08.24 20:50, Peter Eisentraut wrote: >> I suggest that the simplification of the xlocale.h configure tests could >> be committed separately. This would also be useful independent of this, >> and it's a sizeable chunk of this patch. > To keep this moving along a bit, I have extracted this part and > committed it separately. I had to make a few small tweaks, e.g., there > was no check for xlocale.h in configure.ac, and the old > xlocale.h-including stanza could be removed from chklocale.h. Let's see > how this goes. For the archives' sake --- I discovered today during a "git bisect" session that commits between 35eeea623 ("Use thread-safe nl_langinfo_l(), not nl_langinfo()") and 9c2a6c5a5 ("Simplify checking for xlocale.h", the commit Peter refers to here) fail to build on current macOS (26/Tahoe): chklocale.c:330:8: error: call to undeclared function 'nl_langinfo_l'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 330 | sys = nl_langinfo_l(CODESET, loc); | ^ chklocale.c:330:8: note: did you mean 'nl_langinfo'? /Library/Developer/CommandLineTools/SDKs/MacOSX26.1.sdk/usr/include/_langinfo.h:116:20: note: 'nl_langinfo' declared here 116 | char *_LIBC_CSTR nl_langinfo(nl_item); | ^ chklocale.c:330:6: error: incompatible integer to pointer conversion assigning to 'char *' from 'int' [-Wint-conversion] 330 | sys = nl_langinfo_l(CODESET, loc); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. This happens because nl_langinfo_l() is declared in xlocale.h, but chklocale.c elects not to #include that, evidently because it's no longer needed to obtain typedef locale_t. AFAICS there's nothing we can do about this retroactively; it'll be a more or less permanent landmine for bisecting on macOS. Fortunately it's not too difficult to work around, you can just do diff --git a/src/port/chklocale.c b/src/port/chklocale.c index 9506cd87ed8..0e35e0cf55f 100644 --- a/src/port/chklocale.c +++ b/src/port/chklocale.c @@ -23,9 +23,7 @@ #include <langinfo.h> #endif -#ifdef LOCALE_T_IN_XLOCALE #include <xlocale.h> -#endif #include "mb/pg_wchar.h" when trying to build one of the affected commits. Another option that might fit into a bisecting workflow more easily is to inject -DLOCALE_T_IN_XLOCALE into CPPFLAGS. regards, tom lane ^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v52 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting @ 2026-04-03 19:01 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Álvaro Herrera @ 2026-04-03 19:01 UTC (permalink / raw) --- src/backend/commands/repack_worker.c | 3 ++- src/backend/replication/logical/logical.c | 8 +++++--- src/backend/replication/logical/logicalfuncs.c | 2 +- src/backend/replication/slot.c | 18 ++++++++++++------ src/backend/replication/slotfuncs.c | 11 ++++++----- src/backend/replication/walsender.c | 5 +++-- src/include/replication/logical.h | 3 ++- src/include/replication/slot.h | 2 +- 8 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 610592a05b0..ca827223845 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -224,7 +224,7 @@ repack_setup_logical_decoding(Oid relid) * Make sure we can use logical decoding. */ CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(true); /* * A single backend should not execute multiple REPACK commands at a time, @@ -252,6 +252,7 @@ repack_setup_logical_decoding(Oid relid) ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME, NIL, true, + true, InvalidXLogRecPtr, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index f20a0fe70ad..a08aece5731 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -108,9 +108,9 @@ static void LoadOutputPlugin(OutputPluginCallbacks *callbacks, const char *plugi * decoding. */ void -CheckLogicalDecodingRequirements(void) +CheckLogicalDecodingRequirements(bool repack) { - CheckSlotRequirements(); + CheckSlotRequirements(repack); /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() @@ -305,6 +305,7 @@ StartupDecodingContext(List *output_plugin_options, * output_plugin_options -- contains options passed to the output plugin * need_full_snapshot -- if true, must obtain a snapshot able to read all * tables; if false, one that can read only catalogs is acceptable. + * for_repack -- if true, we're going to be decoding for REPACK. * restart_lsn -- if given as invalid, it's this routine's responsibility to * mark WAL as reserved by setting a convenient restart_lsn for the slot. * Otherwise, we set for decoding to start from the given LSN without @@ -325,6 +326,7 @@ LogicalDecodingContext * CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, @@ -341,7 +343,7 @@ CreateInitDecodingContext(const char *plugin, * On a standby, this check is also required while creating the slot. * Check the comments in the function. */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(for_repack); /* shorter lines... */ slot = MyReplicationSlot; diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c index 9760818941d..512013b0ef0 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -115,7 +115,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); if (PG_ARGISNULL(0)) ereport(ERROR, diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 2c6c6773ad2..13004ed547a 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1669,19 +1669,25 @@ CheckLogicalSlotExists(void) * slots. */ void -CheckSlotRequirements(void) +CheckSlotRequirements(bool repack) { + int limit; + /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() * needs the same check. */ - /* XXX we should be able to check exactly which type of slot we need */ - if (max_replication_slots + max_repack_replication_slots == 0) + if (repack) + limit = max_repack_replication_slots; + else + limit = max_replication_slots; + + if (limit == 0) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("replication slots can only be used if \"%s\" > 0 or \"%s\" > 0", - "max_replication_slots", "max_repack_replication_slots"))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("replication slots can only be used if \"%s\" > 0", + repack ? "max_repack_replication_slots" : "max_replication_slots")); if (wal_level < WAL_LEVEL_REPLICA) ereport(ERROR, diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 78dd3c4ea66..16fbd383735 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -90,7 +90,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); create_physical_replication_slot(NameStr(*name), immediately_reserve, @@ -164,6 +164,7 @@ create_logical_replication_slot(char *name, char *plugin, */ ctx = CreateInitDecodingContext(plugin, NIL, false, /* just catalogs is OK */ + false, /* not repack */ restart_lsn, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, @@ -203,7 +204,7 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); create_logical_replication_slot(NameStr(*name), NameStr(*plugin), @@ -240,7 +241,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); ReplicationSlotDrop(NameStr(*name), true); @@ -648,9 +649,9 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot) CheckSlotPermissions(); if (logical_slot) - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); else - CheckSlotRequirements(); + CheckSlotRequirements(false); LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 75ef3419a15..9d7d675fa96 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1240,7 +1240,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(cmd->kind == REPLICATION_KIND_LOGICAL); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); /* * Initially create persistent slot as ephemeral - that allows us to @@ -1309,6 +1309,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(IsLogicalDecodingEnabled()); ctx = CreateInitDecodingContext(cmd->plugin, NIL, need_full_snapshot, + false, InvalidXLogRecPtr, XL_ROUTINE(.page_read = logical_read_xlog_page, .segment_open = WalSndSegmentOpen, @@ -1466,7 +1467,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) QueryCompletion qc; /* make sure that our requirements are still fulfilled */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); Assert(!MyReplicationSlot); diff --git a/src/include/replication/logical.h b/src/include/replication/logical.h index bc9d4ece672..bc075b16741 100644 --- a/src/include/replication/logical.h +++ b/src/include/replication/logical.h @@ -115,11 +115,12 @@ typedef struct LogicalDecodingContext } LogicalDecodingContext; -extern void CheckLogicalDecodingRequirements(void); +extern void CheckLogicalDecodingRequirements(bool repack); extern LogicalDecodingContext *CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index c316a01a807..489af7d8d6c 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -378,7 +378,7 @@ extern void ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname extern void StartupReplicationSlots(void); extern void CheckPointReplicationSlots(bool is_shutdown); -extern void CheckSlotRequirements(void); +extern void CheckSlotRequirements(bool repack); extern void CheckSlotPermissions(void); extern ReplicationSlotInvalidationCause GetSlotInvalidationCause(const char *cause_name); -- 2.47.3 --gp2pyozrd5pweboh-- ^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v51 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting @ 2026-04-03 19:01 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Álvaro Herrera @ 2026-04-03 19:01 UTC (permalink / raw) --- src/backend/commands/repack_worker.c | 3 ++- src/backend/replication/logical/logical.c | 8 +++++--- src/backend/replication/logical/logicalfuncs.c | 2 +- src/backend/replication/slot.c | 18 ++++++++++++------ src/backend/replication/slotfuncs.c | 11 ++++++----- src/backend/replication/walsender.c | 5 +++-- src/include/replication/logical.h | 3 ++- src/include/replication/slot.h | 2 +- 8 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 610592a05b0..ca827223845 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -224,7 +224,7 @@ repack_setup_logical_decoding(Oid relid) * Make sure we can use logical decoding. */ CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(true); /* * A single backend should not execute multiple REPACK commands at a time, @@ -252,6 +252,7 @@ repack_setup_logical_decoding(Oid relid) ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME, NIL, true, + true, InvalidXLogRecPtr, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index f20a0fe70ad..a08aece5731 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -108,9 +108,9 @@ static void LoadOutputPlugin(OutputPluginCallbacks *callbacks, const char *plugi * decoding. */ void -CheckLogicalDecodingRequirements(void) +CheckLogicalDecodingRequirements(bool repack) { - CheckSlotRequirements(); + CheckSlotRequirements(repack); /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() @@ -305,6 +305,7 @@ StartupDecodingContext(List *output_plugin_options, * output_plugin_options -- contains options passed to the output plugin * need_full_snapshot -- if true, must obtain a snapshot able to read all * tables; if false, one that can read only catalogs is acceptable. + * for_repack -- if true, we're going to be decoding for REPACK. * restart_lsn -- if given as invalid, it's this routine's responsibility to * mark WAL as reserved by setting a convenient restart_lsn for the slot. * Otherwise, we set for decoding to start from the given LSN without @@ -325,6 +326,7 @@ LogicalDecodingContext * CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, @@ -341,7 +343,7 @@ CreateInitDecodingContext(const char *plugin, * On a standby, this check is also required while creating the slot. * Check the comments in the function. */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(for_repack); /* shorter lines... */ slot = MyReplicationSlot; diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c index 9760818941d..512013b0ef0 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -115,7 +115,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); if (PG_ARGISNULL(0)) ereport(ERROR, diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 2c6c6773ad2..13004ed547a 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1669,19 +1669,25 @@ CheckLogicalSlotExists(void) * slots. */ void -CheckSlotRequirements(void) +CheckSlotRequirements(bool repack) { + int limit; + /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() * needs the same check. */ - /* XXX we should be able to check exactly which type of slot we need */ - if (max_replication_slots + max_repack_replication_slots == 0) + if (repack) + limit = max_repack_replication_slots; + else + limit = max_replication_slots; + + if (limit == 0) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("replication slots can only be used if \"%s\" > 0 or \"%s\" > 0", - "max_replication_slots", "max_repack_replication_slots"))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("replication slots can only be used if \"%s\" > 0", + repack ? "max_repack_replication_slots" : "max_replication_slots")); if (wal_level < WAL_LEVEL_REPLICA) ereport(ERROR, diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 78dd3c4ea66..16fbd383735 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -90,7 +90,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); create_physical_replication_slot(NameStr(*name), immediately_reserve, @@ -164,6 +164,7 @@ create_logical_replication_slot(char *name, char *plugin, */ ctx = CreateInitDecodingContext(plugin, NIL, false, /* just catalogs is OK */ + false, /* not repack */ restart_lsn, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, @@ -203,7 +204,7 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); create_logical_replication_slot(NameStr(*name), NameStr(*plugin), @@ -240,7 +241,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); ReplicationSlotDrop(NameStr(*name), true); @@ -648,9 +649,9 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot) CheckSlotPermissions(); if (logical_slot) - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); else - CheckSlotRequirements(); + CheckSlotRequirements(false); LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 75ef3419a15..9d7d675fa96 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1240,7 +1240,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(cmd->kind == REPLICATION_KIND_LOGICAL); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); /* * Initially create persistent slot as ephemeral - that allows us to @@ -1309,6 +1309,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(IsLogicalDecodingEnabled()); ctx = CreateInitDecodingContext(cmd->plugin, NIL, need_full_snapshot, + false, InvalidXLogRecPtr, XL_ROUTINE(.page_read = logical_read_xlog_page, .segment_open = WalSndSegmentOpen, @@ -1466,7 +1467,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) QueryCompletion qc; /* make sure that our requirements are still fulfilled */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); Assert(!MyReplicationSlot); diff --git a/src/include/replication/logical.h b/src/include/replication/logical.h index bc9d4ece672..bc075b16741 100644 --- a/src/include/replication/logical.h +++ b/src/include/replication/logical.h @@ -115,11 +115,12 @@ typedef struct LogicalDecodingContext } LogicalDecodingContext; -extern void CheckLogicalDecodingRequirements(void); +extern void CheckLogicalDecodingRequirements(bool repack); extern LogicalDecodingContext *CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index c316a01a807..489af7d8d6c 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -378,7 +378,7 @@ extern void ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname extern void StartupReplicationSlots(void); extern void CheckPointReplicationSlots(bool is_shutdown); -extern void CheckSlotRequirements(void); +extern void CheckSlotRequirements(bool repack); extern void CheckSlotPermissions(void); extern ReplicationSlotInvalidationCause GetSlotInvalidationCause(const char *cause_name); -- 2.47.3 --r2slln3zpmilwu22-- ^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v52 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting @ 2026-04-03 19:01 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Álvaro Herrera @ 2026-04-03 19:01 UTC (permalink / raw) --- src/backend/commands/repack_worker.c | 3 ++- src/backend/replication/logical/logical.c | 8 +++++--- src/backend/replication/logical/logicalfuncs.c | 2 +- src/backend/replication/slot.c | 18 ++++++++++++------ src/backend/replication/slotfuncs.c | 11 ++++++----- src/backend/replication/walsender.c | 5 +++-- src/include/replication/logical.h | 3 ++- src/include/replication/slot.h | 2 +- 8 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 610592a05b0..ca827223845 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -224,7 +224,7 @@ repack_setup_logical_decoding(Oid relid) * Make sure we can use logical decoding. */ CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(true); /* * A single backend should not execute multiple REPACK commands at a time, @@ -252,6 +252,7 @@ repack_setup_logical_decoding(Oid relid) ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME, NIL, true, + true, InvalidXLogRecPtr, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index f20a0fe70ad..a08aece5731 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -108,9 +108,9 @@ static void LoadOutputPlugin(OutputPluginCallbacks *callbacks, const char *plugi * decoding. */ void -CheckLogicalDecodingRequirements(void) +CheckLogicalDecodingRequirements(bool repack) { - CheckSlotRequirements(); + CheckSlotRequirements(repack); /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() @@ -305,6 +305,7 @@ StartupDecodingContext(List *output_plugin_options, * output_plugin_options -- contains options passed to the output plugin * need_full_snapshot -- if true, must obtain a snapshot able to read all * tables; if false, one that can read only catalogs is acceptable. + * for_repack -- if true, we're going to be decoding for REPACK. * restart_lsn -- if given as invalid, it's this routine's responsibility to * mark WAL as reserved by setting a convenient restart_lsn for the slot. * Otherwise, we set for decoding to start from the given LSN without @@ -325,6 +326,7 @@ LogicalDecodingContext * CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, @@ -341,7 +343,7 @@ CreateInitDecodingContext(const char *plugin, * On a standby, this check is also required while creating the slot. * Check the comments in the function. */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(for_repack); /* shorter lines... */ slot = MyReplicationSlot; diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c index 9760818941d..512013b0ef0 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -115,7 +115,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); if (PG_ARGISNULL(0)) ereport(ERROR, diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 2c6c6773ad2..13004ed547a 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1669,19 +1669,25 @@ CheckLogicalSlotExists(void) * slots. */ void -CheckSlotRequirements(void) +CheckSlotRequirements(bool repack) { + int limit; + /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() * needs the same check. */ - /* XXX we should be able to check exactly which type of slot we need */ - if (max_replication_slots + max_repack_replication_slots == 0) + if (repack) + limit = max_repack_replication_slots; + else + limit = max_replication_slots; + + if (limit == 0) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("replication slots can only be used if \"%s\" > 0 or \"%s\" > 0", - "max_replication_slots", "max_repack_replication_slots"))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("replication slots can only be used if \"%s\" > 0", + repack ? "max_repack_replication_slots" : "max_replication_slots")); if (wal_level < WAL_LEVEL_REPLICA) ereport(ERROR, diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 78dd3c4ea66..16fbd383735 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -90,7 +90,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); create_physical_replication_slot(NameStr(*name), immediately_reserve, @@ -164,6 +164,7 @@ create_logical_replication_slot(char *name, char *plugin, */ ctx = CreateInitDecodingContext(plugin, NIL, false, /* just catalogs is OK */ + false, /* not repack */ restart_lsn, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, @@ -203,7 +204,7 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); create_logical_replication_slot(NameStr(*name), NameStr(*plugin), @@ -240,7 +241,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); ReplicationSlotDrop(NameStr(*name), true); @@ -648,9 +649,9 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot) CheckSlotPermissions(); if (logical_slot) - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); else - CheckSlotRequirements(); + CheckSlotRequirements(false); LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 75ef3419a15..9d7d675fa96 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1240,7 +1240,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(cmd->kind == REPLICATION_KIND_LOGICAL); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); /* * Initially create persistent slot as ephemeral - that allows us to @@ -1309,6 +1309,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(IsLogicalDecodingEnabled()); ctx = CreateInitDecodingContext(cmd->plugin, NIL, need_full_snapshot, + false, InvalidXLogRecPtr, XL_ROUTINE(.page_read = logical_read_xlog_page, .segment_open = WalSndSegmentOpen, @@ -1466,7 +1467,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) QueryCompletion qc; /* make sure that our requirements are still fulfilled */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); Assert(!MyReplicationSlot); diff --git a/src/include/replication/logical.h b/src/include/replication/logical.h index bc9d4ece672..bc075b16741 100644 --- a/src/include/replication/logical.h +++ b/src/include/replication/logical.h @@ -115,11 +115,12 @@ typedef struct LogicalDecodingContext } LogicalDecodingContext; -extern void CheckLogicalDecodingRequirements(void); +extern void CheckLogicalDecodingRequirements(bool repack); extern LogicalDecodingContext *CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index c316a01a807..489af7d8d6c 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -378,7 +378,7 @@ extern void ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname extern void StartupReplicationSlots(void); extern void CheckPointReplicationSlots(bool is_shutdown); -extern void CheckSlotRequirements(void); +extern void CheckSlotRequirements(bool repack); extern void CheckSlotPermissions(void); extern ReplicationSlotInvalidationCause GetSlotInvalidationCause(const char *cause_name); -- 2.47.3 --gp2pyozrd5pweboh-- ^ permalink raw reply [nested|flat] 20+ messages in thread
* [PATCH v51 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting @ 2026-04-03 19:01 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Álvaro Herrera @ 2026-04-03 19:01 UTC (permalink / raw) --- src/backend/commands/repack_worker.c | 3 ++- src/backend/replication/logical/logical.c | 8 +++++--- src/backend/replication/logical/logicalfuncs.c | 2 +- src/backend/replication/slot.c | 18 ++++++++++++------ src/backend/replication/slotfuncs.c | 11 ++++++----- src/backend/replication/walsender.c | 5 +++-- src/include/replication/logical.h | 3 ++- src/include/replication/slot.h | 2 +- 8 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c index 610592a05b0..ca827223845 100644 --- a/src/backend/commands/repack_worker.c +++ b/src/backend/commands/repack_worker.c @@ -224,7 +224,7 @@ repack_setup_logical_decoding(Oid relid) * Make sure we can use logical decoding. */ CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(true); /* * A single backend should not execute multiple REPACK commands at a time, @@ -252,6 +252,7 @@ repack_setup_logical_decoding(Oid relid) ctx = CreateInitDecodingContext(REPL_PLUGIN_NAME, NIL, true, + true, InvalidXLogRecPtr, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index f20a0fe70ad..a08aece5731 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -108,9 +108,9 @@ static void LoadOutputPlugin(OutputPluginCallbacks *callbacks, const char *plugi * decoding. */ void -CheckLogicalDecodingRequirements(void) +CheckLogicalDecodingRequirements(bool repack) { - CheckSlotRequirements(); + CheckSlotRequirements(repack); /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() @@ -305,6 +305,7 @@ StartupDecodingContext(List *output_plugin_options, * output_plugin_options -- contains options passed to the output plugin * need_full_snapshot -- if true, must obtain a snapshot able to read all * tables; if false, one that can read only catalogs is acceptable. + * for_repack -- if true, we're going to be decoding for REPACK. * restart_lsn -- if given as invalid, it's this routine's responsibility to * mark WAL as reserved by setting a convenient restart_lsn for the slot. * Otherwise, we set for decoding to start from the given LSN without @@ -325,6 +326,7 @@ LogicalDecodingContext * CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, @@ -341,7 +343,7 @@ CreateInitDecodingContext(const char *plugin, * On a standby, this check is also required while creating the slot. * Check the comments in the function. */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(for_repack); /* shorter lines... */ slot = MyReplicationSlot; diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c index 9760818941d..512013b0ef0 100644 --- a/src/backend/replication/logical/logicalfuncs.c +++ b/src/backend/replication/logical/logicalfuncs.c @@ -115,7 +115,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); if (PG_ARGISNULL(0)) ereport(ERROR, diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 2c6c6773ad2..13004ed547a 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -1669,19 +1669,25 @@ CheckLogicalSlotExists(void) * slots. */ void -CheckSlotRequirements(void) +CheckSlotRequirements(bool repack) { + int limit; + /* * NB: Adding a new requirement likely means that RestoreSlotFromDisk() * needs the same check. */ - /* XXX we should be able to check exactly which type of slot we need */ - if (max_replication_slots + max_repack_replication_slots == 0) + if (repack) + limit = max_repack_replication_slots; + else + limit = max_replication_slots; + + if (limit == 0) ereport(ERROR, - (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), - errmsg("replication slots can only be used if \"%s\" > 0 or \"%s\" > 0", - "max_replication_slots", "max_repack_replication_slots"))); + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("replication slots can only be used if \"%s\" > 0", + repack ? "max_repack_replication_slots" : "max_replication_slots")); if (wal_level < WAL_LEVEL_REPLICA) ereport(ERROR, diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c index 78dd3c4ea66..16fbd383735 100644 --- a/src/backend/replication/slotfuncs.c +++ b/src/backend/replication/slotfuncs.c @@ -90,7 +90,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); create_physical_replication_slot(NameStr(*name), immediately_reserve, @@ -164,6 +164,7 @@ create_logical_replication_slot(char *name, char *plugin, */ ctx = CreateInitDecodingContext(plugin, NIL, false, /* just catalogs is OK */ + false, /* not repack */ restart_lsn, XL_ROUTINE(.page_read = read_local_xlog_page, .segment_open = wal_segment_open, @@ -203,7 +204,7 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); create_logical_replication_slot(NameStr(*name), NameStr(*plugin), @@ -240,7 +241,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS) CheckSlotPermissions(); - CheckSlotRequirements(); + CheckSlotRequirements(false); ReplicationSlotDrop(NameStr(*name), true); @@ -648,9 +649,9 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot) CheckSlotPermissions(); if (logical_slot) - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); else - CheckSlotRequirements(); + CheckSlotRequirements(false); LWLockAcquire(ReplicationSlotControlLock, LW_SHARED); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index 75ef3419a15..9d7d675fa96 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1240,7 +1240,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(cmd->kind == REPLICATION_KIND_LOGICAL); - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); /* * Initially create persistent slot as ephemeral - that allows us to @@ -1309,6 +1309,7 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd) Assert(IsLogicalDecodingEnabled()); ctx = CreateInitDecodingContext(cmd->plugin, NIL, need_full_snapshot, + false, InvalidXLogRecPtr, XL_ROUTINE(.page_read = logical_read_xlog_page, .segment_open = WalSndSegmentOpen, @@ -1466,7 +1467,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) QueryCompletion qc; /* make sure that our requirements are still fulfilled */ - CheckLogicalDecodingRequirements(); + CheckLogicalDecodingRequirements(false); Assert(!MyReplicationSlot); diff --git a/src/include/replication/logical.h b/src/include/replication/logical.h index bc9d4ece672..bc075b16741 100644 --- a/src/include/replication/logical.h +++ b/src/include/replication/logical.h @@ -115,11 +115,12 @@ typedef struct LogicalDecodingContext } LogicalDecodingContext; -extern void CheckLogicalDecodingRequirements(void); +extern void CheckLogicalDecodingRequirements(bool repack); extern LogicalDecodingContext *CreateInitDecodingContext(const char *plugin, List *output_plugin_options, bool need_full_snapshot, + bool for_repack, XLogRecPtr restart_lsn, XLogReaderRoutine *xl_routine, LogicalOutputPluginWriterPrepareWrite prepare_write, diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h index c316a01a807..489af7d8d6c 100644 --- a/src/include/replication/slot.h +++ b/src/include/replication/slot.h @@ -378,7 +378,7 @@ extern void ReplicationSlotDropAtPubNode(WalReceiverConn *wrconn, char *slotname extern void StartupReplicationSlots(void); extern void CheckPointReplicationSlots(bool is_shutdown); -extern void CheckSlotRequirements(void); +extern void CheckSlotRequirements(bool repack); extern void CheckSlotPermissions(void); extern ReplicationSlotInvalidationCause GetSlotInvalidationCause(const char *cause_name); -- 2.47.3 --r2slln3zpmilwu22-- ^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2026-04-03 19:01 UTC | newest] Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-04-28 11:00 [PATCH v17 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]> 2024-11-20 09:00 ` Re: On non-Windows, hard depend on uselocale(3) Thomas Munro <[email protected]> 2024-11-20 23:30 ` Re: On non-Windows, hard depend on uselocale(3) Thomas Munro <[email protected]> 2024-11-21 07:38 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2024-11-22 21:32 ` Re: On non-Windows, hard depend on uselocale(3) Thomas Munro <[email protected]> 2024-11-25 00:42 ` Re: On non-Windows, hard depend on uselocale(3) Michael Paquier <[email protected]> 2024-11-25 00:57 ` Re: On non-Windows, hard depend on uselocale(3) Thomas Munro <[email protected]> 2024-11-26 16:23 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2024-11-26 20:24 ` Re: On non-Windows, hard depend on uselocale(3) Thomas Munro <[email protected]> 2025-02-09 07:32 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2025-03-28 15:30 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2025-03-28 16:14 ` Re: On non-Windows, hard depend on uselocale(3) Masahiko Sawada <[email protected]> 2025-03-28 16:32 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2025-03-28 20:34 ` Re: On non-Windows, hard depend on uselocale(3) Peter Eisentraut <[email protected]> 2025-03-29 00:01 ` Re: On non-Windows, hard depend on uselocale(3) Masahiko Sawada <[email protected]> 2025-12-12 18:07 ` Re: On non-Windows, hard depend on uselocale(3) Tom Lane <[email protected]> 2026-04-03 19:01 [PATCH v51 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting Álvaro Herrera <[email protected]> 2026-04-03 19:01 [PATCH v51 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting Álvaro Herrera <[email protected]> 2026-04-03 19:01 [PATCH v52 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting Álvaro Herrera <[email protected]> 2026-04-03 19:01 [PATCH v52 10/10] CheckLogicalDecodingRequirements: be specific about which GUC is limiting Álvaro Herrera <[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