Hi Sergei!
On Thu, Apr 7, 2022 at 8:09 PM Sergei Golubchik <s...@mariadb.org> 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 secur...@mariadb.org
--
@midenok
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
