Re: bzr commit into mysql-5.0 branch (kgeorge:2648) Bug#37943

[Sergey Petrunia]

Hi!

Some more feedback on error conditions: 

On Tue, Sep 30, 2008 at 12:27:46PM +0300, Georgi Kodinov wrote:
>  2648 Georgi Kodinov	2008-09-30
>       Bug#37943: Reproducible mysqld crash/sigsegv in sel_trees_can_be_ored
>             
>       When analyzing the possible index use cases the server was re-using an internal
> structure.
>       This is wrong, as this internal structure gets updated during the analysis.
>       Fixed by making a copy of the internal structure for every place it needs to be
> used.
>       Also stopped the generation of empty SEL_TREE structures that unnecessary 
>       complicate the analysis.
> modified:
>   mysql-test/r/index_merge.result
>   mysql-test/t/index_merge.test
>   sql/opt_range.cc
> 
> per-file messages:
>   mysql-test/r/index_merge.result
>     Bug#37943: test case
>   mysql-test/t/index_merge.test
>     Bug#37943: test case
>   sql/opt_range.cc
>     Bug#37943: 
>      - Make copy constructors for SEL_TREE and sub-structures and use them when
> OR-ing trees.
>      - don't generate empty SEL_TREEs. Return NULL instead.
> === modified file 'sql/opt_range.cc'
> --- a/sql/opt_range.cc	2008-03-28 18:02:27 +0000
> +++ b/sql/opt_range.cc	2008-09-30 09:27:06 +0000
>  }
>  
>  
> +SEL_TREE::SEL_TREE(SEL_TREE *arg, PARAM *param): Sql_alloc()
> +{
> +  keys_map= arg->keys_map;
> +  type= arg->type;
> +  for (int idx= 0; idx < MAX_KEY; idx++)
> +  {
> +    if ((keys[idx]= arg->keys[idx]))
> +      keys[idx]->increment_use_count(1);
> +  }
> +
> +  List_iterator<SEL_IMERGE> it(arg->merges);
> +  for (SEL_IMERGE *el= it++; el; el= it++)
> +  {
> +    SEL_IMERGE *merge= new SEL_IMERGE(el, param);
> +    if (!merge)
> +    {
> +      merges.empty();
> +      return;
Please also do the same when there was an out of memory condition in
SEL_IMERGE constructor and we ended up with a "dummy" SEL_IMERGE with 
keys_map==0ULL && tree.merges.elements ==0. 
> +    }
> +    merges.push_back (merge);
> +  }
> +}
> +
> +
> +SEL_IMERGE::SEL_IMERGE (SEL_IMERGE *arg, PARAM *param) : Sql_alloc()
> +{
> +  uint elements= (arg->trees_end - arg->trees);
> +  if (elements > PREALLOCED_TREES)
> +  {
> +    uint size= elements * sizeof (SEL_TREE **);
> +    if (!(trees= (SEL_TREE **)alloc_root(param->mem_root, size)))
> +      goto mem_err;
> +  }
> +  else
> +    trees= &trees_prealloced[0];
> +
> +  trees_next= trees;
> +  trees_end= trees + elements;
> +
> +  for (SEL_TREE **tree = trees, **arg_tree= arg->trees; tree < trees_end; 
> +       tree++, arg_tree++)
> +  {
> +    if (!(*tree= new SEL_TREE(*arg_tree, param)))
> +      goto mem_err;
> +  }
> +
> +  return;
> +
> +mem_err:
> +  trees= &trees_prealloced[0];
> +  trees_next= trees;
> +  trees_end= trees;
> +}
> +
> +
>  /*
>    Perform AND operation on two index_merge lists and store result in *im1.
>  */
> @@ -823,10 +880,15 @@ int imerge_list_or_tree(PARAM *param,
>  {
>    SEL_IMERGE *imerge;
>    List_iterator<SEL_IMERGE> it(*im1);
> +  bool tree_used= FALSE;
>    while ((imerge= it++))
>    {
> -    if (imerge->or_sel_tree_with_checks(param, tree))
> +    if (imerge->or_sel_tree_with_checks(param, 
> +                                        tree_used ? 
> +                                          new SEL_TREE(tree, param) : 
> +                                          tree))
Same here. Please check
- if SEL_TREE object was successfully created, and
- if it is not a dummy object we get when there was an out-of-memory
  condition in

>        it.remove();
> +    tree_used= TRUE;
>    }
>    return im1->is_empty();
>  }
> @@ -4238,6 +4300,8 @@ get_mm_parts(PARAM *param, COND *cond_fu
>      }
>    }
>  
> +  if (tree && tree->merges.is_empty() &&
> tree->keys_map.is_clear_all())
> +    tree= NULL;
>    DBUG_RETURN(tree);
>  }
>  

Ok to push after the above is addressed.
 
BR
 Sergey
-- 
Sergey Petrunia, Lead Software Engineer
MySQL AB, www.mysql.com
Office: N/A
Blog: http://s.petrunia.net/blog