Re: Built-in CTYPE provider

On 14.03.24 09:08, Jeff Davis wrote:
> On Wed, 2024-03-13 at 00:44 -0700, Jeff Davis wrote:
>> New series attached. I plan to commit 0001 very soon.
> 
> Committed the basic builtin provider, supporting only the "C" locale.

As you were committing this, I had another review of 
v23-0001-Introduce-collation-provider-builtin.patch in progress.  Some 
of the things I found you have already addressed in what you committed. 
Please check the remaining comments.


* doc/src/sgml/charset.sgml

I don't understand the purpose of this sentence:

"When using this locale, the behavior may depend on the database encoding."


* doc/src/sgml/ref/create_database.sgml

The new parameter builtin_locale is not documented.


* src/backend/commands/collationcmds.c

I think DefineCollation() should set collencoding = -1 for the
COLLPROVIDER_BUILTIN case.  -1 stands for any encoding.  Or at least
explain why not?


* src/backend/utils/adt/pg_locale.c

This part is a bit confusing:

+           cache_entry->collate_is_c = true;
+           cache_entry->ctype_is_c = (strcmp(colllocale, "C") == 0);

Is collate always C but ctype only sometimes?  Does this anticipate
future patches in this series?  Maybe in this patch it should always
be true?


* src/bin/initdb/initdb.c

+   printf(_("      --builtin-locale=LOCALE   set builtin locale name 
for new databases\n"));

Put in a line break so that the right "column" lines up.

This output should line up better:

The database cluster will be initialized with this locale configuration:
   default collation provider:  icu
   default collation locale:    en
   LC_COLLATE:  C
   LC_CTYPE:    C
   ...

Also, why are there two spaces after "provider:  "?

Also we call these locale provider on input, why are they collation
providers on output?  What is a "collation locale"?


* src/bin/pg_upgrade/t/002_pg_upgrade.pl

+if ($oldnode->pg_version >= '17devel')

This is weird.  >= is a numeric comparison, so providing a string with
non-digits is misleading at best.


* src/test/icu/t/010_database.pl

-# Test that LOCALE works for ICU locales if LC_COLLATE and LC_CTYPE
-# are specified

Why remove this test?

+my ($ret, $stdout, $stderr) = $node1->psql('postgres',
+   q{CREATE DATABASE dbicu LOCALE_PROVIDER builtin LOCALE 'C' TEMPLATE 
dbicu}
+);

Change the name of the new database to be different from the name of
the template database.