Re: [Maria-developers] 06942363ebc: MDEV-14347 CREATE PROCEDURE returns no error when using an unknown variable
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?
+{ + /* + 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.
+ 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
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
Hi, Alexander! On Jun 07, Alexander Barkov wrote:
@@ -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); + if (ref && *ref) + return (*ref)->walk(processor, walk_subquery, arg) || + (this->*processor)(arg); + return false; +}
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?
Let's change it to call this->*processor unconditionally. And fix individual processors to check for ref!=NULL. It looks like the only processor that will need it is bool enumerate_field_refs_processor(void *arg) - { return (*ref)->enumerate_field_refs_processor(arg); } + { return ref && (*ref)->enumerate_field_refs_processor(arg); }
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?
I think I prefer what I suggested, to call walk from the processor :) walk() method defines the general API for traversing the tree. An individual processor is the user of that API. If some user has special weird needs, I'd prefer that to be implemented in that user processor, not in the general API used by many.
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:
Agree Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik