List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:February 17 2011 3:33pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327
View as plain text  
Hello,

Jorgen Loland a écrit, Le 16.02.2011 10:27:
> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
> revid:dmitry.lenev@stripped
> 
>  3653 Jorgen Loland	2011-02-16
>       BUG#11766327: Range analysis should not be done many times 
>                     for the   same index
>       
>       (formerly 59415)
>       
>       Pre-bugfix changes: 
>        * Make JOIN_TAB::select_cond private, add getter
>        * Rename JOIN_TAB::select_cond to cond
>        * Rename JOIN_TAB::pre_idx_push_select_cond to 
>          pre_idx_push_cond
>        * Make use of extend_*_cond() where possible to 
>          simplify code
>        * Rename JOIN_TAB::set_select_cond() to 
>          set_jt_cond() since it sets JOIN_TAB->cond, not 
>          JOIN_TAB->select->cond as the name suggested
>        * Rename JOIN_TAB::set_cond() to set_jt_and_sel_cond()
>          since it sets JOIN_TAB->cond AND JOIN_TAB->select->cond

My curiosity: do you know how JOIN_TAB->cond differs from 
JOIN_TAB->select->cond ?

>      @ sql/sql_select.cc
>         JOIN_TAB::cond made private, rename JOIN_TAB functions for getting and
> setting condition to better reflect reality, use extend_*_cond() where possible to
> simplify code.
>      @ sql/sql_select.h
>         Fix naming of JOIN_TAB variables and functions to better reflect reality.
> Make JOIN_TAB::cond (formerly select_cond) private.
>      @ sql/sql_show.cc
>         JOIN_TAB::cond is made private

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-02-10 09:30:20 +0000
> +++ b/sql/sql_select.cc	2011-02-16 09:27:55 +0000

> @@ -9301,9 +9301,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_jt_cond(notnull);

Difference between old and new code: if referred_tab->cond is initially 
NULL, old code (in add_cond_and_fix()) just did
referred_tab->cond= notnull;
but new code additionally does
   notnull->update_used_tables();
   notnull->quick_fix_field();
Is that ok? I see that the old extend_select_cond() did that too, so 
maybe it's ok.

I wonder: after the patch the function add_cond_and_fix() still exists 
is is used, and it seems to have common points with extend_jt_cond(). 
Duplication... can't we have one call the other?

>          }
>        }
>      }
> @@ -9440,24 +9438,6 @@ make_outerjoin_info(JOIN *join)
>    DBUG_VOID_RETURN;
>  }
>  
> -static bool extend_select_cond(JOIN_TAB *cond_tab, Item *tmp_cond)
> -{
> -  DBUG_ENTER("extend_select_cond");
> -
> -  Item *new_cond= !cond_tab->select_cond ? tmp_cond :
> -    new Item_cond_and(cond_tab->select_cond, tmp_cond);
> -  cond_tab->set_select_cond(new_cond, __LINE__);
> -  if (!cond_tab->select_cond)
> -    DBUG_RETURN(1);
> -  cond_tab->select_cond->update_used_tables();
> -  cond_tab->select_cond->quick_fix_field();
> -  if (cond_tab->select)
> -    cond_tab->select->cond= cond_tab->select_cond; 
> -
> -  DBUG_RETURN(0);
> -}
> -
> -

> @@ -10819,19 +10796,19 @@ static bool is_cond_sj_in_equality(Item 
>  }
>  
>  
> -void remove_sj_conds(Item **tree)
> +void remove_sj_conds(Item *cond)
>  {
> -  if (*tree)
> +  if (cond)
>    {
> -    if (is_cond_sj_in_equality(*tree))
> +    if (is_cond_sj_in_equality(cond))
>      {
> -      *tree= NULL;
> +      cond= NULL;

"cond" is a parameter, local for your function; setting it to NULL has 
no effect (it will be lost after return). Surprising that no test fails :-(
Setting *tree had effects outside of the function.

>        return;
>      }

> +/**
> +  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
> +
> +  @return
> +    true   if there was an error, in which case the conditions remain
> +           unchanged
> +    false  otherwise

The multi-line @return shows up like this in the generated doxygen html 
output:

> Returns:
>     true if there was an error, in which case JOIN_TAB::cond remains unchanged false
> otherwise 

So it's hard to parse. I suggest to delete @return and use instead:

   @retval  true   if there was an error, in which case JOIN_TAB::cond 
remains unchanged
   @retval false  otherwise

which produces

> Return values:
>     	true 	if there was an error, in which case JOIN_TAB::cond remains unchanged
>     	false 	otherwise 

("unchanged" can be put on the next line in the comment in the code 
file, doxygen does not care about such new line, it "glues" everything).

> +*/
> +bool JOIN_TAB::extend_jt_and_sel_cond(Item *add_cond)
> +{
> +  if (extend_jt_cond(add_cond))
> +    return true;
> +  if (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
> +  @return
> +    true   if there was an error, in which case JOIN_TAB::cond remains
> +           unchanged

if "cond" is NULL and "add_cond" is NULL, this function will return 
true, but should this really be interpreted as an error? This is 
important to decide, because an error makes extend_jt_and_sel_cond() 
terminate before setting select->cond.
Or maybe it's impossible that add_cond is NULL (can we add DBUG_ASSERT() 
for this then?); that case would indeed be a bit meaningless.

> +    false  otherwise
> +*/
> +bool JOIN_TAB::extend_jt_cond(Item *add_cond)
> +{
> +  DBUG_ENTER("JOIN_TAB::extend_jt_cond");
> +
> +  Item *new_cond= cond ?
> +    new Item_cond_and(cond, add_cond) :
> +    add_cond;
> +
> +  if (!new_cond)
> +    DBUG_RETURN(true);
> +
> +  set_jt_cond(new_cond, __LINE__);
> +
> +  cond->update_used_tables();
> +  cond->quick_fix_field();
> +
> +  DBUG_RETURN(false);
> +}

> @@ -17630,11 +17658,11 @@ evaluate_join_record(JOIN *join, JOIN_TA
>      join->thd->send_kill_message();
>      DBUG_RETURN(NESTED_LOOP_KILLED);            /* purecov: inspected */
>    }
> -  DBUG_PRINT("info", ("select cond 0x%lx", (ulong)select_cond));
> +  DBUG_PRINT("info", ("jointab cond 0x%lx", (ulong)jointab_cond));

can change to %p and no cast, while we're at it.

> @@ -416,21 +418,27 @@ typedef struct st_join_table : public Sq
>    {
>      return first_inner && first_inner == this;
>    }
> -  void set_select_cond(Item *to, uint line)
> +  Item *get_jt_cond()
> +  {
> +    return cond;
> +  }

Good that you didn't declare it const, because logically it's not const 
(assuming there exists plenty of code which calls "modifying" functions 
on the pointer returned by get_jt_cond(); which you can check by 
changing the return type to "const Item*", for fun...).

> +  void set_jt_cond(Item *to, uint line)
>    {
> -    DBUG_PRINT("info", ("select_cond changes %p -> %p at line %u tab %p",
> -                        select_cond, to, line, this));
> -    select_cond= to;
> +    DBUG_PRINT("info", ("JOIN_TAB::cond changes %p -> %p at line %u tab %p",
> +                        cond, to, line, this));
> +    cond= to;
>    }
> -  Item *set_cond(Item *new_cond, uint line)
> +  Item *set_jt_and_sel_cond(Item *new_cond, uint line)

I'd put a function comment to say this returns the old cond.

>    {
> -    Item *tmp_select_cond= select_cond;
> -    set_select_cond(new_cond, line);
> +    Item *tmp_cond= cond;
> +    set_jt_cond(new_cond, line);
>      if (select)
>        select->cond= new_cond;
> -    return tmp_select_cond;
> +    return tmp_cond;
>    }

Thread
bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Jorgen Loland16 Feb
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Guilhem Bichot17 Feb
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Jorgen Loland25 Mar