Date: Fri, 12 Jun 2020 18:40:55 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Thomas Munro <thomas.munro@gmail.com>, Peter Geoghegan <pg@bowt.ie>
Cc: =?UTF-8?B?0KDQvtC80LDQvSDQm9C40YLQvtCy0YfQtdC90LrQvg==?=
 <roman.lytovchenko@gmail.com>, PostgreSQL mailing lists
 <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #15285: Query used index over field with ICU collation in
 some cases wrongly return 0 rows
Message-ID: <20200612184055.205f0159@firost>
In-Reply-To: <20200610002933.6a6d482b@firost>
References: <153201618542.1404.3611626898935613264@wrigleys.postgresql.org>
	<CAH2-Wz=TGN_03qBmqgFXVhw6Kbfys-EBz4MJunzRq+RB5gL7WA@mail.gmail.com>
	<CAH2-Wzkiue6PQzneBJsuDnYdUVMa5jnevVJXXkV3hR0oboOSbQ@mail.gmail.com>
	<CAEepm=1-f3OuO_zA83C0x8dLFqMA+Q=RGUkyMRp4WXxjSrvbww@mail.gmail.com>
	<20200610002933.6a6d482b@firost>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/uQsv9Hg0E9I4l5FtS7QeMNc"
Precedence: bulk

--MP_/uQsv9Hg0E9I4l5FtS7QeMNc
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Wed, 10 Jun 2020 00:29:33 +0200
Jehan-Guillaume de Rorthais <jgdr@dalibo.com> wrote:
[...]
> After playing with ICU regression tests, I found functions ucol_strcollIter
> and ucol_nextSortKeyPart are safe. I'll do some performance tests and report
> here.

I did some benchmarks. See attachment for the script and its header to
reproduce.

It sorts 935895 french phrases from 0 to 122 chars with an average of 49.
Performance tests were done on current master HEAD (buggy) and using the patch
in attachment, relying on ucol_strcollIter.

My preliminary test with ucol_getSortKey was catastrophic, as we might
expect. 15-17x slower than the current HEAD. So I removed it from actual tests.
I didn't try with ucol_nextSortKeyPart though.

Using ucol_strcollIter performs ~20% slower than HEAD on UTF8 databases, but
this might be acceptable. Here are the numbers:

   DB Encoding   HEAD  strcollIter   ratio
   UTF8          2.74         3.27   1.19x
   LATIN1        5.34         5.40   1.01x

I plan to add a regression test soon.

> In the meantime, I've been working on various workarounds. The only one I
> found is to use "fr-u-kr-latn-digit-kn" instead of "fr-u-kr-latn-digit".
> Unfortunately, the two collations are not equivalent, but I believe it might
> be useful in many case.
> 
> I've been working on a second workaround: creating a type (a char variant for
> our usecase), its operators and opfamily. All operators and function 1 relies
> on ucol_getSortKey. Most of the workaround works good but surprisingly, the
> sort order is only enforced if the field is in the first position:
> 
>   * this works: "SORT BY f1 COLLATE digitslast"
>   * this fails: "SORT BY f2, f1 COLLATE digitslast"

I fixed this. I didn't declare my opclass as default for the type I created.
I'm not sure people would like to see/discuss this user workaround here?

Regards,

--MP_/uQsv9Hg0E9I4l5FtS7QeMNc
Content-Type: application/octet-stream; name=test-icu.bash
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=test-icu.bash

IyEvYmluL2Jhc2gKCiMgZGF0YSBhdmFpbGFibGUgaGVyZToKIyBjdXJsIC1zTCBodHRwOi8vZGwu
ZnJlZS5mci9vdnh3aUJ3REQgLW8gdGV4dGVzLmR1bXAKCnNob3B0IC1zIC1vIGVycmV4aXQgbm91
bnNldAoKUFJFPS90bXAvaWN1ClBHREFUQT0vdG1wL2ljdS9wZ2RhdGEKUEdQT1JUPTU0MzIKCmV4
cG9ydCBQR0RBVEEgUEdQT1JUCgojIGJ1aWxkL2luc3RhbGwgY3VycmVudCBiYW5jaAptYWtlIC1q
NCBpbnN0YWxsID4vZGV2L251bGwKcHVzaGQgY29udHJpYi9wZ19wcmV3YXJtID4vZGV2L251bGwK
bWFrZSAtajQgaW5zdGFsbCA+L2Rldi9udWxsCnBvcGQgPi9kZXYvbnVsbAoKIyBjbGVhbnVwIHBy
ZXZpb3VzIHJ1bgokUFJFL2Jpbi9wZ19jdGwgLXMgLW0gaW1tZWRpYXRlIHN0b3AgMj4vZGV2L251
bGx8fCB0cnVlCnJtIC1mciAiJHtQR0RBVEE6P30iCgojIGNyZWF0ZSAmIHN0YXJ0IG5ldyBpbnN0
YW5jZQokUFJFL2Jpbi9pbml0ZGIgLVUgcG9zdGdyZXMgLU4gLWsgIiRQR0RBVEEiID4vZGV2L251
bGwgMj4mMQokUFJFL2Jpbi9wZ19jdGwgLXMgLXcgLWwgIiRQUkUvcG9zdGdyZXNxbC5sb2ciIHN0
YXJ0CgojIGNyZWF0ZSB1dGY4IGFuZCBsYXRpbjEgZGF0YWJhc2UgZm9yIHRlc3RzCiRQUkUvYmlu
L2NyZWF0ZWRiIC1FIHVuaWNvZGUgdW5pCiRQUkUvYmluL2NyZWF0ZWRiIC1FIGxhdGluMSAtbCBm
cl9GUiAtVCB0ZW1wbGF0ZTAgbGF0aW4KCmZvciBkYiBpbiB1bmkgbGF0aW47IGRvCgogICAgJFBS
RS9iaW4vcGdfcmVzdG9yZSAtZCAiJGRiIiB0ZXh0ZXMuZHVtcAoKICAgICRQUkUvYmluL3ZhY3V1
bWRiIC1xeiAiJGRiIgoKICAgICRQUkUvYmluL3BzcWwgLWQgIiRkYiIgLVhBdG8gL2Rldi9udWxs
IDw8LUVPRgogICAgQ1JFQVRFIEVYVEVOU0lPTiBJRiBOT1QgRVhJU1RTIHBnX3ByZXdhcm07CiAg
ICBTRUxFQ1QgKiBGUk9NIHBnX3ByZXdhcm0oJ3RleHRlcyc6OnJlZ2NsYXNzKTsKICAgIEFMVEVS
IERBVEFCQVNFICRkYiBTRVQgd29ya19tZW0gVE8gJzI1Nk1CJzsKICAgIEFMVEVSIERBVEFCQVNF
ICRkYiBTRVQgbWF4X3BhcmFsbGVsX3dvcmtlcnNfcGVyX2dhdGhlciBUTyAwOwogICAgQUxURVIg
VEFCTEUgdGV4dGVzIEFMVEVSIENPTFVNTiBjb250ZW51IFRZUEUgdGV4dCBDT0xMQVRFIGRpZ2l0
c2xhc3QKCUVPRgogICAgXHRpbWUgLWYgJWUgJFBSRS9iaW4vcHNxbCAtZCAiJGRiIiAtWEF0byAv
ZGV2L251bGwgPDwtRU9GCiAgICBTRUxFQ1QgKiBGUk9NIHRleHRlcyBPUkRFUiBCWSBjb250ZW51
OwoJRU9GCgpkb25lCgojJFBSRS9iaW4vcGdfY3RsIC1zIC1tIGltbWVkaWF0ZSBzdG9wCiNybSAt
ZnIgIiR7UEdEQVRBOj99IgoK

--MP_/uQsv9Hg0E9I4l5FtS7QeMNc
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=v1-0001-Replace-buggy-ucol_strcoll-funcs-with-ucol_strcollIt.patch