List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:March 31 2011 5:06pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327
View as plain text  
Hello Jorgen,

Jorgen Loland a écrit, Le 25.03.2011 14:10:
> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
> revid:olav.sandstaa@stripped
> 
>  3323 Jorgen Loland	2011-03-25
>       BUG#11766327: Range analysis should not be done many times 
>                     for the   same index
>       
>       (formerly 59415)

The patch looks good, only minor details below. Quite close to a push.

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-03-24 13:16:36 +0000
> +++ b/sql/sql_select.cc	2011-03-25 13:10:57 +0000
> @@ -3240,8 +3240,8 @@ JOIN::exec()
>  	    DBUG_VOID_RETURN;
>  	  curr_table->select->cond->fix_fields(thd, 0);
>  	}
> -        curr_table->set_select_cond(curr_table->select->cond, __LINE__);
> -	curr_table->select_cond->top_level_item();
> +        curr_table->set_cond(curr_table->select->cond, __LINE__);
> +        curr_table->get_cond()->top_level_item();
>  	DBUG_EXECUTE("where",print_where(curr_table->select->cond,
>  					 "select and having",
>                                           QT_ORDINARY););
> @@ -9244,28 +9244,39 @@ JOIN::make_simple_join(JOIN *parent, TAB
>    DBUG_RETURN(FALSE);
>  }
>  
> +/**
> +  Extend e1 by AND'ing e2 to the condition e1 points to. 
> +
> +  @param[in,out]   e1 Pointer to condition that will be extended with e2
> +  @param           e2 Condition that will extend e1
>  
> -inline void add_cond_and_fix(Item **e1, Item *e2)
> +  @retval true   if there was a memory allocation error, in which case
> +                 e1 remains unchanged
> +  @retval false  otherwise
> +*/
> +inline bool add_cond_and_fix(Item **e1, Item *e2)

now it returns bool but there are a couple of calls to this function 
which ignore the new return code.

>  {
>    if (*e1)
>    {
>      if (!e2)
> -      return;
> -    Item *res;
> -    if ((res= new Item_cond_and(*e1, e2)))
> -    {
> -      *e1= res;
> -      res->quick_fix_field();
> -      res->update_used_tables();
> -    }
> +      return false;
> +    Item *res= new Item_cond_and(*e1, e2);
> +    if (!res)

could add unlikely()

> +      return true;
> +
> +    *e1= res;
> +    res->quick_fix_field();
> +    res->update_used_tables();
> +
>    }
>    else
>      *e1= e2;
> +  return false;
>  }
>  

> @@ -9355,9 +9366,7 @@ static void add_not_null_conds(JOIN *joi
>            DBUG_EXECUTE("where",print_where(notnull,
>                                             referred_tab->table->alias,
>                                             QT_ORDINARY););
> -          Item *new_cond= referred_tab->select_cond;
> -          add_cond_and_fix(&new_cond, notnull);
> -          referred_tab->set_select_cond(new_cond, __LINE__);
> +          referred_tab->extend_and_fix_cond(notnull, __LINE__);

Return code is not checked.
Note: if the code already had this problem before the patch, then ignore 
my remarks.

> @@ -9541,9 +9532,8 @@ static bool pushdown_on_conditions(JOIN*
>        tmp_cond= new Item_func_trig_cond(tmp_cond,
> &cond_tab->not_null_compl);
>        if (!tmp_cond)
>          DBUG_RETURN(1);
> -      tmp_cond->quick_fix_field();

You sure this line is not needed ?

>  
> -      if (extend_select_cond(cond_tab, tmp_cond))
> +      if (cond_tab->extend_and_fix_jt_and_sel_cond(tmp_cond, __LINE__))
>          DBUG_RETURN(1);
>      }       
>    }

> @@ -11072,10 +11056,12 @@ bool setup_sj_materialization(JOIN_TAB *
>      */
>      for (i= 0; i < sjm->table_count; i++)
>      {
> -      remove_sj_conds(&tab[i].select_cond);
> +      Item *cond= tab[i].get_cond();

If this intermediate variable is not needed I suggest to delete it.
But maybe it is needed, I haven't thought about return-by-value, lvalue etc.

> +      remove_sj_conds(&cond);
>        if (tab[i].select)
>          remove_sj_conds(&tab[i].select->cond);
>      }
> +
>      if (!(sjm->in_equality= create_subquery_equalities(thd, emb_sj_nest)))
>        DBUG_RETURN(TRUE); /* purecov: inspected */
>    }
> @@ -11464,6 +11450,58 @@ uint JOIN_TAB::get_sj_strategy() const
>    return s;
>  }
>  
> +/**
> +  Extend JOIN_TAB->cond and JOIN_TAB->select->cond by AND'ing
> +  add_cond to them
> +
> +  @param add_cond   The condition to AND with the existing conditions
> +  @param line       Code line this method was called from
> +
> +  @retval true   if there was a memory allocation error, in which 
> +                 case the conditions remain unchanged
> +  @retval false  otherwise
> +
> +*/
> +bool JOIN_TAB::extend_and_fix_jt_and_sel_cond(Item *add_cond, uint line)
> +{
> +  if (extend_and_fix_cond(add_cond, line))

could add unlikely()

> +    return true;
> +
> +  if (select)
> +  {
> +    DBUG_PRINT("info", 
> +               ("select::cond extended. Change %p -> %p "
> +                "at line %u tab %p select %p",
> +                select->cond, cond, line, this, select));
> +    select->cond= cond;
> +  }
> +  return false;
> +}
> +
> +/**
> +  Extend JOIN_TAB->cond by AND'ing add_cond to it
> +
> +  @param add_cond    The condition to AND with the existing cond
> +                     for this JOIN_TAB
> +  @param line        Code line this method was called from
> +
> +  @retval true   if there was a memory allocation error, in which case 
> +                 JOIN_TAB::cond remains unchanged
> +  @retval false  otherwise
> +*/
> +bool JOIN_TAB::extend_and_fix_cond(Item *add_cond, uint line)
> +{
> +  Item *old_cond= cond;
> +  if (add_cond_and_fix(&cond, add_cond))

could add unlikely()

> +    return true;
> +  DBUG_PRINT("info", ("JOIN_TAB::cond extended. Change %p -> %p "
> +                      "at line %u tab %p",
> +                      old_cond, cond, line, this));
> +  return false;
> +}
> +
> +
> +
>  

Thread
bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327Jorgen Loland25 Mar
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327Guilhem Bichot31 Mar
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327Roy Lyseng31 Mar
      • Re: bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327Guilhem Bichot31 Mar
        • Re: bzr commit into mysql-trunk branch (jorgen.loland:3323) Bug#11766327Roy Lyseng31 Mar