maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

Could you please explain in the commit comment what the bug was and what
you are fixing?

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.

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?

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