On Wed, 2024-06-19 at 11:15 +0200, Peter Eisentraut wrote:
> > * v2-0001-Make-database-default-collation-internal-to-pg_lo.patch
> >
> > +void
> > +pg_init_database_collation()
> >
> > The function argument should be (void).
> >
> > The prefix pg_ makes it sound like it's a user-facing function of >
> > some
> > sort. Is there a reason for it?
> >
> > Maybe add a small comment before the function.
Agreed, done.
> >
> > * v2-0002-Make-database-collation-pg_locale_t-always-non-NU.patch
> >
> > There is quite a bit of code duplication from
> > pg_newlocale_from_collation(). Can we do this better?
I refactored it into make_libc_collator().
> > * v2-0003-ts_locale.c-do-not-use-NULL-to-mean-the-database-.patch
> >
> > The "TODO" markers should be left, because what they refer to is
> > that
> > these functions just use the default collation rather than
> > something
> > passed in from the expression machinery. This is not addressed by
> > this change. (Obviously, the comments could have been clearer
> > about
> > this.)
Done.
> > * v2-0004-Remove-support-for-null-pg_locale_t.patch
> >
> > I found a few more places that should be adjusted in a similar way.
Fixed, thank you.
> > The changes at the call sites of pg_locale_deterministic() are
> > unfortunate
Yeah, that part was a bit annoying.
> > But then after your 0006 patch, lc_locale_is_c() internally also >
> > calls
> > pg_newlocale_from_collation()
Not always. It still returns early for C_COLLATION_OID or
POSIX_COLLATION_OID, and that's actually required because
pg_regcomp(..., C_COLLATION_OID) is called when parsing pg_hba.conf,
before catalog access is available. I don't think that detail is
relevant in the cases you brought up, but it got in the way of some
other refactoring I was trying to do.
> > , so this code just becomes redundant.
> > Better might be if pg_locale_deterministic() itself checked if >
> > collate
> > is C, and then hashtext() would just need to write:
> >
> > mylocale = pg_newlocale_from_collation(collid);
> >
> > if (pg_locale_deterministic(mylocale))
> > {
> >
> > The patch sequencing might be a bit tricky here. Maybe it's ok if
> > patch 0004 stays as is in this respect if 0006 were to fix it back.
Addressed in v3-0006.
> > * v2-0005-Avoid-setlocale-in-lc_collate_is_c-and-lc_ctype_i.patch
> >
> > Nothing uses this, AFAICT, so why?
You're right. It was used to avoid setlocale() in lc_collate_is_c(),
but that's eliminated in the next patch anyway.
Also, in v3-0005, I had to also check for "C" or "POSIX" in
make_libc_collator(), so that it wouldn't try to actually create the
locale_t in that case.
Regards,
Jeff Davis