Feedback-ID: i131946ab:Fastmail
Content-Type: multipart/mixed; boundary="------------OIswMznfw56qgqZ7BDhhq9xv"
Message-ID: <218b963e-dd66-626a-abd8-962afb587876@enterprisedb.com>
Date: Tue, 20 Sep 2022 05:59:04 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.2
Subject: Re: ICU for global collation
Content-Language: en-US
To: Marina Polyakova <m.polyakova@postgrespro.ru>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-hackers@postgresql.org, pryzby@telsasoft.com, rjuju123@gmail.com,
 daniel@manitou-mail.org, AndrewBille@gmail.com, michael@paquier.xyz
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>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
In-Reply-To: <9ba4cd1ea6ed6b7b15c0ff15e6f540cd@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/218b963e-dd66-626a-abd8-962afb587876%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------OIswMznfw56qgqZ7BDhhq9xv
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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?
--------------OIswMznfw56qgqZ7BDhhq9xv
Content-Type: text/plain; charset=UTF-8;
 name="0001-Improve-ICU-option-handling-in-CREATE-DATABASE.patch"
Content-Disposition: attachment;
 filename*0="0001-Improve-ICU-option-handling-in-CREATE-DATABASE.patch"
Content-Transfer-Encoding: base64

RnJvbSA1YmIwN2UzOTBiYWY4YTQ3MWY3ZDMwNTgyNDY5MjY4YzhhNmQ3MWNhIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQZXRlciBFaXNlbnRyYXV0IDxwZXRlckBlaXNlbnRy
YXV0Lm9yZz4KRGF0ZTogVHVlLCAyMCBTZXAgMjAyMiAwNTo1MDo0MSAtMDQwMApTdWJqZWN0
OiBbUEFUQ0hdIEltcHJvdmUgSUNVIG9wdGlvbiBoYW5kbGluZyBpbiBDUkVBVEUgREFUQUJB
U0UKCldlIGNoZWNrIHRoYXQgdGhlIElDVSBsb2NhbGUgaXMgb25seSBzcGVjaWZpZWQgaWYg
dGhlIElDVSBsb2NhbGUKcHJvdmlkZXIgaXMgc2VsZWN0ZWQuICBCdXQgd2UgZGlkIHRoYXQg
dG9vIGVhcmx5LiAgV2UgbmVlZCB0byB3YWl0CnVudGlsIHdlIGxvYWQgdGhlIHNldHRpbmdz
IG9mIHRoZSB0ZW1wbGF0ZSBkYXRhYmFzZSwgc2luY2UgdGhhdCBjb3VsZAphbHNvIHNldCB3
aGF0IHRoZSBsb2NhbGUgcHJvdmlkZXIgaXMuCgpSZXBvcnRlZC1ieTogTWFyaW5hIFBvbHlh
a292YSA8bS5wb2x5YWtvdmFAcG9zdGdyZXNwcm8ucnU+CkRpc2N1c3Npb246IGh0dHBzOi8v
d3d3LnBvc3RncmVzcWwub3JnL21lc3NhZ2UtaWQvOWJhNGNkMWVhNmVkNmI3YjE1YzBmZjE1
ZTZmNTQwY2RAcG9zdGdyZXNwcm8ucnUKLS0tCiBzcmMvYmFja2VuZC9jb21tYW5kcy9kYmNv
bW1hbmRzLmMgfCAxMSArKysrKysrLS0tLQogMSBmaWxlIGNoYW5nZWQsIDcgaW5zZXJ0aW9u
cygrKSwgNCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9jb21tYW5k
cy9kYmNvbW1hbmRzLmMgYi9zcmMvYmFja2VuZC9jb21tYW5kcy9kYmNvbW1hbmRzLmMKaW5k
ZXggY2JhOTI5YjdmOTk4Li41ZGZlYzVjNmIwNTYgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5k
L2NvbW1hbmRzL2RiY29tbWFuZHMuYworKysgYi9zcmMvYmFja2VuZC9jb21tYW5kcy9kYmNv
bW1hbmRzLmMKQEAgLTkwNywxMCArOTA3LDYgQEAgY3JlYXRlZGIoUGFyc2VTdGF0ZSAqcHN0
YXRlLCBjb25zdCBDcmVhdGVkYlN0bXQgKnN0bXQpCiAJCQkJCSBlcnJtc2coInVucmVjb2du
aXplZCBsb2NhbGUgcHJvdmlkZXI6ICVzIiwKIAkJCQkJCQlsb2Nwcm92aWRlcnN0cikpKTsK
IAl9Ci0JaWYgKGRpY3Vsb2NhbGUgJiYgZGJsb2Nwcm92aWRlciAhPSBDT0xMUFJPVklERVJf
SUNVKQotCQllcmVwb3J0KEVSUk9SLAotCQkJCShlcnJjb2RlKEVSUkNPREVfSU5WQUxJRF9P
QkpFQ1RfREVGSU5JVElPTiksCi0JCQkJIGVycm1zZygiSUNVIGxvY2FsZSBjYW5ub3QgYmUg
c3BlY2lmaWVkIHVubGVzcyBsb2NhbGUgcHJvdmlkZXIgaXMgSUNVIikpKTsKIAlpZiAoZGlz
dGVtcGxhdGUgJiYgZGlzdGVtcGxhdGUtPmFyZykKIAkJZGJpc3RlbXBsYXRlID0gZGVmR2V0
Qm9vbGVhbihkaXN0ZW1wbGF0ZSk7CiAJaWYgKGRhbGxvd2Nvbm5lY3Rpb25zICYmIGRhbGxv
d2Nvbm5lY3Rpb25zLT5hcmcpCkBAIC0xMDUwLDYgKzEwNDYsMTMgQEAgY3JlYXRlZGIoUGFy
c2VTdGF0ZSAqcHN0YXRlLCBjb25zdCBDcmVhdGVkYlN0bXQgKnN0bXQpCiAKIAkJY2hlY2tf
aWN1X2xvY2FsZShkYmljdWxvY2FsZSk7CiAJfQorCWVsc2UKKwl7CisJCWlmIChkYmljdWxv
Y2FsZSkKKwkJCWVyZXBvcnQoRVJST1IsCisJCQkJCShlcnJjb2RlKEVSUkNPREVfSU5WQUxJ
RF9PQkpFQ1RfREVGSU5JVElPTiksCisJCQkJCSBlcnJtc2coIklDVSBsb2NhbGUgY2Fubm90
IGJlIHNwZWNpZmllZCB1bmxlc3MgbG9jYWxlIHByb3ZpZGVyIGlzIElDVSIpKSk7CisJfQog
CiAJLyoKIAkgKiBDaGVjayB0aGF0IHRoZSBuZXcgZW5jb2RpbmcgYW5kIGxvY2FsZSBzZXR0
aW5ncyBtYXRjaCB0aGUgc291cmNlCi0tIAoyLjM3LjMKCg==

--------------OIswMznfw56qgqZ7BDhhq9xv--