maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey,

Here's a review of the combined `git diff 4daf9d7c3ee0 f67dcf6ba5f2`:

> diff --git a/mysql-test/suite/vcol/r/vcol_syntax.result b/mysql-test/suite/vcol/r/vcol_syntax.result
> index 0063f38ea36..c5c8a33c0ec 100644
> --- a/mysql-test/suite/vcol/r/vcol_syntax.result
> +++ b/mysql-test/suite/vcol/r/vcol_syntax.result
> @@ -94,6 +94,68 @@ create table t1 (a int, v_a int generated always as (a));
>  update t1 as x set a = 1;
>  alter table t1 force;
>  drop table t1;
> +create table t1 (
> +id int not null auto_increment primary key,
> +order_date_time datetime not null,
> +order_date date generated always as (convert(order_date_time, date)),
> +language_id binary(16) null
> +);
> +update t1 as tx set order_date= null;
> +alter table t1 modify column language_id binary(16) not null;
> +drop table t1;
>  #
> -# End of 10.2 tests
> +# MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
>  #
> +create table t1 (d1 date not null, d2 date not null,
> +gd text as (concat(d1,if(d1 <> d2, date_format(d2, 'to %y-%m-%d '), ''))) );
> +insert into t1(d1,d2) values
> +('2020-09-01','2020-09-01'),('2020-05-01','2020-09-01');

would be good to have a SELECT here, to verify the server not only
not crashes, but actually inserts something

> +drop table t1;
> +# MDEV-25772 (duplicate) and LOCK TABLES case
> +create table t1 (d1 datetime , v_d1 tinyint(1) as (d1 < curdate()));
> +insert into t1 (d1) values ('2021-09-11 08:38:23'), ('2021-09-01 08:38:23');
> +lock tables t1 write;
> +select * from t1 where v_d1=1;
> +d1	v_d1
> +2021-09-11 08:38:23	1
> +2021-09-01 08:38:23	1
> +select * from t1;
> +d1	v_d1
> +2021-09-11 08:38:23	1
> +2021-09-01 08:38:23	1
> +unlock tables;
> +drop table t1;
> +# MDEV-26432 (duplicate)
> +create table t1 (v2 int, v1 int as ((user() like 'x'))) ;
> +select 1 from t1 where v1=1 ;
> +1
> +select * from t1;
> +v2	v1
> +drop table t1;
> +create table t1 (v2 int as ( user () like 'x'));
> +select 1 from t1 order by v2 ;
> +1
> +alter table t1 add i int;
> +drop table t1;
> +# MDEV-26437 (duplicate)
> +create table v0 (v2 int not null,
> +v1 bigint as (case 'x' when current_user() then v2 end));
> +select v2 as v3 from v0 where v1 like 'x' escape 'x';
> +v3
> +insert into v0 (v2) values (-128);
> +drop table v0;
> +create table t1 (vi int as (case 'x' when current_user() then 1 end));
> +select 1 from t1 where vi=1;
> +1
> +show create table t1;
> +Table	Create Table
> +t1	CREATE TABLE `t1` (
> +  `vi` int(11) GENERATED ALWAYS AS (case 'x' when current_user() then 1 end) VIRTUAL
> +) ENGINE=MyISAM DEFAULT CHARSET=latin1
> +drop table t1;
> +create table t1 (vi int as (case 'x' when current_user() then 1 end));
> +select 1 from t1 where vi=1;
> +1
> +select 1 from t1 where vi=1;
> +1
> +drop table t1;
> diff --git a/sql/field.cc b/sql/field.cc
> index 4e6bc6b8341..297edd9e75c 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -2453,6 +2453,7 @@ int Field::set_default()
>    if (default_value)
>    {
>      Query_arena backup_arena;
> +    /* TODO: this imposes memory leak until table flush */

could you add an example here? in what case, exactly, there
can be a memory leak?

you have more comments like that below, I'd suggest to add one example
somewhere, and change other comments to say "see ... for an example"

>      table->in_use->set_n_backup_active_arena(table->expr_arena, &backup_arena);
>      int rc= default_value->expr->save_in_field(this, 0);
>      table->in_use->restore_active_arena(table->expr_arena, &backup_arena);
> diff --git a/sql/item.h b/sql/item.h
> index 2a904c1691a..e3028ae412c 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -2587,6 +2587,12 @@ class Item_ident :public Item_result_field
>    const char *db_name;
>    const char *table_name;
>    const char *field_name;
> +  /*
> +     NOTE: came from TABLE::alias_name_used and this is only a hint! It works
> +     only in need_correct_ident() condition. On other cases it is FALSE even if
> +     table_name is alias! It cannot be TRUE in these cases, lots of spaghetti
> +     logic depends on that.
> +  */

this is a copy of the comment in table.h
you've changed the comment in table.h, but missed this one.

>    bool alias_name_used; /* true if item was resolved against alias */
>    /* 
>      Cached value of index for this field in table->field array, used by prep. 
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 3f0fba8fc10..8b521f90108 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -1781,6 +1781,25 @@ class Drop_table_error_handler : public Internal_error_handler
>  };
>  
>  
> +struct Silence_warnings : public Internal_error_handler
> +{
> +public:
> +  virtual bool handle_condition(THD *,
> +                                uint,
> +                                const char* sqlstate,
> +                                Sql_condition::enum_warning_level *level,
> +                                const char* msg,
> +                                Sql_condition ** cond_hdl)
> +  {
> +    *cond_hdl= NULL;
> +    if (*level == Sql_condition::WARN_LEVEL_WARN)
> +      return TRUE;
> +
> +    return FALSE;
> +  }
> +};

Why do you disable warnings around fix_session_expr() ?

> +
> +
>  /**
>    Internal error handler to process an error from MDL_context::upgrade_lock()
>    and mysql_lock_tables(). Used by implementations of HANDLER READ and
> diff --git a/sql/item.cc b/sql/item.cc
> index 3ff0219c3b3..438bf36060e 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -5507,7 +5506,7 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
>    if (context)
>    {
>      select= context->select_lex;
> -    lex_s= context->select_lex->parent_lex;
> +    lex_s= select ? select->parent_lex : NULL;

There's no this if() in 10.2 anymore, it was removed in d6ee351bbb66.
How should this hunk be modified for d6ee351bbb66 ?

>    }
>    else
>    {
> @@ -5709,8 +5708,6 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
>    }
>  #endif
>    fixed= 1;
> -  if (field->vcol_info)
> -    fix_session_vcol_expr_for_read(thd, field, field->vcol_info);

Did you revert the optimization from 7450cb7f69db?
It was "re-fix vcols on demand, not always for every SELECT"

and as far as I can see now you again always re-fix everything,
is that so?

>    if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY &&
>        !outer_fixed &&
>        select &&
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 5173df260d5..09af805d60c 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -746,6 +746,8 @@ void close_thread_tables(THD *thd)
>      /* Table might be in use by some outer statement. */
>      DBUG_PRINT("tcache", ("table: '%s'  query_id: %lu",
>                            table->s->table_name.str, (ulong) table->query_id));
> +    if (thd->locked_tables_mode)
> +      table->vcol_cleanup_expr(thd);

Why do you check for thd->locked_tables_mode ?
It seems that you'll do vcol_cleanup_expr() unconditionally on
any code path. Either here or later in close_thread_table().

So why not just do it always here?

>      if (thd->locked_tables_mode <= LTM_LOCK_TABLES ||
>          table->query_id == thd->query_id)
>      {
> diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc
> index 005a520ff64..f06dec3a7ff 100644
> --- a/sql/temporary_tables.cc
> +++ b/sql/temporary_tables.cc
> @@ -1128,6 +1128,13 @@ TABLE *THD::open_temporary_table(TMP_TABLE_SHARE *share,
>    table->pos_in_table_list= 0;
>    table->query_id= query_id;
>  
> +  if (table->vcol_fix_expr(this))
> +  {
> +    my_free(table);

Hmm, you sure? I'd expect that after successful open_table_from_share()
you'd need much more than just my_free() to close the table.

> +    DBUG_RETURN(NULL);
> +  }
> +
> +
>    /* Add table to the head of table list. */
>    share->all_tmp_tables.push_front(table);
>  
> diff --git a/sql/table.h b/sql/table.h
> index 38b63d285c6..ab1d96538c0 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -1374,8 +1374,13 @@ struct TABLE
>    */
>    bool auto_increment_field_not_null;
>    bool insert_or_update;             /* Can be used by the handler */
> +  /*
> +     NOTE: alias_name_used is only a hint! It works only in need_correct_ident()
> +     condition. On other cases it is FALSE even if table_name is alias!

As I asked earlier, do you have any test case that proves this claim?

> +  */
>    bool alias_name_used;              /* true if table_name is alias */
>    bool get_fields_in_item_tree;      /* Signal to fix_field */
> +  List<Virtual_column_info> vcol_cleanup_list;

If you have a list of all items that need cleanup,
may be can also use it to refix items? Meaning, you move it to
TABLE_SHARE, remove need_refix, instead write

-  if (table->s->need_refix)
+  if (!table->s->vcol_refix_list.is_empty())

and, of course, don't pop elements on cleanup, but iterate the
list in a non-destructive way.

>  private:
>    bool m_needs_reopen;
>    bool created;    /* For tmp tables. TRUE <=> tmp table was actually created.*/
> diff --git a/sql/table.cc b/sql/table.cc
> index d4f8170e0af..4bed87d7740 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -1005,6 +1004,22 @@ static void mysql57_calculate_null_position(TABLE_SHARE *share,
>  bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table,
>                       bool *error_reported, vcol_init_mode mode)
>  {
> +  struct check_vcol_forward_refs
> +  {
> +    static bool check(Field *field, Virtual_column_info *vcol)
> +    {
> +      return vcol &&
> +             vcol->expr->walk(&Item::check_field_expression_processor, 0, field);
> +    }
> +    static bool check(Field *field)
> +    {
> +      if (check(field, field->vcol_info) ||
> +          check(field, field->check_constraint) ||
> +          check(field, field->default_value))
> +        return true;
> +      return false;
> +    }
> +  };

Hm, somehow I haven't seen this pattern before. nice.

>    CHARSET_INFO *save_character_set_client= thd->variables.character_set_client;
>    CHARSET_INFO *save_collation= thd->variables.collation_connection;
>    Query_arena  *backup_stmt_arena_ptr= thd->stmt_arena;
> @@ -2831,36 +2841,162 @@ static bool fix_vcol_expr(THD *thd, Virtual_column_info *vcol)
>      @note this is done for all vcols for INSERT/UPDATE/DELETE,
>      and only as needed for SELECTs.
>  */
> -bool fix_session_vcol_expr(THD *thd, Virtual_column_info *vcol)
> +bool Virtual_column_info::fix_session_expr(THD *thd, TABLE *table)
>  {
> -  DBUG_ENTER("fix_session_vcol_expr");
> -  if (!(vcol->flags & (VCOL_TIME_FUNC|VCOL_SESSION_FUNC)))
> -    DBUG_RETURN(0);
> +  if (!need_refix())
> +    return false;
>  
> -  vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0);
> -  DBUG_ASSERT(!vcol->expr->fixed);
> -  DBUG_RETURN(fix_vcol_expr(thd, vcol));
> +  DBUG_ASSERT(!expr->fixed);
> +  if (expr->walk(&Item::change_context_processor, 0, thd->lex->current_context()))
> +    return true;

Why do you need to change the context here?

> +  table->vcol_cleanup_list.push_back(this, thd->mem_root);
> +  if (fix_expr(thd))
> +    return true;
> +  if (expr->walk(&Item::change_context_processor, 0, NULL))
> +    return true;
> +  return false;
> +}
> +
> +
> +bool Virtual_column_info::cleanup_session_expr()
> +{
> +  DBUG_ASSERT(need_refix());
> +  if (expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0))
> +    return true;
> +  return false;
>  }
>  
>  
> -/** invoke fix_session_vcol_expr for a vcol
>  
> -    @note this is called for generated column or a DEFAULT expression from
> -    their corresponding fix_fields on SELECT.
> -*/
> -bool fix_session_vcol_expr_for_read(THD *thd, Field *field,
> -                                    Virtual_column_info *vcol)
> +class Vcol_expr_context
>  {
> -  DBUG_ENTER("fix_session_vcol_expr_for_read");
> -  TABLE_LIST *tl= field->table->pos_in_table_list;
> -  if (!tl || tl->lock_type >= TL_WRITE_ALLOW_WRITE)
> -    DBUG_RETURN(0);
> -  Security_context *save_security_ctx= thd->security_ctx;
> -  if (tl->security_ctx)
> +  bool inited;
> +  THD *thd;
> +  TABLE *table;
> +  LEX *old_lex;
> +  LEX lex;
> +  table_map old_map;
> +  bool old_want_privilege;
> +  Security_context *save_security_ctx;
> +  sql_mode_t save_sql_mode;
> +  Silence_warnings disable_warnings;
> +
> +public:
> +  Vcol_expr_context(THD *_thd, TABLE *_table) :
> +    inited(false),
> +    thd(_thd),
> +    table(_table),
> +    old_lex(thd->lex),
> +    old_map(table->map),
> +    old_want_privilege(table->grant.want_privilege),
> +    save_security_ctx(thd->security_ctx),
> +    save_sql_mode(thd->variables.sql_mode) {}
> +  bool init();
> +
> +  ~Vcol_expr_context();
> +};
> +
> +
> +bool Vcol_expr_context::init()
> +{
> +  /*
> +      As this is vcol expression we must narrow down name resolution to
> +      single table.
> +  */
> +  if (init_lex_with_single_table(thd, table, &lex))

Do you have a test that fails otherwise?

> +  {
> +    my_error(ER_OUT_OF_RESOURCES, MYF(0));
> +    table->map= old_map;
> +    return true;
> +  }
> +
> +  thd->push_internal_handler(&disable_warnings);
> +  table->grant.want_privilege= false;

1. why?
2. it's not bool, it's a set of privileges, don't use false here.

> +  lex.sql_command= old_lex->sql_command;
> +  thd->variables.sql_mode= 0;
> +
> +  /*
> +     NOTE: we refix also tmp tables used in ALTER TABLE,

Why? I don't see how a tmp table from ALTER can possibly
be used in two different statements. There should be no need
to re-fix it. Just fixing once, on open, should be enough.

> +     they have (pos_in_table_list == NULL).
> +  */
> +  TABLE_LIST const *tl= table->pos_in_table_list;
> +  DBUG_ASSERT(tl || table->s->tmp_table);
> +
> +  if (tl && tl->security_ctx)
>      thd->security_ctx= tl->security_ctx;
> -  bool res= fix_session_vcol_expr(thd, vcol);
> +
> +  inited= true;
> +  return false;
> +}
> +
> +Vcol_expr_context::~Vcol_expr_context()
> +{
> +  if (!inited)
> +    return;
> +  table->grant.want_privilege= old_want_privilege;
> +  thd->pop_internal_handler();
> +  end_lex_with_single_table(thd, table, old_lex);
> +  table->map= old_map;
>    thd->security_ctx= save_security_ctx;
> -  DBUG_RETURN(res);
> +  thd->variables.sql_mode= save_sql_mode;
> +}
> +
> +
> +bool TABLE::vcol_fix_expr(THD *thd)
> +{
> +  DBUG_ASSERT(pos_in_table_list || s->tmp_table);
> +  if ((pos_in_table_list && pos_in_table_list->placeholder()) ||
> +      !s->vcol_need_refix)
> +    return false;
> +
> +  if (!thd->stmt_arena->is_conventional() &&
> +      !vcol_cleanup_list.is_empty())
> +  {
> +    /* NOTE: Under trigger we already have fixed expressions */
> +    return false;
> +  }
> +  DBUG_ASSERT(vcol_cleanup_list.is_empty());
> +
> +  Vcol_expr_context expr_ctx(thd, this);
> +  if (expr_ctx.init())
> +    return true;
> +
> +  for (Field **vf= vfield; vf && *vf; vf++)
> +    if ((*vf)->vcol_info->fix_session_expr(thd, this))
> +      goto error;
> +
> +  for (Field **df= default_field; df && *df; df++)
> +    if ((*df)->default_value &&
> +        (*df)->default_value->fix_session_expr(thd, this))
> +      goto error;
> +
> +  for (Virtual_column_info **cc= check_constraints; cc && *cc; cc++)
> +    if ((*cc)->fix_session_expr(thd, this))
> +      goto error;
> +
> +  return false;
> +
> +error:
> +  DBUG_ASSERT(thd->get_stmt_da()->is_error());
> +  return true;
> +}
> +
> +
> +bool TABLE::vcol_cleanup_expr(THD *thd)
> +{
> +  if (vcol_cleanup_list.is_empty())
> +    return false;
> +
> +  List_iterator<Virtual_column_info> it(vcol_cleanup_list);
> +  bool result= false;
> +
> +  while (Virtual_column_info *vcol= it++)
> +    result|= vcol->cleanup_session_expr();
> +
> +  vcol_cleanup_list.empty();
> +
> +  DBUG_ASSERT(!result || thd->get_stmt_da()->is_error());
> +  return result;
>  }
>  
>  
> @@ -2939,14 +3072,17 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table,
>      */
>      myf warn= table->s->frm_version < FRM_VER_EXPRESSSIONS ? ME_JUST_WARNING : 0;
>      my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(warn),
> -             "AUTO_INCREMENT", vcol->get_vcol_type_name(), res.name);
> +             "AUTO_INCREMENT", get_vcol_type_name(), res.name);
>      if (!warn)
>        DBUG_RETURN(1);
>    }
> -  vcol->flags= res.errors;
> +  flags= res.errors;
>  
> -  if (vcol->flags & VCOL_SESSION_FUNC)
> -    table->s->vcols_need_refixing= true;
> +  if (need_refix())

Old code only refixed VCOL_SESSION_FUNC, because those Items
can change the medata on fix_fields. See the comment for 73a220aac384
Refixing VCOL_TIME_FUNC seems to be redundant

> +  {
> +    table->s->vcol_need_refix= true;
> +    cleanup_session_expr();

why?

> +  }
>  
>    DBUG_RETURN(0);
>  }
> @@ -3011,11 +3147,13 @@ unpack_vcol_info_from_frm(THD *thd, MEM_ROOT *mem_root, TABLE *table,
>    if (error)
>      goto end;
>  
> +  lex.sql_command= old_lex->sql_command;

why?

> +
>    vcol_storage.vcol_info->set_vcol_type(vcol->get_vcol_type());
>    vcol_storage.vcol_info->stored_in_db=      vcol->stored_in_db;
>    vcol_storage.vcol_info->name=              vcol->name;
>    vcol_storage.vcol_info->utf8=              vcol->utf8;
> -  if (!fix_and_check_vcol_expr(thd, table, vcol_storage.vcol_info))
> +  if (!vcol_storage.vcol_info->fix_and_check_expr(thd, table))
>    {
>      *vcol_ptr= vcol_info= vcol_storage.vcol_info;   // Expression ok
>      DBUG_ASSERT(vcol_info->expr);

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx