List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:July 21 2010 10:32am
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)
Bug#55042
View as plain text  
Hi,

I am not convinced that this fix is safe.  If you skip assigning pos to 
loose_scan_pos, I think you will be returning a cost that is based on 
something else than what is asked for by the calling function. Hence, it 
will assume that loose_scan can be performed with a certain cost when 
the actual case is probably that loose_scan can not be used at all.  In 
the actual test case in subquery_mat_all this does not matter since 
another strategy is picked anyway, but can we be sure that is always the 
case?

I think you at least should ensure that DBL_MAX is returned as the total 
cost from optimizer_wo_join_buffering() in this case.  That should 
prevent loose_scan from being picked as the best strategy for the given 
join order.

--
Øystein


On 07/20/10 03:29 PM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-valgrind/ based on
> revid:roy.lyseng@stripped
>
>   3217 Tor Didriksen	2010-07-20
>        Bug #55042 valgrind reports error in best_access_path when running
> subquery_mat_all
>
>        The code was using loose_scan_pos, even though it was explicitly set to
> invalid.
>       @ sql/sql_select.cc
>          Do not use loose_scan_pos in optimize_wo_join_buffering() if it has a cost
> of DBL_MAX
>          (as determined by best_access_path())
>
>      modified:
>        sql/sql_select.cc
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-07-16 12:21:31 +0000
> +++ b/sql/sql_select.cc	2010-07-20 13:28:59 +0000
> @@ -13320,20 +13320,17 @@ void optimize_wo_join_buffering(JOIN *jo
>                                   double *reopt_rec_count, double *reopt_cost,
>                                   double *sj_inner_fanout)
>   {
> -  double cost, outer_fanout, inner_fanout= 1.0;
> +  double cost=0.0, outer_fanout=1.0, inner_fanout= 1.0;
>     table_map reopt_remaining_tables= last_remaining_tables;
>     uint i;
>
> +  DBUG_ENTER("optimize_wo_join_buffering");
> +
>     if (first_tab>  join->const_tables)
>     {
>       cost=      join->positions[first_tab - 1].prefix_cost.total_cost();
>       outer_fanout= join->positions[first_tab - 1].prefix_record_count;
>     }
> -  else
> -  {
> -    cost= 0.0;
> -    outer_fanout= 1;
> -  }
>
>     for (i= first_tab; i<= last_tab; i++)
>       reopt_remaining_tables |= join->positions[i].table->table->map;
> @@ -13346,14 +13343,19 @@ void optimize_wo_join_buffering(JOIN *jo
>       if ((i == first_tab&&  first_alt) ||
> join->positions[i].use_join_buffer)
>       {
>         /* Find the best access method that would not use join buffering */
> -      best_access_path(join, rs, reopt_remaining_tables, i,
> -                       test(i<  no_jbuf_before), inner_fanout*outer_fanout,
> -&pos,&loose_scan_pos);
> +      best_access_path(join,
> +                       rs,
> +                       reopt_remaining_tables,
> +                       i,
> +                       test(i<  no_jbuf_before),
> +                       inner_fanout*outer_fanout,
> +&pos,
> +&loose_scan_pos);
>       }
>       else
>         pos= join->positions[i];
>
> -    if ((i == first_tab&&  first_alt))
> +    if ((i == first_tab&&  first_alt&&  loose_scan_pos.read_time<
>  DBL_MAX))
>         pos= loose_scan_pos;
>
>       reopt_remaining_tables&= ~rs->table->map;
> @@ -13369,6 +13371,7 @@ void optimize_wo_join_buffering(JOIN *jo
>     *reopt_rec_count= outer_fanout;
>     *reopt_cost= cost;
>     *sj_inner_fanout= inner_fanout;
> +  DBUG_VOID_RETURN;
>   }
>
>
>
>
>
>
>


-- 
Øystein Grøvlen, Senior Staff Engineer
Sun Microsystems, Database Group
Trondheim, Norway
Thread
bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)Bug#55042Tor Didriksen20 Jul
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)Bug#55042Øystein Grøvlen21 Jul