Tarantool development patches archive
 help / color / mirror / Atom feed
From: Roman Khabibov <roman.habibov@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>
Date: Wed, 4 Sep 2019 17:14:10 +0300	[thread overview]
Message-ID: <0FDC0655-79B5-479F-8FAA-5E9A2E93F1CF@tarantool.org> (raw)
In-Reply-To: <20190829175955.GA52650@tarantool.org>



> On Aug 29, 2019, at 20:59, Nikita Pettik <korablev@tarantool.org> wrote:
> 
> On Wed, Aug 28, 2019 at 03:17:41PM +0300, Roman Khabibov wrote:
>> 
>>> On Aug 20, 2019, at 22:41, n.pettik <korablev@tarantool.org> wrote:
>>> You slightly misunderstood me. I proposed to allow such queries
>>> gracefully handling WITH statement and avoiding references
>>> counter incrementation for CTEs.
>> 
>> commit 202677704c455208d6def08d373776cfae82fdbe
>> Author: Roman Khabibov <roman.habibov@tarantool.org>
>> Date:   Mon Jul 29 18:00:34 2019 +0300
>> 
>>    sql: allow to create view as <WITH> clause
>> 
>>    Allow views to use CTEs in <WITH> clauses, which can be in any
>>    (nested) select after <AS>.
> 
> Please, provide decent commit message which would include problem
> explanation, chosen solution, some of implementation details.
Done.

>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 92f1d5b22..d3d6770cf 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -1721,6 +1721,8 @@ update_view_references(struct Select *select, int update_value,
>> 		const char *space_name = sql_src_list_entry_name(list, i);
>> 		if (space_name == NULL)
>> 			continue;
>> +		if (is_cte(select, space_name) == true)
>> +			continue;
> 
> If function returns boolean value, you don't need to use comparison
> to check it:
> 
> 'if (returns_boolean()) ...' or 'if (! returns_boolean()) ...'
> 
> Also, put comment explaining why do you skip reference counter
> increment in case of SELECT containts CTE.
@@ -1759,6 +1759,13 @@ update_view_references(struct Select *select, int update_value,
 		const char *space_name = sql_src_list_entry_name(list, i);
 		if (space_name == NULL)
 			continue;
+		/*
+		 * CTE tables isn't held in space cache and we
+		 * don't need to increment reference counter for
+		 * CTEs.
+		 */
+		if (sql_select_has_cte_with_name(select, space_name))
+			continue;
 		struct space *space = space_by_name(space_name);
 		if (space == NULL) {
 			if (! suppress_error) {

>> diff --git a/src/box/sql.h b/src/box/sql.h
>> index 9ccecf28c..c180e40e1 100644
>> --- a/src/box/sql.h
>> +++ b/src/box/sql.h
>> @@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select);
>> struct SrcList *
>> sql_select_expand_from_tables(struct Select *select);
>> 
>> +/**
>> + * Check if @a name matches with at least one of CTE names typed
>> + * in <WITH> clauses within @a select.
>> + *
>> + * @param select Select to be checked.
> 
> -> Select which may contain CTE.
> 
>> + * @retval true Has CTE with @a name.
>> + * @retval false Hasn't CTE with @a name.
>> +*/
>> +bool
>> +is_cte(struct Select *select, const char *name);
>> +
>> /**
>>  * 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..0ee840b89 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -332,6 +332,25 @@ sql_select_expand_from_tables(struct Select *select)
>> 	return walker.u.pSrcList;
>> }
>> 
>> +bool
>> +is_cte(struct Select *select, const char *name)
> 
> Such a bad name. Please, get in touch with our naming policy
> and come up with better one.
diff --git a/src/box/sql.h b/src/box/sql.h
index 7644051b4..422c8dfc2 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select);
 struct SrcList *
 sql_select_expand_from_tables(struct Select *select);
 
+/**
+ * Check if @a name matches with at least one of CTE names typed
+ * in <WITH> clauses within @a select.
+ *
+ * @param select Select which may contain CTE.
+ * @retval true Has CTE with @a name.
+ * @retval false Hasn't CTE with @a name.
+*/
+bool
+sql_select_has_cte_with_name(struct Select *select, const char *name);
+

>> +{
>> +	assert(select != NULL && name != NULL);
>> +	struct With *with = select->pWith;
>> +	if (with != NULL) {
>> +		if (memcmp(name, with->a->zName, strlen(name)) == 0)
>> +			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 (is_cte(list->a[i].pSelect, name) == true)
>> +				return true;
>> +	}
> 
> It's wrong way of CTE traversal. See sqlTreeViewWith() for correct
> implementation. With your approach this example still fails:
> 
> create view v as WITH RECURSIVE
>  xaxis(x) AS (VALUES(-2.0) UNION ALL SELECT x+0.05 FROM xaxis WHERE x<1.2),
>  yaxis(y) AS (VALUES(-1.0) UNION ALL SELECT y+0.1 FROM yaxis WHERE y<1.0),
>  m(iter, cx, cy, x, y) AS (
>    SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis
>    UNION ALL
>    SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m 
>     WHERE (x*x + y*y) < 4.0 AND iter<28
>  ),
>  m2(iter, cx, cy) AS (
>    SELECT max(iter), cx, cy FROM m GROUP BY cx, cy
>  ),
>  a(t) AS (
>    SELECT group_concat( substr(' .+*#', 1+least(iter/7,4), 1), '') 
>    FROM m2 GROUP BY cy
>  )
> SELECT group_concat(trim(t),x'0a') FROM a;
+bool
+sql_select_has_cte_with_name(struct Select *select, const char *name)
+{
+	assert(select != NULL && name != NULL);
+	struct With *with = select->pWith;
+	if (with != NULL) {
+		for (int i = 0; i < with->nCte; i++) {
+			const struct Cte *cte = &with->a[i];
+			if (memcmp(name, cte->zName, strlen(name)) == 0)
+				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_select_has_cte_with_name(list->a[i].pSelect,
+							 name))
+				return true;
+	}
+	return false;
+}
+

>> diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
>> index 101f4c3e7..4b695fa6a 100755
>> --- a/test/sql-tap/view.test.lua
>> +++ b/test/sql-tap/view.test.lua
>> @@ -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.
> 
> -> gh-4149: make sure that VIEW can be created as CTE.
> 
>> +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_execsql_test(
>> +    "view-24.2",
>> +    [[
>> +        CREATE VIEW v AS WITH w(id) AS (
>> +            SELECT 1)
>> +          SELECT * FROM ts;
>> +    ]], {
> 
> Please test not only the fact that view can be created, but also that
> it is queryable (i.e. SELECT * FROM v is processed without accidents).
> 
>> +        -- <view-24.2>
>> +        -- </view-24.2>
>> +    })
>> 
> 
> All examples below are almost identical since nCte for them equals to 1.
> 
>> +test:do_execsql_test(
>> +    "view-24.3",
>> +    [[
>> +        DROP VIEW v;
>> +        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>
>> +        -- </view-24.3>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "view-24.4",
>> +    [[
>> +        DROP VIEW v;
>> +        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>
>> +        -- </view-24.4>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "view-24.5",
>> +    [[
>> +        DROP VIEW v;
>> +        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>
>> +        -- </view-24.5>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "view-24.6",
>> +    [[
>> +        DROP VIEW v;
>> +        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>
>> +        -- </view-24.6>
>> +    })
>> +
>> +test:do_execsql_test(
>> +    "view-24.7",
>> +    [[
>> +        DROP VIEW v;
>> +        DROP TABLE ts;
> 
> You don't have to provide clean-up in SQL-tap suite.
I have reduced the number of tests. Why? I make sure that
nested select are handled too in these tests.

+-- gh-4149: Make sure that VIEW can be created as CTE.
+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_execsql_test(
+    "view-24.2",
+    [[
+        CREATE VIEW v AS WITH w(id) AS (
+            SELECT 1)
+          SELECT * FROM ts;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.2>
+        1
+        -- </view-24.2>
+    })
+
+test:do_execsql_test(
+    "view-24.3",
+    [[
+        DROP VIEW v;
+        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;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.3>
+        1,2,3,4
+        -- </view-24.3>
+    })
+
+test:do_execsql_test(
+    "view-24.4",
+    [[
+        DROP VIEW v;
+        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);
+          SELECT * FROM v;
+    ]], {
+        -- <view-24.4>
+        1,2,1,1,2,2,1,2,3,1,2,4
+        -- </view-24.4>
+    })
+
+test:do_execsql_test(
+    "view-24.5",
+    [[
+        DROP VIEW v;
+        DROP TABLE ts;
+        CREATE VIEW v AS WITH RECURSIVE
+            xaxis(x) AS (
+                VALUES(-2.0)
+                UNION ALL
+                SELECT x+0.5 FROM xaxis WHERE x<1.2),
+            yaxis(y) AS (
+                VALUES(-1.0)
+                UNION ALL
+                SELECT y+0.5 FROM yaxis WHERE y<1.0),
+            m(iter, cx, cy, x, y) AS (
+                SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis
+                UNION ALL
+                SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m WHERE (x*x + y*y) < 4.0 AND iter<1
+            ),
+            m2(iter, cx, cy) AS (
+            SELECT max(iter), cx, cy FROM m GROUP BY cx, cy
+            ),
+            a(t) AS (
+                SELECT group_concat( substr('a', 1+least(iter/7,4), 1), '') FROM m2 GROUP BY cy
+            )
+          SELECT group_concat(trim(t),x'0a') FROM a;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.5>
+        "aaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa"
+        -- </view-24.5>
+    })
+

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

    sql: allow to create view as <WITH> clause
    
    Allow views to use CTEs in <WITH> clauses, which can be in any
    (nested) select after <AS>. Before this patch, during view
    creation all referenced spaces were fetched by name from SELECT
    and their reference counters were incremented to avoid dangling
    references. It occurred in update_view_references(). Obviously, CTE
    tables weren't held in space cache, ergo error "space doesn’t
    exist" was raised. Add check if a space from FROM is CTE. If it is,
    don't increment its reference counter and don't raise the error.
    
    Closes #4149

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 4f2e34bf0..a625d8c39 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1759,6 +1759,13 @@ update_view_references(struct Select *select, int update_value,
 		const char *space_name = sql_src_list_entry_name(list, i);
 		if (space_name == NULL)
 			continue;
+		/*
+		 * CTE tables isn't held in space cache and we
+		 * don't need to increment reference counter for
+		 * CTEs.
+		 */
+		if (sql_select_has_cte_with_name(select, space_name))
+			continue;
 		struct space *space = space_by_name(space_name);
 		if (space == NULL) {
 			if (! suppress_error) {
diff --git a/src/box/sql.h b/src/box/sql.h
index 7644051b4..422c8dfc2 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -323,6 +323,17 @@ sql_select_delete(struct sql *db, struct Select *select);
 struct SrcList *
 sql_select_expand_from_tables(struct Select *select);
 
+/**
+ * Check if @a name matches with at least one of CTE names typed
+ * in <WITH> clauses within @a select.
+ *
+ * @param select Select which may contain CTE.
+ * @retval true Has CTE with @a name.
+ * @retval false Hasn't CTE with @a name.
+*/
+bool
+sql_select_has_cte_with_name(struct Select *select, const char *name);
+
 /**
  * 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 ddb5de87e..d7dab35cd 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -332,6 +332,29 @@ sql_select_expand_from_tables(struct Select *select)
 	return walker.u.pSrcList;
 }
 
+bool
+sql_select_has_cte_with_name(struct Select *select, const char *name)
+{
+	assert(select != NULL && name != NULL);
+	struct With *with = select->pWith;
+	if (with != NULL) {
+		for (int i = 0; i < with->nCte; i++) {
+			const struct Cte *cte = &with->a[i];
+			if (memcmp(name, cte->zName, strlen(name)) == 0)
+				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_select_has_cte_with_name(list->a[i].pSelect,
+							 name))
+				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..67e068b42 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(78)
 
 --!./tcltestrunner.lua
 -- 2002 February 26
@@ -1233,4 +1233,96 @@ test:do_catchsql_test(
         -- </view-23.8>
     })
 
+-- gh-4149: Make sure that VIEW can be created as CTE.
+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_execsql_test(
+    "view-24.2",
+    [[
+        CREATE VIEW v AS WITH w(id) AS (
+            SELECT 1)
+          SELECT * FROM ts;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.2>
+        1
+        -- </view-24.2>
+    })
+
+test:do_execsql_test(
+    "view-24.3",
+    [[
+        DROP VIEW v;
+        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;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.3>
+        1,2,3,4
+        -- </view-24.3>
+    })
+
+test:do_execsql_test(
+    "view-24.4",
+    [[
+        DROP VIEW v;
+        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);
+          SELECT * FROM v;
+    ]], {
+        -- <view-24.4>
+        1,2,1,1,2,2,1,2,3,1,2,4
+        -- </view-24.4>
+    })
+
+test:do_execsql_test(
+    "view-24.5",
+    [[
+        DROP VIEW v;
+        DROP TABLE ts;
+        CREATE VIEW v AS WITH RECURSIVE
+            xaxis(x) AS (
+                VALUES(-2.0)
+                UNION ALL
+                SELECT x+0.5 FROM xaxis WHERE x<1.2),
+            yaxis(y) AS (
+                VALUES(-1.0)
+                UNION ALL
+                SELECT y+0.5 FROM yaxis WHERE y<1.0),
+            m(iter, cx, cy, x, y) AS (
+                SELECT 0, x, y, 0.0, 0.0 FROM xaxis, yaxis
+                UNION ALL
+                SELECT iter+1, cx, cy, x*x-y*y + cx, 2.0*x*y + cy FROM m WHERE (x*x + y*y) < 4.0 AND iter<1
+            ),
+            m2(iter, cx, cy) AS (
+            SELECT max(iter), cx, cy FROM m GROUP BY cx, cy
+            ),
+            a(t) AS (
+                SELECT group_concat( substr('a', 1+least(iter/7,4), 1), '') FROM m2 GROUP BY cy
+            )
+          SELECT group_concat(trim(t),x'0a') FROM a;
+        SELECT * FROM v;
+    ]], {
+        -- <view-24.5>
+        "aaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa"
+        -- </view-24.5>
+    })
+
 test:finish_test()

  reply	other threads:[~2019-09-04 14:14 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
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 [this message]
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=0FDC0655-79B5-479F-8FAA-5E9A2E93F1CF@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