Subject: Re: Rework of collation code, extensibility

From: Peter Eisentraut <[email-masked]>
Date: Wed, 4 Jan 2023 22:46:23 +0100

On 22.12.22 06:40, Jeff Davis wrote:
> On Sat, 2022-12-17 at 19:14 -0800, Jeff Davis wrote:
>> Attached is a new patch series. I think there are enough changes that
>> this has become more of a "rework" of the collation code rather than
>> just a refactoring. This is a continuation of some prior work[1][2]
>> in
>> a new thread given its new scope.
> 
> Here's version 5. There are a number of fixes, and better tests, and
> it's passing in CI.
> 
> The libc hook support is still experimental, but what's working is
> passing in CI, even on windows. The challenges with libc hook support
> are:
> 
>   * It obviously doesn't replace all of libc, so the separation is not
> as clean and there are a number of callers throughout the code that
> don't necessarily care about specific collations.
> 
>   * libc relies on setlocale() / uselocale(), which is global state and
> not as easy to track.
> 
>   * More platform issues (obviously) and harder to test.

I'm confused by this patch set.

It combines some refactoring that was previously posted with partial 
support for multiple ICU libraries with partial support for some new 
hooks.  Shouldn't those be three separate threads?  I think the multiple 
ICU libraries already does have a separate thread; how does this relate 
to that work?  I don't know what the hooks are supposed to be for?  What 
other locale libraries are you thinking about using this way?  How can 
we asses whether these interfaces are sufficient for that?  The 
refactoring patches don't look convincing just by looking at the numbers:

  3 files changed, 406 insertions(+), 247 deletions(-)
  6 files changed, 481 insertions(+), 150 deletions(-)
  12 files changed, 400 insertions(+), 323 deletions(-)

My sense is this is trying to do too many things at once, and those 
things are each not fully developed yet.

---

From: Jeff Davis <[email-masked]>
Date: Thu, 05 Jan 2023 23:04:32 -0800

On Wed, 2023-01-04 at 22:46 +0100, Peter Eisentraut wrote:
> hooks.  Shouldn't those be three separate threads?

Originally they felt more separate to me, too; but as I worked on them
it seemed better to consider them as a patch series. Whatever is easier
for reviewers works for me, though.

>   I think the multiple 
> to that work?

Multilib ICU support adds complexity, and my hope is that this patch
set cleans up and organizes things to better prepare for that
complexity.

>   I don't know what the hooks are supposed to be for?

I found them very useful for testing during development. One of the
patches adds a test module for the ICU hook, and I think that's a
valuable place to test regardless of whether any other extension uses
the hook. Also, if proper multilib support doesn't land in 16, then the
hooks could be a way to build rudimentary multilib support (or at least
some kind of ICU version lockdown) until it does land.

When Thomas's work is in place, I expect the hooks to change slightly.
The hooks are not meant to set any specific API in stone.

>   What 
> we asses whether these interfaces are sufficient for that?

I'm not considering any other locale libraries, nor did I see much
discussion of that.

>   The 

The existing code is not great, in my opinion: it doesn't have clear
API boundaries, the comments are insufficient, and lots of special
cases need to be handled awkwardly by callers. That style is hard to
beat when it comes to the raw line count; but it's quite difficult to
understand and work on.

I think my changes are an improvement, but obviously that depends on
the opinion of others who are working in this part of the code. What do
you think?

-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 11 Jan 2023 15:08:46 +0100

> the opinion of others who are working in this part of the code. What do

I think the refactoring that you proposed in the thread "Refactor to 
introduce pg_strcoll()." was on a sensible track.  Maybe we should try 
to get that done.  The multiple-ICU stuff is still experimental and has 
its own rather impressive thread, so I don't think it's sensible to try 
to sort that out here.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 11 Jan 2023 15:52:26 -0800

On Wed, 2023-01-11 at 15:08 +0100, Peter Eisentraut wrote:
> to get that done.

Those should be patches 0001-0003 in this thread (now at v6), which are
all pure refactoring.

Let's consider those patches the topic of this thread and I'll move
0004-0007 back to the multi-lib ICU thread on the next revision.