14 Jun
2018
14 Jun
'18
10:39 a.m.
Am 13.06.2018 um 12:08 schrieb Sergei Golubchik:
> 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?
it will lead to an error (overriding virtual function).
> 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?"
Somehow warning is not issued if add the attribute here (it have to be
added to each definition to work as I understand).
>
>> 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?
just missed it, it is not problem to fix
>
>> }
>>
>>
>> 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?
OK
>
>> + 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?
yes, will be fixed
>> + 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"
OK.
>
> and I still don't understand why do you need to check for thd->is_error()
> here
because it catch some other errors which never will be checked and
fixing this IMHO is other matter and in higher version.
>> + 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
the same.
>
>> {
>> // Since changes were done in-place, we can't revert them.
>> (void) quick_rm_table(thd, db_type,
[skip]