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

[Jørgen Løland]

Thank you for the patch, Narayanan. There are a few review comments inline:

* First of all, there is no test case. You'll need one :)

V Narayanan wrote:
> === 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
> +  */

You already documented the return values in the function
documentation, so to me, this doesn't add value

> +  if (error)
> +    return error;

I'm not convinced that there are no Item::save_in_field that
return 0/1 (or even more return values) instead of 0/-1. This
means that you might end up returning 1 ("there was some
conversion made...") if some item's save_in_field returns error.
The best way to fix this is to go through all the save_in_field
functions and verify that 0/-1 is used consistently. If that is
the case, the documentation for these should be updated and the
return could even be changed from int to bool. This would make it
a lot easier to handle the code later. A simpler fix would be to
do "if (error) return -1;".

> +  else if (cuted_fields != thd->cuted_fields)
> +    return 1;
> +  
> +  return 0;
>  }

Some additional comments that go beyond the patch, but you might want to 
fix anyway:

* item.cc#Item_null::save_in_field
   Documentation says return values may be 0 or 1, but correct is
   0/-1

   The same doc problem is also present for save_safe_in_field

* field_conv.cc#set_field_to_null_with_conversions
   Multiple documentation errors, like "@param no_conversions Set
   to 1 if we should return 1 if field can't take null
   values." (correct is -1).

   The same doc problem is also present for set_field_to_null


-- 
Jørgen Løland