[tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW>
Roman Khabibov
roman.habibov at tarantool.org
Mon Aug 19 18:39:46 MSK 2019
> On Aug 16, 2019, at 22:09, Roman Khabibov <roman.habibov at tarantool.org> wrote:
>
>
>
>> On Aug 14, 2019, at 01:10, n.pettik <korablev at tarantool.org> wrote:
>>
>>
>>
>>> On 13 Aug 2019, at 15:42, Roman Khabibov <roman.habibov at tarantool.org> wrote:
>>>> On Aug 9, 2019, at 18:15, n.pettik <korablev at tarantool.org> wrote:
>>>>> On 2 Aug 2019, at 15:52, Roman Khabibov <roman.habibov at 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 at 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 at 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 at 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()
More information about the Tarantool-patches
mailing list