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;
>> }
>>
>>
>>
>>
>
