List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:February 6 2011 10:34pm
Subject:Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833
View as plain text  
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).

> +
>       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?

>         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.

> +      */
> +      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).

> +      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.
>         }
>
> -      /*
> -	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)
>
>
>
>


Thread
bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833Roy Lyseng4 Feb
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833Olav Sandstaa6 Feb
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833Roy Lyseng11 Feb
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833Øystein Grøvlen10 Feb
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3338) Bug#59833Roy Lyseng14 Feb