Re: [Maria-developers] 62d3496969d: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
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
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.
+ */ 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
+ { + 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@mariadb.org
Hi Sergei! On Thu, Apr 7, 2022 at 8:09 PM Sergei Golubchik <serg@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 security@mariadb.org
-- @midenok
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik