maria-developers team mailing list archive
Mailing list archive
Re: 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
On Thu, Jan 6, 2022 at 7:13 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> Hi, Aleksey!
> Could you please explain in the commit comment what the bug was and what
> you are fixing?
I have added a new message to the last commit. This will be the
message of the squashed patch.
> I wasn't able to understad that, so I had to debug those crashes myself.
> So, the reason is, objects allocated in the wrong arena, right?
> And it was done inconsistently, on different code paths the "re-fix"
> context was different.
> I agree that encapsulating all the context in one Vcol_expr_context is a
> good idea, it'll allow to have the same, correct, context everywhere.
> But always using expr_arena doesn't necessarily seem to be a correct
> choice. E.g. Arg_comparator creates Item_cache_temporal on every
> fix_field. If vcol expr is re-fixed all the time, expr_arena will grow
> continuously. Same for charset conversion, CASE, IN, etc. I suspect that
> if the vcol expr is always re-fixed, it has to use stmt arena.
stmt_arena cannot be used as long as 'expr' item is created on
expr_arena. 'expr' item is created at the vcol expression parse and is
reused during the whole TABLE lifetime. If some of the containee items
later were allocated on stmt_arena, 'expr' item will try to access
them in a different statement when they are already released. That is
what this bugfix tries to address.
Yes, this fix introduces a small and in most cases imperceptible
memory leak which can be easily overcame by FLUSH TABLES. The fix for
this leak is non-trivial.
1. vcol expr is not always refixed, but conditionally controlled by
vcols_need_refixing. And we must remove this control and truly refix
2. we must clone 'expr' item into statement mem_root at each statement.
I have attached the leak fix PoC into this email. I do not know
whether this is worth it.
> Why do you re-fix fields? I can hardly imagine that an Item_field can
> find itself in a different table. And if it's always in the same table,
> why to force it to find this table over and over?
That was the alias fix. Refix of Item_field was required. I returned
back cleanup_excluding_fields_processor() and removed VCOL_TABLE_ALIAS
processing despite my opinion to apply that.
> We've already discussed VCOL_TABLE_ALIAS, no need to go there again.
> On Jan 06, Aleksey Midenkov wrote:
> > revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5)
> > parent(s): a565e63c54c
> > author: Aleksey Midenkov
> > committer: Aleksey Midenkov
> > timestamp: 2021-12-29 00:46:31 +0300
> > message:
> > MDEV-24176 Server crashes after insert in the table with virtual
> > column generated using date_format() and if()
> > When table is reopened from cache vcol_info contains stale
> > expression. We refresh expression via TABLE::vcol_fix_exprs() but
> > first we must prepare a proper context (Vcol_expr_context) and do some
> > preparations which meet some requirements:
> > 1. fields must not be fixed, we unfix them with cleanup() via
> > cleanup_processor.
> > 2. Item_ident must have cached_table cleared. Again, this is stale
> > data which was already freed. cached_table interferes with
> > find_field_in_tables().
> > We cannot clear cached_table directly in Item_ident::cleanup()
> > method. Lots of spaghetti logic depends on cached_table existence even
> > after cleanup() done.
> > 3. If Item_ident has table_name non-NULL we trigger expr update. That
> > is done via Item_ident::check_vcol_func_processor() and
> > VCOL_TABLE_ALIAS flag.
> > (4-7) The below conditions are prepared by Vcol_expr_context and used in
> > both fix_session_expr_for_read() and TABLE::vcol_fix_exprs():
> > 4. Any fix_expr() must be done on expr_arena as there may be new items
> > created. It was a bug in fix_session_expr_for_read(). It was just not
> > reproduced because there were no second refix. Now refix is done for
> > more cases so it does reproduce. Tests affected: vcol.binlog
> > 5. Also name resolution context must be narrowed to the single table
> > with all crutches in place. Tests affected: vcol.update
> > 6. sql_mode must be clean and not fail expr update.
> > sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc
> > must not affect vcol expression update. If the table was created
> > successfully any further evaluation must not fail. Tests affected:
> > main.func_like
> > 7. Warnings are disabled during expr update. It is assumed that these
> > warnings were printed before during initialization phase or later
> > during execution phase. Tests affected: vcol.wrong_arena
> > Dictionary: refresh, update and refix are the synonyms for
> > vcol_fix_exprs() or fix_session_expr_for_read() calls.
> > 8. cleanup_excluding_fields_processor() removed. It was just a quick
> > hack to exclude wrongly working Item_field from processing. Now it
> > works due to correct execution environment (Vcol_expr_context).
> > Related to MDEV-10355.
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
diff --git a/sql/field.h b/sql/field.h
index 7b71ddb479f..53a0395221d 100644
@@ -629,6 +629,7 @@ class Virtual_column_info: public Sql_alloc
bool utf8; /* Already in utf8 */
+ Item *global_expr;
LEX_STRING name; /* Name of constraint */
/* see VCOL_* (VCOL_FIELD_REF, ...) */
diff --git a/sql/table.cc b/sql/table.cc
index 24854342ac5..ab203f50065 100644
@@ -2847,6 +2847,8 @@ bool Virtual_column_info::fix_session_expr(THD *thd)
if (!(flags & (VCOL_TIME_FUNC|VCOL_SESSION_FUNC)))
+ expr= global_expr->build_clone(thd, thd->mem_root);
if (expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0))
@@ -2916,8 +2918,8 @@ bool Vcol_expr_context::init()
- thd->set_n_backup_active_arena(expr_arena, &backup_arena);
- thd->stmt_arena= expr_arena;
+// thd->set_n_backup_active_arena(expr_arena, &backup_arena);
+// thd->stmt_arena= expr_arena;
@@ -2933,8 +2935,8 @@ Vcol_expr_context::~Vcol_expr_context()
- thd->restore_active_arena(expr_arena, &backup_arena);
- thd->stmt_arena= stmt_backup;
+// thd->restore_active_arena(expr_arena, &backup_arena);
+// thd->stmt_arena= stmt_backup;
end_lex_with_single_table(thd, table, old_lex);
@@ -2965,7 +2967,7 @@ bool Virtual_column_info::fix_session_expr_for_read(THD *thd, Field *field)
bool TABLE::vcol_fix_exprs(THD *thd)
- if (pos_in_table_list->placeholder() || !s->vcols_need_refixing)
+ if (pos_in_table_list->placeholder())
Vcol_expr_context expr_ctx(thd, this);
@@ -3072,9 +3074,6 @@ bool Virtual_column_info::fix_and_check_expr(THD *thd, TABLE *table)
- if (flags & VCOL_SESSION_FUNC)
- table->s->vcols_need_refixing= true;
@@ -3148,6 +3147,7 @@ unpack_vcol_info_from_frm(THD *thd, TABLE *table,
*vcol_ptr= vcol_info= vcol_storage.vcol_info; // Expression ok
+ vcol_info->global_expr= vcol_info->expr;
diff --git a/sql/table.h b/sql/table.h
index fed8b951b83..805564e75f8 100644
@@ -801,7 +801,6 @@ struct TABLE_SHARE
- bool vcols_need_refixing;
ulong table_map_id; /* for row-based replication */