From: Martin Hansson
Date: March 16 2011 2:02pm
Subject: Re: bzr commit into mysql-trunk branch (martin.hansson:3272) WL#3724
List-Archive: http://lists.mysql.com/commits/133127
Message-Id: <4D80C2F6.8030707@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Roy Lyseng wrote:
>>
>>> ...
>>>>
>>>> +--echo # Should not use short-cuts.
>>>> +EXPLAIN
>>>> +SELECT * FROM t1, t2, t3, t4 WHERE t1.a = t2.a AND t2.b = t3.a AND 
>>>> t1.c = t4.a;
>>>> +SELECT * FROM t1, t2, t3, t4 WHERE t1.a = t2.a AND t2.b = t3.a AND 
>>>> t1.c = t4.a;
>>>> +
>>>> +--echo # Should not use short-cuts.
>>>> +EXPLAIN
>>>> +SELECT * FROM t1, t2, t3, t4 WHERE t1.a = t2.a AND t2.b = t3.a AND 
>>>> t1.c = t4.a;
>>>> +SELECT * FROM t1, t2, t3, t4 WHERE t1.a = t2.a AND t2.b = t3.a AND 
>>>> t1.c = t4.a;
>>>> +
>>>> +SHOW STATUS LIKE 'handler_read_%';
>>>
>>> Please rerun without join buffering and expose the difference.
>> I don't understand what would I use the result for. Do other 
>> optimizations have
>> these kinds of tests?
>
> IMHO it would be a good regression test for join shortcutting.
Done.
>>> ...
>>> ...
>>
>>>> ...
>>>> if (found)
>>>> {
>>>> + ++join_tab->produced_rows;
>>>
>>> I think this should go into the *last* "if (found)" block in the 
>>> function.
>>> Otherwise you need to document why it is here.
>> I'm not very familiar with this code. Can you explain why that is 
>> more correct?
>
> Because this is the place where all matching has been done, and where 
> the row is pushed to the next level.
Ok. Your expertise far exceeds mine in this area so I'll take your word 
for it.
>>
>>>> ....
>>>> + static bool are_inner_joined(JOIN_TAB *a, JOIN_TAB *b)
>>>> + {
>>>> + DBUG_ASSERT(a< b);
>>>> + return
>>>> + a->table->pos_in_table_list->embedding ==
>>>> + b->table->pos_in_table_list->embedding&&
>>>> + (b->on_expr_ref == NULL || *b->on_expr_ref == NULL);
>>>> + }
>>> I think that you can safely delete the part "b->on_expr_ref == NULL".
>> I'd rather not, as said in earlier e-mails. If you disagree, please 
>> reply to
>> that e-mail so we get the whole context.
>
> If you can find even one such use of on_expr_ref in existing code, I 
> would allow it. This will just confuse the use of that field.
Ok, removed it.
>>
>> Thank you for the review!
>>
>> Martin
>>
>
> Thanks,
> Roy
Best Regards

Martin