Tarantool development patches archive
 help / color / mirror / Atom feed
From: Roman Khabibov <roman.habibov@tarantool.org>
To: tarantool-patches@freelists.org
Cc: "n. pettik" <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>
Date: Fri, 16 Aug 2019 22:09:47 +0300	[thread overview]
Message-ID: <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> (raw)
In-Reply-To: <8BD992A5-0A74-4E60-A239-5FDE85783467@tarantool.org>



> 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()

  reply	other threads:[~2019-08-16 19:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-02 12:52 [tarantool-patches] " Roman Khabibov
2019-08-09 15:15 ` [tarantool-patches] " n.pettik
2019-08-13 12:42   ` Roman Khabibov
2019-08-13 22:10     ` n.pettik
2019-08-16 19:09       ` Roman Khabibov [this message]
2019-08-19 15:39         ` Roman Khabibov
2019-08-20 19:41         ` n.pettik
2019-08-28 12:17           ` Roman Khabibov
2019-08-29 17:59             ` Nikita Pettik
2019-09-04 14:14               ` Roman Khabibov
2019-09-11 13:32                 ` Nikita Pettik
2019-09-13 14:57                   ` Roman Khabibov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org \
    --to=roman.habibov@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox