Hi Sergei, Thanks for your review. Comments follow: On 6/7/20 9:59 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jun 07, Alexander Barkov wrote:
revision-id: 06942363ebc (mariadb-10.5.2-311-g06942363ebc) parent(s): 0d6d63e1505 author: Alexander Barkov <bar@mariadb.com> committer: Alexander Barkov <bar@mariadb.com> timestamp: 2020-06-03 12:11:07 +0400 message:
MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable
diff --git a/sql/item.cc b/sql/item.cc index 8ea6366e6c4..5800b9bc95e 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -764,6 +764,29 @@ bool Item_field::collect_item_field_processor(void *arg) }
+void Item_ident::undeclared_spvar_error() const
Currently it's used only in Item_field::unknown_splocal_processor(). Do you plan to use this somewhere else? Why is it a separate method and in Item_ident?
It's also used in Item_ref::unknown_splocal_processor(). Item_ident their common parent.
+{ + /* + We assume this is an unknown SP variable, possibly a ROW variable. + Print the leftmost name in the error: + SET var=a; -> a + SET var=a.b; -> a + SET var=a.b.c; -> a + */ + my_error(ER_SP_UNDECLARED_VAR, MYF(0), db_name.str ? db_name.str : + table_name.str ? table_name.str : + field_name.str); +} + +bool Item_field::unknown_splocal_processor(void *arg) +{ + DBUG_ENTER("Item_field::unknown_splocal_processor"); + DBUG_ASSERT(type() == FIELD_ITEM); + undeclared_spvar_error(); + DBUG_RETURN(true); +} + + bool Item_field::add_field_to_set_processor(void *arg) { DBUG_ENTER("Item_field::add_field_to_set_processor"); @@ -8012,6 +8035,27 @@ void Item_ref::cleanup() }
+bool Item_ref::unknown_splocal_processor(void *arg) +{ + DBUG_ENTER("Item_ref::unknown_splocal_processor"); + DBUG_ASSERT(type() == REF_ITEM); + DBUG_ASSERT(ref_type() == REF); + undeclared_spvar_error(); + DBUG_RETURN(true); +} + + +bool Item_ref::walk(Item_processor processor, bool walk_subquery, void *arg) +{ + if (processor == &Item::unknown_splocal_processor) + return unknown_splocal_processor(arg);
Why is that? This is very unusual, normally walk() should not change its behavior based on the processor.
Note, ref is NULL when walk(Item::unknown_splocal_processor) is called. We have to change Item_ref::walk() somehow: It does not call (this->*processor)(arg) when ref is NULL. Do you have ideas how to do it in a better way? Btw, your proposed solution with calling walk(walk_subquery=false) and with doing recursion in Item_subselect::unknown_splocal_processor() also changes behavior: calling walk() from a processor does not look "normal" :) This solution effectively implements exactly the same logic: Item_subselect::walk() { ... if (processor == &Item::unknown_splocal_processor) do_recursion_not_normally(); ... } just in a less obvious way. I think I'd prefer it to be written the same way ^^^ inside Item_subselect::walk(), like I did in Item_ref, rather than calling the recursion from the processor. A possible option would be to change the parameter "bool walk_subquery" from bool to something else, so the caller can pass more options. What do you think? But returning to Item_ref, I just realized that this statement (without tables) incorrectly reports the unknown variable error: SET a=(SELECT 1 AS x HAVING x); I can see two ways: 1. Skip HAVING for now - don't try to catch unknown variables in HAVING. 2. For every Item_ident in HAVING go through the select list and check aliases. I'm inclined to 1: - The intent of this MDEV was to catch simple obvious cases, like: SET x=long_variable_name_with_a_typo; - Having is not very useful when there are not tables. It's very unlikely that someone will use it, moreover will make a typo. - We don't have to touch Item_ref::walk() if we skip HAVING, it seems. Or do we still have to? Thanks.
+ if (ref && *ref) + return (*ref)->walk(processor, walk_subquery, arg) || + (this->*processor)(arg); + return false; +} + + /** Transform an Item_ref object with a transformer callback function.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org