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()
next prev parent 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