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.
