From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Marina Polyakova <m.polyakova@postgrespro.ru>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com
Cc: rjuju123@gmail.com
Cc: daniel@manitou-mail.org
Cc: AndrewBille@gmail.com
Cc: michael@paquier.xyz
Subject: Re: ICU for global collation
Date: Tue, 20 Sep 2022 05:59:04 -0400
Message-ID: <218b963e-dd66-626a-abd8-962afb587876@enterprisedb.com> (raw)
In-Reply-To: <9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru>
References: <1f86f120-7b02-68f6-a434-132d054afe33@enterprisedb.com>
<38d2aac40a991b20adf032e509c6eff5@postgrespro.ru>
<6dd6db0984d86a51b7255ba79f111971@postgrespro.ru>
<20220915.155210.529619933474389138.horikyota.ntt@gmail.com>
<a739cd973cd3c911d9bf582b6febba1c@postgrespro.ru>
<65ef1a8a-1fc3-3dc3-c7aa-6b9f649014b2@enterprisedb.com>
<9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru>
On 17.09.22 10:33, Marina Polyakova wrote:
> Thanks to Kyotaro Horiguchi review we found out that there're
> interesting cases due to the order of some ICU checks:
>
> 1. ICU locale vs supported encoding:
>
> 1.1.
>
> On 2022-09-15 09:52, Kyotaro Horiguchi wrote:
>> If I executed initdb as follows, I would be told to specify
>> --icu-locale option.
>>
>>> $ initdb --encoding sql-ascii --locale-provider icu hoge
>>> ...
>>> initdb: error: ICU locale must be specified
>>
>> However, when I reran the command, it complains about incompatible
>> encoding this time. I think it's more user-friendly to check for the
>> encoding compatibility before the check for missing --icu-locale
>> option.
This a valid point, but it would require quite a bit of work to move all
those checks around and re-verify the result, so I don't want to do it
in PG15.
> 1.2. (ok?)
>
> $ initdb --encoding sql-ascii --icu-locale en-US hoge
> initdb: error: --icu-locale cannot be specified unless locale provider
> "icu" is chosen
>
> $ initdb --encoding sql-ascii --icu-locale en-US --locale-provider icu hoge
> ...
> initdb: error: encoding mismatch
> initdb: detail: The encoding you selected (SQL_ASCII) is not supported
> with the ICU provider.
> initdb: hint: Rerun initdb and either do not specify an encoding
> explicitly, or choose a matching combination.
>
> $ createdb --encoding sql-ascii --icu-locale en-US hoge
> createdb: error: database creation failed: ERROR: ICU locale cannot be
> specified unless locale provider is ICU
> $ createdb --encoding sql-ascii --icu-locale en-US --locale-provider icu
> hoge
> createdb: error: database creation failed: ERROR: encoding "SQL_ASCII"
> is not supported with ICU provider
I don't see a problem here.
> 2. For builds without ICU:
>
> 2.1.
>
> $ initdb --locale-provider icu hoge
> ...
> initdb: error: ICU locale must be specified
>
> $ initdb --locale-provider icu --icu-locale en-US hoge
> ...
> initdb: error: ICU is not supported in this build
>
> $ createdb --locale-provider icu hoge
> createdb: error: database creation failed: ERROR: ICU locale must be
> specified
>
> $ createdb --locale-provider icu --icu-locale en-US hoge
> createdb: error: database creation failed: ERROR: ICU is not supported
> in this build
>
> IMO, it would be more user-friendly to inform an unsupported build in
> the first runs too..
Again, this would require reorganizing a bunch of code to get some
cosmetic benefit, which isn't a good idea now for PG15.
> 2.2. (ok?)
> 2.3.
same here
> 3.
>
> The locale provider is ICU, but it has not yet been set from the
> template database:
>
>> $ initdb --locale-provider icu --icu-locale en-US -D data &&
>> pg_ctl -D data -l logfile start &&
>> createdb --icu-locale ru-RU --template template0 mydb
>> ...
>> createdb: error: database creation failed: ERROR: ICU locale cannot be
>> specified unless locale provider is ICU
Please see attached patch for a fix. Does that work for you?
From 5bb07e390baf8a471f7d30582469268c8a6d71ca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 20 Sep 2022 05:50:41 -0400
Subject: [PATCH] Improve ICU option handling in CREATE DATABASE
We check that the ICU locale is only specified if the ICU locale
provider is selected. But we did that too early. We need to wait
until we load the settings of the template database, since that could
also set what the locale provider is.
Reported-by: Marina Polyakova <m.polyakova@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru
---
src/backend/commands/dbcommands.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.cindex cba929b7f998..5dfec5c6b056 100644--- a/src/backend/commands/dbcommands.c+++ b/src/backend/commands/dbcommands.c@@ -907,10 +907,6 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
errmsg("unrecognized locale provider: %s",
locproviderstr)));
}
- if (diculocale && dblocprovider != COLLPROVIDER_ICU)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("ICU locale cannot be specified unless locale provider is ICU")));
if (distemplate && distemplate->arg)
dbistemplate = defGetBoolean(distemplate);
if (dallowconnections && dallowconnections->arg)
@@ -1050,6 +1046,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
check_icu_locale(dbiculocale);
}
+ else+ {+ if (dbiculocale)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("ICU locale cannot be specified unless locale provider is ICU")));+ }
/*
* Check that the new encoding and locale settings match the source
--
2.37.3
Attachments:
[text/plain] 0001-Improve-ICU-option-handling-in-CREATE-DATABASE.patch (1.8K, ../218b963e-dd66-626a-abd8-962afb587876@enterprisedb.com/2-0001-Improve-ICU-option-handling-in-CREATE-DATABASE.patch)
download | inline diff:
From 5bb07e390baf8a471f7d30582469268c8a6d71ca Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 20 Sep 2022 05:50:41 -0400
Subject: [PATCH] Improve ICU option handling in CREATE DATABASE
We check that the ICU locale is only specified if the ICU locale
provider is selected. But we did that too early. We need to wait
until we load the settings of the template database, since that could
also set what the locale provider is.
Reported-by: Marina Polyakova <m.polyakova@postgrespro.ru>
Discussion: https://www.postgresql.org/message-id/9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru
---
src/backend/commands/dbcommands.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.cindex cba929b7f998..5dfec5c6b056 100644--- a/src/backend/commands/dbcommands.c+++ b/src/backend/commands/dbcommands.c@@ -907,10 +907,6 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
errmsg("unrecognized locale provider: %s",
locproviderstr)));
}
- if (diculocale && dblocprovider != COLLPROVIDER_ICU)- ereport(ERROR,- (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),- errmsg("ICU locale cannot be specified unless locale provider is ICU")));
if (distemplate && distemplate->arg)
dbistemplate = defGetBoolean(distemplate);
if (dallowconnections && dallowconnections->arg)
@@ -1050,6 +1046,13 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt)
check_icu_locale(dbiculocale);
}
+ else+ {+ if (dbiculocale)+ ereport(ERROR,+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),+ errmsg("ICU locale cannot be specified unless locale provider is ICU")));+ }
/*
* Check that the new encoding and locale settings match the source
--
2.37.3
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter.eisentraut@enterprisedb.com, m.polyakova@postgrespro.ru, horikyota.ntt@gmail.com, pryzby@telsasoft.com, rjuju123@gmail.com, daniel@manitou-mail.org, AndrewBille@gmail.com, michael@paquier.xyz
Subject: Re: ICU for global collation
In-Reply-To: <218b963e-dd66-626a-abd8-962afb587876@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox