From: Roy Lyseng
Date: February 11 2011 3:52pm
Subject: Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833
List-Archive: http://lists.mysql.com/commits/131144
Message-Id: <4D555B41.1080507@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Olav,

thank you for commenting.

On 06.02.11 23.34, Olav Sandstaa wrote:
> Hi Roy,
>
> The patch looks good. Thanks for adding great comments both for the commit and
> in the source files.
>
> The number of explain changes was large, thanks for adding explanation for most
> of these. I have not examined in detail why the plans changes but agree that
> most of them are improvements compared to the previous plan. Is this also the
> case for the new plans that contains "Range checked for each record (index map:
> 0x2)"?
>
> I have some minor comments to the code, see inline. You can feel free to decide
> whether to ignore these or not.
>
> OK to push.
>
> Olav
>
>
> On 02/04/2011 12:29 PM, Roy Lyseng wrote:
>>
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc	2011-02-02 13:48:49 +0000
>> +++ b/sql/sql_select.cc	2011-02-04 11:28:14 +0000
>> @@ -9505,12 +9505,16 @@ static bool pushdown_on_conditions(JOIN*
>>       */
>>       Item *on_expr= *first_inner_tab->on_expr_ref;
>>
>> -    table_map used_tables= (join->const_table_map |
>> -                            OUTER_REF_TABLE_BIT | RAND_TABLE_BIT);
>> +    table_map used_tables= (table_map)0;
>
> Here you have included an explicit cast to (table_map). In the corresponding
> code in make_join_select() you do not have this cast. The cast is not necessary
> but it maybe improves readability? (if so it should also be added in
> make_join_select).

I do not know a reason to have this, so I'm deleting it.
>
>> +
>>       for (JOIN_TAB *join_tab= join->join_tab+join->const_tables;
>>            join_tab<= last_tab ; join_tab++)
>>       {
>>         table_map current_map= join_tab->table->map;
>> +      if (join_tab == join->join_tab+join->const_tables)
>> +        current_map|= join->const_table_map | OUTER_REF_TABLE_BIT;
>> +      if (join_tab == last_tab)
>> +        current_map|= RAND_TABLE_BIT;
>
> I liked the comment you added for the similar code in make_join_select(). Would
> it be a good idea to add the same comment here?

I guess I can add a reference to make_join_select(), Step #2.

>
>>         used_tables|= current_map;
>>         Item *tmp_cond= make_cond_for_table(on_expr, used_tables, current_map, 0);
>>         if (!tmp_cond)
>> @@ -9567,7 +9571,6 @@ static bool make_join_select(JOIN *join,
>>     DBUG_ENTER("make_join_select");
>>     {
>>       add_not_null_conds(join);
>> -    table_map used_tables;
>>       /*
>>         Step #1: Extract constant condition
>>          - Extract and check the constant part of the WHERE
>> @@ -9634,22 +9637,34 @@ static bool make_join_select(JOIN *join,
>>       /*
>>         Step #2: Extract WHERE/ON parts
>>       */
>> +    table_map used_tables= 0;
>>       table_map save_used_tables= 0;
>> -    used_tables= join->const_table_map | OUTER_REF_TABLE_BIT | RAND_TABLE_BIT;
>> -    JOIN_TAB *tab;
>> -    table_map current_map;
>>       for (uint i=join->const_tables ; i<  join->tables ; i++)
>>       {
>> -      tab= join->join_tab+i;
>> +      JOIN_TAB *tab= join->join_tab+i;
>>         /*
>>           first_inner is the X in queries like:
>>           SELECT * FROM t1 LEFT OUTER JOIN (t2 JOIN t3) ON X
>>         */
>>         JOIN_TAB *first_inner_tab= tab->first_inner;
>> -      current_map= tab->table->map;
>>         bool use_quick_range=0;
>>         Item *tmp;
>>
>> +      /*
>> +        Calculate used table information added at this stage.
>> +        The current table is always added. Const tables are assumed to be
>> +        available together with the first table in the join order.
>> +        All outer references are available, so these may be evaluated together
>> +        with the first table.
>> +	Random expressions must be added to the last table's condition.
>> +	It solves problem with queries like SELECT * FROM t1 WHERE rand()>  0.5
>
> Tab is used in front of the two last lines of this comment.

Outch. I'd wish my editor would highlight TAB characters...
>
>> +      */
>> +      table_map current_map= tab->table->map;
>> +      if (i == join->const_tables)
>> +	current_map|= OUTER_REF_TABLE_BIT | join->const_table_map;
>
> I think the line above would be better to read and follow the above comment more
> closely if you reversed the order of OUTER_REF_TABLE_BIT | join->const_table_map.
> (in pushdown_on_condition() you already have it in the opposite order).

Done.

>
>> +      if (i == join->tables-1)
>> +	current_map|= RAND_TABLE_BIT;
>> +
>>         /*
>>           Tables that are within SJ-Materialization nests cannot have their
>>           conditions referring to preceding non-const tables.
>> @@ -9660,17 +9675,10 @@ static bool make_join_select(JOIN *join,
>>             !(used_tables&  tab->emb_sj_nest->sj_inner_tables))
>>         {
>>           save_used_tables= used_tables;
>> -        used_tables= join->const_table_map | OUTER_REF_TABLE_BIT |
>> -                     RAND_TABLE_BIT;
>> +        used_tables= OUTER_REF_TABLE_BIT | join->const_table_map;
>
> Same comment as above: I liked the original ordering better.

Done.

>>         }
>>
>> -      /*
>> -	Following force including random expression in last table condition.
>> -	It solve problem with select like SELECT * FROM t1 WHERE rand()>  0.5
>> -      */
>> -      if (i == join->tables-1)
>> -	current_map|= OUTER_REF_TABLE_BIT | RAND_TABLE_BIT;
>> -      used_tables|=current_map;
>> +      used_tables|= current_map;
>>
>>         if (tab->type == JT_REF&&  tab->quick&&
>>   	(uint) tab->ref.key == tab->quick->index&&
>> @@ -19144,10 +19152,25 @@ static bool replace_subcondition(JOIN *j
>>       specified in 'tables' bitmap are available.
>>       If 'used_table' is 0, extract conditions for all tables in 'tables'.
>>
>> -    The function assumes that
>> -      - Constant parts of the condition has already been checked.
>> -      - Condition that could be checked for tables in 'tables' has already
>> -        been checked.
>> +    This function can be used to extract conditions relevant for a table
>> +    in a join order. It will ensure that all conditions are attached to
>> +    the first table in the join order where all necessary fields are
>> +    available, and it will also ensure that a given condition is attached
>> +    to only one table.
>> +
>> +    To accomplish this, first initialize "tables" to the empty
>> +    set. Then loop over all tables in the join order, set "used_table" to
>> +    the bit representing the current table, accumulate "used_table" into the
>> +    "tables" set, and call this function. To ensure correct handling of
>> +    const expressions and outer references, add the const table map and
>> +    OUTER_REF_TABLE_BIT to "used_table" for the first table. To ensure
>> +    that random expressions are evaluated for the final table, add
>> +    RAND to "used_table" for the final table.
>> +
>> +    The function assumes that constant, inexpensive parts of the condition
>> +    have already been checked. Constant, expensive parts will be attached
>> +    to the first table in the join order, provided that the above call
>> +    sequence is followed.
>
> Great comment!
>
>>
>>       The function takes into account that some parts of the condition are
>>       guaranteed to be true by employed 'ref' access methods (the code that
>> @@ -19176,20 +19199,13 @@ make_cond_for_table_from_pred(Item *root
>>       Ignore this condition if
>>        1. We are extracting conditions for a specific table, and
>>        2. that table is not referenced by the condition, and
>> -     3. exclude constant conditions not checked at optimization time if
>> -        the table we are pushing conditions to is the first one.
>> -        As a result, such conditions are not considered as already checked
>> -        and will be checked at execution time, attached to the first table.
>> +     3. this is a constant condition not checked at optimization time and
>> +        this is not the first table we are extracting conditions for.
>> +       (Assuming that used_table == tables for the first table.)
>>     */
>>     if (used_table&&                                                  // 1
>>         !(cond->used_tables()&  used_table)&&                         // 2
>> -      /*
>> -        psergey: TODO: "used_table&  1" doesn't make sense in nearly any
>> -        context. Look at setup_table_map(), table bits reflect the order
>> -        the tables were encountered by the parser. Check what we should
>> -        replace this condition with.
>> -      */
>> -      !((used_table&  1)&&  cond->is_expensive()))                  // 3
>> +      !(used_table == tables&&  cond->is_expensive()))              // 3
>>       return NULL;
>>
>>     if (cond->type() == Item::COND_ITEM)
>>
>>
>>
>>
>

Regards,
Roy