maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei!

On Thu, Apr 7, 2022 at 8:09 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey,
>
> This is a review of `git diff 5fbbdbc85ba3 62d3496969db`
>
> Basically, no new comments, see below:
>
> On Apr 07, Aleksey Midenkov wrote:
>
> > diff --git a/libmariadb b/libmariadb
> > index f6c3d9fd2af..735a7299dba 160000
> > --- a/libmariadb
> > +++ b/libmariadb
> > @@ -1 +1 @@
> > -Subproject commit f6c3d9fd2af5d17db64cc996574aa312efd70fcf
> > +Subproject commit 735a7299dbae19cc2b82b9697becaf90e9b43047
>
> you've mistakenly checked in a submodule

Removed that.

>
> > diff --git a/sql/field.cc b/sql/field.cc
> > index f18fb25ebe3..04c492e6c69 100644
> > --- a/sql/field.cc
> > +++ b/sql/field.cc
> > @@ -2416,6 +2416,13 @@ int Field::set_default()
> >    if (default_value)
> >    {
> >      Query_arena backup_arena;
> > +    /*
> > +      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);
>
> I wasn't able to repeat that. In your branch (didn't try in vanilla 10.3) and
> for the above test case I see no additional allocations in table->mem_root
> after the table is opened.

Right, that was a different mem_root. I cannot find the test case for
that, though I found test cases for update_virtual_fields(). I updated
the comments. The several stack traces and the patch for catching
mem_root usage can be found here:

https://gist.github.com/midenok/fe9ef9de39ff5ed0f89e4e344db76a91

>
> > +    */
> >      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/table.cc b/sql/table.cc
> > index 08d91678b25..d22da17c31a 100644
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -2970,36 +2988,133 @@ static bool fix_vcol_expr(THD *thd, Virtual_column_info *vcol)
> ...
> > +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))
>
> I've already asked about it in a separate email,
> so here I'm just marking a line that we haven't fully agreed on yet

You are right, I have the same picture now. I tried on some different
(intermediate) revision, so that was the different result.

>
> > +  {
> > +    my_error(ER_OUT_OF_RESOURCES, MYF(0));
> > +    table->map= old_map;
> > +    return true;
> > +  }
> > +
> > +  lex.sql_command= old_lex->sql_command;
> > +  thd->variables.sql_mode= 0;
> > +
> > +  TABLE_LIST const *tl= table->pos_in_table_list;
> > +  DBUG_ASSERT(table->pos_in_table_list);
> > +
> > +  if (table->pos_in_table_list->security_ctx)
> >      thd->security_ctx= tl->security_ctx;
> > -  bool res= fix_session_vcol_expr(thd, vcol);
> > +
> > +  inited= true;
> > +  return false;
> > +}
> > +
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
@midenok