Re: ICU for global collation - Mailing list pgsql-hackers

On 15.03.22 08:41, Julien Rouhaud wrote:
>>>> The locale object in ICU is an identifier that specifies a particular locale
>>>> and has fields for language, country, and an optional code to specify further
>>>> variants or subdivisions. These fields also can be represented as a string
>>>> with the fields separated by an underscore.
>>
>> I think the Localization chapter needs to be reorganized a bit, but I'll
>> leave that for a separate patch.
> 
> WFM.

I ended up writing a bit of content for that chapter.

>>> While on that topic, the doc should probably mention that default ICU
>>> collations can only be deterministic.
>>
>> Well, there is no option to do otherwise, so I'm not sure where/how to
>> mention that.  We usually don't document options that don't exist. ;-)
> 
> Sure, but I'm afraid that users may still be tempted to use ICU locales like
> und-u-ks-level2 from the case_insensitive example in the doc and hope that it
> will work accordingly.  Or maybe it's just me that still sees ICU as dark magic
> and want to be extra cautious.

I have added this to the CREATE DATABASE ref page.

> Unrelated, but I just realized that we have PGC_INTERNAL gucs for lc_ctype and
> lc_collate.  Should we add one for icu_locale too?

I'm not sure.  I think the existing ones are more for backward 
compatibility with the time before it was settable per database.

>>> in AlterCollation(), pg_collation_actual_version(), AlterDatabaseRefreshColl()
>>> and pg_database_collation_actual_version():
>>>
>>> -   datum = SysCacheGetAttr(COLLOID, tup, Anum_pg_collation_collcollate, &isnull);
>>> -   Assert(!isnull);
>>> -   newversion = get_collation_actual_version(collForm->collprovider, TextDatumGetCString(datum));
>>> +   datum = SysCacheGetAttr(COLLOID, tup, collForm->collprovider == COLLPROVIDER_ICU ?
Anum_pg_collation_colliculocale: Anum_pg_collation_collcollate, &isnull);
 
>>> +   if (!isnull)
>>> +       newversion = get_collation_actual_version(collForm->collprovider, TextDatumGetCString(datum));
>>> +   else
>>> +       newversion = NULL;
>>>
>>> The columns are now nullable, but can you actually end up with a null locale
>>> name in the expected field without manual DML on the catalog, corruption or
>>> similar?  I think it should be a plain error explaining the inconsistency
>>> rather then silently assuming there's no version.  Note that at least
>>> pg_newlocale_from_collation() asserts that the specific libc/icu field it's
>>> interested in isn't null.
>>
>> This is required because the default collations's fields are null now.
> 
> Yes I saw that, but that's a specific exception.  Detecting whether it's the
> DEFAULT_COLLATION_OID or not and raise an error when a null value isn't
> expected seems like it could be worthwhile.

I have fixed that as you suggest.

> So apart from the few details mentioned above I'm happy with this patch!

committed!