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 4E7F124CEB for ; Fri, 13 Sep 2019 10:57:41 -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 7Qla1ESftP_o for ; Fri, 13 Sep 2019 10:57:41 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 75FB620C0B for ; Fri, 13 Sep 2019 10:57:40 -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: <20190911133203.GA49615@tarantool.org> Date: Fri, 13 Sep 2019 17:57:37 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: 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> <1B494CAB-704F-4A62-A00D-257388DDB2B8@tarantool.org> <20190829175955.GA52650@tarantool.org> <0FDC0655-79B5-479F-8FAA-5E9A2E93F1CF@tarantool.org> <20190911133203.GA49615@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 Sep 11, 2019, at 16:32, Nikita Pettik = wrote: >=20 > On Wed, Sep 04, 2019 at 05:14:10PM +0300, Roman Khabibov wrote: >>=20 >> commit 4f78ff7cf8b4a3f496d72388235f773ae51d6efc >> Author: Roman Khabibov >> Date: Mon Jul 29 18:00:34 2019 +0300 >>=20 >> sql: allow to create view as clause >>=20 >> Allow views to use CTEs in clauses,=20 >=20 > Nit: all CTEs start from WITH clause; so your statement is a bit = confusing. sql: allow to create view as clause =20 Allow views to use CTEs, which can be in any (nested) select after . 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=E2=80=99t = 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. =20 Closes #4149 >> which can be in any >> (nested) select after . 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=E2=80=99= 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. >>=20 >> Closes #4149 >>=20 >> 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 =3D sql_src_list_entry_name(list, = i); >> if (space_name =3D=3D NULL) >> continue; >> + /* >> + * CTE tables isn't held in space cache and we >=20 > Nit: tables aren't held ... >=20 >> + * don't need to increment reference counter for >> + * CTEs. >=20 > I'd rephrase a bit: >=20 > 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 =3D sql_src_list_entry_name(list, = i); if (space_name =3D=3D 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 =3D space_by_name(space_name); if (space =3D=3D NULL) { if (! suppress_error) { >> + */ >> + if (sql_select_has_cte_with_name(select, space_name)) >> + continue; >> struct space *space =3D space_by_name(space_name); >> if (space =3D=3D 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); >>=20 >> +/** >> + * Check if @a name matches with at least one of CTE names typed >> + * in clauses within @a select. >> + * >> + * @param select Select which may contain CTE. >=20 > Nit: you forgot to describe name argument. >=20 >> + * @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); >=20 > sql_select_contains_cte() is enough, I guess. +/** + * Check if @a name matches with at least one of CTE names typed + * in clauses within @a select except s that are + * nested within other 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; >> } >>=20 >> +bool >> +sql_select_has_cte_with_name(struct Select *select, const char = *name) >> +{ >> + assert(select !=3D NULL && name !=3D NULL); >> + struct With *with =3D select->pWith; >> + if (with !=3D NULL) { >> + for (int i =3D 0; i < with->nCte; i++) { >> + const struct Cte *cte =3D &with->a[i]; >> + if (memcmp(name, cte->zName, strlen(name)) =3D=3D = 0) >> + return true; >> + } >> + } >=20 > Please explain why we don't use recursion here (cte->pSelect->pWith). > I'd attach schema of SELECT for the sake of the clarity. >=20 >> + 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_select_has_cte_with_name(list->a[i].pSelect, >> + name)) >> + return true; >> +=09 >=20 > 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 !=3D NULL && name !=3D NULL); + struct With *with =3D select->pWith; + if (with !=3D NULL) { + for (int i =3D 0; i < with->nCte; i++) { + const struct Cte *cte =3D &with->a[i]; + /* + * Don't use recursive call for + * cte->pSelect, because this function is + * used during view creation. Consider + * the nested 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 is disallowed. + * So, it is pointless to check , + * which is nested to other . + */ + if (memcmp(name, cte->zName, strlen(name)) =3D=3D = 0) + 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_select_constains_cte(list->a[i].pSelect, + name)) + return true; + } + } + return false; +} + commit 32b5b1b2f355f57caf8e84e78bb3154890d7d82e Author: Roman Khabibov Date: Mon Jul 29 18:00:34 2019 +0300 sql: allow to create view as clause =20 Allow views to use CTEs, which can be in any (nested) select after . 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=E2=80=99t = 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. =20 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 =3D sql_src_list_entry_name(list, = i); if (space_name =3D=3D 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 =3D space_by_name(space_name); if (space =3D=3D 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); =20 +/** + * Check if @a name matches with at least one of CTE names typed + * in clauses within @a select except s that are + * nested within other 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; } =20 +bool +sql_select_constains_cte(struct Select *select, const char *name) +{ + assert(select !=3D NULL && name !=3D NULL); + struct With *with =3D select->pWith; + if (with !=3D NULL) { + for (int i =3D 0; i < with->nCte; i++) { + const struct Cte *cte =3D &with->a[i]; + /* + * Don't use recursive call for + * cte->pSelect, because this function is + * used during view creation. Consider + * the nested 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 is disallowed. + * So, it is pointless to check , + * which is nested to other . + */ + if (memcmp(name, cte->zName, strlen(name)) =3D=3D = 0) + 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_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 =3D require("sqltester") -test:plan(73) +test:plan(78) =20 --!./tcltestrunner.lua -- 2002 February 26 @@ -1233,4 +1233,96 @@ test:do_catchsql_test( -- }) =20 +-- 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); + ]], { + -- + -- + }) + +test:do_execsql_test( + "view-24.2", + [[ + CREATE VIEW v AS WITH w(id) AS ( + SELECT 1) + SELECT * FROM ts, w; + SELECT * FROM v; + ]], { + -- + 1, 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; + SELECT * FROM v; + ]], { + -- + 1,2,3,4 + -- + }) + +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; + ]], { + -- + 1,2,1,1,2,2,1,2,3,1,2,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; + ]], { + -- + "aaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa\naaaaaaaa" + -- + }) + test:finish_test()