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
