From: Roy Lyseng
Date: July 30 2010 1:38pm
Subject: Re: bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3222)
 Bug#51457
List-Archive: http://lists.mysql.com/commits/114805
Message-Id: <4C52D5D6.8080104@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Guilhem,

thanks for the prompt review!

On 30.07.10 14.43, Guilhem Bichot wrote:
> Hello Roy,
>
> Code fix is ok, comments are only about the test.
>
> Roy Lyseng a écrit, Le 30.07.2010 13:54:
>> #At file:///home/rl136806/mysql/repo/mysql-work2/ based on
>> revid:roy.lyseng@stripped
>>
>> 3222 Roy Lyseng 2010-07-30
>> Bug#51457: Firstmatch semijoin strategy gives wrong results for
>> certain query plans
>> When the optimizer selects a semijoin FirstMatch plan that has
>> non-correlated tables interspersed with inner tables, the "jumps"
>> are placed so that execution might become incorrect.
>> Example: A query with an outer correlated table ot, a non-correlated
>> table nt and two inner tables it1 and it2 are used in a query and the
>> optimizer chooses the FirstMatch join order ot - it1 - nt - it2.
>> The optimizer assigns a "jump" from table it2 to ot. This strategy will
>> omit duplicates resulting from table nt.
>> The fix assures that the jump from the last table in a consecutive
>> range of inner table goes to the most immediately preceeding outer
>> table.
>
> I guess here "outer" means "outer correlated or non-correlated".
> A very good comment

I will put in a clarification.

> <cut>
>> === modified file 'mysql-test/r/subselect_innodb.result'
>> --- a/mysql-test/r/subselect_innodb.result 2010-06-19 13:22:14 +0000
>> +++ b/mysql-test/r/subselect_innodb.result 2010-07-30 11:54:24 +0000
>> @@ -392,3 +392,93 @@ and t2.a='1' AND t1.a=t3.b) > 0;
>> a
>> 2
>> DROP TABLE t1,t2,t3;
>> +#
>> +# Bug#51457 Firstmatch semijoin strategy gives wrong results for
>> +# certain query plans
>> +#
>> +DROP TABLE IF EXISTS empty, ones, ot1, nt1, nt4, it1, it3, it4;
>> +SET @@default_storage_engine='innodb';
>> +SET
>> @@optimizer_switch='semijoin=on,materialization=off,firstmatch=on,loosescan=off';
>> +SET @@optimizer_join_cache_level=0;
>
> I suggest not setting join_cache_level to 0; 0 is not default, it's never going
> to be (as 1 is what is 5.1-like, picking 0 as default would be a functionality
> regression compared to 5.1).

My reason for setting join_cache_level to 0 was that any join caching 
complicates join order calculation, and the chosen order is very sensible to 
cost, hence I did it like this. If you still want me to change it, I can...
>
> At your option, you could instead add the testcase into subquery_sj_innodb.inc,
> as it's about semijoin and innodb.

The problem is that I need a specific setting of optimizer_switch. All others 
are just redundant, so I prefer to keep it in subselect_innodb. (Yes, I will 
never agree with Øystein on this one...)
>
>> +CREATE TABLE empty(a INTEGER);
>> +CREATE TABLE ones(a INTEGER);
>> +INSERT INTO ones VALUES(1);
>> +CREATE TABLE it1(a INTEGER);
>> +INSERT INTO it1 VALUES(5), (8);
>> +CREATE TABLE it3(a INTEGER);
>> +INSERT INTO it3 VALUES(7), (1), (0), (5), (1), (4);
>> +CREATE TABLE it4(a INTEGER);
>> +INSERT INTO it4 VALUES(1), (3), (5), (7), (9), (7), (3), (1);
>> +CREATE TABLE ot1(a INTEGER);
>> +INSERT INTO ot1 select * from it1;
>> +CREATE TABLE nt1(a INTEGER);
>> +INSERT INTO nt1 select * from it1;
>> +CREATE TABLE nt4(a INTEGER);
>> +INSERT INTO nt4 select * from it4;
>> +EXPLAIN
>> +SELECT *
>> +FROM nt1 AS nt2
>> +WHERE 1 IN (SELECT it1.a
>> +FROM ones AS it1 JOIN it3 ON it1.a=it3.a);
>
> Here "ones" is aliased to "it1", but "it1" is also a real table; it's confusing
> to pick, as alias name, the name of an existing table. What was the intention?

I used some tables from a different test. I see the confusion, so I can try to 
rename them.
>
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 PRIMARY it1 ALL NULL NULL NULL NULL 1 Using where; FirstMatch
>> +1 PRIMARY nt2 ALL NULL NULL NULL NULL 2
>> +1 PRIMARY it3 ALL NULL NULL NULL NULL 6 Using where; FirstMatch(nt2)
>> +SELECT *
>> +FROM nt1 AS nt2
>> +WHERE 1 IN (SELECT it1.a
>> +FROM ones AS it1 JOIN it3 ON it1.a=it3.a);
>> +a
>> +5
>> +8
>> +EXPLAIN
>> +SELECT *
>> +FROM nt1 AS nt2, nt4
>> +WHERE 1 IN (SELECT it1.a
>> +FROM ones AS it1 JOIN it3 ON it1.a=it3.a);
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 PRIMARY it1 ALL NULL NULL NULL NULL 1 Using where; FirstMatch
>> +1 PRIMARY nt2 ALL NULL NULL NULL NULL 2
>> +1 PRIMARY it3 ALL NULL NULL NULL NULL 6 Using where; FirstMatch(nt2)
>> +1 PRIMARY nt4 ALL NULL NULL NULL NULL 8
>> +SELECT *
>> +FROM nt1 AS nt2, nt4
>> +WHERE 1 IN (SELECT it1.a
>> +FROM ones AS it1 JOIN it3 ON it1.a=it3.a);
>> +a a
>> +5 1
>> +5 3
>> +5 5
>> +5 7
>> +5 9
>> +5 7
>> +5 3
>> +5 1
>> +8 1
>> +8 3
>> +8 5
>> +8 7
>> +8 9
>> +8 7
>> +8 3
>> +8 1
>
> Do you know whether query results are correct (I didn't check)?
> Would they be incorrect without the code fix?

I did check that the results were "reasonable" but not "correct" :)

There are 8 rows before patch and 16 rows after, it looks reasonable.
>
>> +EXPLAIN
>> +SELECT *
>> +FROM empty as ot1, nt1 AS nt3
>> +WHERE ot1.a IN (SELECT it2.a
>> +FROM ones AS it2 JOIN it4 ON it2.a=it4.a);
>> +id select_type table type possible_keys key key_len ref rows Extra
>> +1 PRIMARY ot1 ALL NULL NULL NULL NULL 1
>> +1 PRIMARY it2 ALL NULL NULL NULL NULL 1 Using where; FirstMatch(ot1)
>> +1 PRIMARY nt3 ALL NULL NULL NULL NULL 2
>> +1 PRIMARY it4 ALL NULL NULL NULL NULL 8 Using where; FirstMatch(nt3)
>> +SELECT *
>> +FROM empty as ot1, nt1 AS nt3
>> +WHERE ot1.a IN (SELECT it2.a
>> +FROM ones AS it2 JOIN it4 ON it2.a=it4.a);
>> +a a
>> +DROP TABLE empty, ones, ot1, nt1, nt4, it1, it3, it4;
>> +SET @@default_storage_engine=default;
>> +SET @@optimizer_switch=default;
>> +SET @@optimizer_join_cache_level=default;
>> +# End of bug#51457

Roy