Re: [Maria-developers] 46e0c8a9219: MDEV-11071: Assertion `thd->transaction.stmt.is_empty()' failed in Locked_tables_list::unlock_locked_table
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
(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
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(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]
Hi, Oleksandr! On Jun 14, Oleksandr Byelkin wrote:
Am 13.06.2018 um 12:08 schrieb Sergei Golubchik:
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() + */
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.
Some other errors like what? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Oleksandr!
On Jun 14, Oleksandr Byelkin wrote:
Am 13.06.2018 um 12:08 schrieb Sergei Golubchik:
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() + */ 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. Some other errors like what?
Am 14.06.2018 um 11:44 schrieb Sergei Golubchik: like an error in val_* call due to some incompatibility of arguments during mysql_create_frm_image() as I remember. i.e. our tests will fail without this check because the error left unchecked till it is too late. I put TODO remove this tests, but without them it will not just work.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Oleksandr! On Jun 14, Oleksandr Byelkin wrote:
On Jun 14, Oleksandr Byelkin wrote:
Am 13.06.2018 um 12:08 schrieb Sergei Golubchik:
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() + */ 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. Some other errors like what?
Am 14.06.2018 um 11:44 schrieb Sergei Golubchik: like an error in val_* call due to some incompatibility of arguments during mysql_create_frm_image() as I remember. i.e. our tests will fail without this check because the error left unchecked till it is too late. I put TODO remove this tests, but without them it will not just work.
Is there a test that fails without them? Wouldn't the original bug show up again? You're testing for thd->is_error() and it might be unrelated coming from some earlier error, right? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Am 14.06.2018 um 14:19 schrieb Sergei Golubchik:
Hi, Oleksandr!
On Jun 14, Oleksandr Byelkin wrote:
Am 13.06.2018 um 12:08 schrieb Sergei Golubchik:
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() + */ 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. Some other errors like what?
Am 14.06.2018 um 11:44 schrieb Sergei Golubchik: like an error in val_* call due to some incompatibility of arguments during mysql_create_frm_image() as I remember. i.e. our tests will fail without this check because the error left unchecked till it is too late. I put TODO remove this tests, but without them it will not just work. Is there a test that fails without them? yes, completely unrelated tests, before the error will be caught in fix_fields() which was produced for unrelated parts of the query (and only if we have Item_func there). We just have no other mechanisms to catch error during val_* than check thd->is_error(). Wouldn't the original bug show up again? You're testing for
On Jun 14, Oleksandr Byelkin wrote: thd->is_error() and it might be unrelated coming from some earlier error, right?
I think no, because it is on the upper level. I.e. we tried to generate .frm and checking if it succeed before starting preparing other parts of the query, and actually try to execute full query if .frm was not created will lead eventually to crashes and other problems. in other words we stop execution when it is not too late because 1) it has no sens 2) it is impossible without success of the first part.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Oleksandr! On Jun 14, Oleksandr Byelkin wrote:
I put TODO remove this tests, but without them it will not just work. Is there a test that fails without them? yes, completely unrelated tests, before the error will be caught in fix_fields() which was produced for unrelated parts of the query (and only if we have Item_func there). We just have no other mechanisms to catch error during val_* than check thd->is_error().
yeah, thought so :( okay Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik