Hi Tor,
Thank you for the patch. I have a suspicion that there is another problem with
the Item_singlerow_subselect::val_* functions. See comments inline.
> 3323 Tor Didriksen 2011-02-11
> Bug #60085 crash in Item::save_in_field() with time data type
>
> Item_singlerow_subselect::val_str() incorrectly assumed that
> value->val_str() could set value->null_value= true;
JL: "...could *not* set"?
>=== modified file 'sql/item.cc'
>--- a/sql/item.cc 2011-01-12 12:58:47 +0000
>+++ b/ sql/item.cc 2011-02-11 08:48:59 +0000
>@@ -1051,6 +1051,7 @@ CHARSET_INFO *Item::default_charset()
>
> int Item::save_in_field_no_warnings(Field *field, bool no_conversions)
> {
>+ DBUG_ENTER("Item::save_in_field_no_warnings");
> int res;
> TABLE *table= field->table;
> THD *thd= table->in_use;
>@@ -1059,10 +1060,12 @@ int Item::save_in_field_no_warnings(Fiel
> ulonglong sql_mode= thd->variables.sql_mode;
> thd->variables.sql_mode&= ~(MODE_NO_ZERO_IN_DATE | MODE_NO_ZERO_DATE);
> thd->count_cuted_fields= CHECK_FIELD_IGNORE;
>+
> res= save_in_field(field, no_conversions);
>+
> thd->count_cuted_fields= tmp;
> dbug_tmp_restore_column_map(table->write_set, old_map);
> thd->variables.sql_mode= sql_mode;
>- return res;
>+ DBUG_RETURN(res);
> }
JL: IMHO, this DBUG tracing is unnecessary since the function calls
save_in_field() which is traced, but it's ok to keep it if you think it's helpful.
>@@ -7516,10 +7532,12 @@ String *Item_cache_datetime::val_str(Str
> {
> DBUG_ASSERT(fixed == 1);
>
>+ DBUG_ENTER("Item_cache_datetime::val_str");
>+
> if ((value_cached || str_value_cached) && null_value)
>- return NULL;
>+ DBUG_RETURN(NULL);
>
> if (!str_value_cached)
> {
JL: I don't think tracing one "random" function out of the countless *::val_*
functions is worthwile. var_* functions are also typically called for every
evaluated record, so it will significantly increase the size of the trace as
well. Suggest you remove tracing of this function.
>=== modified file 'sql/item_subselect.cc'
>--- a/sql/item_subselect.cc 2011-01-12 12:15:22 +0000
>+++ b/sql/item_subselect.cc 2011-02-11 08:48:59 +0000
>@@ -258,3 +258,4 @@ bool Item_subselect::exec()
> {
> int res;
>
>+ DBUG_ENTER("Item_subselect::exec");
JL: Thank you!
>+
> /*
> Do not execute subselect in case of a fatal error
> or if the query has been killed.
> */
> if (thd->is_error() || thd->killed)
>- return 1;
>+ DBUG_RETURN(1);
JL: This function should return bool :)
> DBUG_ASSERT(!thd->lex->context_analysis_only);
> /*
> Simulate a failure in sub-query execution. Used to test e.g.
> out of memory or query being killed conditions.
> */
>- DBUG_EXECUTE_IF("subselect_exec_fail", return 1;);
>+ DBUG_EXECUTE_IF("subselect_exec_fail", DBUG_RETURN(1););
JL: here as well
> res= engine->exec();
>
> if (engine_changed)
> {
> engine_changed= 0;
>- return exec();
>+ bool retval= exec();
>+ DBUG_RETURN(retval);
JL: actually returns bool!
> }
>- return (res);
>+ DBUG_RETURN(res);
JL: here as well
>@@ -599,15 +604,17 @@ longlong Item_singlerow_subselect::val_i
>
> String *Item_singlerow_subselect::val_str(String *str)
> {
>+ DBUG_ENTER("Item_singlerow_subselect::val_str");
> if (!exec() && !value->null_value)
> {
JL: The bug was that this code assumes that value->null_value will not
change when value->val_* is called. As this bug shows, this is an
incorrect assumption. I think that it's unsafe to assume anything about
val->null_value before calling val_ in all Item_singlerow_subselect::val_*
functions. This is only a theory and remains to be tested. I send this
concern without investigating thoroughly because we are in a hurry with
this patch:)
>- null_value= FALSE;
>- return value->val_str(str);
>+ String *retval= value->val_str(str);
>+ null_value= value->null_value;
>+ DBUG_RETURN(retval);
> }
> else
> {
> reset();
>- return 0;
>+ DBUG_RETURN(0);
> }
> }
JL: Again, I'm not sure tracing random val_ functions is worthwile but I'll
leave it to you to decide.
--
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
