Re: tiny step toward threading: reduce dependence on setlocale()

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