maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei!

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.

Yes.

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

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.
> >
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
@midenok

diff --git a/sql/field.h b/sql/field.h
index 7b71ddb479f..53a0395221d 100644
--- a/sql/field.h
+++ b/sql/field.h
@@ -629,6 +629,7 @@ class Virtual_column_info: public Sql_alloc
   bool stored_in_db;
   bool utf8;                                    /* Already in utf8 */
   bool automatic_name;
+  Item *global_expr;
   Item *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
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -2847,6 +2847,8 @@ bool Virtual_column_info::fix_session_expr(THD *thd)
   if (!(flags & (VCOL_TIME_FUNC|VCOL_SESSION_FUNC)))
     return false;
 
+  expr= global_expr->build_clone(thd, thd->mem_root);
+
   if (expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0))
     return true;
   DBUG_ASSERT(!expr->fixed);
@@ -2916,8 +2918,8 @@ bool Vcol_expr_context::init()
   lex.current_context()->select_lex= tl->select_lex;
   lex.sql_command= old_lex->sql_command;
 
-  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;
 
   if (tl->security_ctx)
     thd->security_ctx= tl->security_ctx;
@@ -2933,8 +2935,8 @@ Vcol_expr_context::~Vcol_expr_context()
   if (!inited)
     return;
   table->grant.want_privilege= old_want_privilege;
-  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;
   thd->pop_internal_handler();
   end_lex_with_single_table(thd, table, old_lex);
   table->map= old_map;
@@ -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())
     return false;
 
   Vcol_expr_context expr_ctx(thd, this);
@@ -3072,9 +3074,6 @@ bool Virtual_column_info::fix_and_check_expr(THD *thd, TABLE *table)
   }
   flags= res.errors;
 
-  if (flags & VCOL_SESSION_FUNC)
-    table->s->vcols_need_refixing= true;
-
   DBUG_RETURN(0);
 }
 
@@ -3148,6 +3147,7 @@ unpack_vcol_info_from_frm(THD *thd, TABLE *table,
   {
     *vcol_ptr= vcol_info= vcol_storage.vcol_info;   // Expression ok
     DBUG_ASSERT(vcol_info->expr);
+    vcol_info->global_expr= vcol_info->expr;
     goto end;
   }
   *error_reported= TRUE;
diff --git a/sql/table.h b/sql/table.h
index fed8b951b83..805564e75f8 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -801,7 +801,6 @@ struct TABLE_SHARE
   bool can_cmp_whole_record;
   bool table_creation_was_logged;
   bool non_determinstic_insert;
-  bool vcols_need_refixing;
   bool has_update_default_function;
   ulong table_map_id;                   /* for row-based replication */