Обсуждение: On non-Windows, hard depend on uselocale(3)

I have been working on adding using thread-safe locale APIs within
Postgres where appropriate[0]. The patch that I originally submitted
crashed during initdb (whoops!), so I worked on fixing the crash, which
led me to having to touch some code in chklocale.c, which became
a frustrating experience because chklocale.c is compiled in 3 different
configurations.

> pgport_variants = {
>   '_srv': internal_lib_args + {
>     'dependencies': [backend_port_code],
>   },
>   '': default_lib_args + {
>     'dependencies': [frontend_port_code],
>   },
>   '_shlib': default_lib_args + {
>     'pic': true,
>     'dependencies': [frontend_port_code],
>   },
> }

This means that some APIs I added or changed in pg_locale.c, can't be
used without conditional compilation depending on what variant is being
compiled. Additionally, I also have conditional compilation based on
HAVE_USELOCALE and WIN32.

I would like to propose removing HAVE_USELOCALE, and just have WIN32,
which means that Postgres would require uselocale(3) on anything that
isn't WIN32.

[0]: https://www.postgresql.org/message-id/CWMW5OZBWJ10.1YFLQWSUE5RE9@neon.tech

--
Tristan Partin
Neon (https://neon.tech)

"Tristan Partin" <tristan@neon.tech> writes:
> I would like to propose removing HAVE_USELOCALE, and just have WIN32, 
> which means that Postgres would require uselocale(3) on anything that 
> isn't WIN32.

You would need to do some research and try to prove that that won't
be a problem on any modern platform.  Presumably it once was a problem,
or we'd not have bothered with a configure check.

(Some git archaeology might yield useful info about when and why
we added the check.)

            regards, tom lane

On Thu, Nov 16, 2023 at 6:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "Tristan Partin" <tristan@neon.tech> writes:
> > I would like to propose removing HAVE_USELOCALE, and just have WIN32,
> > which means that Postgres would require uselocale(3) on anything that
> > isn't WIN32.
>
> You would need to do some research and try to prove that that won't
> be a problem on any modern platform.  Presumably it once was a problem,
> or we'd not have bothered with a configure check.
>
> (Some git archaeology might yield useful info about when and why
> we added the check.)

According to data I scraped from the build farm, the last two systems
we had that didn't have uselocale() were curculio (OpenBSD 5.9) and
wrasse (Solaris 11.3), but those were both shut down (though wrasse
still runs old branches) as they were well out of support.  OpenBSD
gained uselocale() in 6.2, and Solaris in 11.4, as part of the same
suite of POSIX changes that we already required in commit 8d9a9f03.

+1 for the change.

https://man.openbsd.org/uselocale.3
https://docs.oracle.com/cd/E88353_01/html/E37843/uselocale-3c.html

Tom Lane <tgl@sss.pgh.pa.us> writes:

> "Tristan Partin" <tristan@neon.tech> writes:
>> I would like to propose removing HAVE_USELOCALE, and just have WIN32, 
>> which means that Postgres would require uselocale(3) on anything that 
>> isn't WIN32.
>
> You would need to do some research and try to prove that that won't
> be a problem on any modern platform.  Presumably it once was a problem,
> or we'd not have bothered with a configure check.
>
> (Some git archaeology might yield useful info about when and why
> we added the check.)

For reference, the Perl effort to use the POSIX.1-2008 thread-safe
locale APIs have revealed several platform-specific bugs that cause it
to disable them on FreeBSD and macOS:

https://github.com/perl/perl5/commit/9cbc12c368981c56d4d8e40cc9417ac26bec2c35
https://github.com/perl/perl5/commit/dd4eb78c55aab441aec1639b1dd49f88bd960831

and work around bugs on others (e.g. OpenBSD):

https://github.com/perl/perl5/commit/0f3830f3997cf7ef1531bad26d2e0f13220dd862

But Perl actually makes use of per-thread locales, because it has a
separate interpereer per thread, each of which can have a different
locale active.  Since Postgres isn't actually multi-threaded (yet),
these concerns might not apply to the same degree.

>             regards, tom lane

- ilmari

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> "Tristan Partin" <tristan@neon.tech> writes:
>>> I would like to propose removing HAVE_USELOCALE, and just have WIN32,
>>> which means that Postgres would require uselocale(3) on anything that
>>> isn't WIN32.

>> You would need to do some research and try to prove that that won't
>> be a problem on any modern platform.  Presumably it once was a problem,
>> or we'd not have bothered with a configure check.

> For reference, the Perl effort to use the POSIX.1-2008 thread-safe
> locale APIs have revealed several platform-specific bugs that cause it
> to disable them on FreeBSD and macOS:
> https://github.com/perl/perl5/commit/9cbc12c368981c56d4d8e40cc9417ac26bec2c35
> https://github.com/perl/perl5/commit/dd4eb78c55aab441aec1639b1dd49f88bd960831
> and work around bugs on others (e.g. OpenBSD):
> https://github.com/perl/perl5/commit/0f3830f3997cf7ef1531bad26d2e0f13220dd862
> But Perl actually makes use of per-thread locales, because it has a
> separate interpereer per thread, each of which can have a different
> locale active.  Since Postgres isn't actually multi-threaded (yet),
> these concerns might not apply to the same degree.

Interesting.  That need not stop us from dropping the configure
check for uselocale(), but it might be a problem for Tristan's
larger ambitions.

            regards, tom lane

On Thu, Nov 16, 2023 at 7:42 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
> > "Tristan Partin" <tristan@neon.tech> writes:
> >> I would like to propose removing HAVE_USELOCALE, and just have WIN32,
> >> which means that Postgres would require uselocale(3) on anything that
> >> isn't WIN32.
> >
> > You would need to do some research and try to prove that that won't
> > be a problem on any modern platform.  Presumably it once was a problem,
> > or we'd not have bothered with a configure check.
> >
> > (Some git archaeology might yield useful info about when and why
> > we added the check.)
>
> For reference, the Perl effort to use the POSIX.1-2008 thread-safe
> locale APIs have revealed several platform-specific bugs that cause it
> to disable them on FreeBSD and macOS:
>
> https://github.com/perl/perl5/commit/9cbc12c368981c56d4d8e40cc9417ac26bec2c35

Interesting that C vs C.UTF-8 has come up there, something that has
also confused us and others (in fact I still owe Daniel Vérité a
response to his complaint about how we treat the latter; I got stuck
on a logical problem with the proposal and then dumped core...).  The
idea of C.UTF-8 is relatively new, and seems to have shaken a few bugs
out in a few places.  Anyway, that in particular is a brand new
FreeBSD bug report and I am sure it will be addressed soon.

> https://github.com/perl/perl5/commit/dd4eb78c55aab441aec1639b1dd49f88bd960831

As for macOS, one thing I noticed is that the FreeBSD -> macOS
pipeline appears to have re-awoken after many years of slumber.  I
don't know anything about that other than that when I recently
upgraded my Mac to 14.1, suddenly a few userspace tools are now
running the recentish FreeBSD versions of certain userland tools (tar,
grep, ...?), instead of something from the Jurassic.  Whether that
might apply to libc, who can say... they seemed to have quite ancient
BSD locale code last time I checked.

> https://github.com/perl/perl5/commit/0f3830f3997cf7ef1531bad26d2e0f13220dd862

That linked issue appears to be fixed already.

> But Perl actually makes use of per-thread locales, because it has a
> separate interpereer per thread, each of which can have a different
> locale active.  Since Postgres isn't actually multi-threaded (yet),
> these concerns might not apply to the same degree.

ECPG might use them in multi-threaded code.  I'm not sure if it's a
problem and whose problem it is.

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Nov 16, 2023 at 6:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> You would need to do some research and try to prove that that won't
>> be a problem on any modern platform.  Presumably it once was a problem,
>> or we'd not have bothered with a configure check.

> According to data I scraped from the build farm, the last two systems
> we had that didn't have uselocale() were curculio (OpenBSD 5.9) and
> wrasse (Solaris 11.3), but those were both shut down (though wrasse
> still runs old branches) as they were well out of support.

AFAICS, NetBSD still doesn't have it.  They have no on-line man page
for it, and my animal mamba shows it as not found.

            regards, tom lane

On Thu, Nov 16, 2023 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > On Thu, Nov 16, 2023 at 6:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> You would need to do some research and try to prove that that won't
> >> be a problem on any modern platform.  Presumably it once was a problem,
> >> or we'd not have bothered with a configure check.
>
> > According to data I scraped from the build farm, the last two systems
> > we had that didn't have uselocale() were curculio (OpenBSD 5.9) and
> > wrasse (Solaris 11.3), but those were both shut down (though wrasse
> > still runs old branches) as they were well out of support.
>
> AFAICS, NetBSD still doesn't have it.  They have no on-line man page
> for it, and my animal mamba shows it as not found.

Oh :-(  I see that but had missed that sidewinder was NetBSD and my
scraped data predates mamba.  Sorry for the wrong info.

Currently pg_locale.c requires systems to have *either* uselocale() or
mbstowcs_l()/wcstombs_l(), but NetBSD satisfies the second
requirement.  The other uses of uselocale() are in ECPG code that must
be falling back to the setlocale() path.  In other words, isn't it the
case that we don't require uselocale() to compile ECPG stuff, but it'll
probably crash or corrupt itself or give wrong answers if you push it
on NetBSD, so... uhh, really we do require it?

Thomas Munro <thomas.munro@gmail.com> writes:
> Currently pg_locale.c requires systems to have *either* uselocale() or
> mbstowcs_l()/wcstombs_l(), but NetBSD satisfies the second
> requirement.

Check.

> The other uses of uselocale() are in ECPG code that must
> be falling back to the setlocale() path.  In other words, isn't it the
> case that we don't require uselocale() to compile ECPG stuff, but it'll
> probably crash or corrupt itself or give wrong answers if you push it
> on NetBSD, so... uhh, really we do require it?

Dunno.  mamba is getting through the ecpg regression tests okay,
but we all know that doesn't prove a lot.  (AFAICS, ecpg only
cares about this to the extent of not wanting an LC_NUMERIC
locale where the decimal point isn't '.'.  I'm not sure that
NetBSD supports any such locale anyway --- I think they're like
OpenBSD in having only pro-forma locale support.)

            regards, tom lane

On Thu, Nov 16, 2023 at 10:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > The other uses of uselocale() are in ECPG code that must
> > be falling back to the setlocale() path.  In other words, isn't it the
> > case that we don't require uselocale() to compile ECPG stuff, but it'll
> > probably crash or corrupt itself or give wrong answers if you push it
> > on NetBSD, so... uhh, really we do require it?
>
> Dunno.  mamba is getting through the ecpg regression tests okay,
> but we all know that doesn't prove a lot.  (AFAICS, ecpg only
> cares about this to the extent of not wanting an LC_NUMERIC
> locale where the decimal point isn't '.'.  I'm not sure that
> NetBSD supports any such locale anyway --- I think they're like
> OpenBSD in having only pro-forma locale support.)

Idea #1

For output, which happens with sprintf(ptr, "%.15g%s", ...) in
execute.c, perhaps we could use our in-tree Ryu routine instead?

For input, which happens with strtod() in data.c, rats, we don't have
a parser and I understand that it is not for the faint of heart (naive
implementation gets subtle things wrong, cf "How to read floating
point numbers accurately" by W D Clinger + whatever improvements have
happened in this space since 1990).

Idea #2

Perhaps we could use snprintf_l() and strtod_l() where available.
They're not standard, but they are obvious extensions that NetBSD and
Windows have, and those are the two systems for which we are doing
grotty things in that code.  That would amount to extending
pg_locale.c's philosophy: either you must have uselocale(), or the
full set of _l() functions (that POSIX failed to standardise, dunno
what the history is behind that, seems weird).

Thomas Munro <thomas.munro@gmail.com> writes:
> Idea #1

> For output, which happens with sprintf(ptr, "%.15g%s", ...) in
> execute.c, perhaps we could use our in-tree Ryu routine instead?

> For input, which happens with strtod() in data.c, rats, we don't have
> a parser and I understand that it is not for the faint of heart

Yeah.  Getting rid of ecpg's use of uselocale() would certainly be
nice, but I'm not ready to add our own implementation of strtod()
to get there.

> Idea #2

> Perhaps we could use snprintf_l() and strtod_l() where available.
> They're not standard, but they are obvious extensions that NetBSD and
> Windows have, and those are the two systems for which we are doing
> grotty things in that code.

Oooh, shiny.  I do not see any man page for strtod_l, but I do see
that it's declared on mamba's host.  I wonder how long they've had it?
The man page for snprintf_l appears to be quite ancient, so we could
hope that strtod_l is available on all versions anyone cares about.

> That would amount to extending
> pg_locale.c's philosophy: either you must have uselocale(), or the
> full set of _l() functions (that POSIX failed to standardise, dunno
> what the history is behind that, seems weird).

Yeah.  I'd say the _l functions should be preferred over uselocale()
if available, but sadly they're not there on common systems.  (It
looks like glibc has strtod_l but not snprintf_l, which is odd.)

            regards, tom lane

On Thu, Nov 16, 2023 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Perhaps we could use snprintf_l() and strtod_l() where available.
> > They're not standard, but they are obvious extensions that NetBSD and
> > Windows have, and those are the two systems for which we are doing
> > grotty things in that code.
>
> Oooh, shiny.  I do not see any man page for strtod_l, but I do see
> that it's declared on mamba's host.  I wonder how long they've had it?
> The man page for snprintf_l appears to be quite ancient, so we could
> hope that strtod_l is available on all versions anyone cares about.

A decade[1].  And while I'm doing archeology, I noticed that POSIX has
agreed[2] in principle that *all* functions affected by the thread's
current locale should have a _l() variant, it's just that no one has
sent in the patch.

> > That would amount to extending
> > pg_locale.c's philosophy: either you must have uselocale(), or the
> > full set of _l() functions (that POSIX failed to standardise, dunno
> > what the history is behind that, seems weird).
>
> Yeah.  I'd say the _l functions should be preferred over uselocale()
> if available, but sadly they're not there on common systems.  (It
> looks like glibc has strtod_l but not snprintf_l, which is odd.)

Here is a first attempt.  In this version, new functions are exported
by pgtypeslib.  I realised that I had to do it in there because ECPG's
uselocale() jiggery-pokery is clearly intended to affect the
conversions happening in there too, and we probably don't want
circular dependencies between pgtypeslib and ecpglib.  I think this
means that pgtypeslib is actually subtly b0rked if you use it
independently without an ECPG connection (is that a thing people do?),
because all that code copied-and-pasted from the backend when run in
frontend code with eg a French locale will produce eg "0,42"; this
patch doesn't change that.

I also had a go[3] at doing it with static inlined functions, to avoid
creating a load of new exported functions and associated function call
overheads.  It worked fine, except on Windows: I needed a global
variable PGTYPESclocale that all the inlined functions can see when
called from ecpglib or pgtypeslib code, but if I put that in the
exports list then on that platform it seems to contain garbage; there
is probably some other magic needed to export non-function symbols
from the DLL or something like that, I didn't look into it.  See CI
failure + crash dumps.

[1] https://github.com/NetBSD/src/commit/c99aac45e540bc210cc660619a6b5323cbb5c17f
[2] https://www.austingroupbugs.net/view.php?id=1004
[3] https://github.com/macdice/postgres/tree/strtod_l_inline

Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Nov 16, 2023 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Thomas Munro <thomas.munro@gmail.com> writes:
>>> Perhaps we could use snprintf_l() and strtod_l() where available.
>>> They're not standard, but they are obvious extensions that NetBSD and
>>> Windows have, and those are the two systems for which we are doing
>>> grotty things in that code.

>> Yeah.  I'd say the _l functions should be preferred over uselocale()
>> if available, but sadly they're not there on common systems.  (It
>> looks like glibc has strtod_l but not snprintf_l, which is odd.)

> Here is a first attempt.

I've not reviewed this closely, but I did try it on mamba's host.
It compiles and passes regression testing, but I see two warnings:

common.c: In function 'PGTYPESsprintf':
common.c:120:2: warning: function 'PGTYPESsprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
  120 |  return vsprintf_l(str, PGTYPESclocale, format, args);
      |  ^~~~~~
common.c: In function 'PGTYPESsnprintf':
common.c:136:2: warning: function 'PGTYPESsnprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
  136 |  return vsnprintf_l(str, size, PGTYPESclocale, format, args);
      |  ^~~~~~

That happens because on NetBSD, we define PG_PRINTF_ATTRIBUTE as
"__syslog__" so that the compiler will not warn about use of %m
(apparently, they support %m in syslog() but not printf(), sigh).

I think this is telling us about an actual problem: these new
functions are based on libc's printf not what we have in snprintf.c,
and therefore we really shouldn't be assuming that they will support
any format specs beyond what POSIX requires for printf.  If somebody
tried to use %m in one of these calls, we'd like to get warnings about
that.

I experimented with the attached delta patch and it does silence
these warnings.  I suspect that ecpg_log() should be marked as
pg_attribute_std_printf() too, because it has the same issue,
but I didn't try that.  (Probably, we see no warning for that
because the compiler isn't quite bright enough to connect the
format argument with the string that gets passed to vfprintf().)

            regards, tom lane

diff --git a/src/include/c.h b/src/include/c.h
index 82f8e9d4c7..98e3bbf386 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -171,13 +171,19 @@
 #define PG_USED_FOR_ASSERTS_ONLY pg_attribute_unused()
 #endif

-/* GCC and XLC support format attributes */
+/*
+ * GCC and XLC support format attributes.  Use pg_attribute_printf()
+ * for our src/port/snprintf.c implementation and functions based on it.
+ * Use pg_attribute_std_printf() for functions based on libc's printf.
+ */
 #if defined(__GNUC__) || defined(__IBMC__)
 #define pg_attribute_format_arg(a) __attribute__((format_arg(a)))
 #define pg_attribute_printf(f,a) __attribute__((format(PG_PRINTF_ATTRIBUTE, f, a)))
+#define pg_attribute_std_printf(f,a) __attribute__((format(printf, f, a)))
 #else
 #define pg_attribute_format_arg(a)
 #define pg_attribute_printf(f,a)
+#define pg_attribute_std_printf(f,a)
 #endif

 /* GCC, Sunpro and XLC support aligned, packed and noreturn */
diff --git a/src/interfaces/ecpg/include/pgtypes_format.h b/src/interfaces/ecpg/include/pgtypes_format.h
index d6dd06d361..87160fab59 100644
--- a/src/interfaces/ecpg/include/pgtypes_format.h
+++ b/src/interfaces/ecpg/include/pgtypes_format.h
@@ -20,7 +20,7 @@ extern int PGTYPESbegin_clocale(locale_t *old_locale);
 extern void PGTYPESend_clocale(locale_t old_locale);

 extern double PGTYPESstrtod(const char *str, char **endptr);
-extern int PGTYPESsprintf(char *str, const char *format, ...) pg_attribute_printf(2, 3);
-extern int PGTYPESsnprintf(char *str, size_t size, const char *format, ...) pg_attribute_printf(3, 4);
+extern int PGTYPESsprintf(char *str, const char *format, ...) pg_attribute_std_printf(2, 3);
+extern int PGTYPESsnprintf(char *str, size_t size, const char *format, ...) pg_attribute_std_printf(3, 4);

 #endif

I wrote:
> I've not reviewed this closely, but I did try it on mamba's host.
> It compiles and passes regression testing, but I see two warnings:

> common.c: In function 'PGTYPESsprintf':
> common.c:120:2: warning: function 'PGTYPESsprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
>   120 |  return vsprintf_l(str, PGTYPESclocale, format, args);
>       |  ^~~~~~
> common.c: In function 'PGTYPESsnprintf':
> common.c:136:2: warning: function 'PGTYPESsnprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
>   136 |  return vsnprintf_l(str, size, PGTYPESclocale, format, args);
>       |  ^~~~~~

> I think this is telling us about an actual problem: these new
> functions are based on libc's printf not what we have in snprintf.c,
> and therefore we really shouldn't be assuming that they will support
> any format specs beyond what POSIX requires for printf.

Wait, I just realized that there's more to this.  ecpglib *does*
rely on our snprintf.c functions:

$ nm --ext --undef src/interfaces/ecpg/ecpglib/*.o | grep printf
                 U pg_snprintf
                 U pg_fprintf
                 U pg_snprintf
                 U pg_printf
                 U pg_snprintf
                 U pg_sprintf
                 U pg_fprintf
                 U pg_snprintf
                 U pg_vfprintf
                 U pg_snprintf
                 U pg_sprintf
                 U pg_sprintf

We are getting these warnings because vsprintf_l and
vsnprintf_l don't have snprintf.c implementations, so the
compiler sees the attributes attached to them by stdio.h.

This raises the question of whether changing snprintf.c
could be part of the solution.  I'm not sure that we want
to try to emulate vs[n]printf_l directly, but perhaps there's
another way?

In any case, my concern about ecpg_log() is misplaced.
That is really using pg_vfprintf, so it's correctly marked.

            regards, tom lane

Hi,

On 2023-11-17 08:57:47 +1300, Thomas Munro wrote:
> I also had a go[3] at doing it with static inlined functions, to avoid
> creating a load of new exported functions and associated function call
> overheads.  It worked fine, except on Windows: I needed a global
> variable PGTYPESclocale that all the inlined functions can see when
> called from ecpglib or pgtypeslib code, but if I put that in the
> exports list then on that platform it seems to contain garbage; there
> is probably some other magic needed to export non-function symbols
> from the DLL or something like that, I didn't look into it.  See CI
> failure + crash dumps.

I suspect you'd need __declspec(dllimport) on the variable to make that work.
I.e. use PGDLLIMPORT and define BUILDING_DLL while building the libraries, so
they see __declspec (dllexport).  I luckily forgot the details, but functions
just call into some thunk that does necessary magic, but that option doesn't
exist for variables, so the compiler/linker have to do stuff, hence needing
__declspec(dllimport).

Greetings,

Andres Freund

On Sat, Nov 18, 2023 at 11:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > I've not reviewed this closely, but I did try it on mamba's host.
> > It compiles and passes regression testing, but I see two warnings:
>
> > common.c: In function 'PGTYPESsprintf':
> > common.c:120:2: warning: function 'PGTYPESsprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
> >   120 |  return vsprintf_l(str, PGTYPESclocale, format, args);
> >       |  ^~~~~~
> > common.c: In function 'PGTYPESsnprintf':
> > common.c:136:2: warning: function 'PGTYPESsnprintf' might be a candidate for 'gnu_printf' format attribute
[-Wsuggest-attribute=format]
> >   136 |  return vsnprintf_l(str, size, PGTYPESclocale, format, args);
> >       |  ^~~~~~
>
> > I think this is telling us about an actual problem: these new
> > functions are based on libc's printf not what we have in snprintf.c,
> > and therefore we really shouldn't be assuming that they will support
> > any format specs beyond what POSIX requires for printf.

Right, thanks.

> We are getting these warnings because vsprintf_l and
> vsnprintf_l don't have snprintf.c implementations, so the
> compiler sees the attributes attached to them by stdio.h.
>
> This raises the question of whether changing snprintf.c
> could be part of the solution.  I'm not sure that we want
> to try to emulate vs[n]printf_l directly, but perhaps there's
> another way?

Yeah, I have been wondering about that too.

The stuff I posted so far was just about how to remove some gross and
incorrect code from ecpg, a somewhat niche frontend part of
PostgreSQL.  I guess Tristan is thinking bigger: removing obstacles to
going multi-threaded in the backend.  Clearly locales are one of the
places where global state will bite us, so we either need to replace
setlocale() with uselocale() for the database default locale, or use
explicit locale arguments with _l() functions everywhere and pass in
the right locale.  Due to incompleteness of (a) libc implementations
and (b) the standard, we can't directly do either, so we'll need to
cope with that.

Thought experiment:  If we supplied our own fallback _l() replacement
functions where missing, and those did uselocale() save/restore, many
systems wouldn't need them, for example glibc has strtod_l() as you
noted, and several other systems have systematically added them for
all sorts of stuff.  The case of the *printf* family is quite
interesting, because there we already have our own implement for other
reasons, so it might make sense to add the _l() variants to our
snprintf.c implementations.  On glibc, snprintf.c would have to do a
uselocale() save/restore where it punts %g to the system snprintf, but
if that offends some instruction cycle bean counter, perhaps we could
replace that bit with Ryu anyway (or is it not general enough to
handle all the stuff %g et al can do?  I haven't looked).

I am not sure how you would ever figure out what other stuff is
affected by the global locale in general, for example code hiding in
extensions etc, but, I mean, that's what's wrong with global state in
a nutshell and it has often been speculated that multi-threaded
PostgreSQL might have a way to say 'I still want one process per
session because my extensions don't all identify themselves as
thread-safe yet'.

BTW is this comment in snprintf.c true?

 * 1. No locale support: the radix character is always '.' and the '
 * (single quote) format flag is ignored.

It is in the backend but only because we nail down LC_NUMERIC early
on, not because of any property of snprintf.c, no?

Thomas Munro <thomas.munro@gmail.com> writes:
> BTW is this comment in snprintf.c true?

>  * 1. No locale support: the radix character is always '.' and the '
>  * (single quote) format flag is ignored.

> It is in the backend but only because we nail down LC_NUMERIC early
> on, not because of any property of snprintf.c, no?

Hmm, the second part of it is true.  But given that we punt float
formatting to libc, I think you are right that the first part
depends on LC_NUMERIC being frozen.

            regards, tom lane

On Mon, Nov 20, 2023 at 11:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > BTW is this comment in snprintf.c true?
>
> >  * 1. No locale support: the radix character is always '.' and the '
> >  * (single quote) format flag is ignored.
>
> > It is in the backend but only because we nail down LC_NUMERIC early
> > on, not because of any property of snprintf.c, no?
>
> Hmm, the second part of it is true.  But given that we punt float
> formatting to libc, I think you are right that the first part
> depends on LC_NUMERIC being frozen.

If we are sure that we'll *never* want locale-aware printf-family
functions (ie we *always* want "C" locale), then in the thought
experiment above where I suggested we supply replacement _l()
functions, we could just skip that for the printf family, but make
that above comment actually true.  Perhaps with Ryu, but otherwise by
punting to libc _l() or uselocale() save/restore.

Thomas Munro <thomas.munro@gmail.com> writes:
> If we are sure that we'll *never* want locale-aware printf-family
> functions (ie we *always* want "C" locale), then in the thought
> experiment above where I suggested we supply replacement _l()
> functions, we could just skip that for the printf family, but make
> that above comment actually true.  Perhaps with Ryu, but otherwise by
> punting to libc _l() or uselocale() save/restore.

It is pretty annoying that we've got that shiny Ryu code and can't
use it here.  From memory, we did look into that and concluded that
Ryu wasn't amenable to providing "exactly this many digits" as is
required by most variants of printf's conversion specs.  But maybe
somebody should go try harder.  (Worst case, you could do rounding
off by hand on the produced digit string, but that's ugly...)

            regards, tom lane