Thread

Commits

psql: fix \l display for pre-v15 databases.
- 7129a9791eaf 15.0 landed
Fix global ICU collations for ICU < 54
- 3a671e1f7cb8 15.0 landed
Add option to use ICU as global locale provider
- f2553d43060e 15.0 cited
Add support for collation attributes on older ICU versions
- b8f9a2a69a27 12.0 cited

pgsql: Add option to use ICU as global locale provider

Peter Eisentraut <peter@eisentraut.org> — 2022-03-17T10:22:32Z

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.

Reviewed-by: Julien Rouhaud <rjuju123@gmail.com>
Discussion: https://www.postgresql.org/message-id/flat/5e756dd6-0e91-d778-96fd-b1bcb06c161a%402ndquadrant.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/f2553d43060edb210b36c63187d52a632448e1d2

Modified Files
--------------
doc/src/sgml/catalogs.sgml                     |   9 ++
doc/src/sgml/charset.sgml                      | 102 ++++++++++++++++
doc/src/sgml/ref/create_database.sgml          |  32 +++++
doc/src/sgml/ref/createdb.sgml                 |  19 +++
doc/src/sgml/ref/initdb.sgml                   |  72 +++++++++---
src/backend/catalog/pg_collation.c             |  18 ++-
src/backend/commands/collationcmds.c           |  97 +++++++++------
src/backend/commands/dbcommands.c              | 157 +++++++++++++++++++++----
src/backend/utils/adt/pg_locale.c              | 144 ++++++++++++++---------
src/backend/utils/init/postinit.c              |  21 +++-
src/bin/initdb/Makefile                        |   4 +-
src/bin/initdb/initdb.c                        |  97 +++++++++++++--
src/bin/initdb/t/001_initdb.pl                 |  27 +++++
src/bin/pg_dump/pg_dump.c                      |  30 ++++-
src/bin/pg_upgrade/check.c                     |  13 ++
src/bin/pg_upgrade/info.c                      |  18 ++-
src/bin/pg_upgrade/pg_upgrade.h                |   2 +
src/bin/psql/describe.c                        |  23 +++-
src/bin/psql/tab-complete.c                    |   3 +-
src/bin/scripts/Makefile                       |   2 +
src/bin/scripts/createdb.c                     |  20 ++++
src/bin/scripts/t/020_createdb.pl              |  28 +++++
src/include/catalog/catversion.h               |   2 +-
src/include/catalog/pg_collation.dat           |   3 +-
src/include/catalog/pg_collation.h             |  20 +++-
src/include/catalog/pg_database.dat            |   4 +-
src/include/catalog/pg_database.h              |   6 +
src/include/utils/pg_locale.h                  |   5 +
src/test/Makefile                              |   6 +-
src/test/icu/.gitignore                        |   2 +
src/test/icu/Makefile                          |  25 ++++
src/test/icu/README                            |  27 +++++
src/test/icu/t/010_database.pl                 |  58 +++++++++
src/test/regress/expected/collate.icu.utf8.out |  10 +-
src/test/regress/sql/collate.icu.utf8.sql      |   8 +-
35 files changed, 947 insertions(+), 167 deletions(-)

Re: pgsql: Add option to use ICU as global locale provider

Michael Paquier <michael@paquier.xyz> — 2022-03-18T02:01:11Z

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

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-18T03:12:44Z

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.

Re: pgsql: Add option to use ICU as global locale provider

Thomas Munro <thomas.munro@gmail.com> — 2022-03-18T05:15:47Z

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&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...

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-18T06:36:48Z

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.

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-18T07:40:51Z

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?

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-18T09:27:49Z

(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.

Re: pgsql: Add option to use ICU as global locale provider

Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-18T15:07:04Z

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

Re: pgsql: Add option to use ICU as global locale provider

Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-18T17:29:41Z

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

Re: pgsql: Add option to use ICU as global locale provider

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-18T19:28:58Z

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?

Re: pgsql: Add option to use ICU as global locale provider

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-18T19:30:21Z

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.

Re: pgsql: Add option to use ICU as global locale provider

Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-18T20:04:10Z

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

Re: pgsql: Add option to use ICU as global locale provider

Andres Freund <andres@anarazel.de> — 2022-03-18T22:09:59Z

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

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-19T00:59:14Z

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.

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-19T04:14:59Z

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

Re: pgsql: Add option to use ICU as global locale provider

Christoph Berg <myon@debian.org> — 2022-03-19T17:53:30Z

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

Re: pgsql: Add option to use ICU as global locale provider

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-20T10:03:38Z

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

Re: pgsql: Add option to use ICU as global locale provider

Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-20T10:04:17Z

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.

Re: pgsql: Add option to use ICU as global locale provider

Julien Rouhaud <rjuju123@gmail.com> — 2022-03-20T11:46:26Z

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!

Re: pgsql: Add option to use ICU as global locale provider

Andres Freund <andres@anarazel.de> — 2022-03-20T16:31:34Z

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

Re: pgsql: Add option to use ICU as global locale provider

Christoph Berg <myon@debian.org> — 2022-03-21T16:59:56Z

Re: Peter Eisentraut
> > 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.

A possible solution might be to rip out all the locale columns except
"Encoding" from \l, and leave them in place for \l+.

For \l+, I'd suggest moving the database size and the tablespace to
the front, after owner.

Christoph

Re: pgsql: Add option to use ICU as global locale provider

Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-21T18:37:12Z

Christoph Berg <myon@debian.org> writes:
> A possible solution might be to rip out all the locale columns except
> "Encoding" from \l, and leave them in place for \l+.

I'd rather see a single column summarizing the locale situation.
Perhaps it could be COALESCE(daticulocale, datcollate), or
something using a CASE on datlocprovider?
Then \l+ could replace that with all the underlying columns.

> For \l+, I'd suggest moving the database size and the tablespace to
> the front, after owner.

I think it's confusing if the + and non-+ versions of a command
present their columns in inconsistent orders.  I'm not dead set
against this, but -0.5 or so.

			regards, tom lane

Inconsistent "ICU Locale" output on older server versions

Christoph Berg <myon@debian.org> — 2022-04-15T14:58:28Z

Re: To Peter Eisentraut
> 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)

Another gripe here: The above is the output when run against a PG15
cluster, created without an ICU locale set.

When running psql 15 against PG 14, the output is this:

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

The "ICU Locale" column is now populated, that seems wrong.

The problem is in the else branch in src/bin/psql/describe.c around
line 900:

+   if (pset.sversion >= 150000)
+       appendPQExpBuffer(&buf,
+                         "       d.daticulocale as \"%s\",\n"
+                         "       CASE d.datlocprovider WHEN 'c' THEN 'libc' WHEN 'i' THEN 'icu' END AS \"%s\",\
+                         gettext_noop("ICU Locale"),
+                         gettext_noop("Locale Provider"));
+   else
+       appendPQExpBuffer(&buf,
+                         "       d.datcollate as \"%s\",\n"  <--- there
+                         "       'libc' AS \"%s\",\n",
+                         gettext_noop("ICU Locale"),
+                         gettext_noop("Locale Provider"));

I'd think this should rather be

+                         "       '' as \"%s\",\n"

Christoph

Re: Inconsistent "ICU Locale" output on older server versions

Euler Taveira <euler@eulerto.com> — 2022-04-15T15:48:30Z

On Fri, Apr 15, 2022, at 11:58 AM, Christoph Berg wrote:
> When running psql 15 against PG 14, the output is this:
> 
> $ psql -l
>                                                 List of databases
>    Name    │  Owner   │ Encoding │  Collate   │   Ctype    │ ICU Locale │ Locale Provider │   Access privileges
> ───────────┼──────────┼──────────┼────────────┼────────────┼────────────┼─────────────────┼───────────────────────
> postgres  │ postgres │ UTF8     │ de_DE.utf8 │ de_DE.utf8 │ de_DE.utf8 │ libc            │
> template0 │ postgres │ UTF8     │ de_DE.utf8 │ de_DE.utf8 │ de_DE.utf8 │ libc            │ =c/postgres          ↵
>            │          │          │            │            │            │                 │ postgres=CTc/postgres
> template1 │ postgres │ UTF8     │ de_DE.utf8 │ de_DE.utf8 │ de_DE.utf8 │ libc            │ =c/postgres          ↵
>            │          │          │            │            │            │                 │ postgres=CTc/postgres
> (3 rows)
> 
> The "ICU Locale" column is now populated, that seems wrong.
Good catch!

> The problem is in the else branch in src/bin/psql/describe.c around
> line 900:
> 
> +   if (pset.sversion >= 150000)
> +       appendPQExpBuffer(&buf,
> +                         "       d.daticulocale as \"%s\",\n"
> +                         "       CASE d.datlocprovider WHEN 'c' THEN 'libc' WHEN 'i' THEN 'icu' END AS \"%s\",\
> +                         gettext_noop("ICU Locale"),
> +                         gettext_noop("Locale Provider"));
> +   else
> +       appendPQExpBuffer(&buf,
> +                         "       d.datcollate as \"%s\",\n"  <--- there
> +                         "       'libc' AS \"%s\",\n",
> +                         gettext_noop("ICU Locale"),
> +                         gettext_noop("Locale Provider"));
> 
> I'd think this should rather be
> 
> +                         "       '' as \"%s\",\n"
Since dataiculocale allows NULL, my suggestion is to use NULL instead of an
empty string. It is consistent with a cluster whose locale provider is libc.


--
Euler Taveira
EDB   https://www.enterprisedb.com/

Re: Inconsistent "ICU Locale" output on older server versions

Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-15T16:47:42Z

"Euler Taveira" <euler@eulerto.com> writes:
> On Fri, Apr 15, 2022, at 11:58 AM, Christoph Berg wrote:
>> When running psql 15 against PG 14, the output is this:
>> The "ICU Locale" column is now populated, that seems wrong.

> Good catch!

Indeed.

> Since dataiculocale allows NULL, my suggestion is to use NULL instead of an
> empty string. It is consistent with a cluster whose locale provider is libc.

Yeah, I agree.  We should make the pre-v15 output match what you'd see
if looking at a non-ICU v15 database.

			regards, tom lane

psql -l and locales (Re: pgsql: Add option to use ICU as global locale provider)

Christoph Berg <myon@debian.org> — 2022-09-06T12:25:26Z

Re: Tom Lane
> Christoph Berg <myon@debian.org> writes:
> > A possible solution might be to rip out all the locale columns except
> > "Encoding" from \l, and leave them in place for \l+.
> 
> I'd rather see a single column summarizing the locale situation.
> Perhaps it could be COALESCE(daticulocale, datcollate), or
> something using a CASE on datlocprovider?
> Then \l+ could replace that with all the underlying columns.

Fwiw I still think the default psql -l output should be more concise.
Any chance to have that happen for PG15?

I can try creating a patch if it has chances of getting through.

Christoph