Message-ID: <0ab5c536cc778a5f27f51a6998fd4033bb1c21d8.camel@j-davis.com>
Subject: Re: Rework of collation code, extensibility
From: Jeff Davis <pgsql@j-davis.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>, 
	pgsql-hackers@postgresql.org
Date: Thu, 05 Jan 2023 23:04:32 -0800
In-Reply-To: <a0d48a52-8596-763b-7682-d4b9e3b2e10e@enterprisedb.com>
References: <a581136455c940d7bd0ff482d3a2bd51af25a94f.camel@j-davis.com>
	 <a20de213c75fe471d067eecc1ac65e0edb9af377.camel@j-davis.com>
	 <a0d48a52-8596-763b-7682-d4b9e3b2e10e@enterprisedb.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu1 
MIME-Version: 1.0
Archived-At: <https://www.postgresql.org/message-id/0ab5c536cc778a5f27f51a6998fd4033bb1c21d8.camel%40j-davis.com>
Precedence: bulk

On Wed, 2023-01-04 at 22:46 +0100, Peter Eisentraut wrote:
> It combines some refactoring that was previously posted with partial=20
> support for multiple ICU libraries with partial support for some new=20
> hooks.=C2=A0 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.

> =C2=A0 I think the multiple=20
> ICU libraries already does have a separate thread; how does this
> relate=20
> 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.

> =C2=A0 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.

> =C2=A0 What=20
> other locale libraries are you thinking about using this way?=C2=A0 How
> can=20
> 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.

> =C2=A0 The=20
> refactoring patches don't look convincing just by looking at the
> numbers:
>=20
> =C2=A0 3 files changed, 406 insertions(+), 247 deletions(-)
> =C2=A0 6 files changed, 481 insertions(+), 150 deletions(-)
> =C2=A0 12 files changed, 400 insertions(+), 323 deletions(-)

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?


--=20
Jeff Davis
PostgreSQL Contributor Team - AWS