maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,

On 05/16/2016 10:35 AM, Sergei Golubchik wrote:
> Hi, Oleksandr!
>
> On May 16, Oleksandr Byelkin wrote:
> > On 16.05.2016 06:00, Alexander Barkov wrote:
> > > Hi Sanja, Sergei,
> > >
> > > Sanja, you asked me to review a patch for MDEV-10035:
> > >
> > > revision-id: 4ffe2295e78538dde93df078421726f0c5a7d2a2
> > > (mariadb-10.2.0-29-g4ffe229)
> > > parent(s): b79944950e5e5db40cf7ad49061edf5f105512c4
> > > committer: Oleksandr Byelkin
> > > timestamp: 2016-05-15 15:25:33 +0200
> > > message:
> > >
> > > I earlier also proposed the same idea to disallow FOR UPDATE,
> > > and even created a patch disallowing this in the parser
> > > syntactically (see attached).
> > >
> > > But Sergei worried that we should not do it this way and proposed some
> > > other solutions. Please see Sergei's comments in:
> > >
> > > MDEV-10063 VIEWs and subqueries with FOR UPDATE
> > >
> > > Sergei, are you still in doubts?
>
> Yes :) Out of all approaches, I think, I'd prefer to issue a warning
> like "Clause FOR UPDATE" won't work unless you specify TEMPTABLE
> algorithm. Making it an error in the strict mode, as usual.
>
> > Just now this parameter for VIEWs effectively (and silently) ignored. So
> > error IMHO is better.
>
> It will break existing application where users have FOR UPDATE in the
> view definition. But, probably, there won't be many?

I think there won't be many.
For me it looks like when we added FOR UPDATE,
we just forgot to disallow this in VIEW.

A more clear way is to use FOR UPDATE when invoking views than when 
creating views:
SELECT * FROM view1 FOR UPDATE;



And by the way, it does not work even with TEMPTABLE. Here is an example:


# Connection#1:

DROP TABLE IF EXISTS t1,t2;
DROP VIEW IF EXISTS v1;
CREATE TABLE t1 (a INT);
CREATE TABLE t2 (a INT);

INSERT INTO t1 VALUES (10);
INSERT INTO t2 VALUES (10);

CREATE ALGORITHM=TEMPTABLE VIEW v1 AS
SELECT * FROM t1 UNION ALL (SELECT * FROM t2 FOR UPDATE);

START TRANSACTION;
SELECT * FROM v1 FOR UPDATE;


# Connection#2  (does not lock, returns results immediately):

START TRANSACTION;
SELECT * FROM v1 FOR UPDATE;
COMMIT;

+------+
| a    |
+------+
|   10 |
|   10 |
+------+


# Connection#3 (does not lock either, returns results immediately):

START TRANSACTION;
SELECT * FROM t2 FOR UPDATE;
COMMIT;

+------+
| a    |
+------+
|   10 |
+------+


> But also this patch goes directly against the work that Bar is doing
> now. This patch adds new manual syntax checks into the code, while Bar
> is trying to have the grammar to allow only syntaxically corect queries.

I even earlier made a patch disallowing this syntactically.
It was in the previous letter. I'm attaching it again.

I made it before you commented in MDEV-10063 :)
But then would not sent for review, because we had a discussing
that it might be useful with TEMPTABLE...


But now when we know that TEMPTABLE does not really work...
Ok to push the attached patch?

Thanks!

> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
diff --git a/mysql-test/r/sp-error.result b/mysql-test/r/sp-error.result
index c56597e..447afe3 100644
--- a/mysql-test/r/sp-error.result
+++ b/mysql-test/r/sp-error.result
@@ -1227,14 +1227,14 @@ DROP PROCEDURE IF EXISTS bug14702;
 DROP TABLE IF EXISTS t1;
 CREATE TABLE t1 (i INT);
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO @a;
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'INTO @a' at line 1
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO DUMPFILE "file";
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'INTO DUMPFILE "file"' at line 1
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO OUTFILE "file";
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'INTO OUTFILE "file"' at line 1
 CREATE PROCEDURE bug20953()
 CREATE VIEW v AS SELECT i FROM t1 PROCEDURE ANALYSE();
-ERROR HY000: View's SELECT contains a 'PROCEDURE' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'PROCEDURE ANALYSE()' at line 2
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 FROM (SELECT 1) AS d1;
 ERROR HY000: View's SELECT contains a subquery in the FROM clause
 CREATE PROCEDURE bug20953(i INT) CREATE VIEW v AS SELECT i;
diff --git a/mysql-test/r/view.result b/mysql-test/r/view.result
index 3fccd6e..47a29a4 100644
--- a/mysql-test/r/view.result
+++ b/mysql-test/r/view.result
@@ -923,12 +923,12 @@ select * from v4;
 ERROR 21000: Subquery returns more than 1 row
 drop view v4, v3, v2, v1;
 create view v1 as select 5 into @w;
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'into @w' at line 1
 create view v1 as select 5 into outfile 'ttt';
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'into outfile 'ttt'' at line 1
 create table t1 (a int);
 create view v1 as select a from t1 procedure analyse();
-ERROR HY000: View's SELECT contains a 'PROCEDURE' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'procedure analyse()' at line 1
 create view v1 as select 1 from (select 1) as d1;
 ERROR HY000: View's SELECT contains a subquery in the FROM clause
 drop table t1;
@@ -5953,5 +5953,13 @@ t3	CREATE TABLE `t3` (
 DROP VIEW v1;
 DROP TABLE t1,t2,t3;
 #
+# MDEV-10035 DBUG_ASSERT on CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE
+#
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (10);
+CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'FOR UPDATE' at line 1
+DROP TABLE t1;
+#
 # End of 10.2 tests
 #
diff --git a/mysql-test/suite/funcs_1/r/innodb_views.result b/mysql-test/suite/funcs_1/r/innodb_views.result
index 59eec5b..45de953 100644
--- a/mysql-test/suite/funcs_1/r/innodb_views.result
+++ b/mysql-test/suite/funcs_1/r/innodb_views.result
@@ -3497,7 +3497,7 @@ DROP VIEW  IF EXISTS v2 ;
 CREATE TABLE t1 (f1 BIGINT) ;
 SET @x=0;
 CREATE or REPLACE VIEW v1 AS Select 1 INTO @x;
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'INTO @x' at line 1
 Select @x;
 @x
 0
diff --git a/mysql-test/suite/funcs_1/r/memory_views.result b/mysql-test/suite/funcs_1/r/memory_views.result
index 995787a..ab4e2a9 100644
--- a/mysql-test/suite/funcs_1/r/memory_views.result
+++ b/mysql-test/suite/funcs_1/r/memory_views.result
@@ -3498,7 +3498,7 @@ DROP VIEW  IF EXISTS v2 ;
 CREATE TABLE t1 (f1 BIGINT) ;
 SET @x=0;
 CREATE or REPLACE VIEW v1 AS Select 1 INTO @x;
-ERROR HY000: View's SELECT contains a 'INTO' clause
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'INTO @x' at line 1
 Select @x;
 @x
 0
diff --git a/mysql-test/suite/funcs_1/views/views_master.inc b/mysql-test/suite/funcs_1/views/views_master.inc
index bb9bbdb..f55788d 100644
--- a/mysql-test/suite/funcs_1/views/views_master.inc
+++ b/mysql-test/suite/funcs_1/views/views_master.inc
@@ -266,7 +266,7 @@ CREATE TABLE t1 (f1 BIGINT) ;
 
 # SELECT INTO is illegal
 SET @x=0;
---error ER_VIEW_SELECT_CLAUSE
+--error ER_PARSE_ERROR
 CREATE or REPLACE VIEW v1 AS Select 1 INTO @x;
 Select @x;
 
diff --git a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test
index d403b19..b1d4f67 100644
--- a/mysql-test/t/sp-error.test
+++ b/mysql-test/t/sp-error.test
@@ -1785,13 +1785,13 @@ CREATE TABLE t1 (i INT);
 
 # We do not have to drop this procedure and view because they won't be
 # created.
---error ER_VIEW_SELECT_CLAUSE
+--error ER_PARSE_ERROR
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO @a;
---error ER_VIEW_SELECT_CLAUSE
+--error ER_PARSE_ERROR
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO DUMPFILE "file";
---error ER_VIEW_SELECT_CLAUSE
+--error ER_PARSE_ERROR
 CREATE PROCEDURE bug20953() CREATE VIEW v AS SELECT 1 INTO OUTFILE "file";
---error ER_VIEW_SELECT_CLAUSE
+--error ER_PARSE_ERROR
 CREATE PROCEDURE bug20953()
   CREATE VIEW v AS SELECT i FROM t1 PROCEDURE ANALYSE();
 --error ER_VIEW_SELECT_DERIVED
diff --git a/mysql-test/t/view.test b/mysql-test/t/view.test
index d11b7f0..83fad50 100644
--- a/mysql-test/t/view.test
+++ b/mysql-test/t/view.test
@@ -840,12 +840,12 @@ drop view v4, v3, v2, v1;
 #
 # VIEW over SELECT with prohibited clauses
 #
--- error ER_VIEW_SELECT_CLAUSE
+-- error ER_PARSE_ERROR
 create view v1 as select 5 into @w;
--- error ER_VIEW_SELECT_CLAUSE
+-- error ER_PARSE_ERROR
 create view v1 as select 5 into outfile 'ttt';
 create table t1 (a int);
--- error ER_VIEW_SELECT_CLAUSE
+-- error ER_PARSE_ERROR
 create view v1 as select a from t1 procedure analyse();
 -- error ER_VIEW_SELECT_DERIVED
 create view v1 as select 1 from (select 1) as d1;
@@ -5784,6 +5784,14 @@ SHOW CREATE TABLE t3;
 DROP VIEW v1;
 DROP TABLE t1,t2,t3;
 
+--echo #
+--echo # MDEV-10035 DBUG_ASSERT on CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE
+--echo #
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (10);
+--error ER_PARSE_ERROR
+CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE;
+DROP TABLE t1;
 
 --echo #
 --echo # End of 10.2 tests
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index 9468254..bb9ec0c 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -1968,7 +1968,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, ulong *yystacksize);
         table_to_table_list table_to_table opt_table_list opt_as
         handler_rkey_function handler_read_or_scan
         single_multi table_wild_list table_wild_one opt_wild
-        union_clause union_list
+        union_clause union_list union_list_view
         subselect_start opt_and charset
         subselect_end select_var_list select_var_list_init help 
         opt_extended_describe shutdown
@@ -8540,6 +8540,27 @@ select_paren:
         | '(' select_paren ')'
         ;
 
+/*
+  Similar to select_paren, but does not allow
+  INTO, PROCEDURE, opt_select_lock_type (e.g. FOR UPDATE).
+*/
+select_paren_view:
+          {
+            Lex->current_select->set_braces(true);
+          }
+          select_expression_view
+          {
+            if (setup_select_in_parentheses(Lex))
+              MYSQL_YYABORT;
+          }
+        | '(' select_paren_view ')'
+        ;
+
+select_expression_view:
+          query_specification
+        | query_specification order_or_limit
+        ;
+
 /* The equivalent of select_paren for nested queries. */
 select_paren_derived:
           {
@@ -16306,6 +16327,13 @@ union_list:
           }
         ;
 
+union_list_view:
+          union_head_non_top view_select_aux
+          {
+            Lex->pop_context();
+          }
+        ;
+
 union_opt:
           opt_union_order_or_limit
         | union_list { $$= 1; }
@@ -16359,6 +16387,18 @@ union_option:
         | ALL       { $$=0; }
         ;
 
+/*
+  This corresponds to the SQL standard query expression:
+  <query specification> ::=
+     SELECT [ <set quantifier> ] <select list> <table expression>
+
+  The difference is that we allow more options instead of <set quantifier>,
+  and also allow <table expression> to be optional.
+*/
+query_specification:
+         SELECT_SYM select_options_and_item_list opt_table_expression
+        ;
+
 query_term:
           SELECT_SYM select_init2_derived
           opt_table_expression
@@ -16633,8 +16673,15 @@ view_select:
         ;
 
 view_select_aux:
-          SELECT_SYM select_options_and_item_list select_init3
-        | '(' select_paren ')' union_opt
+          query_specification { Lex->current_select->set_braces(false); }
+        | query_specification { Lex->current_select->set_braces(false); }
+          union_list_view
+        | query_specification order_or_limit
+          {
+            Lex->current_select->set_braces(false);
+          }
+        | '(' select_paren_view ')' opt_union_order_or_limit
+        | '(' select_paren_view ')' union_list_view
         ;
 
 view_check_option: