[Maria-developers] Please review MDEV-10030 sql_yacc.yy: Split table_expression and remove PROCEDURE from create_select, select_paren_derived, select_derived2, query_specification
Hi Sergei, Please review a patch for MDEV-10030, which is a part of MDEV-8909 union parser cleanup. Thanks!
Hi, Alexander! Sure, that's ok. A couple of comments below: On May 05, Alexander Barkov wrote:
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index a6bbfc8..0d8bfb3 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -988,6 +988,7 @@ JOIN::prepare(TABLE_LIST *tables_init, } if (thd->lex->derived_tables) { + // SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
What does that mean? An example of a query that is not covered by your current refactoring and still needs this check?
my_error(ER_WRONG_USAGE, MYF(0), "PROCEDURE", thd->lex->derived_tables & DERIVED_VIEW ? "view" : "subquery"); diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 5e1f5dc..445f986 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -5695,7 +5695,10 @@ create_select: { Select->parsing_place= NO_MATTER; } - table_expression + opt_table_expression + opt_order_clause + opt_limit_clause + opt_select_lock_type {
Looks like you might want to create a rule for that: opt_xxx: opt_table_expression opt_order_clause opt_limit_clause opt_select_lock_type; because it seems to be used many times in different parts of the grammar Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On 05/05/2016 11:27 PM, Sergei Golubchik wrote:
Hi, Alexander!
Sure, that's ok. A couple of comments below:
On May 05, Alexander Barkov wrote:
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index a6bbfc8..0d8bfb3 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -988,6 +988,7 @@ JOIN::prepare(TABLE_LIST *tables_init, } if (thd->lex->derived_tables) { + // SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
What does that mean? An example of a query that is not covered by your current refactoring and still needs this check?
Yes. I added this comment: if (thd->lex->derived_tables) { /* Queries with derived tables and PROCEDURE are not allowed. Many of such queries are disallowed grammatically, but there are still some complex cases: SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE() */ my_error(ER_WRONG_USAGE, MYF(0), "PROCEDURE", thd->lex->derived_tables & DERIVED_VIEW ? "view" : "subquery"); goto err; } By the way, fixing this particular restriction grammatically might need huge refactoring in table_ref and friends. We'll see later, if it's really feasible.
my_error(ER_WRONG_USAGE, MYF(0), "PROCEDURE", thd->lex->derived_tables & DERIVED_VIEW ? "view" : "subquery"); diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 5e1f5dc..445f986 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -5695,7 +5695,10 @@ create_select: { Select->parsing_place= NO_MATTER; } - table_expression + opt_table_expression + opt_order_clause + opt_limit_clause + opt_select_lock_type {
Looks like you might want to create a rule for that:
opt_xxx: opt_table_expression opt_order_clause opt_limit_clause opt_select_lock_type;
because it seems to be used many times in different parts of the grammar
I tried to do it but failed with new shift/reduce conflicts, which I was not able to find quickly a reason for. Now I think I'll just clean-up the other things first and then try to group these things again later. I added a remainder: /* TODO: The following sequence repeats a few times: opt_table_expression opt_order_clause opt_limit_clause opt_select_lock_type Perhaps they can be grouped into a dedicated rule. */ opt_table_expression opt_order_clause opt_limit_clause opt_select_lock_type Btw, in the MDEV-8909 branch you also had this block four times. Perhaps for some similar reason :)
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! On May 06, Alexander Barkov wrote:
On May 05, Alexander Barkov wrote:
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index a6bbfc8..0d8bfb3 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -988,6 +988,7 @@ JOIN::prepare(TABLE_LIST *tables_init, } if (thd->lex->derived_tables) { + // SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
What does that mean? An example of a query that is not covered by your current refactoring and still needs this check?
Yes. I added this comment: ... By the way, fixing this particular restriction grammatically might need huge refactoring in table_ref and friends. We'll see later, if it's really feasible.
Hmm, if I'm not mistaken, this if() was removed in branch. Does it mean I have done this huge refactoring, or I've removed it incorrectly and introduced a bug? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On 05/08/2016 12:39 PM, Sergei Golubchik wrote:
Hi, Alexander!
On May 06, Alexander Barkov wrote:
On May 05, Alexander Barkov wrote:
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index a6bbfc8..0d8bfb3 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -988,6 +988,7 @@ JOIN::prepare(TABLE_LIST *tables_init, } if (thd->lex->derived_tables) { + // SELECT 1 FROM (SELECT 1) a PROCEDURE ANALYSE()
What does that mean? An example of a query that is not covered by your current refactoring and still needs this check?
Yes. I added this comment: ... By the way, fixing this particular restriction grammatically might need huge refactoring in table_ref and friends. We'll see later, if it's really feasible.
Hmm, if I'm not mistaken, this if() was removed in branch. Does it mean I have done this huge refactoring, or I've removed it incorrectly and introduced a bug?
It was not removed. It's still there, to distinguish invalid: SELECT 1 FROM ((((SELECT 1)))) AS t1 PROCEDURE ANALYSE(); from valid: SELECT 1 FROM ((((SELECT 1)))) AS t1; SELECT 1 FROM (((t1))) PROCEDURE ANALYSE(); with any nested parentheses level.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik