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

On Thu, Aug 15, 2024 at 11:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> There's a similar function in initdb, check_locale_name. I wonder if
> that could reuse the same code.

Thanks, between this comment and some observations from Peter E and
Tom, I think I have a better plan now.  I think they should *not*
match, and a comment saying so should be deleted.  In the backend, we
should do neither ""-expansion (ie getenv("LC_...") whether direct or
indirect) nor canonicalisation (of Windows' deprecated pre-BCP 47
locale names), making this v4 patch extremely simple.

1.  CREATE DATABASE doesn't really need to accept LOCALE = ''.  What
is the point?  It's not documented or desirable behavior AFAIK.  If
you like defaults you can just not provide a locale at all and get the
template database's (which often comes from initdb, which often uses
the server environment).  That behavior was already inconsistent with
CREATE COLLATION.  So I think we should just reject "" in the backend
check_locale().

2.  A similar argument applies to Windows canonicalisation.  CREATE
COLLATION isn't doing it.  CREATE DATABASE is, but again, what is the
point?  See previous.

(I also think that initdb should use a different mechanism for finding
the native locale on Windows, but I already have a CF #3772 for that,
ie migration plan for BCP 47 and native UTF-8 on Windows, but I don't
want *this* thread to get blocked by our absence of Windows
reviewers/testers, so let's not tangle that up with this thread-safety
expedition.)

To show a concrete example of commands no longer accepted with this
version, because they call check_locale():

postgres=# set lc_monetary = '';
ERROR:  invalid value for parameter "lc_monetary": ""

postgres=# create database db2 locale = '';
ERROR:  invalid LC_COLLATE locale name: ""
HINT:  If the locale name is specific to ICU, use ICU_LOCALE.

Does anyone see a problem with that?

I do see a complication for CREATE COLLATION, though.  It doesn't call
check_locale(), is not changed in this patch, and still accepts ''.
Reasoning: There may be systems with '' in their pg_collation catalog
in the wild, since we never canonicalised with setlocale(), so it
might create some kind of unforeseen dump/restore/upgrade hazard if we
just ban '' outright, I just don't know what yet.

There is no immediate problem, ie there is no setlocale() to excise,
for *this* project.  Longer term, we can't actually continue to allow
'' in COLLATION objects, though: that tells newlocale() to call
getenv(), which may be technically OK in a multi-threaded program
(that never calls setenv()), but is hardly desirable.  But it will
also give the wrong result, if we pursue the plan that Jeff and I
discussed: we'll stop doing setenv("LC_COLLATE", datacollate) and
setenv("LC_CTYPE", datctype) in postinit.c (see pg_perm_setlocale()
calls).  So I think any pg_collation catalog entries holding '' need
to be translated to datcollate/datctype... somewhere.  I just don't
know where yet and don't want to tackle that in the same patch.