From: Roy Lyseng
Date: January 28 2011 12:11pm
Subject: Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186
List-Archive: http://lists.mysql.com/commits/129852
Message-Id: <4D42B28C.1000204@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Olav,

the new fix looks good to me.

Another thing is that the interface of make_cond_for_index() does not encourage 
proper error handling (it is impossible to distingush a valid NULL value return 
from an error return), but that should rather be cleaned up in a separate round.

Thanks,
Roy

On 28.01.11 12.24, Olav Sandstaa wrote:
> Hi Roy,
>
> Thanks for the review.
>
> I have looked at your suggestions for improving the coding style in
> make_cond_for_index(). I agree with what you suggest and have made a new patch
> that includes these. Let me know if you think these changes look ok.
>
> The new patch is available here:
>
> http://lists.mysql.com/commits/129844
>
> Thanks,
> Olav
>
>
> On 01/26/2011 09:54 AM, Roy Lyseng wrote:
>> Hi Olav,
>>
>> thank you for the bugfix. The fix is approved.
>>
>> I have a couple of coding style comments in addition. They are not about your
>> patch, so you are free to ignore them:
>>
>> 1. Interface of make_cond_for_index()
>>
>> This would become slightly simpler if cond is assumed to be non-NULL always.
>> The only place it can be NULL is when tab->select_cond == NULL in
>> push_index_cond(). In this case, I would add this condition to the enclosing
>> if test inside push_index_cond(), and remove the if (cond) test from
>> make_cond_for_index(). You can also move the declaration of idx_cond inside
>> the if scope.
>>
>> 2. Return values of make_cond_for_index()
>>
>> "return (Item*) 0" should be "return NULL" everywhere.
>>
>> Thanks,
>> Roy
>>
>> On 20.01.11 23.32, Olav Sandstaa wrote:
>>> #Atfile:///home/olav/mysql/develop/trunk-tmp/  based onrevid:olav.sandstaa@stripped
>>>
>>>   3539 Olav Sandstaa	2011-01-20
>>>        Fix for Bug#59186 Wrong results of join when ICP is enabled
>>>
>>>        When index condition pushdown was used an extra row appeared in the
>>>        result set due to parts of the select condition for the second table
>>>        in the JOIN was not evaluated.
>>>
>>>        This was caused when computing the "remainder" for the second table's
>>>        select condition after pushing down parts of it to the storage
>>>        engine. If parts of the select condition was common for both tables in
>>>        the JOIN and the common part was pushed down for the first table the
>>>        common part could have the marker field set to
>>>        ICP_COND_USES_INDEX_ONLY during evaluation of make_cond_for_index()
>>>        for the first table. If the common part of the select condition was
>>>        not pushed down for the second table it would still be marked with
>>>        ICP_COND_USES_INDEX_ONLY when computing the remainder for the select
>>>        condition for the second table. This would cause that this part of the
>>>        select condition neither was pushed down to the storage engine nor
>>>        included in the select condition to be evaluated by the server.
>>>
>>>        The fix for this is to extend make_cond_for_index() so that it clears
>>>        the marker field for the parts of the item tree that it decides should
>>>        not be pushed down to the storage engine. This will prevent that
>>>        common items in select conditions for different tables that have been
>>>        marked with ICP_COND_USES_INDEX_ONLY when evaluating one table does
>>>        not keep this value when computing the "remainder" for a following
>>>        table in a JOIN.
>>>       @ mysql-test/include/icp_tests.inc
>>>          Test case for Bug#59186 Wrong results of join when ICP is enabled.
>>>       @ mysql-test/r/innodb_icp.result
>>>          Result file for the test for Bug#59186 Wrong results of join when
>>>          ICP is enabled.
>>>       @ mysql-test/r/innodb_icp_none.result
>>>          Result file for the test for Bug#59186 Wrong results of join when
>>>          ICP is enabled.
>>>       @ mysql-test/r/myisam_icp.result
>>>          Result file for the test for Bug#59186 Wrong results of join when
>>>          ICP is enabled.
>>>       @ mysql-test/r/myisam_icp_none.result
>>>          Result file for the test for Bug#59186 Wrong results of join when
>>>          ICP is enabled.
>>>       @ sql/sql_select.cc
>>>          Reset the condition's marker field in make_cond_for_index() when it has
>>>          determined that this part of the select condition should not be
>>>          included in the condition to be pushed down to the storage engine.
>>>
>>>          The reason this must be cleared is that if the condition
>>>          is a common part of for the select condition of two tables in
>>>          a JOIN operation then the marker field might have gotten the value
>>>          set to ICP_COND_USES_INDEX_ONLY when evaluating the select
>>>          condition for the first table. If this is the case we need
>>>          to reset it to avoid that this part of the select condition
>>>          is wrongly concluded to not be needed in the select condition
>>>          to be evaluated by the server after pushing parts of the
>>>          condition down to the storage engine.
>>>
>>>      modified:
>>>        mysql-test/include/icp_tests.inc
>>>        mysql-test/r/innodb_icp.result
>>>        mysql-test/r/innodb_icp_none.result
>>>        mysql-test/r/myisam_icp.result
>>>        mysql-test/r/myisam_icp_none.result
>>>        sql/sql_select.cc
>>> === modified file 'mysql-test/include/icp_tests.inc'
>>> --- a/mysql-test/include/icp_tests.inc	2011-01-20 14:57:03 +0000
>>> +++ b/mysql-test/include/icp_tests.inc	2011-01-20 22:32:32 +0000
>>> @@ -724,3 +724,34 @@ WHERE NOT EXISTS
>>>   --echo
>>>
>>>   DROP TABLE t1,t2;
>>> +
>>> +--echo #
>>> +--echo # Bug#59186 Wrong results of join when ICP is enabled
>>> +--echo #
>>> +
>>> +CREATE TABLE t1 (
>>> +  pk INTEGER NOT NULL,
>>> +  c1 VARCHAR(3) NOT NULL,
>>> +  PRIMARY KEY (pk)
>>> +);
>>> +
>>> +INSERT INTO t1 VALUES (1,'y'),(0,'or');
>>> +
>>> +CREATE TABLE t2 (
>>> +  pk INTEGER NOT NULL,
>>> +  c1 VARCHAR(3) NOT NULL,
>>> +  c2 VARCHAR(6) NOT NULL,
>>> +  PRIMARY KEY (pk)
>>> +);
>>> +
>>> +INSERT INTO t2 VALUES (6,'y','RPOYT'),(10,'m','JINQE');
>>> +
>>> +let query=
>>> +SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +      (t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +
>>> +eval EXPLAIN $query;
>>> +eval $query;
>>> +
>>> +DROP TABLE t1, t2;
>>>
>>> === modified file 'mysql-test/r/innodb_icp.result'
>>> --- a/mysql-test/r/innodb_icp.result	2011-01-20 14:57:03 +0000
>>> +++ b/mysql-test/r/innodb_icp.result	2011-01-20 22:32:32 +0000
>>> @@ -672,5 +672,32 @@ id	select_type	table	type	possible_keys
>>>   3	DEPENDENT SUBQUERY	t2	unique_subquery	PRIMARY	PRIMARY	4	func	1	Using where; Full scan on NULL key
>>>
>>>   DROP TABLE t1,t2;
>>> +#
>>> +# Bug#59186 Wrong results of join when ICP is enabled
>>> +#
>>> +CREATE TABLE t1 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t1 VALUES (1,'y'),(0,'or');
>>> +CREATE TABLE t2 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +c2 VARCHAR(6) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t2 VALUES (6,'y','RPOYT'),(10,'m','JINQE');
>>> +EXPLAIN SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>>> +1	SIMPLE	t1	range	PRIMARY	PRIMARY	4	NULL	1	Using index condition
>>> +1	SIMPLE	t2	range	PRIMARY	PRIMARY	4	NULL	2	Using index condition; Using where; Using join buffer (BNL, incremental buffers)
>>> +SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +c2
>>> +DROP TABLE t1, t2;
>>>   set default_storage_engine= @save_storage_engine;
>>>   set optimizer_switch=default;
>>>
>>> === modified file 'mysql-test/r/innodb_icp_none.result'
>>> --- a/mysql-test/r/innodb_icp_none.result	2011-01-20 14:57:03 +0000
>>> +++ b/mysql-test/r/innodb_icp_none.result	2011-01-20 22:32:32 +0000
>>> @@ -671,5 +671,32 @@ id	select_type	table	type	possible_keys
>>>   3	DEPENDENT SUBQUERY	t2	unique_subquery	PRIMARY	PRIMARY	4	func	1	Using where; Full scan on NULL key
>>>
>>>   DROP TABLE t1,t2;
>>> +#
>>> +# Bug#59186 Wrong results of join when ICP is enabled
>>> +#
>>> +CREATE TABLE t1 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t1 VALUES (1,'y'),(0,'or');
>>> +CREATE TABLE t2 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +c2 VARCHAR(6) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t2 VALUES (6,'y','RPOYT'),(10,'m','JINQE');
>>> +EXPLAIN SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>>> +1	SIMPLE	t1	range	PRIMARY	PRIMARY	4	NULL	1	Using where
>>> +1	SIMPLE	t2	range	PRIMARY	PRIMARY	4	NULL	2	Using where; Using join buffer (BNL, incremental buffers)
>>> +SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +c2
>>> +DROP TABLE t1, t2;
>>>   set default_storage_engine= @save_storage_engine;
>>>   set optimizer_switch=default;
>>>
>>> === modified file 'mysql-test/r/myisam_icp.result'
>>> --- a/mysql-test/r/myisam_icp.result	2011-01-20 14:57:03 +0000
>>> +++ b/mysql-test/r/myisam_icp.result	2011-01-20 22:32:32 +0000
>>> @@ -670,4 +670,31 @@ id	select_type	table	type	possible_keys
>>>   3	DEPENDENT SUBQUERY	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE
>>>
>>>   DROP TABLE t1,t2;
>>> +#
>>> +# Bug#59186 Wrong results of join when ICP is enabled
>>> +#
>>> +CREATE TABLE t1 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t1 VALUES (1,'y'),(0,'or');
>>> +CREATE TABLE t2 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +c2 VARCHAR(6) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t2 VALUES (6,'y','RPOYT'),(10,'m','JINQE');
>>> +EXPLAIN SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>>> +1	SIMPLE	t1	range	PRIMARY	PRIMARY	4	NULL	2	Using index condition
>>> +1	SIMPLE	t2	range	PRIMARY	PRIMARY	4	NULL	2	Using index condition; Using where; Using join buffer (BNL, incremental buffers)
>>> +SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +c2
>>> +DROP TABLE t1, t2;
>>>   set optimizer_switch=default;
>>>
>>> === modified file 'mysql-test/r/myisam_icp_none.result'
>>> --- a/mysql-test/r/myisam_icp_none.result	2011-01-20 14:57:03 +0000
>>> +++ b/mysql-test/r/myisam_icp_none.result	2011-01-20 22:32:32 +0000
>>> @@ -669,4 +669,31 @@ id	select_type	table	type	possible_keys
>>>   3	DEPENDENT SUBQUERY	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE
>>>
>>>   DROP TABLE t1,t2;
>>> +#
>>> +# Bug#59186 Wrong results of join when ICP is enabled
>>> +#
>>> +CREATE TABLE t1 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t1 VALUES (1,'y'),(0,'or');
>>> +CREATE TABLE t2 (
>>> +pk INTEGER NOT NULL,
>>> +c1 VARCHAR(3) NOT NULL,
>>> +c2 VARCHAR(6) NOT NULL,
>>> +PRIMARY KEY (pk)
>>> +);
>>> +INSERT INTO t2 VALUES (6,'y','RPOYT'),(10,'m','JINQE');
>>> +EXPLAIN SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
>>> +1	SIMPLE	t1	range	PRIMARY	PRIMARY	4	NULL	2	Using where
>>> +1	SIMPLE	t2	range	PRIMARY	PRIMARY	4	NULL	2	Using where; Using join buffer (BNL, incremental buffers)
>>> +SELECT c2 FROM t1 JOIN t2 ON t1.c1 = t2.c1
>>> +WHERE (t2.pk<= 4 AND t1.pk IN (2,1)) OR
>>> +(t1.pk>  1 AND t2.pk BETWEEN 6 AND 6);
>>> +c2
>>> +DROP TABLE t1, t2;
>>>   set optimizer_switch=default;
>>>
>>> === modified file 'sql/sql_select.cc'
>>> --- a/sql/sql_select.cc	2011-01-20 09:09:26 +0000
>>> +++ b/sql/sql_select.cc	2011-01-20 22:32:32 +0000
>>> @@ -10068,7 +10068,15 @@ Item *make_cond_for_index(Item *cond, TA
>>>     }
>>>
>>>     if (!uses_index_fields_only(cond, table, keyno, other_tbls_ok))
>>> +  {
>>> +    /*
>>> +      Reset marker since it might have the value
>>> +      ICP_COND_USES_INDEX_ONLY if this condition is part of the select
>>> +      condition for multiple tables.
>>> +    */
>>> +    cond->marker= 0;
>>>       return (Item*) 0;
>>> +  }
>>>     cond->marker= ICP_COND_USES_INDEX_ONLY;
>>>     return cond;
>>>   }
>>>
>>>
>>>
>>>
>>
>