Message-ID: <32e1b3e0-590c-4eac-852e-f681ee36594a@iki.fi>
Date: Tue, 13 Aug 2024 09:23:10 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Thread-safe nl_langinfo() and localeconv()
To: Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <CA+hUKGJqVe0+Pv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A@mail.gmail.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <CA+hUKGJqVe0+Pv9dvC9dSums_PXxGo9SWcxYAMBguWJUGbWz-A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/32e1b3e0-590c-4eac-852e-f681ee36594a%40iki.fi>
Precedence: bulk

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)