Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:3197) Bug#47762

[Jørgen Løland]

I discovered one more thing:

store_val_in_field is used in another location that may break by the 
suggested three-value return:

sql_select.cc#test_if_ref()
return !store_val_in_field(field, right_item, CHECK_FIELD_WARN);

V Narayanan wrote:
> #At
> file:///home/narayanan/Work/mysql_checkouts/shared_repository_directory/mysql-5.1-bugteam-47762/
> based on revid:joro@stripped
> 
>  3197 V Narayanan	2009-10-30
>       Bug#47762 Incorrect result from MIN() when WHERE tests NOT NULL column for
> NULL
>       
>       The lookup of a NULL value in a column having a NOT NULL
>       index was resulting in wrong query result. This was happening
>       due to the return value, from the function that stored the
>       field value, being ignored.
>      @ sql/opt_sum.cc
>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT NULL column for
> NULL
>         
>         Ensure that the error (or not) from the store_val_in_field
>         function is used while determining the usability of the index
>         (determined by the return value of the matching_cond function).
>      @ sql/sql_select.cc
>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT NULL column for
> NULL
>         
>         Ensure that all the three cases of no error, conversion
>         and error are reported by the store_val_in_field function.
>      @ sql/sql_select.h
>         Bug#47762 Incorrect result from MIN() when WHERE tests NOT NULL column for
> NULL
>         
>         The store_val_in_field function returns a int instead of
>         a bool so that it can handle all the three cases, namely,
>         error, conversion and no error.
> 
>     modified:
>       sql/opt_sum.cc
>       sql/sql_select.cc
>       sql/sql_select.h
> === modified file 'sql/opt_sum.cc'
> --- a/sql/opt_sum.cc	2009-10-14 08:46:50 +0000
> +++ b/sql/opt_sum.cc	2009-10-30 12:24:33 +0000
> @@ -599,6 +599,12 @@ static bool matching_cond(bool max_fl, T
>                            key_part_map *key_part_used, uint *range_fl,
>                            uint *prefix_len)
>  {
> +  /*
> +    contains the error that may occur when trying to store a
> +    field value
> +  */
> +  int store_value_in_field_error;
> +
>    if (!cond)
>      return 1;
>    Field *field= field_part->field;
> @@ -723,8 +729,10 @@ static bool matching_cond(bool max_fl, T
>      }
>      else
>      {
> -      store_val_in_field(part->field, args[between && max_fl ? 2 : 1],
> -                         CHECK_FIELD_IGNORE);
> +      store_value_in_field_error=
> +                         store_val_in_field(part->field,
> +                                            args[between && max_fl ? 2 :
> 1],
> +                                            CHECK_FIELD_IGNORE);
>        if (part->null_bit) 
>          *key_ptr++= (uchar) test(part->field->is_null());
>        part->field->get_key_image(key_ptr, part->length, Field::itRAW);
> @@ -751,6 +759,14 @@ static bool matching_cond(bool max_fl, T
>    }
>    else if (is_field_part)
>      *range_fl&= ~(max_fl ? NO_MIN_RANGE : NO_MAX_RANGE);
> +  
> +  /*
> +    If an error has occurred when trying to store the value
> +    return 0 to indicate failure
> +  */
> +  if (store_value_in_field_error == -1)
> +    return 0;
> +
>    return 1;  
>  }
>  
> 
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2009-10-22 09:40:15 +0000
> +++ b/sql/sql_select.cc	2009-10-30 12:24:33 +0000
> @@ -5735,13 +5735,15 @@ get_store_key(THD *thd, KEYUSE *keyuse, 
>    This function is only called for const items on fields which are keys.
>  
>    @return
> -    returns 1 if there was some conversion made when the field was stored.
> +    returns  0 to signify no error or conversion
> +    returns  1 if there was some conversion made when the field was stored.
> +    returns -1 to signify error
>  */
>  
> -bool
> +int
>  store_val_in_field(Field *field, Item *item, enum_check_fields check_flag)
>  {
> -  bool error;
> +  int error;
>    TABLE *table= field->table;
>    THD *thd= table->in_use;
>    ha_rows cuted_fields=thd->cuted_fields;
> @@ -5758,7 +5760,19 @@ store_val_in_field(Field *field, Item *i
>    error= item->save_in_field(field, 1);
>    thd->count_cuted_fields= old_count_cuted_fields;
>    dbug_tmp_restore_column_map(table->write_set, old_map);
> -  return error || cuted_fields != thd->cuted_fields;
> +
> +  /*
> +    In the value being returned we handle three cases
> +    1)  0 meaning no error or truncation has occurred
> +    2)  1 meaning there has been a truncation
> +    3) -1 meaning there is an error when trying to store the value
> +  */
> +  if (error)
> +    return error;
> +  else if (cuted_fields != thd->cuted_fields)
> +    return 1;
> +  
> +  return 0;
>  }
>  
>  
> 
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2009-10-14 08:46:50 +0000
> +++ b/sql/sql_select.h	2009-10-30 12:24:33 +0000
> @@ -558,7 +558,7 @@ extern const char *join_type_str[];
>  void TEST_join(JOIN *join);
>  
>  /* Extern functions in sql_select.cc */
> -bool store_val_in_field(Field *field, Item *val, enum_check_fields check_flag);
> +int store_val_in_field(Field *field, Item *val, enum_check_fields check_flag);
>  TABLE *create_tmp_table(THD *thd,TMP_TABLE_PARAM *param,List<Item>
> &fields,
>  			ORDER *group, bool distinct, bool save_sum_fields,
>  			ulonglong select_options, ha_rows rows_limit,
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 


-- 
Jørgen Løland