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 A3AF826760 for ; Fri, 16 Aug 2019 15:09:50 -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 HRcD3zdeoNMB for ; Fri, 16 Aug 2019 15:09:50 -0400 (EDT) Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 3A0FD24F06 for ; Fri, 16 Aug 2019 15:09:50 -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: <8BD992A5-0A74-4E60-A239-5FDE85783467@tarantool.org> Date: Fri, 16 Aug 2019 22:09:47 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <326482ED-8190-4B72-B2B8-F20763A86F7E@tarantool.org> References: <20190802125252.54621-1-roman.habibov@tarantool.org> <6B834CCD-6A77-42D0-8AAD-B1BD2B8F51A6@tarantool.org> <8BD992A5-0A74-4E60-A239-5FDE85783467@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 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 " + -- + }) > 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. 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) { >> 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 " + -- + }) + commit ad813d6d0e60afd3030528eaf8f34a0b54ce7fb2 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/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()