On Wed, 2023-01-04 at 22:46 +0100, Peter Eisentraut wrote: > 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?
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 > ICU libraries already does have a separate thread; how does this > relate > 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 > other locale libraries are you thinking about using this way? How > can > 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 > 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(-) 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