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.