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.
