Thread: Thread-safe nl_langinfo() and localeconv()
Hi, Over on the discussion thread about remaining setlocale() work[1], I wrote out some long boring theories about $SUBJECT. Here are some draft patches to try those theories out, and make a commitfest entry. nl_langinfo_l() is a trivial drop-in replacement, and pg_localeconv_r() has 4 different implementation strategies: 1. Windows, with ugly _configthreadlocale() and thread-local result. 2. Glibc, with nice nl_langinfo_l() extensions. 3. macOS/*BSD, with nice localeconv_l(). 4. Baseline POSIX: uselocale() + localeconv() + honking great lock. In reality it'd just be Solaris running #4 (and AIX if it comes back). Whether they truly implement it as pessimally as the standard allows, who knows... you could drop the lock if you somehow knew that they returned a pointer to thread-local storage or a member of the locale_t object. [1] https://www.postgresql.org/message-id/flat/4c5da86af36a0d5e430eee3f60ce5e06f1b5cd34.camel%40j-davis.com
Attachment
On 13/08/2024 08:45, Thomas Munro wrote:
> Hi,
>
> Over on the discussion thread about remaining setlocale() work[1], I
> wrote out some long boring theories about $SUBJECT. Here are some
> draft patches to try those theories out, and make a commitfest entry.
> nl_langinfo_l() is a trivial drop-in replacement, and
> pg_localeconv_r() has 4 different implementation strategies:
>
> 1. Windows, with ugly _configthreadlocale() and thread-local result.
> 2. Glibc, with nice nl_langinfo_l() extensions.
> 3. macOS/*BSD, with nice localeconv_l().
> 4. Baseline POSIX: uselocale() + localeconv() + honking great lock.
>
> In reality it'd just be Solaris running #4 (and AIX if it comes back).
> Whether they truly implement it as pessimally as the standard allows,
> who knows... you could drop the lock if you somehow knew that they
> returned a pointer to thread-local storage or a member of the locale_t
> object.
Patches 1 and 2 look good to me.
Patch 3 makes sense too, some comments on the details:
The #ifdefs and the LCONV_MEMBER stuff makes it a bit hard to follow
what happens in each implementation strategy. I wonder if it would be
more clear to duplicate more code.
There's a comment at the top of pg_locale.c ("!!! NOW HEAR THIS !!!")
that needs to be removed or adjusted now.
> * The POSIX standard explicitly says that it is undefined what happens if
> * LC_MONETARY or LC_NUMERIC imply an encoding (codeset) different from
> * that implied by LC_CTYPE. In practice, all Unix-ish platforms seem to
> * believe that localeconv() should return strings that are encoded in the
> * codeset implied by the LC_MONETARY or LC_NUMERIC locale name. Hence,
> * once we have successfully collected the localeconv() results, we will
> * convert them from that codeset to the desired server encoding.
The patch loses this comment, leaving just a much shorter comment in the
WIN32 implementation. But it still seems like a relevant comment for the
!WIN32 implementation too.
> This gets rid of some setlocale() calls and makes the returned value
> unclobberable with a defined lifetime. The remaining call to
> setlocale() is only a query of the name of the current local (in a
typo: local -> locale
> multi-threaded future this would have to be changed, perhaps to use a
> per-database or per-backend locale_t instead of LC_GLOBAL_LOCALE).
>
> All known non-Windows targets have nl_langinfo_l(), from POSIX 2018.
I think that's supposed to be POSIX 2008
--
Heikki Linnakangas
Neon (https://neon.tech)
On Tue, Aug 13, 2024 at 6:23 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 13/08/2024 08:45, Thomas Munro wrote: > Patches 1 and 2 look good to me. Thanks. I went ahead and pushed these ones. A couple of Macs in the build farm are failing, as if they didn't include <xlocale.h> and haven't seen the type locale_t. CI's macOS build is OK, and my own local Mac is building master OK, and animal "indri" is OK... hmm, those are all using MacPorts, but I don't yet see why that would be it...
On Tue, Aug 13, 2024 at 11:25 PM Thomas Munro <thomas.munro@gmail.com> wrote: > On Tue, Aug 13, 2024 at 6:23 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > On 13/08/2024 08:45, Thomas Munro wrote: > > Patches 1 and 2 look good to me. > > Thanks. I went ahead and pushed these ones. A couple of Macs in the > build farm are failing, as if they didn't include <xlocale.h> and > haven't seen the type locale_t. CI's macOS build is OK, and my own > local Mac is building master OK, and animal "indri" is OK... hmm, > those are all using MacPorts, but I don't yet see why that would be > it... Ah, got it. It was OK under meson but not autoconf for my Mac, so I guess it must be transitive headers coming from somewhere making it work for some systems. I just have a typo in an #ifdef macro. Will fix.
Here's another mystery from Windows + MinGW. Although "fairywren" is green, that is because it lacks ICU, which would activate extra tests. CI is green too, but the optional CI task "Windows - Server 2019, MinGW64 - Meson" has ICU and it is now failing if you trigger it[1] after commit 35eeea62, in initdb/001_initdb: [05:43:49.764] | 146/305 - options --locale-provider=icu --locale=und --lc-*=C: no stderr FAIL ... because it logs a warning to stderr: WARNING: no usable system locales were found I can only assume there was some extra dependency on setlocale() global state changes in the removed code. I don't quite get it, but whatever the reason, it's less than helpful to have different compilers taking different code paths on our weirdest OS that most of us don't use, so I propose to push this change to take the regular MSVC code path for MinGW too, when looking up code pages. Somehow, this fixes that, though it'd probably take someone with a local MinGW setup to dig into what exactly is happening there. (There are plenty more places where we do something different for MinGW. I suspect they are all obsolete problems. We should probably just harmonise everything and see what breaks now that we have a CI system, but that can be for another day.) That warning is from pg_import_system_locales(), which is new-ish (v16) on that OS. It was recently discovered to trigger a pre-existing problem[2]: the simple setlocale() save/restore pattern doesn't work in general on Windows, because some local names are non-ASCII, and the restore can fail (abort in the system library due to bad encoding, because the intermediate setlocale() changed the expected encoding of the locale name itself). So it's good that we aren't doing that anymore in this location; I'm just thinking out loud about whether that phenomenon could also be somehow connected to this failure, though I don't see it. Another realisation is that my pg_localeconv_r() patch, which can't avoid a thread-safe setlocale() save-and-restore on that OS (and might finish up being the only one left in the tree by the time we're done?), had better use wsetlocale() instead to defend itself against that particular madness. [1] https://cirrus-ci.com/task/5928104793735168 [2] https://www.postgresql.org/message-id/CA%2BhUKG%2BFxeRLURZ%3Dn8NPyLwgjFds_SqU_cQvE40ks6RQKUGbGg%40mail.gmail.com
Attachment
On Tue, Aug 13, 2024 at 6:23 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> Patch 3 makes sense too, some comments on the details:
> The #ifdefs and the LCONV_MEMBER stuff makes it a bit hard to follow
> what happens in each implementation strategy. I wonder if it would be
> more clear to duplicate more code.
I tried to make it easier to follow.
> There's a comment at the top of pg_locale.c ("!!! NOW HEAR THIS !!!")
> that needs to be removed or adjusted now.
Yeah. We can remove that PSA if we also fix up the equivalent code
for LC_TIME. First attempt at that attached.
> > * The POSIX standard explicitly says that it is undefined what happens if
> > * LC_MONETARY or LC_NUMERIC imply an encoding (codeset) different from
> > * that implied by LC_CTYPE. In practice, all Unix-ish platforms seem to
> > * believe that localeconv() should return strings that are encoded in the
> > * codeset implied by the LC_MONETARY or LC_NUMERIC locale name. Hence,
> > * once we have successfully collected the localeconv() results, we will
> > * convert them from that codeset to the desired server encoding.
>
> The patch loses this comment, leaving just a much shorter comment in the
> WIN32 implementation. But it still seems like a relevant comment for the
> !WIN32 implementation too.
New version makes it much clearer, and also is much more careful about
what exactly happens if you have mismatched encodings.
(Over in CF #3772 I was exploring the idea of banning the use of
locales that are not compatible with the database encoding. As far as
I can guess, that idea must have come from the time when Windows
didn't have native UTF-8 support. Now it does. There I was mostly
interested in killing all the whcar_t conversion code, but maybe we
could also delete a few lines of transcoding around here too?)