public inbox for [email protected]  

help / color / mirror / Atom feed

From: Peter Eisentraut <[email protected]>
To: Jeff Davis <[email protected]>
To: Daniel Verite <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Jeremy Schneider <[email protected]>
Cc: [email protected]
Subject: Re: Built-in CTYPE provider
Date: Thu, 14 Mar 2024 09:54:35 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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.

view thread (39+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Built-in CTYPE provider
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox