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

[Sergey Petrunia]

Hi Georgi,

Please find my comments below

On Thu, Aug 21, 2008 at 05:28:08PM +0300, Georgi Kodinov wrote:
> #At file:///home/kgeorge/mysql/bzr/old-B37943-5.0-bugteam/
> 
>  2648 Georgi Kodinov	2008-08-21
>       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 structures that unnecessary complicate the
> analysis.
Please specify the names of the structures, they are not classified.
> 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-08-21 14:27:27 +0000
> @@ -481,36 +481,6 @@ public:
>  
>  class SEL_IMERGE;
>  
> -
> -class SEL_TREE :public Sql_alloc
> -{
> -public:
> -  enum Type { IMPOSSIBLE, ALWAYS, MAYBE, KEY, KEY_SMALLER } type;
> -  SEL_TREE(enum Type type_arg) :type(type_arg) {}
> -  SEL_TREE() :type(KEY)
> -  {
> -    keys_map.clear_all();
> -    bzero((char*) keys,sizeof(keys));
> -  }
> -  SEL_ARG *keys[MAX_KEY];
> -  key_map keys_map;        /* bitmask of non-NULL elements in keys */
> -
> -  /*
> -    Possible ways to read rows using index_merge. The list is non-empty only
> -    if type==KEY. Currently can be non empty only if keys_map.is_clear_all().
> -  */
> -  List<SEL_IMERGE> merges;
> -
> -  /* The members below are filled/used only after get_mm_tree is done */
> -  key_map ror_scans_map;   /* bitmask of ROR scan-able elements in keys */
> -  uint    n_ror_scans;     /* number of set bits in ror_scans_map */
> -
> -  struct st_ror_scan_info **ror_scans;     /* list of ROR key scans */
> -  struct st_ror_scan_info **ror_scans_end; /* last ROR scan */
> -  /* Note that #records for each key scan is stored in table->quick_rows */
> -};
> -
>  typedef struct st_qsel_param {
>    THD	*thd;
>    TABLE *table;
> @@ -548,6 +518,37 @@ typedef struct st_qsel_param {
>    uint alloced_sel_args; 
>  } PARAM;
>  
> +
> +class SEL_TREE :public Sql_alloc
> +{
> +public:
> +  enum Type { IMPOSSIBLE, ALWAYS, MAYBE, KEY, KEY_SMALLER } type;
> +  SEL_TREE(enum Type type_arg) :type(type_arg) {}
> +  SEL_TREE() :type(KEY)
> +  {
> +    keys_map.clear_all();
> +    bzero((char*) keys,sizeof(keys));
> +  }
> +  SEL_TREE(SEL_TREE *arg, PARAM *param);
Please change PARAM to "struct st_qsel_param" (defined in opt_range.h), and
then put the class definition back. (unless you have an intent to became the
target of all questions about SEL_TREE)

> +  SEL_ARG *keys[MAX_KEY];
> +  key_map keys_map;        /* bitmask of non-NULL elements in keys */
> +
> +  /*
> +    Possible ways to read rows using index_merge. The list is non-empty only
> +    if type==KEY. Currently can be non empty only if keys_map.is_clear_all().
> +  */
> +  List<SEL_IMERGE> merges;
> +
> +  /* The members below are filled/used only after get_mm_tree is done */
> +  key_map ror_scans_map;   /* bitmask of ROR scan-able elements in keys */
> +  uint    n_ror_scans;     /* number of set bits in ror_scans_map */
> +
> +  struct st_ror_scan_info **ror_scans;     /* list of ROR key scans */
> +  struct st_ror_scan_info **ror_scans_end; /* last ROR scan */
> +  /* Note that #records for each key scan is stored in table->quick_rows */
> +};
> +
...

> +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);
add { } for the loop body as it is requested by the coding style.
> +
> +  List_iterator<SEL_IMERGE> it(arg->merges);
> +  for (SEL_IMERGE *el= it++; el; el= it++)
> +    merges.push_back (new SEL_IMERGE(el, param));
> +}
> +
> +
> +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 **);
> +    trees= (SEL_TREE **)alloc_root(param->mem_root, size);
> +  }
> +  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++)
> +    *tree= new SEL_TREE(*arg_tree, param);
What about checks for out-of-memory conditions? I know it is highly unlikely
in practice but I believe the rest of opt_range.cc follows the line of
behavior where you check if memory allocation was successful, and if it
wasn't, you handle it by discarding candidate query plans. I think it is
quite feasible to the same for the code in this patch.
> +}
> +
> +
>  /*
>    Perform AND operation on two index_merge lists and store result in *im1.
>  */
> @@ -823,10 +859,15 @@ int imerge_list_or_tree(PARAM *param,
>  {
>    SEL_IMERGE *imerge;
>    List_iterator<SEL_IMERGE> it(*im1);
> +  bool tree_used= false;
s/false/FALSE/, per coding style
>    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))
>        it.remove();
> +    tree_used= true;
s/true/TRUE/, per coding style
>    }
>    return im1->is_empty();
>  }
> @@ -4238,6 +4279,8 @@ get_mm_parts(PARAM *param, COND *cond_fu
>      }
>    }
>  
> +  if (tree && tree->merges.is_empty() &&
> tree->keys_map.is_clear_all())
> +    tree= NULL;
OK
>    DBUG_RETURN(tree);
>  }
 
BR
 Sergey
-- 
Sergey Petrunia, Lead Software Engineer
MySQL AB, www.mysql.com
Office: N/A
Blog: http://s.petrunia.net/blog