From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 8D1F623197 for ; Mon, 19 Aug 2019 11:39:49 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id AECHeNy5Naz3 for ; Mon, 19 Aug 2019 11:39:49 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1EE251FF49 for ; Mon, 19 Aug 2019 11:39:48 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH] sql: add check for absence in From: Roman Khabibov In-Reply-To: <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> Date: Mon, 19 Aug 2019 18:39:46 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <13ADE619-05C1-44DD-9950-C6F7008702E3@tarantool.org> References: <20190802125252.54621-1-roman.habibov@tarantool.org> <6B834CCD-6A77-42D0-8AAD-B1BD2B8F51A6@tarantool.org> <8BD992A5-0A74-4E60-A239-5FDE85783467@tarantool.org> <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: "n. pettik" > On Aug 16, 2019, at 22:09, Roman Khabibov = wrote: >=20 >=20 >=20 >> On Aug 14, 2019, at 01:10, n.pettik wrote: >>=20 >>=20 >>=20 >>> On 13 Aug 2019, at 15:42, Roman Khabibov = wrote: >>>> On Aug 9, 2019, at 18:15, n.pettik wrote: >>>>> On 2 Aug 2019, at 15:52, Roman Khabibov = wrote: >>>>>=20 >>>>> Check that hasn't after : "CREATE VIEW v >>>>> AS WITH ... SELECT ...". Throw error, if it has. >>>>=20 >>>> What is the reason for that? I run example from ticket: >>>>=20 >>>> tarantool> \set language sql >>>> tarantool> \set delimiter ; >>>>=20 >>>> 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 >>>> ... >>>>=20 >>>> So, it fails at the stage of view creation. Please, ask Peter >>>> to provide valid example. Otherwise there=E2=80=99s no problem and >>>> issue can be closed. >>>>=20 >>> = https://github.com/tarantool/tarantool/issues/4149#issuecomment-520390204 >>>=20 >>>> 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=E2=80=99s one-line fix, but error isn't so detailed. >>> But now it's a syntax error, as was required in the ticket. >>=20 >> Still, one can a bit modify query and get old error: >>=20 >> 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 >> =E2=80=A6 > +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); > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) >=20 >> 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: >>=20 >> 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; >>=20 >> This is quite unpleasant. >>=20 >>> commit e311d35bd64422bf6468b28a28057d9045817eca >>> Author: Roman Khabibov >>> Date: Mon Jul 29 18:00:34 2019 +0300 >>>=20 >>> sql: disallow in select >>=20 >> -> sql: disallow WITH clause in CREATE VIEW statement >>=20 >>> We don't support queries with the syntax: >>> "CREATE VIEW v AS WITH ... SELECT =E2=80=A6" >>=20 >> Strictly speaking, we do support some of such queries. >> See example at the end of letter. >>=20 >>> Throw the syntax error in this case. >>=20 >> Instead of banning CTEs in view=E2=80=99s 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 =E2=80=9Cspace doesn=E2=80=99t exist=E2=80=9D= is raised. >> So, what we can do is add graceful check to update_view_references(). > If I understand you correctly, we should check presence in any = select, > which can meet after . 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. >=20 > 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 =3D make_scoped_guard([=3D] { > sql_select_delete(sql_get(), select); > }); > + if (sql_check_compound_with(select) =3D=3D true) > + tnt_raise(ClientError, = ER_CREATE_VIEW_WITH, > + def->name); > const char *disappeared_space; > if (update_view_references(select, 1, false, > &disappeared_space) = !=3D 0) { >=20 >>> Closes #4149 >>>=20 >>> 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) ::=3D . {A =3D 0;} >>> ///////////////////// The CREATE VIEW statement = ///////////////////////////// >>> // >>> cmd ::=3D 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 =3D 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 =3D require("sqltester") >>> -test:plan(73) >>> +test:plan(76) >>>=20 >>> --!./tcltestrunner.lua >>> -- 2002 February 26 >>> @@ -1233,4 +1233,39 @@ test:do_catchsql_test( >>> -- >>> }) >>>=20 >>> +-- 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); >>> + ]], { >>> + -- >>> + -- >>> + }) >>> + >>> +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; >>> + ]], { >>> + -- >>> + 1,"Keyword 'WITH' is reserved. Please use double quotes if = 'WITH' is an identifier." >>> + -- >>> + }) >>=20 >> Look at WITH rule in parse.y: >>=20 >> with(A) ::=3D . {A =3D 0;} >> with(A) ::=3D WITH wqlist(W). { A =3D W; } >> with(A) ::=3D WITH RECURSIVE wqlist(W). { A =3D W; } >>=20 >> So, basically you should also check cases like: >>=20 >> CREATE VIEW v AS WITH v1(id) as (select 1) SELECT * FROM ts; >>=20 >> (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; > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + >=20 > commit ad813d6d0e60afd3030528eaf8f34a0b54ce7fb2 > Author: Roman Khabibov > Date: Mon Jul 29 18:00:34 2019 +0300 >=20 > sql: disallow in selects >=20 > Disallow clause using in any select within > statement. Throw the error in this case. >=20 > Closes #4149 >=20 > 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 =3D make_scoped_guard([=3D] { > sql_select_delete(sql_get(), select); > }); > + if (sql_check_compound_with(select) =3D=3D true) > + tnt_raise(ClientError, = ER_CREATE_VIEW_WITH, > + def->name); > const char *disappeared_space; > if (update_view_references(select, 1, false, > &disappeared_space) = !=3D 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 ") \ >=20 > /* > * !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); >=20 > +/** > + * Check if select has any compound selects with clause. > + * > + * @param select Select to be checked. > + * @retval true Has s. > + * @retval false Hasn't 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; > } >=20 > +bool > +sql_check_compound_with(struct Select *select) > +{ > + if (select->pWith !=3D NULL) > + return true; > + struct SrcList *list =3D select->pSrc; > + int item_count =3D sql_src_list_entry_count(list); > + for (int i =3D 0; i < item_count; ++i) { > + if (list->a[i].pSelect !=3D NULL) > + if (sql_check_compound_with(list->a[i].pSelect) = =3D=3D 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 =3D require("sqltester") > -test:plan(73) > +test:plan(80) >=20 > --!./tcltestrunner.lua > -- 2002 February 26 > @@ -1233,4 +1233,99 @@ test:do_catchsql_test( > -- > }) >=20 > +-- gh-4149: Check error message for view creation with (nested) > +-- select with clause. > +test:do_execsql_test( > + "view-24.1", > + [[ > + CREATE TABLE ts (s1 INT PRIMARY KEY); > + INSERT INTO ts VALUES (1); > + ]], { > + -- > + -- > + }) > + > +test:do_catchsql_test( > + "view-24.2", > + [[ > + CREATE VIEW v AS WITH w(id) AS ( > + SELECT 1) > + SELECT * FROM ts; > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + > +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; > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + > +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); > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + > +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)); > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + > +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); > + ]], { > + -- > + 1,"Failed to create view 'V' with " > + -- > + }) > + > +test:do_execsql_test( > + "view-24.7", > + [[ > + DROP TABLE ts > + ]], { > + -- > + -- > + }) > + > test:finish_test() Sorry, forgot to correct one .result file. commit a8b2998140fad34c8eab0b6df737b63710038e2b Author: Roman Khabibov Date: Mon Jul 29 18:00:34 2019 +0300 sql: disallow in selects =20 Disallow clause using in any select within statement. Throw the error in this case. =20 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 =3D make_scoped_guard([=3D] { sql_select_delete(sql_get(), select); }); + if (sql_check_compound_with(select) =3D=3D true) + tnt_raise(ClientError, = ER_CREATE_VIEW_WITH, + def->name); const char *disappeared_space; if (update_view_references(select, 1, false, &disappeared_space) = !=3D 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 ") \ =20 /* * !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); =20 +/** + * Check if select has any compound selects with clause. + * + * @param select Select to be checked. + * @retval true Has s. + * @retval false Hasn't 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; } =20 +bool +sql_check_compound_with(struct Select *select) +{ + if (select->pWith !=3D NULL) + return true; + struct SrcList *list =3D select->pSrc; + int item_count =3D sql_src_list_entry_count(list); + for (int i =3D 0; i < item_count; ++i) { + if (list->a[i].pSelect !=3D NULL) + if (sql_check_compound_with(list->a[i].pSelect) = =3D=3D 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 =3D require("sqltester") -test:plan(73) +test:plan(80) =20 --!./tcltestrunner.lua -- 2002 February 26 @@ -1233,4 +1233,99 @@ test:do_catchsql_test( -- }) =20 +-- gh-4149: Check error message for view creation with (nested) +-- select with clause. +test:do_execsql_test( + "view-24.1", + [[ + CREATE TABLE ts (s1 INT PRIMARY KEY); + INSERT INTO ts VALUES (1); + ]], { + -- + -- + }) + +test:do_catchsql_test( + "view-24.2", + [[ + CREATE VIEW v AS WITH w(id) AS ( + SELECT 1) + SELECT * FROM ts; + ]], { + -- + 1,"Failed to create view 'V' with " + -- + }) + +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; + ]], { + -- + 1,"Failed to create view 'V' with " + -- + }) + +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); + ]], { + -- + 1,"Failed to create view 'V' with " + -- + }) + +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)); + ]], { + -- + 1,"Failed to create view 'V' with " + -- + }) + +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); + ]], { + -- + 1,"Failed to create view 'V' with " + -- + }) + +test:do_execsql_test( + "view-24.7", + [[ + DROP TABLE ts + ]], { + -- + -- + }) + test:finish_test()