Horton

Thread

Commits

  1. Fix planner's test for case-foldable characters in ILIKE with ICU.

  1. BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    The Post Office <noreply@postgresql.org> — 2019-07-03T22:50:17Z

    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

  2. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Daniel Verite <daniel@manitou-mail.org> — 2019-07-04T10:36:59Z

    	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

  3. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    James Inform <james.inform@pharmapp.de> — 2019-07-04T13:41:45Z

    <!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:&#160;</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 &#34;de-x-icu&#34; primary key, data2 text collate &#34;de-x-icu&#34;);</p><p>-- Insert a record with uppercase string<br>insert into icutest values (&#39;MYTEST&#39;,&#39;MYTEST&#39;);</p><p>-- This is not giving a match<br>select * from icutest where data ilike &#39;mytest&#39;;</p><p>-- BUT THIS GIVES A MATCH:</p><p>select * from icutest where data2 ilike &#39;mytest&#39;;</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 &#60;<a href="mailto:daniel@manitou-mail.org">daniel@manitou-mail.org</a>&#62; 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 &#34;de-x-icu&#34; primary key); <br><br>-- Insert a record with uppercase string <br>insert into icutest values (&#39;MYTEST&#39;); <br><br>-- This is not giving a match <br>select * from icutest where data ilike &#39;mytest&#39;;</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&#39;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 &#62;= &#39;A&#39; &#38;&#38; c &#60;= &#39;Z&#39;) || (c &#62;= &#39;a&#39; &#38;&#38; c &#60;= &#39;z&#39;);<br> else if (is_multibyte &#38;&#38; IS_HIGHBIT_SET(c))<br> return true;<br> else if (locale &#38;&#38; locale-&#62;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 &#39;mytest&#39;.<br><br>I think this eventually leads the caller to incorrectly believe that it<br>can optimize the test into an exact match (data=&#39;mytest&#39;), 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&#39;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&#39;s done with libc, we&#39;d need to pass a UChar32 argument,<br>not a char, and since we&#39;re in a char-oriented context, I don&#39;t see how<br>to do that.<br><br><br>Best regards,<br>-- <br>Daniel V&#233;rit&#233;<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>

  4. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    James Inform <james.inform@pharmapp.de> — 2019-07-04T16:40:11Z

    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.


> 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

  5. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Daniel Verite <daniel@manitou-mail.org> — 2019-07-04T16:50:27Z

    	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

  6. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-07-04T17:57:19Z

    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

  7. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-11T21:32:49Z

    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

  8. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Artur Zakirov <a.zakirov@postgrespro.ru> — 2019-08-12T09:12:13Z

    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

  9. Re: BUG #15892: URGENT: Using an ICU collation in a primary key column breaks ILIKE query

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-08-12T17:17:38Z

    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