List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:April 29 2011 8:10am
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327
View as plain text  
Hi Jørgen,

Thank you for the bugfix. I have a few additional suggestions, and some problem 
areas that I think you need to look further into (ie the issue of fixed 
arguments to extend_...()).

Generally I like the changes, but I think that you may be even more consistent 
and e.g. spell out "condition" in full, at least in function and member names.

On 06.04.11 14.55, Jorgen Loland wrote:
> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
> revid:georgi.kodinov@stripped
>
>   3333 Jorgen Loland	2011-04-06
>        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
>         * Make JOIN_TAB::remove_sj_conditions(), needed because
>           select_cond is now private.
>         * 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_condition() since it sets JOIN_TAB->cond, not
>           JOIN_TAB->select->cond as the name suggested

It probably made more sense before your name change ;)

>         * Rename JOIN_TAB::set_cond() to
>           set_jt_and_sel_condition() since it sets
>           JOIN_TAB->cond AND 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:
>        sql/sql_select.cc
>        sql/sql_select.h
>        sql/sql_show.cc
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2011-04-04 08:47:25 +0000
> +++ b/sql/sql_select.cc	2011-04-06 12:55:38 +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_condition(curr_table->select->cond, __LINE__);
> +        curr_table->condition()->top_level_item();
>   	DBUG_EXECUTE("where",print_where(curr_table->select->cond,
>   					 "select and having",
>                                            QT_ORDINARY););
> @@ -3255,20 +3255,20 @@ JOIN::exec()
>             to get sort_table_cond. An alternative could be to use
>             Item::copy_andor_structure() to make a copy of sort_table_cond.
>           */
> -        if (curr_table->pre_idx_push_select_cond)
> +        if (curr_table->pre_idx_push_cond)
>           {
>             sort_table_cond= make_cond_for_table(curr_join->tmp_having,
>                                                  used_tables, used_tables, 0);
>             if (!sort_table_cond)
>               DBUG_VOID_RETURN;
> -          Item* new_pre_idx_push_select_cond=
> -            new Item_cond_and(curr_table->pre_idx_push_select_cond,
> +          Item* new_pre_idx_push_cond=
> +            new Item_cond_and(curr_table->pre_idx_push_cond,
>                                 sort_table_cond);
> -          if (!new_pre_idx_push_select_cond)
> +          if (!new_pre_idx_push_cond)
>               DBUG_VOID_RETURN;
> -          if (new_pre_idx_push_select_cond->fix_fields(thd, 0))
> +          if (new_pre_idx_push_cond->fix_fields(thd, 0))
>               DBUG_VOID_RETURN;
> -          curr_table->pre_idx_push_select_cond= new_pre_idx_push_select_cond;
> +          curr_table->pre_idx_push_cond= new_pre_idx_push_cond;
>   	}
>
>   	curr_join->tmp_having= make_cond_for_table(curr_join->tmp_having,
> @@ -3296,7 +3296,7 @@ JOIN::exec()
>   	    table->keyuse is set in the case there was an original WHERE clause
>   	    on the table that was optimized away.
>   	  */
> -	  if (curr_table->select_cond ||
> +	  if (curr_table->condition() ||
>   	      (curr_table->keyuse&&  !curr_table->first_inner))
>   	  {
>   	    /* We have to sort all rows */
> @@ -5666,7 +5666,7 @@ add_key_field(KEY_FIELD **key_fields,uin
>       othertbl.field can be NULL, there will be no matches if othertbl.field
>       has NULL value.
>       We use null_rejecting in add_not_null_conds() to add
> -    'othertbl.field IS NOT NULL' to tab->select_cond, if this is not an outer
> +    'othertbl.field IS NOT NULL' to tab->cond, if this is not an outer
>       join. We also use it to shortcut reading "tbl" when othertbl.field is
>       found to be a NULL value (in join_read_always_key() and BKA).
>     */
> @@ -9248,28 +9248,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)

add_conjunctive_condition is probably a better name for this function. Your choice.

The "fix" thing may just as well be documented in the description...

I think that this function requires inputs that are already fixed. Please 
reflect that in the documentation and add DBUG_ASSERTs on <input>->fixed.

>   {
>     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 (unlikely(!res))
> +      return true; //memory allocation error

Comment is redundant, the pattern is so common.

> +
> +    *e1= res;
> +    res->quick_fix_field();
> +    res->update_used_tables();
> +
>     }
>     else
>       *e1= e2;
> +  return false;
>   }
>
>
>   /**
> -  Add to join_tab->select_cond[i] "table.field IS NOT NULL" conditions
> +  Add to join_tab->cond[i] "table.field IS NOT NULL" conditions

Is "join_tab[i]->cond" what is really meant?

>     we've inferred from ref/eq_ref access performed.
>
>       This function is a part of "Early NULL-values filtering for ref access"
> @@ -9315,7 +9326,7 @@ inline void add_cond_and_fix(Item **e1,
>            predicates in in KEY_FIELD::null_rejecting
>         1.1 add_key_part saves these to Key_use.
>         2. create_ref_for_key copies them to TABLE_REF.
> -      3. add_not_null_conds adds "x IS NOT NULL" to join_tab->select_cond of
> +      3. add_not_null_conds adds "x IS NOT NULL" to join_tab->cond of
>            appropiate JOIN_TAB members.
>   */
>
> @@ -9359,9 +9370,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_cond(notnull, __LINE__);
>           }
>         }
>       }
> @@ -9498,24 +9507,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);
> -}
> -
> -
>   /**
>      Local helper function for make_join_select().
>
> @@ -9545,9 +9536,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();

I think this one is actually needed.
>
> -      if (extend_select_cond(cond_tab, tmp_cond))
> +      if (cond_tab->extend_jt_and_sel_cond(tmp_cond, __LINE__))
>           DBUG_RETURN(1);

DBUG_RETURN(true) ?
>       }
>     }
> @@ -9598,11 +9588,9 @@ static bool pushdown_on_conditions(JOIN*
>         */
>         tmp_cond= new Item_func_trig_cond(tmp_cond,
>                                           &first_inner_tab->not_null_compl);

Missing error check here?

> -      if (tmp_cond)
> -        tmp_cond->quick_fix_field();

I think that this quick_fix_field() call is actually needed. The extend_...() 
functions only seem to fix the new root node that they add.

>
>         /* Add the predicate to other pushed down predicates */
> -      if (extend_select_cond(cond_tab, tmp_cond))
> +      if (cond_tab->extend_jt_and_sel_cond(tmp_cond, __LINE__))
>           DBUG_RETURN(1);

DBUG_RETURN(true) ?

>       }
>       first_inner_tab= first_inner_tab->first_upper;
> @@ -9662,8 +9650,11 @@ static bool make_join_select(JOIN *join,
>                                 (table_map) 0, 1);
>           /* Add conditions added by add_not_null_conds(). */
>           for (uint i= 0 ; i<  join->const_tables ; i++)
> -          add_cond_and_fix(&const_cond, join->join_tab[i].select_cond);
> -
> +        {
> +          if (add_cond_and_fix(&const_cond, join->join_tab[i].condition()))
> +            DBUG_RETURN(1);
> +        }
> +
>           DBUG_EXECUTE("where",print_where(const_cond,"constants", QT_ORDINARY););
>           for (JOIN_TAB *tab= join->join_tab+join->const_tables;
>                tab<  join->join_tab+join->tables ; tab++)
> @@ -9679,13 +9670,9 @@ static bool make_join_select(JOIN *join,
>               tmp= new Item_func_trig_cond(tmp,&cond_tab->not_null_compl);
>               if (!tmp)
>                 DBUG_RETURN(1);
> -            tmp->quick_fix_field();

Same here...

> -            Item *new_cond= !cond_tab->select_cond ? tmp :
> -              new Item_cond_and(cond_tab->select_cond, tmp);
> -            cond_tab->set_select_cond(new_cond, __LINE__);
> -            if (!cond_tab->select_cond)
> -	      DBUG_RETURN(1);
> -            cond_tab->select_cond->quick_fix_field();
> +
> +            if (cond_tab->extend_cond(tmp, __LINE__))
> +              DBUG_RETURN(1);

DBUG_RETURN(true) ?
>             }
>           }
>           if (const_cond&&  !const_cond->val_int())
> @@ -9765,8 +9752,9 @@ static bool make_join_select(JOIN *join,
>         if (cond)
>           tmp= make_cond_for_table(cond,used_tables,current_map, 0);
>         /* Add conditions added by add_not_null_conds(). */
> -      if (tab->select_cond)
> -        add_cond_and_fix(&tmp, tab->select_cond);
> +      if (tab->condition()&&  add_cond_and_fix(&tmp,
> tab->condition()))
> +        DBUG_RETURN(1);
> +
DBUG_RETURN(true) ?
>
>         if (cond&&  !tmp&&  tab->quick)
>         {						// Outer join
> @@ -9815,7 +9803,7 @@ static bool make_join_select(JOIN *join,
>             if (!(tmp= add_found_match_trig_cond(first_inner_tab, tmp, 0)))
>               DBUG_RETURN(1);
>             sel->cond= tmp;
> -          tab->set_select_cond(tmp, __LINE__);
> +          tab->set_condition(tmp, __LINE__);
>             /* Push condition to storage engine if this is enabled
>                and the condition is not guarded */
>   	  if
> (thd->optimizer_switch_flag(OPTIMIZER_SWITCH_ENGINE_CONDITION_PUSHDOWN)&&
> @@ -9834,7 +9822,7 @@ static bool make_join_select(JOIN *join,
>           else
>           {
>             sel->cond= NULL;
> -          tab->set_select_cond(NULL, __LINE__);
> +          tab->set_condition(NULL, __LINE__);
>           }
>
>   	sel->head=tab->table;
> @@ -10054,7 +10042,7 @@ static bool uses_index_fields_only(Item
>       TODO: Consider cloning the triggered condition and using the copies for:
>         1. push the first copy down, to have most restrictive index condition
>            possible
> -      2. Put the second copy into tab->select_cond.
> +      2. Put the second copy into tab->cond.
>     */
>     if (item_type == Item::FUNC_ITEM&&
>         ((Item_func*)item)->functype() == Item_func::TRIG_COND_FUNC)
> @@ -10291,7 +10279,7 @@ Item *make_cond_remainder(Item *cond, bo
>     SYNOPSIS
>       push_index_cond()
>         tab            A join tab that has tab->table->file and its condition
> -                     in tab->select_cond
> +                     in tab->cond
>         keyno          Index for which extract and push the condition
>         other_tbls_ok  TRUE<=>  Fields of other non-const tables are allowed
>
> @@ -10319,7 +10307,7 @@ static void push_index_cond(JOIN_TAB *ta
>          that can be turned on or off during execution of a 'Full scan on NULL
>          key'.
>     */
> -  if (tab->select_cond&&
> +  if (tab->condition()&&
>         tab->table->file->index_flags(keyno, 0, 1)&
>         HA_DO_INDEX_COND_PUSHDOWN&&
>        
> tab->join->thd->optimizer_switch_flag(OPTIMIZER_SWITCH_INDEX_CONDITION_PUSHDOWN)&&
> @@ -10327,15 +10315,15 @@ static void push_index_cond(JOIN_TAB *ta
>         tab->join->thd->lex->sql_command !=
> SQLCOM_DELETE_MULTI&&
>         !tab->has_guarded_conds())
>     {
> -    DBUG_EXECUTE("where", print_where(tab->select_cond, "full cond",
> +    DBUG_EXECUTE("where", print_where(tab->condition(), "full cond",
>                    QT_ORDINARY););
> -    Item *idx_cond= make_cond_for_index(tab->select_cond, tab->table, keyno,
> -                                        other_tbls_ok);
> +    Item *idx_cond= make_cond_for_index(tab->condition(), tab->table,
> +                                        keyno, other_tbls_ok);
>       DBUG_EXECUTE("where", print_where(idx_cond, "idx cond", QT_ORDINARY););
>       if (idx_cond)
>       {
>         Item *idx_remainder_cond= 0;
> -      tab->pre_idx_push_select_cond= tab->select_cond;
> +      tab->pre_idx_push_cond= tab->condition();
>
>         /*
>           For BKA cache we store condition to special BKA cache field
> @@ -10373,30 +10361,30 @@ static void push_index_cond(JOIN_TAB *ta
>         if (idx_remainder_cond != idx_cond)
>           tab->ref.disable_cache= TRUE;
>
> -      Item *row_cond= make_cond_remainder(tab->select_cond, TRUE);
> +      Item *row_cond= make_cond_remainder(tab->condition(), TRUE);
>         DBUG_EXECUTE("where", print_where(row_cond, "remainder cond",
>                      QT_ORDINARY););
>
>         if (row_cond)
>         {
>           if (!idx_remainder_cond)
> -          tab->set_select_cond(row_cond, __LINE__);
> +          tab->set_condition(row_cond, __LINE__);
>           else
>           {
>             Item *new_cond= new Item_cond_and(row_cond, idx_remainder_cond);

Missing error check here. Can you use the add_cond...() function here as well?

> -          tab->set_select_cond(new_cond, __LINE__);
> -	  tab->select_cond->quick_fix_field();
> -          ((Item_cond_and*)tab->select_cond)->used_tables_cache=
> +          tab->set_condition(new_cond, __LINE__);
> +          tab->condition()->quick_fix_field();
> +          ((Item_cond_and*)tab->condition())->used_tables_cache=
>               row_cond->used_tables() | idx_remainder_cond->used_tables();
>           }
>         }
>         else
> -        tab->set_select_cond(idx_remainder_cond, __LINE__);
> +        tab->set_condition(idx_remainder_cond, __LINE__);
>         if (tab->select)
>         {
> -        DBUG_EXECUTE("where", print_where(tab->select->cond, "select_cond",
> +        DBUG_EXECUTE("where", print_where(tab->select->cond, "cond",
>                        QT_ORDINARY););
> -        tab->select->cond= tab->select_cond;
> +        tab->select->cond= tab->condition();
>         }
>       }
>     }
> @@ -10897,19 +10885,16 @@ static bool is_cond_sj_in_equality(Item
>   }
>
>
> -void remove_sj_conds(Item **tree)
> +Item *remove_sj_conds(Item *tree)

Please document this function with a header.

>   {
> -  if (*tree)
> +  if (tree)
>     {
> -    if (is_cond_sj_in_equality(*tree))
> -    {
> -      *tree= NULL;
> -      return;
> -    }
> -    else if ((*tree)->type() == Item::COND_ITEM)
> +    if (is_cond_sj_in_equality(tree))
> +      return NULL;
> +    else if (tree->type() == Item::COND_ITEM)
>       {
>         Item *item;
> -      List_iterator<Item>  li(*(((Item_cond*)*tree)->argument_list()));
> +      List_iterator<Item>  li(*(((Item_cond*)tree)->argument_list()));
>         while ((item= li++))
>         {
>           if (is_cond_sj_in_equality(item))
> @@ -10917,9 +10902,9 @@ void remove_sj_conds(Item **tree)
>         }
>       }
>     }
> +  return tree;
>   }
>
> -
>   /*
>     Create subquery equalities assuming use of materialization strategy
>
> @@ -11078,10 +11063,12 @@ bool setup_sj_materialization(JOIN_TAB *
>       */
>       for (i= 0; i<  sjm->table_count; i++)
>       {
> -      remove_sj_conds(&tab[i].select_cond);
> +      Item *new_cond= remove_sj_conds(tab[i].condition());
> +      tab[i].set_condition(new_cond, __LINE__);

"new_cond" may be replaced with the function call here (for symmetry :)

>         if (tab[i].select)
> -        remove_sj_conds(&tab[i].select->cond);
> +        tab[i].select->cond= remove_sj_conds(tab[i].select->cond);
>       }
> +
>       if (!(sjm->in_equality= create_subquery_equalities(thd, emb_sj_nest)))
>         DBUG_RETURN(TRUE); /* purecov: inspected */
>     }
> @@ -11470,6 +11457,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

I think there is unnecessary information here. We should ALWAYS leave things 
unchanged when there is an error, thus we only need to document if there is an 
exception to this rule.

> +
> +*/
> +bool JOIN_TAB::extend_jt_and_sel_cond(Item *add_cond, uint line)
> +{
> +  if (extend_cond(add_cond, line))
> +    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_cond(Item *add_cond, uint line)
> +{
> +  Item *old_cond= cond;
> +  if (add_cond_and_fix(&cond, add_cond))
> +    return true;
> +  DBUG_PRINT("info", ("JOIN_TAB::cond extended. Change %p ->  %p "
> +                      "at line %u tab %p",
> +                      old_cond, cond, line, this));
> +  return false;
> +}
> +
> +
> +

I do not grasp the difference between extend_cond and extend_jt_and_sel_cond(). 
Is it possible to document the difference and insert an advice on which function 
to use in which condition?

Besides, we should make up our mind on whether to "extend" a condition or "add" 
something to a condition, cf. add_cond_and_fix().

>
>   /**
>     Partially cleanup JOIN after it has executed: close index or rnd read
> @@ -17259,10 +17298,10 @@ sub_select_sjm(JOIN *join, JOIN_TAB *joi
>       /* Do full scan of the materialized table */
>       JOIN_TAB *last_tab= join_tab + (sjm->table_count - 1);
>
> -    Item *save_cond= last_tab->select_cond;
> -    last_tab->set_select_cond(sjm->join_cond, __LINE__);
> +    Item *save_cond= last_tab->condition();
> +    last_tab->set_condition(sjm->join_cond, __LINE__);
>       rc= sub_select(join, last_tab, end_of_records);
> -    last_tab->set_select_cond(save_cond, __LINE__);
> +    last_tab->set_condition(save_cond, __LINE__);
>       DBUG_RETURN(rc);
>     }
>     else
> @@ -17391,7 +17430,7 @@ sub_select_cache(JOIN *join, JOIN_TAB *j
>       given the selected plan prescribes to nest retrievals of the
>       joined tables in the following order: t1,t2,t3.
>       A pushed down predicate are attached to the table which it pushed to,
> -    at the field join_tab->select_cond.
> +    at the field join_tab->cond.
>       When executing a nested loop of level k the function runs through
>       the rows of 'join_tab' and for each row checks the pushed condition
>       attached to the table.
> @@ -17689,14 +17728,14 @@ evaluate_join_record(JOIN *join, JOIN_TA
>   {
>     bool not_used_in_distinct=join_tab->not_used_in_distinct;
>     ha_rows found_records=join->found_records;
> -  Item *select_cond= join_tab->select_cond;
> +  Item *jointab_cond= join_tab->condition();

SImply "condition" would do well. It may also be const.
>     bool found= TRUE;
>
>     DBUG_ENTER("evaluate_join_record");
>
>     DBUG_PRINT("enter",
>                ("evaluate_join_record join: %p join_tab: %p"
> -              " cond: %p error: %d", join, join_tab, select_cond, error));
> +              " cond: %p error: %d", join, join_tab, jointab_cond, error));
>     if (error>  0 || (join->thd->is_error()))     // Fatal error
>       DBUG_RETURN(NESTED_LOOP_ERROR);
>     if (error<  0)
> @@ -17706,11 +17745,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 %p", jointab_cond));
>
> -  if (select_cond)
> +  if (jointab_cond)
>     {
> -    found= test(select_cond->val_int());
> +    found= test(jointab_cond->val_int());
>
>       /* check for errors evaluating the condition */
>       if (join->thd->is_error())
> @@ -17719,7 +17758,7 @@ evaluate_join_record(JOIN *join, JOIN_TA
>     if (found)
>     {
>       /*
> -      There is no select condition or the attached pushed down
> +      There is no condition on this JOIN_TAB or the attached pushed down
>         condition is true =>  a match is found.
>       */
>       while (join_tab->first_unmatched&&  found)
> @@ -17745,7 +17784,7 @@ evaluate_join_record(JOIN *join, JOIN_TA
>           */
>           /*
>             not_exists_optimize has been created from a
> -          select_cond containing 'is_null'. This 'is_null'
> +          condition containing 'is_null'. This 'is_null'
>             predicate is still present on any 'tab' with
>             'not_exists_optimize'. Furthermore, the usual rules
>             for condition guards also applies for
> @@ -17754,9 +17793,9 @@ evaluate_join_record(JOIN *join, JOIN_TA
>             the 'not_exists_optimize'.
>           */
>           DBUG_ASSERT(!(tab->table->reginfo.not_exists_optimize&&
> -                     !tab->select_cond));
> +                     !tab->condition()));
>
> -        if (tab->select_cond&&  !tab->select_cond->val_int())
> +        if (tab->condition()&&  !tab->condition()->val_int())
>           {
>             /* The condition attached to table tab is false */
>
> @@ -17921,7 +17960,7 @@ evaluate_null_complemented_join_record(J
>     */
>     JOIN_TAB *last_inner_tab= join_tab->last_inner;
>     /* Cache variables for faster loop */
> -  Item *select_cond;
> +  Item *jointab_cond;

This variable is better removed.

>
>     DBUG_ENTER("evaluate_null_complemented_join_record");
>
> @@ -17933,9 +17972,9 @@ evaluate_null_complemented_join_record(J
>       /* The outer row is complemented by nulls for each inner tables */
>       restore_record(join_tab->table,s->default_values);  // Make empty record
>       mark_as_null_row(join_tab->table);       // For group by without error
> -    select_cond= join_tab->select_cond;
> +    jointab_cond= join_tab->condition();
>       /* Check all attached conditions for inner table rows. */
> -    if (select_cond&&  !select_cond->val_int())
> +    if (jointab_cond&&  !jointab_cond->val_int())
>         DBUG_RETURN(NESTED_LOOP_OK);
>     }
>     join_tab= last_inner_tab;
> @@ -18661,8 +18700,8 @@ end_send(JOIN *join, JOIN_TAB *join_tab
>         {
>   	JOIN_TAB *jt=join->join_tab;
>   	if ((join->tables == 1)&&  !join->tmp_table&& 
> !join->sort_and_group
> -	&&  !join->send_group_parts&&  !join->having&& 
> !jt->select_cond&&
> -	    !(jt->select&&  jt->select->quick)&&
> +&&  !join->send_group_parts&&  !join->having&&
> +            !jt->condition()&&  !(jt->select&& 
> jt->select->quick)&&
>   	(jt->table->file->ha_table_flags()& 
> HA_STATS_RECORDS_IS_EXACT)&&
>               (jt->ref.key<  0))

What about spelling this if test out with one predicate per line?

>   	{
> @@ -19995,8 +20034,8 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
>     SQL_SELECT *select=tab->select;
>     QUICK_SELECT_I *save_quick= 0;
>     int best_key= -1;
> -  Item *orig_select_cond= 0;
> -  bool orig_select_cond_saved= false;
> +  Item *orig_cond= 0;
> +  bool orig_cond_saved= false;
>     bool changed_key= false;
>     DBUG_ENTER("test_if_skip_sort_order");
>     LINT_INIT(ref_key_parts);
> @@ -20070,10 +20109,11 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
>           select condition is saved so that it can be restored when
>           exiting this function (if we have not changed index).
>         */
> -      if (tab->pre_idx_push_select_cond)
> +      if (tab->pre_idx_push_cond)
>         {
> -        orig_select_cond= tab->set_cond(tab->pre_idx_push_select_cond,
> __LINE__);
> -        orig_select_cond_saved= true;
> +        orig_cond=
> +          tab->set_jt_and_sel_condition(tab->pre_idx_push_cond, __LINE__);
> +        orig_cond_saved= true;
>         }
>
>         if ((new_ref_key= test_if_subkey(order, table, ref_key, ref_key_parts,
> @@ -20244,15 +20284,15 @@ check_reverse_order:
>
>           if (table->covering_keys.is_set(best_key))
>             table->set_keyread(TRUE);
> -        if (tab->pre_idx_push_select_cond)
> +        if (tab->pre_idx_push_cond)
>           {
> -          tab->set_cond(tab->pre_idx_push_select_cond, __LINE__);
> +          tab->set_jt_and_sel_condition(tab->pre_idx_push_cond, __LINE__);
>             /*
> -            orig_select_cond is a part of pre_idx_push_select_cond,
> +            orig_cond is a part of pre_idx_push_cond,
>               no need to restore it.
>             */
> -          orig_select_cond= 0;
> -          orig_select_cond_saved= false;
> +          orig_cond= 0;
> +          orig_cond_saved= false;
>           }
>           table->file->ha_index_or_rnd_end();
>           if (tab->join->select_options&  SELECT_DESCRIBE)
> @@ -20334,8 +20374,8 @@ skipped_filesort:
>       Restore condition only if we didn't chose index different to what we used
>       for ICP.
>     */
> -  if (orig_select_cond_saved&&  !changed_key)
> -    tab->set_cond(orig_select_cond, __LINE__);
> +  if (orig_cond_saved&&  !changed_key)
> +    tab->set_jt_and_sel_condition(orig_cond, __LINE__);
>     DBUG_RETURN(1);
>
>   use_filesort:
> @@ -20345,8 +20385,8 @@ use_filesort:
>       delete select->quick;
>       select->quick= save_quick;
>     }
> -  if (orig_select_cond_saved)
> -    tab->set_cond(orig_select_cond, __LINE__);
> +  if (orig_cond_saved)
> +    tab->set_jt_and_sel_condition(orig_cond, __LINE__);
>     DBUG_RETURN(0);
>   }
>
> @@ -20487,7 +20527,7 @@ create_sort_index(THD *thd, JOIN *join,
>       table->quick_keys.clear_all();  // as far as we cleanup select->quick
>       table->sort.io_cache= tablesort_result_cache;
>     }
> -  tab->set_select_cond(NULL, __LINE__);
> +  tab->set_condition(NULL, __LINE__);
>     tab->last_inner= 0;
>     tab->first_unmatched= 0;
>     tab->type=JT_ALL;				// Read with normal read_record
> @@ -22261,26 +22301,26 @@ static bool add_ref_to_table_cond(THD *t
>       if (join_tab->select->cond)
>         error=(int) cond->add(join_tab->select->cond);
>       join_tab->select->cond= cond;
> -    join_tab->set_select_cond(cond, __LINE__);
> +    join_tab->set_condition(cond, __LINE__);
>     }
>     else if ((join_tab->select= make_select(join_tab->table, 0, 0, cond, 0,
>                                             &error)))
> -    join_tab->set_select_cond(cond, __LINE__);
> +    join_tab->set_condition(cond, __LINE__);
>
>     /*
>       If we have pushed parts of the select condition down to the
>       storage engine we also need to add the condition for the const
> -    reference to the pre_idx_push_select_cond since this might be used
> -    later (in test_if_skip_sort_order()) instead of the select_cond.
> +    reference to the pre_idx_push_cond since this might be used
> +    later (in test_if_skip_sort_order()) instead of the cond.
>     */
> -  if (join_tab->pre_idx_push_select_cond)
> +  if (join_tab->pre_idx_push_cond)
>     {
>       cond= create_cond_for_const_ref(thd, join_tab);
>       if (!cond)
>         DBUG_RETURN(TRUE);
> -    if (cond->add(join_tab->pre_idx_push_select_cond))
> +    if (cond->add(join_tab->pre_idx_push_cond))
>         DBUG_RETURN(TRUE);
> -    join_tab->pre_idx_push_select_cond = cond;
> +    join_tab->pre_idx_push_cond = cond;
>     }
>
>     DBUG_RETURN(error ? TRUE : FALSE);
>
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2011-03-29 07:20:17 +0000
> +++ b/sql/sql_select.h	2011-04-06 12:55:38 +0000
> @@ -258,7 +258,9 @@ typedef struct st_join_table : public Sq
>     TABLE		*table;
>     Key_use	*keyuse;			/**<  pointer to first used key */
>     SQL_SELECT	*select;
> -  Item		*select_cond;
> +private:
> +  Item          *cond;          /**<  condition for this JOIN_TAB              
> */

Four-letter member names are too brief in large class definitions (albeit this 
is pretty well-defined). I suggest m_condition instead. (The prefix is needed 
because of the clash with condition()).

> +public:
>     QUICK_SELECT_I *quick;
>     Item	       **on_expr_ref;   /**<  pointer to the associated on expression  
> */
>     COND_EQUAL    *cond_equal;    /**<  multiple equalities for the on expression
> */
> @@ -269,13 +271,13 @@ typedef struct st_join_table : public Sq
>     st_join_table *first_upper;  /**<  first inner table for embedding outer join
> */
>     st_join_table *first_unmatched; /**<  used for optimization purposes only    
> */
>     /*
> -    The value of select_cond before we've attempted to do Index Condition
> +    The value of cond before we've attempted to do Index Condition
>       Pushdown. We may need to restore everything back if we first choose one
>       index but then reconsider (see test_if_skip_sort_order() for such
>       scenarios).
>       NULL means no index condition pushdown was performed.
>     */
> -  Item          *pre_idx_push_select_cond;
> +  Item          *pre_idx_push_cond;
>
>     /* Special content for EXPLAIN 'Extra' column or NULL if none */
>     const char	*info;
> @@ -437,22 +439,29 @@ typedef struct st_join_table : public Sq
>     {
>       return first_inner&&  first_inner == this;
>     }
> -  void set_select_cond(Item *to, uint line)
> +  Item *condition()
> +  {
> +    return cond;
> +  }
> +  void set_condition(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_condition(Item *new_cond, uint line)
>     {
> -    Item *tmp_select_cond= select_cond;
> -    set_select_cond(new_cond, line);
> +    Item *tmp_cond= cond;
> +    set_condition(new_cond, line);
>       if (select)
>         select->cond= new_cond;
> -    return tmp_select_cond;
> +    return tmp_cond;
>     }
>     uint get_sj_strategy() const;
>
> +  bool extend_cond(Item *tmp_cond, uint line);
> +  bool extend_jt_and_sel_cond(Item *tmp_cond, uint line);
>
>     /**
>       Check if there are triggered/guarded conditions that might be
> @@ -468,13 +477,12 @@ typedef struct st_join_table : public Sq
>     }
>   } JOIN_TAB;
>
> -
>   inline
>   st_join_table::st_join_table()
>     : table(NULL),
>       keyuse(NULL),
>       select(NULL),
> -    select_cond(NULL),
> +    cond(NULL),
>       quick(NULL),
>       on_expr_ref(NULL),
>       cond_equal(NULL),
> @@ -484,7 +492,7 @@ st_join_table::st_join_table()
>       last_inner(NULL),
>       first_upper(NULL),
>       first_unmatched(NULL),
> -    pre_idx_push_select_cond(NULL),
> +    pre_idx_push_cond(NULL),
>       info(NULL),
>       packed_info(0),
>       read_first_record(NULL),
>
> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2011-04-04 08:47:25 +0000
> +++ b/sql/sql_show.cc	2011-04-06 12:55:38 +0000
> @@ -6739,7 +6739,7 @@ bool get_schema_tables_result(JOIN *join
>           table_list->table->file->stats.records= 0;
>
>         if (table_list->schema_table->fill_table(thd, table_list,
> -                                               tab->select_cond))
> +                                               tab->condition()))
>         {
>           result= 1;
>           join->error= 1;
Thanks,
Roy

Thread
bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Jorgen Loland6 Apr
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Guilhem Bichot7 Apr
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Roy Lyseng29 Apr
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Jorgen Loland2 May
      • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Roy Lyseng2 May