Horton

Re: ICU locale validation / canonicalization

Peter Eisentraut <peter.eisentraut@enterprisedb.com>

View thread View on postgr.es
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Jeff Davis <pgsql@j-davis.com>, Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org
Date: 2023-03-24T09:10:35Z
Lists: pgsql-hackers 
On 24.03.23 07:39, Jeff Davis wrote:
> On Thu, 2023-03-23 at 10:16 -0700, Jeff Davis wrote:
>> I could get rid of the SQL-callable function and move the rest of the
>> changes into 0006. I'll see if that arrangement works better, and
>> that
>> way we can add the SQL-callable function later (or perhaps not at all
>> if it's not desired).
> 
> Attached a new series that doesn't include the SQL-callable function.
> It's probably better to just wait and see what functions seem actually
> useful to users.
> 
> I included a new small patch to fix a potential UCollator leak and make
> the errors more consistent.

[PATCH v8 1/4] Avoid potential UCollator leak for older ICU versions.

Couldn't we do this in a simpler way by just freeing the collator before 
the ereport() calls.  Or wrap a PG_TRY/PG_FINALLY around the whole thing?

It would be nicer to not make the callers of 
icu_set_collation_attributes() responsible for catching and reporting 
the errors.


[PATCH v8 2/4] initdb: emit message when using default ICU locale.

I'm not able to make initdb print this message.  Under what 
circumstances am I supposed to see this?  Do you have some examples?

The function check_icu_locale() has now gotten a lot more functionality 
than its name suggests.  Maybe the part that assigns the default ICU 
locale should be moved up one level to setlocales(), which has a better 
name and does something similar for the libc locale realm.


[PATCH v8 3/4] Canonicalize ICU locale names to language tags.

I'm still on the fence about whether we actually want to do this, but 
I'm warming up to it, now that the issues with pre-54 versions are fixed.

But if we do this, the documentation needs to be updated.  There is a 
bunch of text there that says, like, you can do this format or that 
format, whatever you like.  At least the guidance should be changed there.


[PATCH v8 4/4] Validate ICU locales.

I would make icu_locale_validation true by default.

Or maybe it should be a log-level type option, so you can set it to 
error, warning, and also completely off?

Commits

  1. Add missing source file to nls.mk

  2. Fix MSVC warning introduced in ea1db8ae70.

  3. Canonicalize ICU locale names to language tags.

  4. Validate ICU locales.

  5. initdb: emit message when using default ICU locale.

  6. initdb: replace check_icu_locale() with default_icu_locale().

  7. Fix error inconsistency in older ICU versions.

  8. Avoid potential UCollator leak for older ICU versions.

  9. pg_locale.c: change ereport() to elog().

  10. Handle the "und" locale in ICU versions 54 and older.

  11. Wrap ICU ucol_open().

  12. Support language tags in older ICU versions (53 and earlier).

  13. Improve support for UNICODE collation on older ICU

  14. Use ICU by default at initdb time.