List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:January 31 2011 8:23pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3527) Bug#59415
View as plain text  
Hello Jorgen,

Jorgen Loland a écrit, Le 18.01.2011 10:07:
> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
> revid:alexander.barkov@stripped
> 
>  3527 Jorgen Loland	2011-01-18
>       BUG#59415: Range analysis should not be done many times for the
>                  same index
>       
>       The optimizer often choses to access a table through an index 
>       that does not provide the correct ordering for free. To remedy 
>       this, the function test_if_skip_sort_order() is called to see 
>       if another index is as good as the chosen index and at the 
>       same time is able to provide ordering.
>       
>       This implies that test_if_skip_sort_ordering() goes through a 
>       full range analysis (if range access is applicable) to check 
>       whether or not another range access plan should be used 
>       instead of the currently chosen ref/range access method.
>       
>       The problem is that if range analysis is performed and it is 
>       decided that it is not better than whatever we had, the range 
>       analysis will most likely be performed again and again with 
>       the same outcome because test_if_skip_sort_order() is called 
>       from multiple locations.
>       
>       This patch avoids the unnecessarily repeated range analysis
>       described above by introducing key_map 
>       JOIN_TAB::quick_order_tested which is checked to see
>       if range analysis has already been performed for a given key.

Good that this is being fixed. It will reduce the size of certain 
optimizer traces :-)

>      @ sql/sql_select.h
>         Introduce JOIN_TAB::quick_order_tested used to avoid repeated range analysis
> for the same key in test_if_skip_sort_order()
> 
>     modified:
>       sql/sql_select.cc
>       sql/sql_select.h
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-01-14 08:20:08 +0000
> +++ b/sql/sql_select.cc	2011-01-18 09:07:10 +0000
> @@ -19973,8 +19973,11 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
>      if (best_key >= 0)
>      {
>        bool quick_created= FALSE;
> -      if (table->quick_keys.is_set(best_key) && best_key != ref_key)
> +      if (table->quick_keys.is_set(best_key) &&
> +          !tab->quick_order_tested.is_set(best_key) &&
> +          best_key != ref_key)
>        {
> +        tab->quick_order_tested.set_bit(best_key);
>          key_map map;           // Force the creation of quick select
>          map.set_bit(best_key); // only best_key.
>          quick_created=         
> 
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2011-01-14 08:20:08 +0000
> +++ b/sql/sql_select.h	2011-01-18 09:07:10 +0000
> @@ -279,6 +279,15 @@ typedef struct st_join_table : public Sq
>    key_map	checked_keys;			/**< Keys checked */
>    key_map	needed_reg;
>    key_map       keys;                           /**< all keys with can be used
> */
> +  /**
> +    Used to avoid repeated range analysis for the same key in
> +    test_if_skip_sort_order(). This would otherwise happen if the best
> +    range access plan found for a key is turned down.
> +    quick_order_tested is cleared every time the select condition for
> +    this jointab changes since a new condition may give another plan

jointab -> JOIN_TAB ?

> +    and cost from range analysis.
> +   */
> +  key_map       quick_order_tested;

1) do you have an idea why we repeat calls to test_if_skip_sort_order()?

2) are we sure that the repeated calls to test_if_skip_sort_order() 
operate on the same input data? if not, there is no guarantee that the 
result of the Nth call is like the result of the first call, and so it's 
not possible to skip the Nth call;

3) JOIN_TAB::select_cond is a public member; so it can be changed 
directly without calling set_select_cond(); so if someone accidentally 
does that, quick_order_tested won't be cleared and there will be a bug. 
Grepping for "select_cond.*=" this doesn't seem to be the case now, but 
to protect the future, maybe we should make select_cond private, and 
introduce a getter for it?

>    /* Either #rows in the table or 1 for const table.  */
>    ha_rows	records;
> @@ -421,6 +430,7 @@ typedef struct st_join_table : public Sq
>      DBUG_PRINT("info", ("select_cond changes %p -> %p at line %u tab %p",
>                          select_cond, to, line, this));
>      select_cond= to;
> +    quick_order_tested.clear_all();
>    }
>    Item *set_cond(Item *new_cond, uint line)
>    {
> @@ -462,6 +472,7 @@ st_join_table::st_join_table()
>      checked_keys(),
>      needed_reg(),
>      keys(),
> +    quick_order_tested(),
>  
>      records(0),
>      found_records(0),
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 


-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3527) Bug#59415Jorgen Loland18 Jan
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3527) Bug#59415Guilhem Bichot31 Jan
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3527) Bug#59415Jorgen Loland1 Feb