* [tarantool-patches] [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> @ 2019-08-02 12:52 Roman Khabibov 2019-08-09 15:15 ` [tarantool-patches] " n.pettik 0 siblings, 1 reply; 12+ messages in thread From: Roman Khabibov @ 2019-08-02 12:52 UTC (permalink / raw) To: tarantool-patches; +Cc: korablev Check that <CREATE VIEW> hasn't <WITH> after <AS>: "CREATE VIEW v AS WITH ... SELECT ...". Throw error, if it has. Closes #4149 --- Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4149-view Issue: https://github.com/tarantool/tarantool/issues/4149 src/box/sql/build.c | 7 +++++++ test/sql-tap/view.test.lua | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index ccec10543..e34202c9a 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1320,6 +1320,13 @@ sql_create_view(struct Parse *parse_context) &create_entity_def->name); if (space == NULL || parse_context->is_aborted) goto create_view_fail; + assert(view_def->select != NULL); + if (view_def->select->pWith != NULL) { + diag_set(ClientError, ER_CREATE_SPACE, space->def->name, + "can't create view as <WITH>"); + parse_context->is_aborted = true; + goto create_view_fail; + } struct space *select_res_space = sqlResultSetOfSelect(parse_context, view_def->select); if (select_res_space == NULL) diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua index 101f4c3e7..a554c17a7 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,"Failed to create space 'V': can't create view as <WITH>" + -- </view-24.2> + }) + +test:do_execsql_test( + "view-24.3", + [[ + DROP TABLE ts + ]], { + -- <view-24.3> + -- </view-24.3> + }) + test:finish_test() -- 2.20.1 (Apple Git-117) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-02 12:52 [tarantool-patches] [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> Roman Khabibov @ 2019-08-09 15:15 ` n.pettik 2019-08-13 12:42 ` Roman Khabibov 0 siblings, 1 reply; 12+ messages in thread From: n.pettik @ 2019-08-09 15:15 UTC (permalink / raw) To: tarantool-patches; +Cc: Roman Khabibov > 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. > > Closes #4149 > --- > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4149-view > Issue: https://github.com/tarantool/tarantool/issues/4149 > > src/box/sql/build.c | 7 +++++++ > test/sql-tap/view.test.lua | 37 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 43 insertions(+), 1 deletion(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index ccec10543..e34202c9a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1320,6 +1320,13 @@ sql_create_view(struct Parse *parse_context) > &create_entity_def->name); > if (space == NULL || parse_context->is_aborted) > goto create_view_fail; > + assert(view_def->select != NULL); > + if (view_def->select->pWith != NULL) { > + diag_set(ClientError, ER_CREATE_SPACE, space->def->name, > + "can't create view as <WITH>"); > + parse_context->is_aborted = true; > + goto create_view_fail; > + } 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-09 15:15 ` [tarantool-patches] " n.pettik @ 2019-08-13 12:42 ` Roman Khabibov 2019-08-13 22:10 ` n.pettik 0 siblings, 1 reply; 12+ messages in thread From: Roman Khabibov @ 2019-08-13 12:42 UTC (permalink / raw) To: tarantool-patches; +Cc: n. pettik > 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. 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 We don't support queries with the syntax: "CREATE VIEW v AS WITH ... SELECT ..." Throw the syntax error in this case. 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> + }) + +test:do_execsql_test( + "view-24.3", + [[ + DROP TABLE ts + ]], { + -- <view-24.3> + -- </view-24.3> + }) + test:finish_test() ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-13 12:42 ` Roman Khabibov @ 2019-08-13 22:10 ` n.pettik 2019-08-16 19:09 ` Roman Khabibov 0 siblings, 1 reply; 12+ messages in thread From: n.pettik @ 2019-08-13 22:10 UTC (permalink / raw) To: tarantool-patches; +Cc: Roman Khabibov > 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 … 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(). > 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) ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 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 0 siblings, 2 replies; 12+ messages in thread From: Roman Khabibov @ 2019-08-16 19:09 UTC (permalink / raw) To: tarantool-patches; +Cc: n. pettik > 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() ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-16 19:09 ` Roman Khabibov @ 2019-08-19 15:39 ` Roman Khabibov 2019-08-20 19:41 ` n.pettik 1 sibling, 0 replies; 12+ messages in thread From: Roman Khabibov @ 2019-08-19 15:39 UTC (permalink / raw) To: tarantool-patches; +Cc: n. pettik > 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() ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 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 1 sibling, 1 reply; 12+ messages in thread From: n.pettik @ 2019-08-20 19:41 UTC (permalink / raw) To: tarantool-patches; +Cc: Roman Khabibov > On 16 Aug 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. You slightly misunderstood me. I proposed to allow such queries gracefully handling WITH statement and avoiding references counter incrementation for CTEs. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-20 19:41 ` n.pettik @ 2019-08-28 12:17 ` Roman Khabibov 2019-08-29 17:59 ` Nikita Pettik 0 siblings, 1 reply; 12+ messages in thread From: Roman Khabibov @ 2019-08-28 12:17 UTC (permalink / raw) To: tarantool-patches; +Cc: n. pettik > 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>. Closes #4149 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; 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. + * @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) +{ + 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; + } + 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..4b695fa6a 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_execsql_test( + "view-24.2", + [[ + CREATE VIEW v AS WITH w(id) AS ( + SELECT 1) + SELECT * FROM ts; + ]], { + -- <view-24.2> + -- </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; + ]], { + -- <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; + ]], { + -- <view-24.7> + -- </view-24.7> + }) + test:finish_test() ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-28 12:17 ` Roman Khabibov @ 2019-08-29 17:59 ` Nikita Pettik 2019-09-04 14:14 ` Roman Khabibov 0 siblings, 1 reply; 12+ messages in thread From: Nikita Pettik @ 2019-08-29 17:59 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches 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. > 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. > 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. > +{ > + 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; > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-08-29 17:59 ` Nikita Pettik @ 2019-09-04 14:14 ` Roman Khabibov 2019-09-11 13:32 ` Nikita Pettik 0 siblings, 1 reply; 12+ messages in thread From: Roman Khabibov @ 2019-09-04 14:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Nikita Pettik > 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() ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-09-04 14:14 ` Roman Khabibov @ 2019-09-11 13:32 ` Nikita Pettik 2019-09-13 14:57 ` Roman Khabibov 0 siblings, 1 reply; 12+ messages in thread From: Nikita Pettik @ 2019-09-11 13:32 UTC (permalink / raw) To: tarantool-patches; +Cc: roman.habibov On Wed, Sep 04, 2019 at 05:14:10PM +0300, Roman Khabibov wrote: > > 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, Nit: all CTEs start from WITH clause; so your statement is a bit confusing. > 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 Nit: tables aren't held ... > + * don't need to increment reference counter for > + * CTEs. I'd rephrase a bit: Views are allowed to contain CTEs. CTE is a temporary object, created and destroyed at SQL runtime (it is represented by an ephemeral table). So, it is absent in space cache and as a consequence we can't increment its reference counter. Skip iteration. > + */ > + 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. Nit: you forgot to describe name argument. > + * @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); sql_select_contains_cte() is enough, I guess. > + > /** > * 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; > + } > + } Please explain why we don't use recursion here (cte->pSelect->pWith). I'd attach schema of SELECT for the sake of the clarity. > + 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; > + Nit: in case 'if' body contains more than one string it is enclosed in brackets. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 2019-09-11 13:32 ` Nikita Pettik @ 2019-09-13 14:57 ` Roman Khabibov 0 siblings, 0 replies; 12+ messages in thread From: Roman Khabibov @ 2019-09-13 14:57 UTC (permalink / raw) To: tarantool-patches; +Cc: n. pettik > On Sep 11, 2019, at 16:32, Nikita Pettik <korablev@tarantool.org> wrote: > > On Wed, Sep 04, 2019 at 05:14:10PM +0300, Roman Khabibov wrote: >> >> 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, > > Nit: all CTEs start from WITH clause; so your statement is a bit confusing. sql: allow to create view as <WITH> clause Allow views to use CTEs, 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 >> 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 > > Nit: tables aren't held ... > >> + * don't need to increment reference counter for >> + * CTEs. > > I'd rephrase a bit: > > Views are allowed to contain CTEs. CTE is a temporary object, created > and destroyed at SQL runtime (it is represented by an ephemeral table). > So, it is absent in space cache and as a consequence we can't increment > its reference counter. Skip iteration. Ok. @@ -1759,6 +1759,16 @@ 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; + /* + * Views are allowed to contain CTEs. CTE is a + * temporary object, created and destroyed at SQL + * runtime (it is represented by an ephemeral + * table). So, it is absent in space cache and as + * a consequence we can't increment its reference + * counter. Skip iteration. + */ + if (sql_select_constains_cte(select, space_name)) + continue; struct space *space = space_by_name(space_name); if (space == NULL) { if (! suppress_error) { >> + */ >> + 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. > > Nit: you forgot to describe name argument. > >> + * @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); > > sql_select_contains_cte() is enough, I guess. +/** + * Check if @a name matches with at least one of CTE names typed + * in <WITH> clauses within @a select except <WITH>s that are + * nested within other <WITH>s. + * + * @param select Select which may contain CTE. + * @param name The name of CTE, that may contained. + * @retval true Has CTE with @a name. + * @retval false Hasn't CTE with @a name. +*/ +bool +sql_select_constains_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 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; >> + } >> + } > > Please explain why we don't use recursion here (cte->pSelect->pWith). > I'd attach schema of SELECT for the sake of the clarity. > >> + 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; >> + > > Nit: in case 'if' body contains more than one string it is enclosed in > brackets. +bool +sql_select_constains_cte(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]; + /* + * Don't use recursive call for + * cte->pSelect, because this function is + * used during view creation. Consider + * the nested <WITH>s query schema: + * CREATE VIEW v AS + * WITH w AS ( + * WITH w_nested AS + * (...) + * SELECT ...) + * SELECT ... FROM ...; + * The use of CTE "w_nested" after the + * external select's <FROM> is disallowed. + * So, it is pointless to check <WITH>, + * which is nested to other <WITH>. + */ + 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_constains_cte(list->a[i].pSelect, + name)) + return true; + } + } + return false; +} + commit 32b5b1b2f355f57caf8e84e78bb3154890d7d82e 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, 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 c0780d6da..516d6abac 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -1759,6 +1759,16 @@ 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; + /* + * Views are allowed to contain CTEs. CTE is a + * temporary object, created and destroyed at SQL + * runtime (it is represented by an ephemeral + * table). So, it is absent in space cache and as + * a consequence we can't increment its reference + * counter. Skip iteration. + */ + if (sql_select_constains_cte(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..0fa52fc0b 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -323,6 +323,19 @@ 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 except <WITH>s that are + * nested within other <WITH>s. + * + * @param select Select which may contain CTE. + * @param name The name of CTE, that may contained. + * @retval true Has CTE with @a name. + * @retval false Hasn't CTE with @a name. +*/ +bool +sql_select_constains_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 cdcdbaf2b..8f93edd16 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -332,6 +332,46 @@ sql_select_expand_from_tables(struct Select *select) return walker.u.pSrcList; } +bool +sql_select_constains_cte(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]; + /* + * Don't use recursive call for + * cte->pSelect, because this function is + * used during view creation. Consider + * the nested <WITH>s query schema: + * CREATE VIEW v AS + * WITH w AS ( + * WITH w_nested AS + * (...) + * SELECT ...) + * SELECT ... FROM ...; + * The use of CTE "w_nested" after the + * external select's <FROM> is disallowed. + * So, it is pointless to check <WITH>, + * which is nested to other <WITH>. + */ + 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_constains_cte(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..6234f863e 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, w; + SELECT * FROM v; + ]], { + -- <view-24.2> + 1, 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() ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-09-13 14:57 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-02 12:52 [tarantool-patches] [PATCH] sql: add check for <WITH> absence in <CREATE VIEW> 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 2019-09-11 13:32 ` Nikita Pettik 2019-09-13 14:57 ` Roman Khabibov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox