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-21T09:35:56Z
Lists: pgsql-hackers 
On 17.03.23 18:55, Jeff Davis wrote:
> On Wed, 2023-03-15 at 15:18 -0700, Jeff Davis wrote:
>> I left out the validation patch for now, and I'm evaluating a
>> different
>> approach that will attempt to match to the locales retrieved with
>> uloc_countAvailable()/uloc_getAvailable().
> 
> I like this approach, attached new patch series with that included as
> 0006.

I have looked at the first three patches.  I think we want what those 
patches do.


[PATCH v6 1/6] Support language tags in older ICU versions (53 and
  earlier).

In pg_import_system_collations(), this is now redundant and can be 
simplified:

-		if (!pg_is_ascii(langtag) || !pg_is_ascii(iculocstr))
+		if (!pg_is_ascii(langtag) || !pg_is_ascii(langtag))

icu_set_collation_attributes() needs more commenting about what is going 
on.  My guess is that uloc_canonicalize() converts from language tag to 
ICU locale ID, and then the existing logic to parse that apart would 
apply.  Is that how it works?


[PATCH v6 2/6] Wrap ICU ucol_open().

It makes sense to try to unify some of this.  But I find the naming 
confusing.  If I see pg_ucol_open(), then I would expect that all calls 
to ucol_open() would be replaced by this.  But here it's only a few, 
without explanation.  (pg_ucol_open() has no explanation at all AFAICT.)

I have in my notes that check_icu_locale() and make_icu_collator() 
should be combined into a single function.  I think that would be a 
better way to slice it.

Btw., I had intentionally not written code like this

+#if U_ICU_VERSION_MAJOR_NUM < 54
+	icu_set_collation_attributes(collator, loc_str);
+#endif

The disadvantage of doing it that way is that you then need to dig out 
an old version of ICU in order to check whether the code compiles at 
all.  With the current code, you can be sure that that code compiles if 
you make changes elsewhere.


[PATCH v6 3/6] Handle the "und" locale in ICU versions 54 and older.

This makes sense, but the same comment about not #if'ing out code for 
old ICU versions applies here.

The

+#ifdef USE_ICU
+

before pg_ucol_open() probably belongs in patch 2.

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.