BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query
The following bug has been logged on the website:
Bug reference: 15892
Logged by: James Inform
Email address: james.inform@pharmapp.de
PostgreSQL version: 11.4
Operating system: Linux Mint 19 Tara / Mac OS X 10.13.6
Description:
-- This is tested under Linux Mint 19 Tara with uconv v2.1 ICU 60.2
-- and Mac OS X 10.13.6 with uconv v2.1 ICU 58.2
-- PostgreSQL Version 11.4
-- PostgreSQL was built with: ./configure '--prefix=/home/myself/pg114'
'--with-systemd' '--with-icu' '--with-libxml' '--with-libxslt' '--with-perl'
'--with-python' '--disable-rpath' '--with-extra-version= ICU'
-- STEPS to reproduce
-- First you need PostgreSQL 11.4 built with ICU support
create database testdb;
\c testdb;
-- Just create a simple table with one column
create table icutest(data text not null collate "de-x-icu" primary key);
-- Insert a record with uppercase string
insert into icutest values ('MYTEST');
-- This is not giving a match
select * from icutest where data ilike 'mytest';
-- These two queries give the exspected result
select * from icutest where lower(data) ilike 'mytest';
select * from icutest where data ilike ('mytest' collate "default");
-- If you do the same with a non primary source column, then everything
works like exspected
-- I'm not an Expert, but maybe the index behind the primary key is based on
the wrong collation
PG Bug reporting form wrote:
-- Just create a simple table with one column
create table icutest(data text not null collate "de-x-icu" primary key);-- Insert a record with uppercase string
insert into icutest values ('MYTEST');-- This is not giving a match
select * from icutest where data ilike 'mytest';
This also happens on v10 and on the master branch.
The bug seems to come from a mistake in like_support.c:
/*
* Check whether char is a letter (and, hence, subject to case-folding)
*
* In multibyte character sets or with ICU, we can't use isalpha, and it does
* not seem worth trying to convert to wchar_t to use iswalpha. Instead,
just
* assume any multibyte char is potentially case-varying.
*/
static int
pattern_char_isalpha(char c, bool is_multibyte,
pg_locale_t locale, bool locale_is_c)
{
if (locale_is_c)
return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
else if (is_multibyte && IS_HIGHBIT_SET(c))
return true;
else if (locale && locale->provider == COLLPROVIDER_ICU)
return IS_HIGHBIT_SET(c) ? true : false;
With an ICU locale, this returns false for all characters in 'mytest'.
I think this eventually leads the caller to incorrectly believe that it
can optimize the test into an exact match (data='mytest'), given
there are otherwise no wildcards in the pattern.
On fixing the bug, if we make this function returns true for all
characters under an ICU locale, it appears to work, but we're loosing an
opportunity to optimize for some patterns.
If OTOH we wanted to use an ICU call like u_isalpha(), to be closer
to what's done with libc, we'd need to pass a UChar32 argument,
not a char, and since we're in a char-oriented context, I don't see how
to do that.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
<!DOCTYPE html>
<html><head>
<meta charset="UTF-8">
</head><body><p>The weird thing is, that this behavior only occures when used on a field field a primary key:</p><p>If you do: </p><p>create database testdb;</p><p>\c testdb;</p><p>-- Just create a simple table with one column<br>create table icutest(data text not null collate "de-x-icu" primary key, data2 text collate "de-x-icu");</p><p>-- Insert a record with uppercase string<br>insert into icutest values ('MYTEST','MYTEST');</p><p>-- This is not giving a match<br>select * from icutest where data ilike 'mytest';</p><p>-- BUT THIS GIVES A MATCH:</p><p>select * from icutest where data2 ilike 'mytest';</p><p>-- So it seems to be especially related to the scenario where a primary key / index exists.</p><blockquote type="cite">On July 4, 2019 at 12:36 PM Daniel Verite <<a href="mailto:daniel@manitou-mail.org">daniel@manitou-mail.org</a>> wrote:<br><br><br> PG Bug reporting form wrote:<blockquote type="cite">-- Just create a simple table with one column <br>create table icutest(data text not null collate "de-x-icu" primary key); <br><br>-- Insert a record with uppercase string <br>insert into icutest values ('MYTEST'); <br><br>-- This is not giving a match <br>select * from icutest where data ilike 'mytest';</blockquote><br>This also happens on v10 and on the master branch.<br><br>The bug seems to come from a mistake in like_support.c:<br><br> <br>/* * Check whether char is a letter (and, hence, subject to case-folding) <br> * * In multibyte character sets or with ICU, we can't use isalpha, and it does * not seem worth trying to convert to wchar_t to use iswalpha. Instead,<br>just * assume any multibyte char is potentially case-varying. <br> */<br>static int<br>pattern_char_isalpha(char c, bool is_multibyte,<br> pg_locale_t locale, bool locale_is_c)<br>{<br> if (locale_is_c)<br> return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');<br> else if (is_multibyte && IS_HIGHBIT_SET(c))<br> return true;<br> else if (locale && locale->provider == COLLPROVIDER_ICU)<br> return IS_HIGHBIT_SET(c) ? true : false;<br><br><br>With an ICU locale, this returns false for all characters in 'mytest'.<br><br>I think this eventually leads the caller to incorrectly believe that it<br>can optimize the test into an exact match (data='mytest'), given<br>there are otherwise no wildcards in the pattern.<br><br>On fixing the bug, if we make this function returns true for all<br>characters under an ICU locale, it appears to work, but we're loosing an<br>opportunity to optimize for some patterns.<br>If OTOH we wanted to use an ICU call like u_isalpha(), to be closer<br>to what's done with libc, we'd need to pass a UChar32 argument,<br>not a char, and since we're in a char-oriented context, I don't see how<br>to do that.<br><br><br>Best regards,<br>-- <br>Daniel Vérité<br>PostgreSQL-powered mailer: <a href="http://www.manitou-mail.org" rel="noopener" target="_blank">http://www.manitou-mail.org</a><br>Twitter: @DanielVerite</blockquote></body></html>
The weird thing is, that this behavior only occures when used on a field field a primary key:
If you do:
create database testdb;
\c testdb;
-- Just create a simple table with one column
create table icutest(data text not null collate "de-x-icu" primary key, data2 text collate "de-x-icu");
-- Insert a record with uppercase string
insert into icutest values ('MYTEST','MYTEST');
-- This is not giving a match
select * from icutest where data ilike 'mytest';
-- BUT THIS GIVES A MATCH:
select * from icutest where data2 ilike 'mytest';
-- So it seems to be especially related to the scenario where a primary key / index exists.
Show quoted text
On July 4, 2019 at 12:36 PM Daniel Verite <daniel@manitou-mail.org> wrote:
PG Bug reporting form wrote:
-- Just create a simple table with one column
create table icutest(data text not null collate "de-x-icu" primary key);-- Insert a record with uppercase string
insert into icutest values ('MYTEST');-- This is not giving a match
select * from icutest where data ilike 'mytest';This also happens on v10 and on the master branch.
The bug seems to come from a mistake in like_support.c:
/* * Check whether char is a letter (and, hence, subject to case-folding)
* * In multibyte character sets or with ICU, we can't use isalpha, and it does * not seem worth trying to convert to wchar_t to use iswalpha. Instead,
just * assume any multibyte char is potentially case-varying.
*/
static int
pattern_char_isalpha(char c, bool is_multibyte,
pg_locale_t locale, bool locale_is_c)
{
if (locale_is_c)
return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z');
else if (is_multibyte && IS_HIGHBIT_SET(c))
return true;
else if (locale && locale->provider == COLLPROVIDER_ICU)
return IS_HIGHBIT_SET(c) ? true : false;With an ICU locale, this returns false for all characters in 'mytest'.
I think this eventually leads the caller to incorrectly believe that it
can optimize the test into an exact match (data='mytest'), given
there are otherwise no wildcards in the pattern.On fixing the bug, if we make this function returns true for all
characters under an ICU locale, it appears to work, but we're loosing an
opportunity to optimize for some patterns.
If OTOH we wanted to use an ICU call like u_isalpha(), to be closer
to what's done with libc, we'd need to pass a UChar32 argument,
not a char, and since we're in a char-oriented context, I don't see how
to do that.Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
James Inform wrote:
-- This is not giving a match
select * from icutest where data ilike 'mytest';-- BUT THIS GIVES A MATCH:
select * from icutest where data2 ilike 'mytest';
-- So it seems to be especially related to the scenario where a primary key
/ index exists.
Yes. It is because of the index that the code checks if the ILIKE
can be evaluated with an index lookup. Otherwise it doesn't.
If you feel like recompiling with a temporary fix against v11.4 to do
your own tests, please try the attached patch.
Here's the result for me:
* Unpatched version:
postgres=# explain analyze select * from icutest where data ilike 'mytest';
QUERY PLAN
-------------------------------------------------------------------------------------------------------
---------------------
Index Only Scan using icutest_pkey on icutest (cost=0.15..8.17 rows=1
width=32) (actual time=0.013..0
.013 rows=0 loops=1)
Index Cond: (data = 'mytest'::text)
Filter: (data ~~* 'mytest'::text)
Heap Fetches: 0
Planning Time: 0.593 ms
Execution Time: 0.035 ms
* Patched version:
postgres=# explain analyze select * from icutest where data ilike 'mytest';
QUERY PLAN
---------------------------------------------------------------------------------------------------
Seq Scan on icutest (cost=0.00..27.00 rows=1 width=32) (actual
time=0.122..0.123 rows=1 loops=1)
Filter: (data ~~* 'mytest'::text)
Planning Time: 0.081 ms
Execution Time: 0.144 ms
Notice how the patched version ignores the index and correctly finds the
row versus the unpatched version.
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
Attachments:
fix-selfuncs-ilike.patchtext/plainDownload+1-1
On 04.07.2019 19:50, Daniel Verite wrote:
Yes. It is because of the index that the code checks if the ILIKE
can be evaluated with an index lookup. Otherwise it doesn't.If you feel like recompiling with a temporary fix against v11.4 to do
your own tests, please try the attached patch.
Also this patch works. I thinks we need to use some ICU function to
check whether a char is a letter.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Attachments:
fix-selfuncs-ilike-v2.patchtext/x-patch; name=fix-selfuncs-ilike-v2.patchDownload+8-1
Arthur Zakirov <a.zakirov@postgrespro.ru> writes:
On 04.07.2019 19:50, Daniel Verite wrote:
Yes. It is because of the index that the code checks if the ILIKE
can be evaluated with an index lookup. Otherwise it doesn't.
If you feel like recompiling with a temporary fix against v11.4 to do
your own tests, please try the attached patch.
Also this patch works. I thinks we need to use some ICU function to
check whether a char is a letter.
This topic seems to have slipped through the cracks, which is too bad
because we surely could have gotten it fixed in time for last week's
releases. Oh well.
I think Daniel's patch leaves more on the table than it needs to,
and I don't like Arthur's patch because it's assuming more than it
should about whether it's okay to pass a "char" to u_isalpha().
I propose that we do things as attached, instead.
(I also added a regression test case.)
regards, tom lane
Attachments:
fix-selfuncs-ilike-v3.patchtext/x-diff; charset=us-ascii; name=fix-selfuncs-ilike-v3.patchDownload+46-3
On 12.08.2019 00:32, Tom Lane wrote:
This topic seems to have slipped through the cracks, which is too bad
because we surely could have gotten it fixed in time for last week's
releases. Oh well.
It is true. Also July commitfest had started by that time and it wasn't
possible to add a new entry.
I think Daniel's patch leaves more on the table than it needs to,
and I don't like Arthur's patch because it's assuming more than it
should about whether it's okay to pass a "char" to u_isalpha().
I propose that we do things as attached, instead.
Yeah, u_isalpha() wasn't such good idea here. BTW Daniel already
mentioned it above.
With the patch regression tests pass for me and James test case works
fine as expected.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Arthur Zakirov <a.zakirov@postgrespro.ru> writes:
On 12.08.2019 00:32, Tom Lane wrote:
I think Daniel's patch leaves more on the table than it needs to,
and I don't like Arthur's patch because it's assuming more than it
should about whether it's okay to pass a "char" to u_isalpha().
I propose that we do things as attached, instead.
Yeah, u_isalpha() wasn't such good idea here. BTW Daniel already
mentioned it above.
With the patch regression tests pass for me and James test case works
fine as expected.
Thanks for double-checking! Pushed now.
regards, tom lane