Re: Rework of collation code, extensibility

On 01.02.23 00:33, Jeff Davis wrote:
> On Tue, 2023-01-31 at 11:40 +0100, Peter Eisentraut wrote:
>> I don't know to what extent this depends on the abbreviated key GUC
>> discussion.  Does the rest of this patch set depend on this?
> 
> The overall refactoring is not dependent logically on the GUC patch. It
> may require some trivial fixup if you eliminate the GUC patch.
> 
> I left it there because it makes exploring/testing easier (at least for
> me), but the GUC patch doesn't need to be committed if there's no
> consensus.

I took another closer look at the 0002 and 0003 patches.

The commit message for 0002 says "Also remove the TRUST_STRXFRM define", 
but I think this is incorrect, as that is done in the 0001 patch.

I don't like that the pg_strnxfrm() function requires these kinds of 
repetitive error checks:

+           if (rsize != bsize)
+               elog(ERROR, "pg_strnxfrm() returned unexpected result");

This could be checked inside the function itself, so that the callers 
don't have to do this themselves every time.

I don't really understand the 0003 patch.  It's a lot of churn but I'm 
not sure that it achieves more clarity or something.

The new function pg_locale_deterministic() seems sensible.  Maybe this 
could be proposed as a separate patch.

I don't understand the new header pg_locale_internal.h.  What is 
"internal" and what is not internal?  What are we hiding from whom? 
There are no code comments about this AFAICT.

pg_locale_struct has new fields

+   char       *collate;
+   char       *ctype;

that are not explained anywhere.

I think this patch would need a bit more explanation and commenting.