← Back to team overview

maria-developers team mailing list archive

Re: 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()


Hi Sergei!

On Mon, Jan 17, 2022 at 7:56 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> Hi, Aleksey!
> On Jan 13, Aleksey Midenkov wrote:
> > >
> > > 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.
> vcols_need_refixing isn't run-time conditional, it depends on vcol
> flags, a specific vcol always needs refixing or never does.
> And a table has vcols_need_refixing if any of the vcols nees it.
> Thus, I think, a simpler fix would be to split the cleanup and
> fix_fields. Now both are done in fix_session_vcol_expr().
> We can do cleanup at the end of every statement and fix_fields at the
> beginning of a statement. Just like for normal non-persistent items.
> For every vcol that needs it. This way they can safely use execution
> arena.

Nice catch! Done. Btw, expr_arena was already used in
update_virtual_fields(), update_default_fields(), set_default(). So
the leak is already there and now we just didn't make it bigger. I
marked leak places with TODO comments.

> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx