Message-ID: <550302f9fd427f29673fdb99d94f8f64b1ee3d61.camel@j-davis.com>
Subject: Re: Built-in CTYPE provider
From: Jeff Davis <pgsql@j-davis.com>
To: Peter Eisentraut <peter@eisentraut.org>, Daniel Verite
	 <daniel@manitou-mail.org>
Cc: Robert Haas <robertmhaas@gmail.com>, Jeremy Schneider
	 <schneider@ardentperf.com>, pgsql-hackers@postgresql.org
Date: Wed, 27 Mar 2024 15:13:55 -0700
In-Reply-To: <a8804ef9-fda6-4660-9f98-ecd1315f958c@eisentraut.org>
References: <6f3e94c0-f174-4380-9b69-072f8a838881@manitou-mail.org>
	 <12e4f6a78403b33c303c20e44976f891d879be09.camel@j-davis.com>
	 <612987b5-38fa-44c6-8b78-c0be5a177a70@eisentraut.org>
	 <2692039df6b0cac20c5e488fa0209e8958c5fedb.camel@j-davis.com>
	 <27bb0e52-801d-4f73-a0a4-02cfdd4a9ada@eisentraut.org>
	 <4a69d067374d2f6bfb66f5bfb2ab9a020493d49f.camel@j-davis.com>
	 <3bc653b5d562ae9e2838b11cb696816c328a489a.camel@j-davis.com>
	 <163f4e2190cdf67f67016044e503c5004547e5a9.camel@j-davis.com>
	 <6bdb98e68b2b05aa71f7f934e227738eac84ecee.camel@j-davis.com>
	 <19b34a70-f5cf-4faf-88dc-917db44ed48d@eisentraut.org>
	 <846a7e1fa2024918b58ff7583523a2f3de9a11b4.camel@j-davis.com>
	 <3117d30aa911b408b90420f4f280fa0c3b5851be.camel@j-davis.com>
	 <4135cf11-206d-40ed-96c0-9363c1232379@eisentraut.org>
	 <7451f81ba0cb512222ab759de8ca1cffe44e9acb.camel@j-davis.com>
	 <1f309153-8198-4efa-86dd-8c304ec0040c@eisentraut.org>
	 <a21b515000f121244db2a5570d4292d19df23379.camel@j-davis.com>
	 <a8804ef9-fda6-4660-9f98-ecd1315f958c@eisentraut.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/550302f9fd427f29673fdb99d94f8f64b1ee3d61.camel%40j-davis.com>
Precedence: bulk

On Tue, 2024-03-26 at 08:04 +0100, Peter Eisentraut wrote:
> The patch set v27 is ok with me, modulo (a) discussion about initcap=20
> semantics, and (b) what collation to assign to ucs_basic, which can
> be=20
> revisited later.

I held off on the refactoring patch for lc_{ctype|collate}_is_c().
There's an explicit "NB: pg_newlocale_from_collation is only supposed
to be called on non-C-equivalent locales" comment in DefineCollation().

What I'd like to do is make it possible to create valid pg_locale_t
objects out of C locales, which can be used anywhere a real locale can
be used. Callers can still check lc_{collate|ctype}_is_c() for various
reasons; but if they did call pg_newlocale_from_collation on a C locale
it would at least work for the pg_locale.h APIs. That would be a
slightly simpler and safer API, and make it easier to do the collation
version check consistently.

That's not very complicated, but it's a bit invasive and probably out
of scope for v17. It might be part of another change I had intended for
a while, which is to make NULL an invalid pg_locale_t, and use a
different representation to mean "use the server environment". That
would clean up a lot of checks for NULL.

For now, we'd still like to add the version number to the builtin
collations, so that leaves us with two options:

(a) Perform the version check in lc_{collate|ctype}_is_c(), which
duplicates some code and creates some inconsistency in how the version
is checked for different providers.

(b) Don't worry about it and just commit the version change in v27-
0001. The version check is already performed correctly on the database
without changes, even if the locale is "C". And there are already three
built-in "C" collations: "C", "POSIX", and UCS_BASIC; so it's not clear
why someone would create even more of them. And even if they did, there
would be no reason to give them a warning because we haven't
incremented the version, so there's no chance of a mismatch.

I'm inclined toward (b). Thoughts?

Regards,
	Jeff Davis