maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

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



-- 
@midenok