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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
