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 Company
From da761095a7e39463ec6083484c4b1b924cacf387 Mon Sep 17 00:00:00 2001
From: Marina Polyakova <m.polyak...@postgrespro.ru>
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_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 &&
- !check_icu_locale_encoding(encodingid))
- exit(1);
+ if (locale_provider == COLLPROVIDER_ICU)
+ check_icu_locale_encoding(encodingid);
}
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 164fc11cbf..884506a1bc 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -123,28 +123,41 @@ if ($ENV{with_icu} eq 'yes')
[
'initdb', '--no-sync',
'--locale-provider=icu', '--encoding=SQL_ASCII',
- '--icu-locale=en', "$tempdir/dataX"
+ "$tempdir/dataX"
],
qr/error: encoding mismatch/,
'fails for encoding not supported by ICU');
}
else
{
- command_fails(
+ command_fails_like(
[ 'initdb', '--no-sync', '--locale-provider=icu', "$tempdir/data2" ],
+ qr/error: ICU is not supported in this build/,
'locale provider ICU fails since no ICU support');
+
+ command_fails_like(
+ [
+ 'initdb', '--no-sync',
+ '--locale-provider=icu', '--encoding=SQL_ASCII',
+ "$tempdir/data2"
+ ],
+ qr/error: ICU is not supported in this build/,
+ 'locale provider ICU with not supported ICU encoding fails since no ICU support'
+ );
}
-command_fails(
+command_fails_like(
[ 'initdb', '--no-sync', '--locale-provider=xyz', "$tempdir/dataX" ],
+ qr/error: unrecognized locale provider: xyz/,
'fails for invalid locale provider');
-command_fails(
+command_fails_like(
[
'initdb', '--no-sync',
'--locale-provider=libc', '--icu-locale=en',
"$tempdir/dataX"
],
+ qr/error: --icu-locale cannot be specified unless locale provider "icu" is chosen/,
'fails for invalid option combination');
done_testing();
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index a74bf3b0d8..45b44fce46 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -30,11 +30,12 @@ if ($ENV{with_icu} eq 'yes')
# This fails because template0 uses libc provider and has no ICU
# locale set. It would succeed if template0 used the icu
# provider. XXX Maybe split into multiple tests?
- $node->command_fails(
+ $node->command_fails_like(
[
'createdb', '-T', 'template0', '-E', 'UTF8',
'--locale-provider=icu', 'foobar4'
],
+ qr/ERROR: ICU locale must be specified/,
'create database with ICU fails without ICU locale specified');
$node->issues_sql_like(
@@ -46,12 +47,13 @@ if ($ENV{with_icu} eq 'yes')
qr/statement: CREATE DATABASE foobar5 .* LOCALE_PROVIDER icu ICU_LOCALE 'en'/,
'create database with ICU locale specified');
- $node->command_fails(
+ $node->command_fails_like(
[
'createdb', '-T', 'template0', '-E', 'UTF8',
'--locale-provider=icu',
'--icu-locale=@colNumeric=lower', 'foobarX'
],
+ qr/ERROR: could not open collator for locale/,
'fails for invalid ICU locale');
$node->command_fails_like(
@@ -78,18 +80,39 @@ if ($ENV{with_icu} eq 'yes')
}
else
{
- $node->command_fails(
+ $node->command_fails_like(
[ 'createdb', '-T', 'template0', '--locale-provider=icu', 'foobar4' ],
+ qr/ERROR: ICU is not supported in this build/,
'create database with ICU fails since no ICU support');
+
+ $node->command_fails_like(
+ [
+ 'createdb', '-T',
+ 'template0', '--locale-provider=icu',
+ '--encoding=SQL_ASCII', 'foobar4'
+ ],
+ qr/ERROR: ICU is not supported in this build/,
+ 'create database with ICU and not supported ICU encoding fails since no ICU support'
+ );
}
$node->command_fails([ 'createdb', 'foobar1' ],
'fails if database already exists');
-$node->command_fails(
+$node->command_fails_like(
[ 'createdb', '-T', 'template0', '--locale-provider=xyz', 'foobarX' ],
+ qr/ERROR: unrecognized locale provider: xyz/,
'fails for invalid locale provider');
+$node->command_fails_like(
+ [
+ 'createdb', '-T',
+ 'template0', '--locale-provider=libc',
+ '--icu-locale=en', 'foobarX'
+ ],
+ qr/ERROR: ICU locale cannot be specified unless locale provider is ICU/,
+ 'fails for invalid option combination');
+
# Check use of templates with shared dependencies copied from the template.
my ($ret, $stdout, $stderr) = $node->psql(
'foobar2',
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index a875942123..6e8af39976 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -104,8 +104,8 @@ extern char *get_collation_actual_version(char collprovider, const char *collcol
#ifdef USE_ICU
extern int32_t icu_to_uchar(UChar **buff_uchar, const char *buff, size_t nbytes);
extern int32_t icu_from_uchar(char **result, const UChar *buff_uchar, int32_t len_uchar);
-#endif
extern void check_icu_locale(const char *icu_locale);
+#endif
/* These functions convert from/to libc's wchar_t, *not* pg_wchar_t */
extern size_t wchar2char(char *to, const wchar_t *from, size_t tolen,
--
2.25.1
