Thread: Re: pgsql: Add option to use ICU as global locale provider
On 18.03.22 10:27, Julien Rouhaud wrote: > I'm attaching a patch that fixes both issues for me with ICU 50. Note that > there's already a test that would have failed for CREATE DATABASE if initdb > tests didn't fail first, so no new test needed. > > I ended up copy/pasting icu_set_collation_attributes() in initdb.c. There > shouldn't be new attributes added in old ICU versions, and there are enough > differences to make it work in the frontend that it didn't seems worth to have > a single function. Another option is that we just don't do the check in initdb. As the tests show, you will then get an error from the backend call, so it's really just a question of when the error is reported. Why does your patch introduce a function check_icu_locale() that is only called once? Did you have further plans for that?
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Another option is that we just don't do the check in initdb. As the
> tests show, you will then get an error from the backend call, so it's
> really just a question of when the error is reported.
+1 ... seems better to not have two copies of the code.
regards, tom lane
Hi, On 2022-03-18 20:28:58 +0100, Peter Eisentraut wrote: > Why does your patch introduce a function check_icu_locale() that is only > called once? Did you have further plans for that? I like that it moves ICU code out of dbcommands.c - imo there should be few calls to ICU functions outside of pg_locale.c. There might be an argument for moving *more* into such a function though. Greetings, Andres Freund
On Fri, Mar 18, 2022 at 04:04:10PM -0400, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: > > Another option is that we just don't do the check in initdb. As the > > tests show, you will then get an error from the backend call, so it's > > really just a question of when the error is reported. > > +1 ... seems better to not have two copies of the code. Ok, I also prefer to not have two copies of the code but wasn't sure that having the error in the boostrapping phase was ok. I will change that.
On Fri, Mar 18, 2022 at 03:09:59PM -0700, Andres Freund wrote: > Hi, > > On 2022-03-18 20:28:58 +0100, Peter Eisentraut wrote: > > Why does your patch introduce a function check_icu_locale() that is only > > called once? Did you have further plans for that? > > I like that it moves ICU code out of dbcommands.c Yes, it seemed cleaner this way. But more importantly code outside pg_locale.c really shouldn't have to deal with ICU specific version code. I'm attaching a v2, addressing Peter and Tom comments to not duplicate the old ICU versions attribute function. I removed the ICU locale check entirely (for consistency across ICU version) thus removing any need for ucol.h include in initdb. For the problem you reported at [1] with the meson branch, I changed createdb tests with s/en-u-kf-upper/en@colCaseFirst=upper/, as older ICU versions don't understand the former notation. check-world now pass for me, using either ICU < 54 or >= 54. > imo there should be few > calls to ICU functions outside of pg_locale.c. There might be an argument for > moving *more* into such a function though. I think it would be a good improvement. I can work on that next week if needed. [1] https://www.postgresql.org/message-id/20220318000140.vzri3qw3p4aebn5p@alap3.anarazel.de
Attachment
On 19.03.22 05:14, Julien Rouhaud wrote: > On Fri, Mar 18, 2022 at 03:09:59PM -0700, Andres Freund wrote: >> Hi, >> >> On 2022-03-18 20:28:58 +0100, Peter Eisentraut wrote: >>> Why does your patch introduce a function check_icu_locale() that is only >>> called once? Did you have further plans for that? >> >> I like that it moves ICU code out of dbcommands.c > > Yes, it seemed cleaner this way. But more importantly code outside pg_locale.c > really shouldn't have to deal with ICU specific version code. > > I'm attaching a v2, addressing Peter and Tom comments to not duplicate the old > ICU versions attribute function. I removed the ICU locale check entirely (for > consistency across ICU version) thus removing any need for ucol.h include in > initdb. committed
On Sun, Mar 20, 2022 at 11:03:38AM +0100, Peter Eisentraut wrote: > On 19.03.22 05:14, Julien Rouhaud wrote: > > On Fri, Mar 18, 2022 at 03:09:59PM -0700, Andres Freund wrote: > > > Hi, > > > > > > On 2022-03-18 20:28:58 +0100, Peter Eisentraut wrote: > > > > Why does your patch introduce a function check_icu_locale() that is only > > > > called once? Did you have further plans for that? > > > > > > I like that it moves ICU code out of dbcommands.c > > > > Yes, it seemed cleaner this way. But more importantly code outside pg_locale.c > > really shouldn't have to deal with ICU specific version code. > > > > I'm attaching a v2, addressing Peter and Tom comments to not duplicate the old > > ICU versions attribute function. I removed the ICU locale check entirely (for > > consistency across ICU version) thus removing any need for ucol.h include in > > initdb. > > committed Thanks!
Hi, On 2022-03-20 11:03:38 +0100, Peter Eisentraut wrote: > committed Thanks. Rebasing over that fixed the meson Centos 7 build in my meson tree. https://cirrus-ci.com/build/5265480968568832 Greetings, Andres Freund