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

From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Peter Eisentraut <peter(at)eisentraut(dot)org>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: tiny step toward threading: reduce dependence on setlocale()
Date: 2024-07-26 06:07:37
Message-ID: 56af65bde2190cc5ee20c807be7b7bdcda54f40c.camel@j-davis.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v3-0001-Make-database-default-collation-internal-to-pg_lo.patch text/x-patch 5.8 KB
v3-0002-Make-database-collation-pg_locale_t-always-non-NU.patch text/x-patch 6.6 KB
v3-0003-ts_locale.c-do-not-use-NULL-to-mean-the-database-.patch text/x-patch 3.5 KB
v3-0004-Remove-support-for-null-pg_locale_t.patch text/x-patch 26.5 KB
v3-0005-Simplify-collation-cache.patch text/x-patch 10.8 KB
v3-0006-Clean-up-more-checks-for-NULL-pg_locale_t.patch text/x-patch 7.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2024-07-26 06:16:30 Re: Logical Replication of sequences
Previous Message Zhang Mingli 2024-07-26 06:02:05 Is it necessary to keep am routine finish_bulk_insert?