Fix order of checking ICU options in initdb and create database
Hello! This is the last proposed patch on this subject [1] moved to a separate thread for Commitfest.. It looks like that the current order of checking ICU options in initdb and create database in PG 15+ is not user-friendly. Examples: 1. initdb reports a missing ICU locale although it may already report that the selected encoding cannot be used: $ initdb --encoding sql-ascii --locale-provider icu hoge ... initdb: error: ICU locale must be specified $ initdb --encoding sql-ascii --locale-provider icu --icu-locale en-US 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. 2. initdb/create database report problems with the ICU locale/encoding although they may already report that ICU is not supported in this build: 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 2.2. $ createdb --locale-provider icu --icu-locale en-US --encoding sql-ascii hoge createdb: error: database creation failed: ERROR: encoding "SQL_ASCII" is not supported with ICU provider $ createdb --locale-provider icu --icu-locale en-US --encoding utf8 hoge createdb: error: database creation failed: ERROR: ICU is not supported in this build The patch v1-0001-Fix-order-of-checking-ICU-options-in-initdb-and-c.patch fixes this: 1. $ initdb --encoding sql-ascii --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. 2.1. $ initdb --locale-provider icu hoge ... initdb: error: ICU is not supported in this build $ createdb --locale-provider icu hoge createdb: error: database creation failed: ERROR: ICU is not supported in this build 2.2. $ createdb --locale-provider icu --icu-locale en-US --encoding sql-ascii hoge createdb: error: database creation failed: ERROR: ICU is not supported in this build A side effect of the proposed patch in initdb is that if ICU locale is missed (or ICU is not supported in this build), the provider, locales and encoding are reported before the error message: $ initdb --locale-provider icu hoge The files belonging to this database system will be owned by user "marina". This user must also own the server process. The database cluster will be initialized with this locale configuration: provider:icu LC_COLLATE: en_US.UTF-8 LC_CTYPE:en_US.UTF-8 LC_MESSAGES: en_US.UTF-8 LC_MONETARY: ru_RU.UTF-8 LC_NUMERIC: ru_RU.UTF-8 LC_TIME: ru_RU.UTF-8 The default database encoding has been set to "UTF8". initdb: error: ICU locale must be specified $ initdb --locale-provider icu hoge The files belonging to this database system will be owned by user "marina". This user must also own the server process. The database cluster will be initialized with this locale configuration: provider:icu LC_COLLATE: en_US.UTF-8 LC_CTYPE:en_US.UTF-8 LC_MESSAGES: en_US.UTF-8 LC_MONETARY: ru_RU.UTF-8 LC_NUMERIC: ru_RU.UTF-8 LC_TIME: ru_RU.UTF-8 The default database encoding has been set to "UTF8". initdb: error: ICU is not supported in this build I was thinking about another master-only version of the patch that first checks everything for provider, locales and encoding but IMO it is worse [2].. [1] https://www.postgresql.org/message-id/e94aca035bf0b92fac42d204ad385552%40postgrespro.ru [2] https://www.postgresql.org/message-id/79f410460c4fc9534000785adb8bf39a%40postgrespro.ru -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres CompanyFrom da761095a7e39463ec6083484c4b1b924cacf387 Mon Sep 17 00:00:00 2001 From: Marina Polyakova Date: Sat, 29 Oct 2022 14:25:05 +0300 Subject: [PATCH v1] Fix order of checking ICU options in initdb and create database First check if ICU is supported in this build. Then check that the selected encoding is supported BY ICU (the encoding can be set from lc_ctype which can be set from an environment variable). Only then check the option for the ICU locale. --- src/backend/commands/dbcommands.c | 6 ++ src/backend/utils/adt/pg_locale.c | 10 ++--- src/bin/initdb/initdb.c | 36 ++- src/bin/initdb/t/001_initdb.pl| 21 ++ src/bin/scripts/t/020_create
Re: Fix order of checking ICU options in initdb and create database
Is this feedback enough to focus the work on the right things? I feel like Marina Polyakova pointed out some real confusing behaviour and perhaps there's a way to solve them by focusing on one at a time without making large changes in the code. Perhaps an idea would be to have each module provide two functions, one which is called early and signals an error if that module's parameters are provided when it's not compiled in, and a second which verifies that the parameters are consistent at the point in time where that's appropriate. (Not entirely unlike what we do for GUCs, though simpler) If every module did that consistently then it would avoid making the changes "unprincipled" or "spaghetti" though frankly I find words like that not very helpful to someone receiving that feedback. The patch is obviously not ready for commit now but it also seems like the feedback has not been really sufficient for Marina Polyakova to make progress either. -- Gregory Stark As Commitfest Manager
Re: Fix order of checking ICU options in initdb and create database
On 2022-10-29 14:33, Marina Polyakova wrote: Hello! This is the last proposed patch on this subject [1] moved to a separate thread for Commitfest.. Also added a patch to export with_icu when running src/bin/scripts tests [1]. The problem can be reproduced as $ meson setup && ninja && meson test --print-errorlogs --suite setup --suite scripts [1] https://cirrus-ci.com/task/4825664661487616 -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres CompanyFrom a6de5922fc533c88c7288051d8797d021ae91dae Mon Sep 17 00:00:00 2001 From: Marina Polyakova Date: Sat, 29 Oct 2022 14:25:05 +0300 Subject: [PATCH v1 2/2] Fix order of checking ICU options in initdb and create database First check if ICU is supported in this build. Then check that the selected encoding is supported BY ICU (the encoding can be set from lc_ctype which can be set from an environment variable). Only then check the option for the ICU locale. --- src/backend/commands/dbcommands.c | 6 ++ src/backend/utils/adt/pg_locale.c | 10 ++--- src/bin/initdb/initdb.c | 36 ++- src/bin/initdb/t/001_initdb.pl| 21 ++ src/bin/scripts/t/020_createdb.pl | 31 ++ src/include/utils/pg_locale.h | 2 +- 6 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 16e422138b..4c1e79fca3 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1030,6 +1030,11 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) if (dblocprovider == COLLPROVIDER_ICU) { +#ifndef USE_ICU + ereport(ERROR, +(errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("ICU is not supported in this build"))); +#else if (!(is_encoding_supported_by_icu(encoding))) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -1046,6 +1051,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) errmsg("ICU locale must be specified"))); check_icu_locale(dbiculocale); +#endif } else { diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 2b42d9ccd8..743f11e1d1 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1945,15 +1945,12 @@ icu_set_collation_attributes(UCollator *collator, const char *loc) } } -#endif /* USE_ICU */ - /* * Check if the given locale ID is valid, and ereport(ERROR) if it isn't. */ void check_icu_locale(const char *icu_locale) { -#ifdef USE_ICU UCollator *collator; UErrorCode status; @@ -1967,13 +1964,10 @@ check_icu_locale(const char *icu_locale) if (U_ICU_VERSION_MAJOR_NUM < 54) icu_set_collation_attributes(collator, icu_locale); ucol_close(collator); -#else - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("ICU is not supported in this build"))); -#endif } +#endif /* USE_ICU */ + /* * These functions convert from/to libc's wchar_t, *not* pg_wchar_t. * Therefore we keep them here rather than with the mbutils code. diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index f61a043055..82e6644f89 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2047,9 +2047,12 @@ check_locale_encoding(const char *locale, int user_enc) * * this should match the similar check in the backend createdb() function */ -static bool +static void check_icu_locale_encoding(int user_enc) { +#ifndef USE_ICU + pg_fatal("ICU is not supported in this build"); +#else if (!(is_encoding_supported_by_icu(user_enc))) { pg_log_error("encoding mismatch"); @@ -2058,9 +2061,17 @@ check_icu_locale_encoding(int user_enc) pg_log_error_hint("Rerun %s and either do not specify an encoding explicitly, " "or choose a matching combination.", progname); - return false; + exit(1); } - return true; + + if (!icu_locale) + pg_fatal("ICU locale must be specified"); + + /* + * In supported builds, the ICU locale ID will be checked by the backend + * during post-bootstrap initialization. + */ +#endif } /* @@ -2113,20 +2124,6 @@ setlocales(void) check_locale_name(LC_CTYPE, lc_messages, &canonname); lc_messages = canonname; #endif - - if (locale_provider == COLLPROVIDER_ICU) - { - if (!icu_locale) - pg_fatal("ICU locale must be specified"); - - /* - * In supported builds, the ICU locale ID will be checked by the - * backend during post-bootstrap initialization. - */ -#ifndef USE_ICU - pg_fatal("ICU is not supported in this build"); -#endif - } } /* @@ -2388,9 +2385,8 @@ setup_locale_encoding(void) !check_locale_encoding(lc_collate, encodingid)) exit(1);/* check_locale_encoding printed the error */ - if (locale_provider == COLLPROVIDER_ICU &&a
Re: Fix order of checking ICU options in initdb and create database
Hello Marina. I just reviewed your patch. It applied cleanly at my current master (commit d6a3dbe14f98d867b2fc3faeb99d2d3c2a48ca67). Also, it worked as described in email. Since it's a clarification in an error message, I think the documentation is fine. I played a bit with "make check", creating a database in my native language (pt_BR), testing with some data and everything worked as expected. -- Jose Arthur Benetasso Villanova
Re: Fix order of checking ICU options in initdb and create database
On 2022-11-12 22:43, Jose Arthur Benetasso Villanova wrote: Hello Marina. I just reviewed your patch. It applied cleanly at my current master (commit d6a3dbe14f98d867b2fc3faeb99d2d3c2a48ca67). Also, it worked as described in email. Since it's a clarification in an error message, I think the documentation is fine. I played a bit with "make check", creating a database in my native language (pt_BR), testing with some data and everything worked as expected. Hello! Thank you! -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Fix order of checking ICU options in initdb and create database
On 29.10.22 15:09, Marina Polyakova wrote: On 2022-10-29 14:33, Marina Polyakova wrote: Hello! This is the last proposed patch on this subject [1] moved to a separate thread for Commitfest.. Also added a patch to export with_icu when running src/bin/scripts tests [1]. I have committed the meson change.
Re: Fix order of checking ICU options in initdb and create database
On 29.10.22 13:33, Marina Polyakova wrote: 2. initdb/create database report problems with the ICU locale/encoding although they may already report that ICU is not supported in this build: 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 I'm not in favor of changing this. The existing code intentionally tries to centralize the "ICU is not supported in this build" knowledge in few places. Your patch tries to make this check early, but in the process adds more places where ICU support needs to be checked explicitly. This increases the code size and also creates a future burden to maintain that level of checking. I think building without ICU should be considered a marginal configuration at this point, so we don't need to go out of our way to create a perfect user experience for this configuration, as long as we check somewhere in the end.
Re: Fix order of checking ICU options in initdb and create database
чт, 17 нояб. 2022 г. в 09:54, Peter Eisentraut : > > On 29.10.22 15:09, Marina Polyakova wrote: > > On 2022-10-29 14:33, Marina Polyakova wrote: > >> Hello! > >> > >> This is the last proposed patch on this subject [1] moved to a > >> separate thread for Commitfest.. > > > > Also added a patch to export with_icu when running src/bin/scripts tests > > [1]. > > I have committed the meson change. Thank you! (Sorry, I'm having problems sending emails from corporate email :( ) -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Fix order of checking ICU options in initdb and create database
чт, 17 нояб. 2022 г. в 09:58, Peter Eisentraut : > > On 29.10.22 13:33, Marina Polyakova wrote: > > 2. initdb/create database report problems with the ICU locale/encoding > > although they may already report that ICU is not supported in this build: > > > > 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 > > I'm not in favor of changing this. The existing code intentionally > tries to centralize the "ICU is not supported in this build" knowledge > in few places. Your patch tries to make this check early, but in the > process adds more places where ICU support needs to be checked > explicitly. This increases the code size and also creates a future > burden to maintain that level of checking. I think building without ICU > should be considered a marginal configuration at this point, so we don't > need to go out of our way to create a perfect user experience for this > configuration, as long as we check somewhere in the end. Maybe this should be written in the documentation [1] or --with-icu should be used by default? As a developer I usually check something with the simplest configure run to make sure other options do not affect the checked behaviour. And some other developers in our company also use simple configure runs, without --with-icu etc. [1] https://www.postgresql.org/docs/15/install-procedure.html -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Fix order of checking ICU options in initdb and create database
On 19.11.22 13:12, Марина Полякова wrote: I'm not in favor of changing this. The existing code intentionally tries to centralize the "ICU is not supported in this build" knowledge in few places. Your patch tries to make this check early, but in the process adds more places where ICU support needs to be checked explicitly. This increases the code size and also creates a future burden to maintain that level of checking. I think building without ICU should be considered a marginal configuration at this point, so we don't need to go out of our way to create a perfect user experience for this configuration, as long as we check somewhere in the end. Maybe this should be written in the documentation [1] or --with-icu should be used by default? As a developer I usually check something with the simplest configure run to make sure other options do not affect the checked behaviour. And some other developers in our company also use simple configure runs, without --with-icu etc. Well, this isn't a hard rule, just my opinion and where I see the world moving. It's similar to --with-openssl and --with-lz4 etc.
Re: Fix order of checking ICU options in initdb and create database
сб, 19 нояб. 2022 г. в 15:51, Peter Eisentraut : > > On 19.11.22 13:12, Марина Полякова wrote: > >> I'm not in favor of changing this. The existing code intentionally > >> tries to centralize the "ICU is not supported in this build" knowledge > >> in few places. Your patch tries to make this check early, but in the > >> process adds more places where ICU support needs to be checked > >> explicitly. This increases the code size and also creates a future > >> burden to maintain that level of checking. I think building without ICU > >> should be considered a marginal configuration at this point, so we don't > >> need to go out of our way to create a perfect user experience for this > >> configuration, as long as we check somewhere in the end. > > Maybe this should be written in the documentation [1] or --with-icu > > should be used by default? As a developer I usually check something > > with the simplest configure run to make sure other options do not > > affect the checked behaviour. And some other developers in our company > > also use simple configure runs, without --with-icu etc. > > Well, this isn't a hard rule, just my opinion and where I see the world > moving. It's similar to --with-openssl and --with-lz4 etc. Here is another set of proposed patches: v2-0001-Fix-encoding-check-in-initdb-when-the-option-icu-.patch Target: PG 15+ Fix encoding check in initdb when the option --icu-locale is not used: $ initdb --encoding sql-ascii --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. As with the previous version of this fix a side effect is that if ICU locale is missed (or ICU is not supported in this build), the provider, locales and encoding are reported before the error message: $ initdb --locale-provider icu hoge The files belonging to this database system will be owned by user "marina". This user must also own the server process. The database cluster will be initialized with this locale configuration: provider:icu LC_COLLATE: en_US.UTF-8 LC_CTYPE:en_US.UTF-8 LC_MESSAGES: en_US.UTF-8 LC_MONETARY: ru_RU.UTF-8 LC_NUMERIC: ru_RU.UTF-8 LC_TIME: ru_RU.UTF-8 The default database encoding has been set to "UTF8". initdb: error: ICU locale must be specified $ initdb --locale-provider icu --icu-locale en hoge The files belonging to this database system will be owned by user "marina". This user must also own the server process. The database cluster will be initialized with this locale configuration: provider:icu ICU locale: en LC_COLLATE: en_US.UTF-8 LC_CTYPE:en_US.UTF-8 LC_MESSAGES: en_US.UTF-8 LC_MONETARY: ru_RU.UTF-8 LC_NUMERIC: ru_RU.UTF-8 LC_TIME: ru_RU.UTF-8 The default database encoding has been set to "UTF8". initdb: error: ICU is not supported in this build v2-0002-doc-building-without-ICU-is-not-recommended.patch Target: PG 15+ Fix the documentation that --without-icu is a marginal configuration. v2-0003-Build-with-ICU-by-default.patch Target: PG 16+ Build with ICU by default as already done for readline and zlib libraries. -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company From b4dca9097633c585da232bc640f2517b3c443f09 Mon Sep 17 00:00:00 2001 From: Marina Polyakova Date: Sat, 19 Nov 2022 17:24:55 +0300 Subject: [PATCH v2 1/3] Fix encoding check in initdb when the option --icu-locale is not used First check that the selected encoding is supported by ICU (the encoding can be set from lc_ctype which can be set from an environment variable). Only then check the option for the ICU locale. --- src/bin/initdb/initdb.c | 35 +-- src/bin/initdb/t/001_initdb.pl| 17 ++- src/bin/scripts/t/020_createdb.pl | 26 ++- 3 files changed, 48 insertions(+), 30 deletions(-) diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index f61a043055..444c8a505a 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2047,7 +2047,7 @@ check_locale_encoding(const char *locale, int user_enc) * * this should match the similar check in the backend createdb() function */ -static bool +static void check_icu_locale_encoding(int user_enc) { if (!(is_encoding_supported_by_icu(user_enc))) @@ -2058,9 +2058,19 @@ check_icu_locale_encoding(int user_enc) pg_log_error_hint("Rerun %s and either do not specify an encoding explicitly, " "or choose a matching combination.", progname); - return false; + exit(1); } - return true; + + if (!icu_locale) + pg_fatal("ICU locale must be specified"); + + /* + * In supported builds, the ICU locale ID will be checked by the backend + * during post-bootstrap initialization. + */ +#ifndef USE_ICU + pg_fatal("ICU is not supported in this b
Re: Fix order of checking ICU options in initdb and create database
On 19.11.22 20:36, Марина Полякова wrote: Here is another set of proposed patches: v2-0001-Fix-encoding-check-in-initdb-when-the-option-icu-.patch Target: PG 15+ Fix encoding check in initdb when the option --icu-locale is not used: I'm having a hard time figuring out from your examples what you are trying to change. Which one is the "before" example and which one is the "after", and which aspect specifically is the issue and what specifically is being addressed? I tried out the examples in the current code and didn't find anything obviously wrong in the behavior. I'm concerned that the initdb.c changes are a bit unprincipled. They just move code around to achieve some behavior without keeping the overall structure in mind. For example, check_icu_locale_encoding() intentionally had the same API as check_locale_encoding(), but now that's being changed. And setlocales() did all the locale parameter validity checking, but now part of that is being moved around. I'm afraid this makes initdb.c even more spaghetti code than it already is. What about those test changes? I can't tell if they are related. createdb isn't being changed; is that test change related or separate? v2-0002-doc-building-without-ICU-is-not-recommended.patch Target: PG 15+ Fix the documentation that --without-icu is a marginal configuration. v2-0003-Build-with-ICU-by-default.patch Target: PG 16+ Build with ICU by default as already done for readline and zlib libraries. We are not going to make these kinds of changes specifically for ICU. I'd say, for example, the same applies to --with-openssl and --with-lz4, and probably others. If this is an issue, then we need a more general approach than just ICU. This should be a separate thread in any case.
Re: Fix order of checking ICU options in initdb and create database
This patch has now been waiting for author since December, with the thread stalled. I am marking this returned with feedback for now, please feel free to re-submit the patch to a future CF when there is renewed interest in working on this. -- Daniel Gustafsson