List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:July 21 2010 2:46pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)
Bug#55042
View as plain text  
Looks OK, but I have two suggestions that I think you should consider:

On 07/21/10 02:45 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-21
 >        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())
 >
 >          Remove sj_inner_fanout output argument, it was unused.
 >
 >      modified:
 >        sql/sql_select.cc
 > === modified file 'sql/sql_select.cc'

...

 > -    if ((i == first_tab&&  first_alt))
 > +    if (i == first_tab&&  first_alt)
 > +    {
 > +      if (loose_scan_pos.read_time == DBL_MAX)
 > +      {
 > +        /* Loose scan not appliccable. */
 > +        *reopt_rec_count = DBL_MAX;
 > +        *reopt_cost= DBL_MAX;
 > +        DBUG_VOID_RETURN;
 > +      }
 >         pos= loose_scan_pos;
 > +    }

I suggest doing the test for DBL_MAX not only for loose_scan_pos, but
for all pos.  That would catch any cases where pos.read_time is
DBL_MAX, in which case I think we also should avoid adding it to
cost. In other words:

     if (i == first_tab && first_alt)
        pos= loose_scan_pos;

     if (pos.read_time == DBL_MAX)
     {
       *reopt_rec_count = DBL_MAX;
       *reopt_cost= DBL_MAX;
       DBUG_VOID_RETURN;
     }

 >
 >       reopt_remaining_tables&= ~rs->table->map;
 >       cost += pos.read_time;

...

 > +      if (reopt_cost == DBL_MAX)
 > +      {
 > +        DBUG_PRINT("info", ("Cannot use LooseScan"));
 > +      }
 > +      else
 > +      {
 > +        /*
 > +          We don't yet have any other strategies that could handle this
 > +          semi-join nest (the other options are Duplicate Elimination or
 > +          Materialization, which need at least the same set of tables in
 > +          the join prefix to be considered) so unconditionally pick the
 > +          LooseScan.
 > +        */
 > +        pos->sj_strategy= SJ_OPT_LOOSE_SCAN;
 > +        *current_read_time=    reopt_cost;
 > +        *current_record_count= reopt_rec_count;
 > +        handled_by_fm_or_ls= first->table->emb_sj_nest->sj_inner_tables;
 > +      }

I am not sure the extra test for DBL_MAX is necessary since one of the
other strategies will surely be cheaper.  But if you insists, I like
it better the other way around, but that is a mattter of taste:

       if (reopt_cost < DBL_MAX)
       {
         /*
           We don't yet have any other strategies that could handle this
           semi-join nest (the other options are Duplicate Elimination or
           Materialization, which need at least the same set of tables in
           the join prefix to be considered) so unconditionally pick the
           LooseScan.
         */
         pos->sj_strategy= SJ_OPT_LOOSE_SCAN;
         *current_read_time=    reopt_cost;
         *current_record_count= reopt_rec_count;
         handled_by_fm_or_ls= first->table->emb_sj_nest->sj_inner_tables;
       }
       else
       {
         DBUG_PRINT("info", ("Cannot use LooseScan"));
       }

-- 
Øystein
Thread
bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)Bug#55042Tor Didriksen21 Jul
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217)Bug#55042Øystein Grøvlen21 Jul