List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:January 28 2011 11:24am
Subject:Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186
View as plain text  
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;
>>   }
>>
>>
>>
>>
>


Thread
bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Olav Sandstaa20 Jan
  • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Jorgen Loland25 Jan
    • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Olav Sandstaa28 Jan
  • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Roy Lyseng26 Jan
    • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Olav Sandstaa28 Jan
      • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Roy Lyseng28 Jan
        • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186Olav Sandstaa28 Jan