List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:August 4 2010 4:01pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch
(martin.hansson:3203) Bug#33062
View as plain text  
Hi Martin,

On 04.08.10 16.59, Martin Hansson wrote:
> #At file:///data0/martin/bzr/bug33062/n-mr-o-b/ based on
> revid:epotemkin@stripped
>
>   3203 Martin Hansson	2010-08-04
>        Bug#33062: subquery in stored routine cause crash
>
>        Post back-port fix. The semantics for replace_where_subcondition has changed,
> so the name and documentation has been changed to reflect the new. Some warnings are also
> eliminated.
>
>      modified:
>        sql/sql_select.cc
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-06-25 12:30:30 +0000
> +++ b/sql/sql_select.cc	2010-08-04 14:59:48 +0000
> @@ -250,9 +250,9 @@ void select_describe(JOIN *join, bool ne
>   			    bool distinct, const char *message=NullS);
>   static Item *remove_additional_cond(Item* conds);
>   static void add_group_and_distinct_keys(JOIN *join, JOIN_TAB *join_tab);
> -static bool replace_where_subcondition(JOIN *join, Item **tree,
> -                                       Item *old_cond, Item *new_cond,
> -                                       bool do_fix_fields);
> +static bool replace_subcondition(JOIN *join, Item **tree,
> +                                 Item *old_cond, Item *new_cond,
> +                                 bool do_fix_fields);
>   static bool test_if_ref(COND *root_cond,
>                           Item_field *left_item,Item *right_item);
>
> @@ -3751,8 +3751,7 @@ bool JOIN::flatten_subqueries()
>     {
>       Item **tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)?
>                      &conds :&((*in_subq)->emb_on_expr_nest->on_expr);
> -    if (replace_where_subcondition(this, tree, *in_subq, new Item_int(1),
> -                                   FALSE))
> +    if (replace_subcondition(this, tree, *in_subq, new Item_int(1), FALSE))
>         DBUG_RETURN(TRUE); /* purecov: inspected */
>     }
>
> @@ -3795,8 +3794,7 @@ skip_conversion:
>       bool do_fix_fields= !(*in_subq)->substitution->fixed;
>       Item **tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)?
>                      &conds :&((*in_subq)->emb_on_expr_nest->on_expr);
> -    if (replace_where_subcondition(this, tree, *in_subq, substitute,
> -                                   do_fix_fields))
> +    if (replace_subcondition(this, tree, *in_subq, substitute, do_fix_fields))
>         DBUG_RETURN(TRUE);
>       (*in_subq)->substitution= NULL;
>
> @@ -3805,8 +3803,7 @@ skip_conversion:
>         tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)?
>                        &select_lex->prep_where
> :&((*in_subq)->emb_on_expr_nest->prep_on_expr);
>
> -      if (replace_where_subcondition(this, tree, *in_subq, substitute,
> -                                     FALSE))
> +      if (replace_subcondition(this, tree, *in_subq, substitute, FALSE))
>           DBUG_RETURN(TRUE);
>       }
>     }
> @@ -10048,8 +10045,8 @@ uint check_join_cache_usage(JOIN_TAB *ta
>           (tab->first_inner || tab_sj_strategy == SJ_OPT_FIRST_MATCH))
>         goto no_join_cache;
>       if ((options&  SELECT_DESCRIBE) ||
> -        ((tab->cache= new JOIN_CACHE_BNL(join, tab, prev_cache)))&&
> -        !tab->cache->init())
> +        ((tab->cache= new JOIN_CACHE_BNL(join, tab, prev_cache))&&
> +         !tab->cache->init()))
>       {
>         *icp_other_tables_ok= FALSE;
>         return JOIN_CACHE::ALG_BNL | force_unlinked_cache;
> @@ -10069,11 +10066,11 @@ uint check_join_cache_usage(JOIN_TAB *ta
>       if ((rows != HA_POS_ERROR)&&  !(flags& 
> HA_MRR_USE_DEFAULT_IMPL)&&
>           (!(flags&  HA_MRR_NO_ASSOCIATION) || cache_level>  6)&&
>           ((options&  SELECT_DESCRIBE) ||
> -         (cache_level<= 6&&
> -          (tab->cache= new JOIN_CACHE_BKA(join, tab, flags, prev_cache)) ||
> -	  cache_level>  6&&
> -          (tab->cache= new JOIN_CACHE_BKA_UNIQUE(join, tab, flags, prev_cache))
> -         )&&  !tab->cache->init()))
> +         (((cache_level<= 6&&
> +            (tab->cache= new JOIN_CACHE_BKA(join, tab, flags, prev_cache))) ||
> +           (cache_level>  6&&
> +            (tab->cache= new JOIN_CACHE_BKA_UNIQUE(join, tab, flags,
> prev_cache)))
> +           )&&  !tab->cache->init())))
>       {
>         if (cache_level<= 6)
>           return JOIN_CACHE::ALG_BKA | force_unlinked_cache;
> @@ -15269,49 +15266,6 @@ err:
>     DBUG_RETURN(NULL);				/* purecov: inspected */
>   }
>
> -/**
> -   @brief Replaces an expression destructively inside the expression tree of
> -   the WHERE clase.
> -
> -   @note Because of current requirements for semijoin flattening, we do not
> -   need to recurse here, hence this function will only examine the top-level
> -   AND conditions. (see JOIN::prepare, comment above the line
> -   'if (do_materialize)'
> -
> -   @param join The top-level query.
> -   @param old_cond The expression to be replaced.
> -   @param new_cond The expression to be substituted.
> -   @param do_fix_fields If true, Item::fix_fields(THD*, Item**) is called for
> -   the new expression.
> -   @return<code>true</code>  if there was an
> error,<code>false</code>  if
> -   successful.
> -*/
> -static bool replace_where_subcondition(JOIN *join, Item *old_cond,
> -                                       Item *new_cond, bool do_fix_fields)
> -{
> -  if (join->conds == old_cond) {
> -    join->conds= new_cond;
> -    if (do_fix_fields)
> -      new_cond->fix_fields(join->thd,&join->conds);
> -    return FALSE;
> -  }
> -
> -  if (join->conds->type() == Item::COND_ITEM) {
> -    List_iterator<Item> 
> li(*((Item_cond*)join->conds)->argument_list());
> -    Item *item;
> -    while ((item= li++))
> -      if (item == old_cond)
> -      {
> -        li.replace(new_cond);
> -        if (do_fix_fields)
> -          new_cond->fix_fields(join->thd, li.ref());
> -        return FALSE;
> -      }
> -  }
> -
> -  return TRUE;
> -}
> -
>   /*
>     Create a temporary table to weed out duplicate rowid combinations
>
> @@ -18336,53 +18290,68 @@ static bool test_if_ref(COND *root_cond,
>
>
>   /**
> -   @brief Replaces an expression destructively inside the expression tree of
> -   the WHERE clase.
> +   Destructively replaces a sub-condition inside a condition tree. The
> +   parse tree is also altered.
>
>      @note Because of current requirements for semijoin flattening, we do not
>      need to recurse here, hence this function will only examine the top-level
>      AND conditions. (see JOIN::prepare, comment starting with "Check if the
> -   subquery predicate can be executed via materialization".
> +   subquery predicate can be executed via materialization".)
>
>      @param join The top-level query.
> -   @param old_cond The expression to be replaced.
> -   @param new_cond The expression to be substituted.
> +
> +   @param tree Must be the handle to the top level condition. This is needed
> +   when the top-level condition changes.
> +
> +   @param old_cond The condition to be replaced.
> +
> +   @param new_cond The condition to be substituted.
> +
>      @param do_fix_fields If true, Item::fix_fields(THD*, Item**) is called for
> -   the new expression.
> -   @return<code>true</code>  if there was an
> error,<code>false</code>  if
> -   successful.
> -*/
> -static bool replace_where_subcondition(JOIN *join, Item **expr,
> -                                       Item *old_cond, Item *new_cond,
> -                                       bool do_fix_fields)
> -{
> -  //Item **expr= (emb_nest == (TABLE_LIST*)1)?&join->conds
> :&emb_nest->on_expr;
> -  if (*expr == old_cond)
> -  {
> -    *expr= new_cond;
> -    if (do_fix_fields)
> -      new_cond->fix_fields(join->thd, expr);
> -    join->select_lex->where= *expr;
> +   the new condition.
> +
> +   @return error status
> +
> +   @retval true If there was an error during prerequisites check.
> +   @retval false If successful.
> +*/
> +static bool replace_subcondition(JOIN *join, Item **tree,
> +                                 Item *old_cond, Item *new_cond,
> +                                 bool do_fix_fields)
> +{
> +  if (*tree != old_cond&&  ((*tree)->type() != Item::COND_ITEM))
> +  {
> +    // Prerequisites not fulfilled.
> +    DBUG_ASSERT(FALSE);
> +    return TRUE;
> +  }

This is inefficient. You may rewrite it as:

DBUG_ASSERT(*tree == old_cond || ((*tree)->type() == Item::COND_ITEM));

return TRUE is wrong because you do not have a set error condition, and callers 
are not prepared to handle this situation.

> +
> +  if (*tree == old_cond)
> +  {
> +    *tree= new_cond;
> +    if (do_fix_fields&&  new_cond->fix_fields(join->thd, tree))
> +      return TRUE;
> +    join->select_lex->where= *tree;
>       return FALSE;
>     }
>
> -  if ((*expr)->type() == Item::COND_ITEM)
> +  if ((*tree)->type() == Item::COND_ITEM)
>     {
> -    List_iterator<Item>  li(*((Item_cond*)(*expr))->argument_list());
> +    List_iterator<Item>  li(*((Item_cond*)(*tree))->argument_list());
>       Item *item;
>       while ((item= li++))
>       {
>         if (item == old_cond)
>         {
>           li.replace(new_cond);
> -        if (do_fix_fields)
> -          new_cond->fix_fields(join->thd, li.ref());
> +        if (do_fix_fields&&  new_cond->fix_fields(join->thd,
> li.ref()))
> +          return TRUE;
>           return FALSE;
>         }
>       }
>     }
> -  // If we came here it means there were an error during prerequisites check.
> -  DBUG_ASSERT(0);
> +
> +  DBUG_ASSERT(FALSE); // can't happen
>     return TRUE;
>   }

Thanks,
Roy
Thread
bzr commit into mysql-next-mr-opt-backporting branch (martin.hansson:3203)Bug#33062Martin Hansson4 Aug
Re: bzr commit into mysql-next-mr-opt-backporting branch(martin.hansson:3203) Bug#33062Roy Lyseng4 Aug
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (martin.hansson:3203)Bug#33062Martin Hansson4 Aug
Re: bzr commit into mysql-next-mr-opt-backporting branch(martin.hansson:3203) Bug#33062Roy Lyseng4 Aug