Feedback-ID: i131946ab:Fastmail
Message-ID: <a0d48a52-8596-763b-7682-d4b9e3b2e10e@enterprisedb.com>
Date: Wed, 4 Jan 2023 22:46:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.0
Subject: Re: Rework of collation code, extensibility
Content-Language: en-US
To: Jeff Davis <pgsql@j-davis.com>, pgsql-hackers@postgresql.org
References: <a581136455c940d7bd0ff482d3a2bd51af25a94f.camel@j-davis.com>
 <a20de213c75fe471d067eecc1ac65e0edb9af377.camel@j-davis.com>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <a20de213c75fe471d067eecc1ac65e0edb9af377.camel@j-davis.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/a0d48a52-8596-763b-7682-d4b9e3b2e10e%40enterprisedb.com>
Precedence: bulk

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.