maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jacob!

On Apr 06, Jacob Mathew wrote:
> Hi Sergei,
> 
> After doing more testing with the recursive call to
> Item::cleanup_processor(), I have found problems with column default values
> that reference another column as an argument to a date/time function.  At
> the point when fix_session_vcol_expr() is called,
> Item::check_vcol_func_processor() has already been called recursively and
> Item_field::check_vcol_func_processor() has set Name_resolution_context
> *context= 0.  This causes a crash when the recursive call to
> Item::cleanup_processor() accesses members of context.

Exactly. There is no need to run fix_fields for actual fields, for
Item_field. See, I've written in the review

-  vcol->expr->cleanup();
+  vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0);

Note there is no "cleanup_excluding_fields_processor" in the code. There
is cleanup_processor(), and it doesn't work because it tries to re-fix
fields. And there is cleanup_excluding_const_fields_processor(), it
doesn't work either, because it only excludes const fields, and still
re-fixes other fields. That's why I wrote that you need
"cleanup_excluding_const_fields_processor" which is a processor that
will exclude *all* fields, const or not.

> I think I will need to spend a lot more time learning this area to get
> this working.  So, I am currently leaning toward using a separate
> recursive call to Item::clean_cached_value() in
> fix_session_vcol_expr() and leaving its non-recursive call to
> cleanup() as is.

But the cleanup must be recursive! Unrelated to cached items, I had it
in my review - there is a test case with no cached items, that shows a
bug with non-recursive cleanup. Basically the result is correct for a
virtual column that uses current_user(), but not correct when the
virtual column uses concat(current_user()).

> Is there a reason why the Virtual_column_info class doesn't include the
> vcol type?  Is there a reason why the Virtual_column_info class shouldn't
> include the vcol type?

I think it could include the type. At least I cannot think of any reason
why it shouldn't.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx