Hi Sergei! On Mon, Jan 17, 2022 at 7:56 PM Sergei Golubchik <s...@mariadb.org> 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 secur...@mariadb.org -- @midenok _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp