Re: Thread-safe nl_langinfo() and localeconv()

On 28.03.25 09:13, Peter Eisentraut wrote:
>>> About patch 0003:
>>>
>>> I had previously pointed out that the canonicalization might have 
>>> been intentional, and that we have canonicalization of ICU locale 
>>> names. But we don't have to keep the setlocale()-based locale 
>>> checking implementation just for that, I think.  (If this was meant 
>>> to be a real feature offered by libc, there should be a 
>>> get_locale_name(locale_t) function.)
> 
>> Anyway, this patch fails tests on CI on NetBSD, so it will need some 
>> further investigation.
> 
> It turns out the problem was that nl_langinfo_l() returns a codeset name 
> of "646" for the C locale, which we didn't have in our mapping table. If 
> we add that, then everything passes there as well.  (But the use of 
> nl_langinfo_l() wasn't added by this patch, it just exposes it to the 
> tests.  So this is apparently a pre-existing problem.)

Further analysis:

(But I have not tested any of this.)

It appears that the new implementation of check_locale() provided by 
this patch, using newlocale() instead of setlocale(), works differently 
on NetBSD.  Specifically, it apparently does not catch garbage locale 
names.  Instead, it just assumes they are C locale.  Then, in 
pg_get_encoding_from_locale(), we have special cases mapping "C" and 
"POSIX" to SQL_ASCII.  But as discussed, with this patch, we no longer 
do canonicalization of the passed-in locale name, so if you pass a 
garbage locale name, it will not match "C" or "POSIX".  Then, you fall 
through to the mapping table, and that's where we get the error about 
the missing "646" entry.  That's why this was not a problem before, even 
though we've used nl_langinfo_l() for a few months and nl_langinfo() 
before that.

I'm not sure what to do with this.  If setlocale() and newlocale() 
indeed behave differently in what set of locale names they accept, then 
technically we ought to test both of them, since we do use both of them 
later on.  Or maybe we push on with the effort to get rid of setlocale() 
calls and then just worry about testing newlocale() (as this patch 
does).  But right now, if newlocale() is more permissive, then we could 
accept locale names that will later fail setlocale() calls, which might 
be a problem.