Re: ICU locale validation / canonicalization

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?