maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi, Sergei!

On Thu, Mar 31, 2022 at 11:30 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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

Added.

>
> > +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?

The change was introduced by db7edfed17ef
Updated the comment to:

    /*
      TODO: this imposes memory leak until table flush when save_in_field()
            does expr_arena allocation. F.ex. case from main.default:

            CREATE TABLE t1 (a INT DEFAULT CONCAT('1 '));
            INSERT INTO t1 VALUES (DEFAULT);
    */

>
> 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"

Ok, but the other code does allocations for other cases. I'm not sure
if it is needed to find all the test cases and why it is needed.

>
> >      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.

Fixed.

>
> >    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() ?

It failed vcol.wrong_arena. Now it doesn't fail, so I removed that.

>
> > +
> > +
> >  /**
> >    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 ?
>

This was merged successfully as the original code chunk (no changes needed).

> >    }
> >    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?

It was your suggestion in the previous review to refix always
(therefore everything). The original fix worked with
fix_session_vcol_expr_for_read(). But the current fix is simpler and
cleaner. Was there a real performance problem or was it a theoretical
issue?

>
> >    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?

close_thread_table() is called not only from close_thread_tables().

>
> >      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.

True. Should be replaced with drop_temporary_table() but I removed
that completely (since we don't need the refix for temporary tables
like you've noted).

>
> > +    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?

I replied that MDEV-25672 bug's own test case proves that, didn't I?

(rr) p alias_name_used
$4 = false
(rr) p table_name
$5 = 0x7fdc28024790 "x"

#0  Item_field::set_field (this=0x7fdc28012e60,
field_par=0x7fdc28024440) at /home/midenok/src/mariadb/10
.3/src/sql/item.cc:3289
#1  0x0000000000b7bc28 in Item_field::fix_fields (this=0x7fdc28012e60,
thd=0x7fdc28000d28, reference=0x7f
dc28034ad8) at /home/midenok/src/mariadb/10.3/src/sql/item.cc:6332
...
#15 0x00000000007c65f6 in mysql_parse (thd=0x7fdc28000d28,
rawbuf=0x7fdc28010520 "UPDATE `test_table` as
`x` SET order_date = NULL", length=48, parser_state=0x7fdc39d09548,
is_com_multi=false, is_next_command=f
alse) at /home/midenok/src/mariadb/10.3/src/sql/sql_parse.cc:7870

>
> > +  */
> >    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.

I made that in TABLE, now it looks cleaner, thanks. It cannot be in
TABLE_SHARE as unpack_vcol_info_from_frm() is done for TABLE (and we
don't want such refactorings in a bugfix).

> >    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?

That failed tests on the first revision of the bugfix. Now it seems to
work fine AFAICS, so I removed that. Sorry for not cleaning the 2nd
revision well.

>
> > +  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?

That fails at least 3 tests:

main.default vcol.vcol_syntax gcol.gcol_bugfixes

CURRENT_TEST: main.default
mysqltest: At line 1302: query 'INSERT INTO t1 VALUES( DEFAULT )'
failed: 2013: Lost connection to MySQL server during query

CURRENT_TEST: vcol.vcol_syntax
mysqltest: At line 118: query 'select * from t1' failed: 2013: Lost
connection to MySQL server during query

CURRENT_TEST: gcol.gcol_bugfixes
mysqltest: At line 579: query 'INSERT INTO t1 (suppliersenttoday)
VALUES (0)' failed: 2013: Lost connection to MySQL server during query

>
> > +  {
> > +    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?

Again I don't see failing tests. Removed that.

> 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.

Ok, I disabled refix for tmp tables. That seems to work fine.

>
> > +     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

Ok, I removed VCOL_TIME_FUNC from the condition.

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

I did the refix on open_table() even for the first time, when the
table was opened from share. Now I fixed that with from_share
condition.

>
> > +  }
> >
> >    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?

Looks like again this is an old remnant... Removed that.

>
> > +
> >    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



--
@midenok