List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:February 20 2011 10:08pm
Subject:Re: bzr commit into mysql-trunk branch (ole.john.aske:3551) Bug#59326
View as plain text  
Hi Ole John,

Thanks for making this re-factoring as it simplifies the code.

It seems like there also is a small change in the behaviour. In the 
following call to advance_sj_state():

       if (has_sj)
       {
         /*
           Even if there are no semijoins, advance_sj_state() has a 
significant
           cost (takes 9% of time in a 20-table plan search), hence the if()
           above, which is also more efficient than the same if() inside
           advance_sj_state() would be.
         */
         advance_sj_state(join, remaining_tables, s, idx,
&current_record_count, &current_read_time,
&loose_scan_pos);

before the patch the "compare cost" is not included in the 
current_read_time argument. After your patch current_read_time will 
contain the "compare cost".

In mysql-trunk this change does not change any existing plans for any of 
our mtr tests since it seems like we have no tests that goes into this 
part of this if test. In mysql-next-mr-opt-backporting we have multiple 
tests that execute this part of the code. A simple "copy and paste 
porting" of this change to the mysql-next-mr-opt-backporting tree it 
seems like it will change multiple plans. A quick look at the changed 
plans seems like most of them are improvements but it might be a good 
idea to let someone who knows more about optimisations of semi-joins 
have a look at the changed plans.

Anyway, I think that including the "compare cost" in the 
current_read_time also when calling advance_sj_state() is correct.

OK to push.

Olav



On 01/31/2011 04:10 PM, Ole John Aske wrote:
> #At file:///net/fimafeng09/export/home/tmp/oleja/mysql/mysql-trunk/ based on
> revid:ole.john.aske@stripped
>
>   3551 Ole John Aske	2011-01-31
>        Refactoring cleanup related to bug#59326 and my previous set of fixes for this
> bug:
>
>        - Added 'TIME_FOR_COMPARE' to current_read_time where
>          it is initially calculated - Removed later adding of
>          TIME_FOR_COMPARE whenever it is used.
>
>        - Used local variable '*position' instead of 'join->positions + idx'
>          a few places.
>
>        - Replaced a few references to 'restore_prev_nj_state()'
>          with 'backout_nj_sj_state()' in comments (Has been renamed)
>
>        This fix contains no (intentional) changes of logic.
>
>      modified:
>        sql/sql_select.cc
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-01-31 14:42:02 +0000
> +++ b/sql/sql_select.cc	2011-01-31 15:10:55 +0000
> @@ -8209,12 +8209,13 @@ best_extension_by_limited_search(JOIN
>         /* Find the best access method from 's' to the current partial plan */
>         POSITION loose_scan_pos;
>         best_access_path(join, s, remaining_tables, idx, FALSE, record_count,
> -                       join->positions + idx,&loose_scan_pos);
> +                       position,&loose_scan_pos);
>
>         /* Compute the cost of extending the plan with 's' */
> -
>         current_record_count= record_count * position->records_read;
> -      current_read_time=    read_time + position->read_time;
> +      current_read_time=    read_time
> +                            + position->read_time
> +                            + current_record_count / (double) TIME_FOR_COMPARE;
>
>         if (has_sj)
>         {
> @@ -8232,15 +8233,12 @@ best_extension_by_limited_search(JOIN
>           join->positions[idx].sj_strategy= SJ_OPT_NONE;
>
>         /* Expand only partial plans with lower cost than the best QEP so far */
> -      if ((current_read_time +
> -           current_record_count / (double) TIME_FOR_COMPARE)>=
> join->best_read)
> +      if (current_read_time>= join->best_read)
>         {
>           DBUG_EXECUTE("opt", print_plan(join, idx+1,
>                                          current_record_count,
>                                          read_time,
> -                                       (current_read_time +
> -                                        current_record_count /
> -                                        (double) TIME_FOR_COMPARE),
> +                                       current_read_time,
>                                          "prune_by_cost"););
>           backout_nj_sj_state(remaining_tables, s);
>           continue;
> @@ -8251,10 +8249,7 @@ best_extension_by_limited_search(JOIN
>           the optimal QEPs, thus it results in a non-exhaustive search.
>         */
>         if (prune_level == 1)
> -      {
> -        // Compare cost is also part of 'current cost'
> -        current_read_time+= current_record_count / (double) TIME_FOR_COMPARE;
> -
> +      {
>           if (best_record_count>  current_record_count ||
>               best_read_time>  current_read_time ||
>               (idx == join->const_tables&&   // 's' is the first table in
> the QEP
> @@ -8280,13 +8275,11 @@ best_extension_by_limited_search(JOIN
>             backout_nj_sj_state(remaining_tables, s);
>             continue;
>           }
> -        // Compensate above '+=' in order to keep remaining logic unchanged:
> -        current_read_time-= current_record_count / (double) TIME_FOR_COMPARE;
>         }
>
>         if ( (search_depth>  1)&&  (remaining_tables& 
> ~real_table_bit)&  allowed_tables )
> -      { /* Recursively expand the current partial plan */
> -        current_read_time+= current_record_count / (double) TIME_FOR_COMPARE;
> +      {
> +        /* Explore more best extensions of plan */
>           if (best_extension_by_limited_search(join,
>                                                remaining_tables& 
> ~real_table_bit,
>                                                idx + 1,
> @@ -8301,7 +8294,6 @@ best_extension_by_limited_search(JOIN
>             'join' is either the best partial QEP with 'search_depth' relations,
>             or the best complete QEP so far, whichever is smaller.
>           */
> -        current_read_time+= current_record_count / (double) TIME_FOR_COMPARE;
>           if (join->sort_by_table&&
>               join->sort_by_table !=
>               join->positions[join->const_tables].table->table)
> @@ -14334,7 +14326,7 @@ void advance_sj_state(JOIN *join, table_
>     proceeds up the tree to NJ1, incrementing its counter as well. All join
>     nests are now completely covered by the QEP.
>
> -  restore_prev_nj_state() does the above in reverse. As seen above, the node
> +  backout_nj_sj_state() does the above in reverse. As seen above, the node
>     NJ1 contains the nodes t2, t3, and NJ2. Its counter being equal to 3 means
>     that the plan covers t2, t3, and NJ2, @e and that the sub-plan (t4 x t5)
>     completely covers NJ2. The removal of t5 from the partial plan will first
> @@ -14345,7 +14337,7 @@ void advance_sj_state(JOIN *join, table_
>     NJ2.
>
>     SYNOPSIS
> -    restore_prev_nj_state()
> +    backout_nj_sj_state()
>         last  join table to remove, it is assumed to be the last in current
>               partial join order.
>
>
>
>
>


Thread
bzr commit into mysql-trunk branch (ole.john.aske:3551) Bug#59326Ole John Aske31 Jan
  • Re: bzr commit into mysql-trunk branch (ole.john.aske:3551) Bug#59326Olav Sandstaa20 Feb