pgsql: Add option to use ICU as global locale provider

Hi Peter,

On Thu, Mar 17, 2022 at 10:22:32AM +0000, Peter Eisentraut wrote:

Add option to use ICU as global locale provider

This adds the option to use ICU as the default locale provider for
either the whole cluster or a database. New options for initdb,
createdb, and CREATE DATABASE are used to select this.

Since some (legacy) code still uses the libc locale facilities
directly, we still need to set the libc global locale settings even if
ICU is otherwise selected. So pg_database now has three
locale-related fields: the existing datcollate and datctype, which are
always set, and a new daticulocale, which is only set if ICU is
selected. A similar change is made in pg_collation for consistency,
but in that case, only the libc-related fields or the ICU-related
field is set, never both.

FYI, prion is complaining here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-18%2001%3A43%3A13

Some details:
# Failed test 'fails for invalid ICU locale: matches'
# at t/001_initdb.pl line 107.
# '2022-03-18 01:54:58.563 UTC [504] FATAL: could
# not open collator for locale "@colNumeric=lower":
# U_ILLEGAL_ARGUMENT_ERROR
--
Michael

Hi,

On Fri, Mar 18, 2022 at 11:01:11AM +0900, Michael Paquier wrote:

FYI, prion is complaining here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&dt=2022-03-18%2001%3A43%3A13

Some details:
# Failed test 'fails for invalid ICU locale: matches'
# at t/001_initdb.pl line 107.
# '2022-03-18 01:54:58.563 UTC [504] FATAL: could
# not open collator for locale "@colNumeric=lower":
# U_ILLEGAL_ARGUMENT_ERROR

That's very strange, apparently initdb doesn't detect any problem when checking
ucol_open() for initial checks, since it's expecting:

# doesn't match '(?^:initdb: error: could not open collator for locale)'

but then postgres in single-backend mode does detect the problem, with the
exact same check, so it's not like --icu-locale=@colNumeric=lower wasn't
correctly interpreted. Unfortunately we don't have the full initdb output, so
we can't check what setup_locale_encoding reported. The only difference is
that in initdb's setlocale(), the result of ucol_open is discarded, maybe the
compiler is optimizing it away for some reason, even though it seems really
unlikely.

That being said, we could save the result and explicitly close the collator.
That wouldn't make much difference in initdb (but may be a bit cleaner), but I
see that there's a similar coding in createdb(), which seems like it could leak
some memory according to ucol_close man page.

On Fri, Mar 18, 2022 at 4:12 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Fri, Mar 18, 2022 at 11:01:11AM +0900, Michael Paquier wrote:

FYI, prion is complaining here:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prion&amp;dt=2022-03-18%2001%3A43%3A13

Some details:
# Failed test 'fails for invalid ICU locale: matches'
# at t/001_initdb.pl line 107.
# '2022-03-18 01:54:58.563 UTC [504] FATAL: could
# not open collator for locale "@colNumeric=lower":
# U_ILLEGAL_ARGUMENT_ERROR

That's very strange, apparently initdb doesn't detect any problem when checking
ucol_open() for initial checks, since it's expecting:

# doesn't match '(?^:initdb: error: could not open collator for locale)'

but then postgres in single-backend mode does detect the problem, with the
exact same check, so it's not like --icu-locale=@colNumeric=lower wasn't
correctly interpreted. Unfortunately we don't have the full initdb output, so
we can't check what setup_locale_encoding reported. The only difference is
that in initdb's setlocale(), the result of ucol_open is discarded, maybe the
compiler is optimizing it away for some reason, even though it seems really
unlikely.

That being said, we could save the result and explicitly close the collator.
That wouldn't make much difference in initdb (but may be a bit cleaner), but I
see that there's a similar coding in createdb(), which seems like it could leak
some memory according to ucol_close man page.

No idea what's happening here but one observation is that that animal
is running an older distro that shipped with ICU 5.0. Commit b8f9a2a6
may hold a clue...

On Fri, Mar 18, 2022 at 06:15:47PM +1300, Thomas Munro wrote:

No idea what's happening here but one observation is that that animal
is running an older distro that shipped with ICU 5.0. Commit b8f9a2a6
may hold a clue...

Right. I'm setting up a similar podman environment, hopefully more info soon.

On Fri, Mar 18, 2022 at 02:36:48PM +0800, Julien Rouhaud wrote:

On Fri, Mar 18, 2022 at 06:15:47PM +1300, Thomas Munro wrote:

No idea what's happening here but one observation is that that animal
is running an older distro that shipped with ICU 5.0. Commit b8f9a2a6
may hold a clue...

Right. I'm setting up a similar podman environment, hopefully more info soon.

And indeed b8f9a2a6 is the problem. We would need some form of
icu_set_collation_attributes() on the frontend side if we want to detect such a
problem on older ICU version at the expected moment rather than when
bootstrapping the info. A similar check is also needed in createdb().

I was thinking that this could be the cause of problem reported by Andres on
centos 7 (which seems to ship ICU 50), but postinit.c calls
make_icu_collator(), which sets the attribute as expected. Maybe it's because
old ICU version simply don't understand locale ID like "en-u-kf-upper" and
silently falls back to the root collator?

(moving to -hackers)

On Fri, Mar 18, 2022 at 03:40:51PM +0800, Julien Rouhaud wrote:

On Fri, Mar 18, 2022 at 02:36:48PM +0800, Julien Rouhaud wrote:

On Fri, Mar 18, 2022 at 06:15:47PM +1300, Thomas Munro wrote:

No idea what's happening here but one observation is that that animal
is running an older distro that shipped with ICU 5.0. Commit b8f9a2a6
may hold a clue...

Right. I'm setting up a similar podman environment, hopefully more info soon.

And indeed b8f9a2a6 is the problem. We would need some form of
icu_set_collation_attributes() on the frontend side if we want to detect such a
problem on older ICU version at the expected moment rather than when
bootstrapping the info. A similar check is also needed in createdb().

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.

Julien Rouhaud <rjuju123@gmail.com> writes:

That being said, we could save the result and explicitly close the collator.
That wouldn't make much difference in initdb (but may be a bit cleaner), but I
see that there's a similar coding in createdb(), which seems like it could leak
some memory according to ucol_close man page.

FYI, I verified using valgrind that (as of HEAD) there is a leak
when creating a database with ICU collation that doesn't appear
when creating one with libc collation. It's not a lot, a few
hundred bytes per iteration, but it's there.

regards, tom lane

I found a different problem with src/test/icu/: it fails altogether
if the prevailing locale is "C", because then the database encoding
defaults to SQL_ASCII which our ICU code won't cope with. I'm not
sure if that explains any of the buildfarm failures, but it broke
my local build (yeah, I'm that guy). I un-broke it for the moment
by forcing the test to use UTF8 encoding, but do we want to do
anything smarter than that?

regards, tom lane

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?

On 18.03.22 18:29, Tom Lane wrote:

I found a different problem with src/test/icu/: it fails altogether
if the prevailing locale is "C", because then the database encoding
defaults to SQL_ASCII which our ICU code won't cope with. I'm not
sure if that explains any of the buildfarm failures, but it broke
my local build (yeah, I'm that guy). I un-broke it for the moment
by forcing the test to use UTF8 encoding, but do we want to do
anything smarter than that?

This is an appropriate solution, I think.

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]/messages/by-id/20220318000140.vzri3qw3p4aebn5p@alap3.anarazel.de 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]: /messages/by-id/20220318000140.vzri3qw3p4aebn5p@alap3.anarazel.de

Re: Peter Eisentraut

Since some (legacy) code still uses the libc locale facilities
directly, we still need to set the libc global locale settings even if
ICU is otherwise selected. So pg_database now has three
locale-related fields: the existing datcollate and datctype, which are
always set, and a new daticulocale, which is only set if ICU is
selected. A similar change is made in pg_collation for consistency,
but in that case, only the libc-related fields or the ICU-related
field is set, never both.

Since the intended usage seems to be that databases should either be
using libc, or the ICU locales, but probably not both at the same
time, does it make sense to clutter the already very wide `psql -l`
output with two new extra columns?

This hardly fits in normal-size terminals:

=# \l
List of databases
Name │ Owner │ Encoding │ Collate │ Ctype │ ICU Locale │ Locale Provider │ Access privileges
───────────┼───────┼──────────┼────────────┼────────────┼────────────┼─────────────────┼───────────────────
postgres │ myon │ UTF8 │ de_DE.utf8 │ de_DE.utf8 │ │ libc │
template0 │ myon │ UTF8 │ de_DE.utf8 │ de_DE.utf8 │ │ libc │ =c/myon ↵
│ │ │ │ │ │ │ myon=CTc/myon
template1 │ myon │ UTF8 │ de_DE.utf8 │ de_DE.utf8 │ │ libc │ =c/myon ↵
│ │ │ │ │ │ │ myon=CTc/myon
(3 rows)

(Even longer if the username is "postgres")

It also makes \l+ even harder to read when the most often only
relevant new column, the database size, is even more to the far right.

Couldn't that be a single "Locale" column, possibly extended by more
info in parentheses if the values differ?

Locale
de_DE.utf8
de-x-icu-whatever
de_DE.utf8 (Ctype: C.UTF-8)
SQL_ASCII (ICU Locale: en-x-something)

Christoph

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 19.03.22 18:53, Christoph Berg wrote:

Re: Peter Eisentraut

Since some (legacy) code still uses the libc locale facilities
directly, we still need to set the libc global locale settings even if
ICU is otherwise selected. So pg_database now has three
locale-related fields: the existing datcollate and datctype, which are
always set, and a new daticulocale, which is only set if ICU is
selected. A similar change is made in pg_collation for consistency,
but in that case, only the libc-related fields or the ICU-related
field is set, never both.

Since the intended usage seems to be that databases should either be
using libc, or the ICU locales, but probably not both at the same
time, does it make sense to clutter the already very wide `psql -l`
output with two new extra columns?

Good point, let me think about that.

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