List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:November 4 2010 12:27pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928
View as plain text  
On Wed, Nov 3, 2010 at 2:54 PM, Guilhem Bichot <guilhem@stripped> wrote:

> #At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-bugfixing2/ based on
> revid:guilhem@stripped
>
>  3330 Guilhem Bichot    2010-11-03
>      Fix for BUG#57928 "More Valgrind warnings in print_keyuse() with
> --debug":
>      for a fulltext index, the KEYUSE object had some uninitialized
> members.
>     @ include/my_sys.h
>        insert_dynamic() and set_dynamic() treat "element" as bytes and uses
> it as
>        the source of memcpy(). As memcpy() accepts void* it sounds natural
> to
>        use void* as type for "element". It then avoids casts in calling
> code.
>        Before the change, we had to write:
>        insert_dynamic(array, (uchar*)keyuse);
>        because conversion from KEYUSE* to uchar* requires an explicit cast.
>        After the change we can write
>        insert_dynamic(array, keyuse);
>        because conversion from KEYUSE* to void* is always allowed.
>        If reviewers like this change, I will commit it separately from the
> bugfix
>        and also remove all now-unneeded casts in calls to insert_dynamic()
>        and set_dynamic().
>     @ mysys/array.c
>        void* is more natural.
>     @ sql/sql_select.cc
>        use constructor which sets all members. The second change is the
> bugfix: we forgot
>        to set ref_table_rows, null_rejecting and cond_guard. Now there is
> the question
>        of: what value to pick for them, for a fulltext index?
>        * ref_table_rows: we now set it to ~(ha_rows)0 which seems to be a
> default
>        value (see optimize_keyuse() and add_key_part()).
>        * null_rejecting: it's not used for a fulltext index, and it serves
>        only for the so-called early-NULL-filtering optimization, so we play
>        safe and set it to "false"; setting it to "true" would require
>        intelligent code determining when the optimization is allowed.
>        * cond_guard: it's not used for a fulltext index (see
> create_ref_for_key()),
>        so we set it to NULL.
>     @ sql/sql_select.h
>        constructor, comments. With that,
>        "KEYUSE keyuse;" will not compile anymore.
>
>    modified:
>      include/my_sys.h
>      mysys/array.c
>      sql/sql_select.cc
>      sql/sql_select.h
> === modified file 'include/my_sys.h'
> --- a/include/my_sys.h  2010-08-06 08:54:01 +0000
> +++ b/include/my_sys.h  2010-11-03 13:54:05 +0000
> @@ -778,10 +778,11 @@ extern my_bool init_dynamic_array2(DYNAM
>  /* init_dynamic_array() function is deprecated */
>  extern my_bool init_dynamic_array(DYNAMIC_ARRAY *array, uint element_size,
>                                   uint init_alloc, uint alloc_increment);
> -extern my_bool insert_dynamic(DYNAMIC_ARRAY *array,uchar * element);
> +extern my_bool insert_dynamic(DYNAMIC_ARRAY *array, const void *element);
>  extern uchar *alloc_dynamic(DYNAMIC_ARRAY *array);
>  extern uchar *pop_dynamic(DYNAMIC_ARRAY*);
> -extern my_bool set_dynamic(DYNAMIC_ARRAY *array,uchar * element,uint
> array_index);
> +extern my_bool set_dynamic(DYNAMIC_ARRAY *array, const void *element,
> +                           uint array_index);
>  extern my_bool allocate_dynamic(DYNAMIC_ARRAY *array, uint max_elements);
>  extern void get_dynamic(DYNAMIC_ARRAY *array,uchar * element,uint
> array_index);
>  extern void delete_dynamic(DYNAMIC_ARRAY *array);
>
> === modified file 'mysys/array.c'
> --- a/mysys/array.c     2010-07-08 21:20:08 +0000
> +++ b/mysys/array.c     2010-11-03 13:54:05 +0000
> @@ -92,7 +92,7 @@ my_bool init_dynamic_array(DYNAMIC_ARRAY
>     FALSE      Ok
>  */
>
> -my_bool insert_dynamic(DYNAMIC_ARRAY *array, uchar* element)
> +my_bool insert_dynamic(DYNAMIC_ARRAY *array, const void *element)
>  {
>   uchar* buffer;
>   if (array->elements == array->max_element)
> @@ -196,7 +196,7 @@ uchar *pop_dynamic(DYNAMIC_ARRAY *array)
>     FALSE      Ok
>  */
>
> -my_bool set_dynamic(DYNAMIC_ARRAY *array, uchar* element, uint idx)
> +my_bool set_dynamic(DYNAMIC_ARRAY *array, const void *element, uint idx)
>  {
>   if (idx >= array->elements)
>   {
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc 2010-10-21 13:30:19 +0000
> +++ b/sql/sql_select.cc 2010-11-03 13:54:05 +0000
> @@ -5861,20 +5861,15 @@ add_key_part(DYNAMIC_ARRAY *keyuse_array
>       {
>        if (field->eq(form->key_info[key].key_part[part].field))
>        {
> -          KEYUSE keyuse;
> -          keyuse.table=          field->table;
> -          keyuse.val=            key_field->val;
> -          keyuse.used_tables=    key_field->val->used_tables();
> -          keyuse.key=            key;
> -          keyuse.keypart=        part;
> -          keyuse.optimize=       key_field->optimize &
> KEY_OPTIMIZE_REF_OR_NULL;
> -          keyuse.keypart_map=    (key_part_map) 1 << part;
> -          /* This will be set accordingly in optimize_keyuse */
> -          keyuse.ref_table_rows= ~(ha_rows) 0;
> -          keyuse.null_rejecting= key_field->null_rejecting;
> -          keyuse.cond_guard=     key_field->cond_guard;
> -          keyuse.sj_pred_no=     key_field->sj_pred_no;
> -          if (insert_dynamic(keyuse_array, (uchar*) &keyuse))
> +          const KEYUSE keyuse(field->table, key_field->val,
> +                              key_field->val->used_tables(), key, part,
> +                              key_field->optimize &
> KEY_OPTIMIZE_REF_OR_NULL,
> +                              (key_part_map) 1 << part,
> +                              ~(ha_rows) 0, // will be set in
> optimize_keyuse
> +                              key_field->null_rejecting,
> +                              key_field->cond_guard,
> +                              key_field->sj_pred_no);
> +          if (insert_dynamic(keyuse_array, &keyuse))
>             return TRUE;
>        }
>       }
>

I prefer

          const KEYUSE keyuse(field->table,
                              key_field->val,
                              key_field->val->used_tables(),
                              key,
                              part,
                              key_field->optimize &
KEY_OPTIMIZE_REF_OR_NULL,
                              (key_part_map) 1 << part,
                              ~(ha_rows) 0, // will be set in
optimize_keyuse
                              key_field->null_rejecting,
                              key_field->cond_guard,
                              key_field->sj_pred_no);



> @@ -5938,16 +5933,11 @@ add_ft_keys(DYNAMIC_ARRAY *keyuse_array,
>       !(usable_tables & cond_func->table->map))
>     return FALSE;
>
> -  KEYUSE keyuse;
> -  keyuse.table= cond_func->table;
> -  keyuse.val =  cond_func;
> -  keyuse.key =  cond_func->key;
> -  keyuse.keypart= FT_KEYPART;
> -  keyuse.used_tables=cond_func->key_item()->used_tables();
> -  keyuse.optimize= 0;
> -  keyuse.keypart_map= 0;
> -  keyuse.sj_pred_no= UINT_MAX;
> -  return insert_dynamic(keyuse_array,(uchar*) &keyuse);
> +  const KEYUSE keyuse(cond_func->table, cond_func,
> +                      cond_func->key_item()->used_tables(),
> cond_func->key,
> +                      FT_KEYPART, 0, 0, ~(ha_rows) 0, false, NULL,
> +                      UINT_MAX);
> +  return insert_dynamic(keyuse_array, &keyuse);
>  }
>
>
I would *strongly* prefer

  const KEYUSE keyuse(cond_func->table,
                      cond_func,
                      cond_func->key_item()->used_tables(),
                      cond_func->key,
                      FT_KEYPART,
                      0,                        // optimize
                      0,                        // keypart_map
                      ~(ha_rows) 0,             // ref_table_rows
                      false,                    // null_rejecting
                      NULL,                     // cond_guard
                      UINT_MAX);                // sj_pred_no



>
> @@ -6184,17 +6174,17 @@ update_ref_and_keys(THD *thd, DYNAMIC_AR
>   */
>   if (keyuse->elements)
>   {
> -    KEYUSE key_end,*prev,*save_pos,*use;
> +    KEYUSE *save_pos,*use;
>
>     my_qsort(keyuse->buffer,keyuse->elements,sizeof(KEYUSE),
>          (qsort_cmp) sort_keyuse);
>
> -    bzero((char*) &key_end,sizeof(key_end));    /* Add for easy testing */
> -    if (insert_dynamic(keyuse,(uchar*) &key_end))
> +    const KEYUSE key_end(NULL, NULL, 0, 0, 0, 0, 0, 0, false, NULL, 0);
> +    if (insert_dynamic(keyuse, &key_end)) // added for easy testing
>       return TRUE;
>
>     use=save_pos=dynamic_element(keyuse,0,KEYUSE*);
> -    prev= &key_end;
> +    const KEYUSE *prev= &key_end;
>     found_eq_constant=0;
>     for (i=0 ; i < keyuse->elements-1 ; i++,use++)
>     {
>
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h  2010-09-23 12:16:36 +0000
> +++ b/sql/sql_select.h  2010-11-03 13:54:05 +0000
> @@ -39,17 +39,34 @@
>  #define KEY_OPTIMIZE_EXISTS            1
>  #define KEY_OPTIMIZE_REF_OR_NULL       2
>
> -typedef struct keyuse_t {
> +/**
> +  @note such objects are stored in DYNAMIC_ARRAY which uses sizeof(), so
> keep
> +  this class as POD as possible.
> +*/
> +class KEYUSE {
> +public:
> +  KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
> +         uint key_arg, uint keypart_arg, uint optimize_arg,
> +         key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
> +         bool null_rejecting_arg, bool *cond_guard_arg,
> +         uint sj_pred_no_arg) :
> +  table(table_arg), val(val_arg), used_tables(used_tables_arg),
> key(key_arg),
> +  keypart(keypart_arg), optimize(optimize_arg),
> keypart_map(keypart_map_arg),
> +  ref_table_rows(ref_table_rows_arg), null_rejecting(null_rejecting_arg),
> +  cond_guard(cond_guard_arg), sj_pred_no(sj_pred_no_arg)

{}
>

Please put the empty body on a separate line.
And if you want to squish as many character as possible on each line,
could you keep the argument list, and the initializers aligned?
something like:

  KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
         uint key_arg, uint keypart_arg, uint optimize_arg,
         key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
         bool null_rejecting_arg, bool *cond_guard_arg,
         uint sj_pred_no_arg) :
  table(table_arg), val(val_arg), used_tables(used_tables_arg),
  key(key_arg), keypart(keypart_arg), optimize(optimize_arg),
  keypart_map(keypart_map_arg), ref_table_rows(ref_table_rows_arg),
  null_rejecting(null_rejecting_arg), cond_guard(cond_guard_arg),
  sj_pred_no(sj_pred_no_arg)
  {}




>   TABLE *table;
>   Item *val;                           /**< or value if no field */
>   table_map used_tables;
>   uint key, keypart;
>   uint optimize; // 0, or KEY_OPTIMIZE_*
>   key_part_map keypart_map;
> +  /** Estimate of how many rows for a key value */
>

The style guide was recently extended (at your request :-) to allow
// Estimate of how many rows for a key value.

Now we have a mix of five different comment styles, just for the member
fields of this class.



>   ha_rows      ref_table_rows;
>   /**
>     If true, the comparison this value was created from will not be
>     satisfied if val has NULL 'value'.
> +    Not used if the index is fulltext (such index cannot be used for
> +    equalities).
>   */
>   bool null_rejecting;
>   /*
> @@ -61,14 +78,20 @@ typedef struct keyuse_t {
>               *cond_guard == FALSE <=> equality condition is off
>
>     NULL  - Otherwise (the source equality can't be turned off)
> +
> +    Not used if the index is fulltext (such index cannot be used for
> +    equalities).
>   */
>   bool *cond_guard;
>   /*
>      0..64    <=> This was created from semi-join IN-equality # sj_pred_no.
>      MAX_UINT  Otherwise
> +
> +     Not used if the index is fulltext (such index cannot be used for
> +     semijoin).
>   */
>   uint         sj_pred_no;
> -} KEYUSE;
> +};
>
>  class store_key;
>
>
>
>
-- didrik


>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>

Thread
bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Guilhem Bichot3 Nov
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot4 Nov
    • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen5 Nov
      • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot5 Nov
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Jorgen Loland4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot4 Nov