List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:January 28 2011 12:53pm
Subject:Re: bzr commit into mysql-trunk branch (olav.sandstaa:3539) Bug#59186
View as plain text  
Hi Roy,

Thanks for looking at the second version of the patch.

I agree that it is impossible to distinguish between an error and a NULL 
item being returned and that this should be fixed. I do not think this 
is urgent and should not be improved as part of this fix. As the code is 
today I think that if the only consequence of an "error NULL" being 
returned from make_cond_for_index() is that ICP will not be used.

Olav.

On 01/28/2011 01:11 PM, Roy Lyseng wrote:
> 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;
>>>>   }
>>>>
>>>>
>>>>
>>>>
>>>
>>
>

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