Re: ICU locale validation / canonicalization

On Tue, 2023-03-28 at 08:41 +0200, Peter Eisentraut wrote:
> [PATCH v9 1/5] Fix error inconsistency in older ICU versions.
>
> ok

Committed 0001.

> [PATCH v9 2/5] initdb: replace check_icu_locale() with
>   default_icu_locale().
>
> I would keep the #ifdef USE_ICU inside the lower-level function
> default_icu_locale(), like it was before, so that the higher-level
> setlocales() doesn't need to know about it.
>
> Otherwise ok.

Done and committed 0002.

>
> [PATCH v9 3/5] initdb: emit message when using default ICU locale.

Done and committed 0003.

> [PATCH v9 4/5] Validate ICU locales.
>
> Also here, let's keep the #ifdef USE_ICU in the lower-level function
> and
> move more logic in there.  Otherwise you have to repeat various
> things
> in DefineCollation() and createdb().

Done.

> I'm not sure we need the IsBinaryUpgrade checks.  Users can set
> icu_validation_level on the target instance if they don't want that.

I committed a version that still performs the checks during binary
upgrade, but degrades the message to a WARNING if it's set higher than
that. I tried some upgrades with invalid locales, and getting an error
deep in the logs after the upgrade actually starts is not very user-
friendly. We could add something during the --check phase, which would
be more helpful, but I didn't do that for this patch.


Attached is a new version of the final patch, which performs
canonicalization. I'm not 100% sure that it's wanted, but it still
seems like a good idea to get the locales into a standard format in the
catalogs, and if a lot more people start using ICU in v16 (because it's
the default), then it would be a good time to do it. But perhaps there
are risks?


--
Jeff Davis
PostgreSQL Contributor Team - AWS