Hi, Oleksandr! Looks good, but please see a few comments below. On May 08, Oleksandr Byelkin wrote:
revision-id: 46e0c8a921978e7b95adf3f17f3c85b18d9f9ef6 (mariadb-10.2.14-78-g46e0c8a9219) parent(s): 9bcd0f5fea8ca26742b10d37b95a966c69909ff1 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2018-05-08 15:26:26 +0200 message:
MDEV-11071: Assertion `thd->transaction.stmt.is_empty()' failed in Locked_tables_list::unlock_locked_table
fix_length_and_dec now return result (error/OK)
diff --git a/sql/item.h b/sql/item.h index 8921ee76f6a..e9713730a1c 100644 --- a/sql/item.h +++ b/sql/item.h @@ -4210,7 +4210,7 @@ class Item_func_or_sum: public Item_result_field, also to make printing of items inherited from Item_sum uniform. */ virtual const char *func_name() const= 0; - virtual void fix_length_and_dec()= 0; + virtual bool fix_length_and_dec()= 0;
1. I wonder, if you change fix_length_and_dec to void in one of the derived classes, will you get a compiler warning for that? 2. may be also __attribute__ ((warn_unused_result)) here? I don't know how it'll work on virtual methods, may be it won't. both questions are for the use case "what if we merge some fix_length_and_dec related changes from 10.1, will the compiler tell us to update them?"
bool const_item() const { return const_item_cache; } table_map used_tables() const { return used_tables_cache; } Item* build_clone(THD *thd, MEM_ROOT *mem_root); diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 44cc4f3cae9..46cf412e229 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -535,8 +535,9 @@ void Item_bool_rowready_func2::fix_length_and_dec() we have to check for out of memory conditions here */ if (!args[0] || !args[1]) - return; - setup_args_and_comparator(current_thd, &cmp); + return FALSE; + bool res= setup_args_and_comparator(current_thd, &cmp); + return res;
not that it matters much (no need to change), but why didn't you return setup_args_and_comparator(current_thd, &cmp); directly?
}
diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h index de1b27cff1a..5efb98bc81d 100644 --- a/sql/item_cmpfunc.h +++ b/sql/item_cmpfunc.h @@ -911,10 +911,12 @@ class Item_func_strcmp :public Item_int_func longlong val_int(); uint decimal_precision() const { return 1; } const char *func_name() const { return "strcmp"; } - void fix_length_and_dec() + bool fix_length_and_dec() { - agg_arg_charsets_for_comparison(cmp_collation, args, 2); + if (agg_arg_charsets_for_comparison(cmp_collation, args, 2)) + return TRUE; fix_char_length(2); // returns "1" or "0" or "-1"
this comment is kinda weird. remove?
+ return FALSE; } Item *get_copy(THD *thd, MEM_ROOT *mem_root) { return get_item_copy<Item_func_strcmp>(thd, mem_root, this); } @@ -1987,10 +1997,10 @@ class Item_func_like :public Item_bool_func2 const char *func_name() const { return "like"; } enum precedence precedence() const { return CMP_PRECEDENCE; } bool fix_fields(THD *thd, Item **ref); - void fix_length_and_dec() + bool fix_length_and_dec() { max_length= 1; - agg_arg_charsets_for_comparison(cmp_collation, args, 2);
should this one also be __attribute__ ((warn_unused_result)) ?
+ return agg_arg_charsets_for_comparison(cmp_collation, args, 2); } void cleanup();
diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc index 4214980c36c..ca49e06a9e7 100644 --- a/sql/item_subselect.cc +++ b/sql/item_subselect.cc @@ -904,9 +905,10 @@ Item::Type Item_subselect::type() const }
-void Item_subselect::fix_length_and_dec() +bool Item_subselect::fix_length_and_dec() { engine->fix_length_and_dec(0);
what about subselect_engine::fix_length_and_dec() ? should return bool too, I guess, right?
+ return FALSE; }
diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 9e7973b745c..f3cb85f01d3 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -4894,7 +4894,13 @@ int create_table_impl(THD *thd, file= mysql_create_frm_image(thd, orig_db, orig_table_name, create_info, alter_info, create_table_mode, key_info, key_count, frm); - if (!file) + /* + We have to check thd->is_error() here because it can be set by + Item::val* for example, and before it will be cought accidentally by + Item_func::fix_fields() of the next call. Now we removed the check + from Item_func::fix_fields() + */
this is a very confusing comment, don't write in comments that "it used to be this way, but now we've changed it and it is that way" and I still don't understand why do you need to check for thd->is_error() here
+ if (!file || thd->is_error()) goto err; if (rea_create_table(thd, frm, path, db, table_name, create_info, file, frm_only)) @@ -7377,7 +7383,8 @@ static bool mysql_inplace_alter_table(THD *thd, */ if (mysql_rename_table(db_type, alter_ctx->new_db, alter_ctx->tmp_name, alter_ctx->db, alter_ctx->alias, - FN_FROM_IS_TMP | NO_HA_TABLE)) + FN_FROM_IS_TMP | NO_HA_TABLE) || + thd->is_error())
and here
{ // Since changes were done in-place, we can't revert them. (void) quick_rm_table(thd, db_type,
Regards, Sergei Chief Architect MariaDB and security@mariadb.org