maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx