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
>
