[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>

[Roman Khabibov <roman.habibov@tarantool.org>]

> On Aug 16, 2019, at 22:09, Roman Khabibov <roman.habibov@tarantool.org> wrote:
> 
> 
> 
>> On Aug 14, 2019, at 01:10, n.pettik <korablev@tarantool.org> wrote:
>> 
>> 
>> 
>>> On 13 Aug 2019, at 15:42, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>>>> On Aug 9, 2019, at 18:15, n.pettik <korablev@tarantool.org> wrote:
>>>>> On 2 Aug 2019, at 15:52, Roman Khabibov <roman.habibov@tarantool.org> wrote:
>>>>> 
>>>>> Check that <CREATE VIEW> hasn't <WITH> after <AS>: "CREATE VIEW v
>>>>> AS WITH ... SELECT ...". Throw error, if it has.
>>>> 
>>>> What is the reason for that? I run example from ticket:
>>>> 
>>>> tarantool> \set language sql
>>>> tarantool> \set delimiter ;
>>>> 
>>>> tarantool> CREATE TABLE ts (s1 INT PRIMARY KEY);
>>>> tarantool> INSERT INTO ts VALUES (1);
>>>> tarantool> WITH RECURSIVE w AS (
>>>>>         SELECT s1 FROM ts
>>>>>         UNION ALL
>>>>>         SELECT s1+1 FROM w WHERE s1 < 4)
>>>>>       SELECT * FROM w;
>>>> tarantool> CREATE VIEW v AS WITH RECURSIVE w AS (
>>>>>         SELECT s1 FROM ts
>>>>>         UNION ALL
>>>>>         SELECT s1+1 FROM w WHERE s1 < 4)
>>>>>       SELECT * FROM w;
>>>> - null
>>>> - Space 'W' does not exist
>>>> ...
>>>> 
>>>> So, it fails at the stage of view creation. Please, ask Peter
>>>> to provide valid example. Otherwise there’s no problem and
>>>> issue can be closed.
>>>> 
>>> https://github.com/tarantool/tarantool/issues/4149#issuecomment-520390204
>>> 
>>>> This looks like a crutch. Please, either find out if it is
>>>> possible to fix this at parser level (changing grammar),
>>>> or simply close issue.
>>> Yes, now it’s one-line fix, but error isn't so detailed.
>>> But now it's a syntax error, as was required in the ticket.
>> 
>> Still, one can a bit modify query and get old error:
>> 
>> tarantool> CREATE VIEW v AS SELECT * FROM ( WITH RECURSIVE w AS ( SELECT s1 FROM ts UNION ALL SELECT s1+1 FROM w WHERE s1 < 4) SELECT * fROM w);
>> ---
>> - error: Space 'W' does not exist
>> …
> +test:do_catchsql_test(
> +    "view-24.4",
> +    [[
> +        CREATE VIEW v AS SELECT * FROM (
> +            WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +              SELECT * FROM w);
> +    ]], {
> +        -- <view-24.4>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.4>
> +    })
> 
>> What is more, if user created view with WITH clause
>> (see example at the end of letter) and updated tnt to
>> your branch, user would be forced to use force_recovery option:
>> 
>> 2019-08-14 00:30:42.835 [27409] main/102/interactive box.cc:329 E> error applying row: {type: 'INSERT', replica_id: 1, lsn: 19, space_id: 280, index_id: 0, tuple: [514, 1, "V", "memtx", 1, {"sql": "CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts", "view": true}, [{"name": "S1", "type": "integer", "is_nullable": true, "nullable_action": "none"}]]}
>> 2019-08-14 00:30:42.835 [27409] main/102/interactive tokenize.c:571 E> ER_SQL_EXECUTE: Failed to execute SQL statement: CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts
>> 2019-08-14 00:30:42.835 [27409] main/102/interactive F> can't initialize storage: Failed to execute SQL statement: CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts;
>> 
>> This is quite unpleasant.
>> 
>>> commit e311d35bd64422bf6468b28a28057d9045817eca
>>> Author: Roman Khabibov <roman.habibov@tarantool.org>
>>> Date:   Mon Jul 29 18:00:34 2019 +0300
>>> 
>>>  sql: disallow <WITH> in <CREATE VIEW> select
>> 
>> -> sql: disallow WITH clause in CREATE VIEW statement
>> 
>>>  We don't support queries with the syntax:
>>>  "CREATE VIEW v AS WITH ... SELECT …"
>> 
>> Strictly speaking, we do support some of such queries.
>> See example at the end of letter.
>> 
>>>  Throw the syntax error in this case.
>> 
>> Instead of banning CTEs in view’s body, we can fix their creation.
>> The problem is that during view creation all referenced spaces
>> are fetched by name from SELECT and their reference counters
>> are incremented to avoid dangling references. It occurs in
>> update_view_references(). Obviously, CTE tables are not held
>> in space cache, ergo error “space doesn’t exist” is raised.
>> So, what we can do is add graceful check to update_view_references().
> If I understand you correctly, we should check <WITH> presence in any select,
> which can meet after <CREATE VIEW>. update_view_references() is called in
> on_drop_view_rollback(), that don't relate to our case. So, I decided to
> check it before update_view_references(), as you can see in diff.
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 92f1d5b22..793df538f 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1908,6 +1908,9 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> 			auto select_guard = make_scoped_guard([=] {
> 				sql_select_delete(sql_get(), select);
> 			});
> +			if (sql_check_compound_with(select) == true)
> +				tnt_raise(ClientError, ER_CREATE_VIEW_WITH,
> +					  def->name);
> 			const char *disappeared_space;
> 			if (update_view_references(select, 1, false,
> 						   &disappeared_space) != 0) {
> 
>>>  Closes #4149
>>> 
>>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>>> index 4c9d95f9c..d9ce239ca 100644
>>> --- a/src/box/sql/parse.y
>>> +++ b/src/box/sql/parse.y
>>> @@ -399,7 +399,7 @@ ifexists(A) ::= .            {A = 0;}
>>> ///////////////////// The CREATE VIEW statement /////////////////////////////
>>> //
>>> cmd ::= CREATE(X) VIEW ifnotexists(E) nm(Y) eidlist_opt(C)
>>> -          AS select(S). {
>>> +          AS selectnowith(S). {
>>> if (!pParse->parse_only) {
>>>   create_view_def_init(&pParse->create_view_def, &Y, &X, C, S, E);
>>>   pParse->initiateTTrans = true;
>>> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
>>> index 101f4c3e7..cdba27e04 100755
>>> --- a/test/sql-tap/view.test.lua
>>> +++ b/test/sql-tap/view.test.lua
>>> @@ -1,6 +1,6 @@
>>> #!/usr/bin/env tarantool
>>> test = require("sqltester")
>>> -test:plan(73)
>>> +test:plan(76)
>>> 
>>> --!./tcltestrunner.lua
>>> -- 2002 February 26
>>> @@ -1233,4 +1233,39 @@ test:do_catchsql_test(
>>>       -- </view-23.8>
>>>   })
>>> 
>>> +-- gh-4149: Check error message for
>>> +-- "CREATE VIEW v AS WITH ... SELECT ...".
>>> +test:do_execsql_test(
>>> +    "view-24.1",
>>> +    [[
>>> +        CREATE TABLE ts (s1 INT PRIMARY KEY);
>>> +        INSERT INTO ts VALUES (1);
>>> +    ]], {
>>> +        -- <view-24.1>
>>> +        -- </view-24.1>
>>> +    })
>>> +
>>> +test:do_catchsql_test(
>>> +    "view-24.2",
>>> +    [[
>>> +        CREATE VIEW v AS WITH RECURSIVE w AS (
>>> +            SELECT s1 FROM ts
>>> +            UNION ALL
>>> +            SELECT s1+1 FROM w WHERE s1 < 4)
>>> +          SELECT * FROM w;
>>> +    ]], {
>>> +        -- <view-24.2>
>>> +        1,"Keyword 'WITH' is reserved. Please use double quotes if 'WITH' is an identifier."
>>> +        -- </view-24.2>
>>> +    })
>> 
>> Look at WITH rule in parse.y:
>> 
>> with(A) ::= . {A = 0;}
>> with(A) ::= WITH wqlist(W).              { A = W; }
>> with(A) ::= WITH RECURSIVE wqlist(W).    { A = W; }
>> 
>> So, basically you should also check cases like:
>> 
>> CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts;
>> 
>> (It works on master branch, but this query is absolutely meaningless)
> +test:do_catchsql_test(
> +    "view-24.2",
> +    [[
> +        CREATE VIEW v AS WITH w(id) AS (
> +            SELECT 1)
> +          SELECT * FROM ts;
> +    ]], {
> +        -- <view-24.2>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.2>
> +    })
> +
> 
> commit ad813d6d0e60afd3030528eaf8f34a0b54ce7fb2
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Mon Jul 29 18:00:34 2019 +0300
> 
>    sql: disallow <WITH> in <CREATE VIEW> selects
> 
>    Disallow <WITH> clause using in any select within <CREATE VIEW>
>    statement. Throw the error in this case.
> 
>    Closes #4149
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 92f1d5b22..793df538f 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1908,6 +1908,9 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> 			auto select_guard = make_scoped_guard([=] {
> 				sql_select_delete(sql_get(), select);
> 			});
> +			if (sql_check_compound_with(select) == true)
> +				tnt_raise(ClientError, ER_CREATE_VIEW_WITH,
> +					  def->name);
> 			const char *disappeared_space;
> 			if (update_view_references(select, 1, false,
> 						   &disappeared_space) != 0) {
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index 817275b97..5acdccc70 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -253,6 +253,7 @@ struct errcode_record {
> 	/*198 */_(ER_FUNC_INDEX_FUNC,		"Failed to build a key for functional index '%s' of space '%s': %s") \
> 	/*199 */_(ER_FUNC_INDEX_FORMAT,		"Key format doesn't match one defined in functional index '%s' of space '%s': %s") \
> 	/*200 */_(ER_FUNC_INDEX_PARTS,		"Wrong functional index definition: %s") \
> +	/*201 */_(ER_CREATE_VIEW_WITH,		"Failed to create view '%s' with <WITH>") \
> 
> /*
>  * !IMPORTANT! Please follow instructions at start of the file
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 9ccecf28c..c1263d9c5 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -323,6 +323,16 @@ sql_select_delete(struct sql *db, struct Select *select);
> struct SrcList *
> sql_select_expand_from_tables(struct Select *select);
> 
> +/**
> + * Check if select has any compound selects with <WITH> clause.
> + *
> + * @param select Select to be checked.
> + * @retval true Has <WITH>s.
> + * @retval false Hasn't <WITH>s.
> +*/
> +bool
> +sql_check_compound_with(struct Select *select);
> +
> /**
>  * Temporary getter in order to avoid including sqlInt.h
>  * in alter.cc.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index c312f61f1..9b46f07cf 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -332,6 +332,21 @@ sql_select_expand_from_tables(struct Select *select)
> 	return walker.u.pSrcList;
> }
> 
> +bool
> +sql_check_compound_with(struct Select *select)
> +{
> +	if (select->pWith != NULL)
> +		return true;
> +	struct SrcList *list = select->pSrc;
> +	int item_count = sql_src_list_entry_count(list);
> +	for (int i = 0; i < item_count; ++i) {
> +		if (list->a[i].pSelect != NULL)
> +			if (sql_check_compound_with(list->a[i].pSelect) == true)
> +				return true;
> +	}
> +	return false;
> +}
> +
> /*
>  * Given 1 to 3 identifiers preceding the JOIN keyword, determine the
>  * type of join.  Return an integer constant that expresses that type
> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
> index 101f4c3e7..985f9131f 100755
> --- a/test/sql-tap/view.test.lua
> +++ b/test/sql-tap/view.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(73)
> +test:plan(80)
> 
> --!./tcltestrunner.lua
> -- 2002 February 26
> @@ -1233,4 +1233,99 @@ test:do_catchsql_test(
>         -- </view-23.8>
>     })
> 
> +-- gh-4149: Check error message for view creation with (nested)
> +-- select with <WITH> clause.
> +test:do_execsql_test(
> +    "view-24.1",
> +    [[
> +        CREATE TABLE ts (s1 INT PRIMARY KEY);
> +        INSERT INTO ts VALUES (1);
> +    ]], {
> +        -- <view-24.1>
> +        -- </view-24.1>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.2",
> +    [[
> +        CREATE VIEW v AS WITH w(id) AS (
> +            SELECT 1)
> +          SELECT * FROM ts;
> +    ]], {
> +        -- <view-24.2>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.2>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.3",
> +    [[
> +        CREATE VIEW v AS WITH RECURSIVE w AS (
> +            SELECT s1 FROM ts
> +            UNION ALL
> +            SELECT s1+1 FROM w WHERE s1 < 4)
> +          SELECT * FROM w;
> +    ]], {
> +        -- <view-24.3>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.3>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.4",
> +    [[
> +        CREATE VIEW v AS SELECT * FROM (
> +            WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +              SELECT * FROM w);
> +    ]], {
> +        -- <view-24.4>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.4>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.5",
> +    [[
> +        CREATE VIEW v AS SELECT * FROM (
> +            SELECT * FROM (
> +                WITH RECURSIVE w AS (
> +                    SELECT s1 FROM ts
> +                    UNION ALL
> +                    SELECT s1+1 FROM w WHERE s1 < 4)
> +                  SELECT * FROM w));
> +    ]], {
> +        -- <view-24.5>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.5>
> +    })
> +
> +test:do_catchsql_test(
> +    "view-24.6",
> +    [[
> +        CREATE VIEW v AS SELECT * FROM
> +            (SELECT 1),
> +            (SELECT 2) JOIN
> +            (WITH RECURSIVE w AS (
> +                SELECT s1 FROM ts
> +                UNION ALL
> +                SELECT s1+1 FROM w WHERE s1 < 4)
> +                SELECT * FROM w);
> +    ]], {
> +        -- <view-24.6>
> +        1,"Failed to create view 'V' with <WITH>"
> +        -- </view-24.6>
> +    })
> +
> +test:do_execsql_test(
> +    "view-24.7",
> +    [[
> +        DROP TABLE ts
> +    ]], {
> +        -- <view-24.7>
> +        -- </view-24.7>
> +    })
> +
> test:finish_test()

Sorry, forgot to correct one .result file.

commit a8b2998140fad34c8eab0b6df737b63710038e2b
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Mon Jul 29 18:00:34 2019 +0300

    sql: disallow <WITH> in <CREATE VIEW> selects
    
    Disallow <WITH> clause using in any select within <CREATE VIEW>
    statement. Throw the error in this case.
    
    Closes #4149

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 92f1d5b22..793df538f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1908,6 +1908,9 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 			auto select_guard = make_scoped_guard([=] {
 				sql_select_delete(sql_get(), select);
 			});
+			if (sql_check_compound_with(select) == true)
+				tnt_raise(ClientError, ER_CREATE_VIEW_WITH,
+					  def->name);
 			const char *disappeared_space;
 			if (update_view_references(select, 1, false,
 						   &disappeared_space) != 0) {
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 817275b97..5acdccc70 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -253,6 +253,7 @@ struct errcode_record {
 	/*198 */_(ER_FUNC_INDEX_FUNC,		"Failed to build a key for functional index '%s' of space '%s': %s") \
 	/*199 */_(ER_FUNC_INDEX_FORMAT,		"Key format doesn't match one defined in functional index '%s' of space '%s': %s") \
 	/*200 */_(ER_FUNC_INDEX_PARTS,		"Wrong functional index definition: %s") \
+	/*201 */_(ER_CREATE_VIEW_WITH,		"Failed to create view '%s' with <WITH>") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql.h b/src/box/sql.h
index 9ccecf28c..c1263d9c5 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -323,6 +323,16 @@ sql_select_delete(struct sql *db, struct Select *select);
 struct SrcList *
 sql_select_expand_from_tables(struct Select *select);
 
+/**
+ * Check if select has any compound selects with <WITH> clause.
+ *
+ * @param select Select to be checked.
+ * @retval true Has <WITH>s.
+ * @retval false Hasn't <WITH>s.
+*/
+bool
+sql_check_compound_with(struct Select *select);
+
 /**
  * Temporary getter in order to avoid including sqlInt.h
  * in alter.cc.
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index c312f61f1..9b46f07cf 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -332,6 +332,21 @@ sql_select_expand_from_tables(struct Select *select)
 	return walker.u.pSrcList;
 }
 
+bool
+sql_check_compound_with(struct Select *select)
+{
+	if (select->pWith != NULL)
+		return true;
+	struct SrcList *list = select->pSrc;
+	int item_count = sql_src_list_entry_count(list);
+	for (int i = 0; i < item_count; ++i) {
+		if (list->a[i].pSelect != NULL)
+			if (sql_check_compound_with(list->a[i].pSelect) == true)
+				return true;
+	}
+	return false;
+}
+
 /*
  * Given 1 to 3 identifiers preceding the JOIN keyword, determine the
  * type of join.  Return an integer constant that expresses that type
diff --git a/test/box/misc.result b/test/box/misc.result
index 7a15dabf0..975705739 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -529,6 +529,7 @@ t;
   198: box.error.FUNC_INDEX_FUNC
   199: box.error.FUNC_INDEX_FORMAT
   200: box.error.FUNC_INDEX_PARTS
+  201: box.error.CREATE_VIEW_WITH
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
index 101f4c3e7..985f9131f 100755
--- a/test/sql-tap/view.test.lua
+++ b/test/sql-tap/view.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(73)
+test:plan(80)
 
 --!./tcltestrunner.lua
 -- 2002 February 26
@@ -1233,4 +1233,99 @@ test:do_catchsql_test(
         -- </view-23.8>
     })
 
+-- gh-4149: Check error message for view creation with (nested)
+-- select with <WITH> clause.
+test:do_execsql_test(
+    "view-24.1",
+    [[
+        CREATE TABLE ts (s1 INT PRIMARY KEY);
+        INSERT INTO ts VALUES (1);
+    ]], {
+        -- <view-24.1>
+        -- </view-24.1>
+    })
+
+test:do_catchsql_test(
+    "view-24.2",
+    [[
+        CREATE VIEW v AS WITH w(id) AS (
+            SELECT 1)
+          SELECT * FROM ts;
+    ]], {
+        -- <view-24.2>
+        1,"Failed to create view 'V' with <WITH>"
+        -- </view-24.2>
+    })
+
+test:do_catchsql_test(
+    "view-24.3",
+    [[
+        CREATE VIEW v AS WITH RECURSIVE w AS (
+            SELECT s1 FROM ts
+            UNION ALL
+            SELECT s1+1 FROM w WHERE s1 < 4)
+          SELECT * FROM w;
+    ]], {
+        -- <view-24.3>
+        1,"Failed to create view 'V' with <WITH>"
+        -- </view-24.3>
+    })
+
+test:do_catchsql_test(
+    "view-24.4",
+    [[
+        CREATE VIEW v AS SELECT * FROM (
+            WITH RECURSIVE w AS (
+                SELECT s1 FROM ts
+                UNION ALL
+                SELECT s1+1 FROM w WHERE s1 < 4)
+              SELECT * FROM w);
+    ]], {
+        -- <view-24.4>
+        1,"Failed to create view 'V' with <WITH>"
+        -- </view-24.4>
+    })
+
+test:do_catchsql_test(
+    "view-24.5",
+    [[
+        CREATE VIEW v AS SELECT * FROM (
+            SELECT * FROM (
+                WITH RECURSIVE w AS (
+                    SELECT s1 FROM ts
+                    UNION ALL
+                    SELECT s1+1 FROM w WHERE s1 < 4)
+                  SELECT * FROM w));
+    ]], {
+        -- <view-24.5>
+        1,"Failed to create view 'V' with <WITH>"
+        -- </view-24.5>
+    })
+
+test:do_catchsql_test(
+    "view-24.6",
+    [[
+        CREATE VIEW v AS SELECT * FROM
+            (SELECT 1),
+            (SELECT 2) JOIN
+            (WITH RECURSIVE w AS (
+                SELECT s1 FROM ts
+                UNION ALL
+                SELECT s1+1 FROM w WHERE s1 < 4)
+                SELECT * FROM w);
+    ]], {
+        -- <view-24.6>
+        1,"Failed to create view 'V' with <WITH>"
+        -- </view-24.6>
+    })
+
+test:do_execsql_test(
+    "view-24.7",
+    [[
+        DROP TABLE ts
+    ]], {
+        -- <view-24.7>
+        -- </view-24.7>
+    })
+
 test:finish_test()