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 7ECA02D22A for ; Mon, 3 Dec 2018 10:00:44 -0500 (EST) 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 tZ7KfItYu41B for ; Mon, 3 Dec 2018 10:00:44 -0500 (EST) 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 014602D214 for ; Mon, 3 Dec 2018 10:00:42 -0500 (EST) From: Nikita Pettik Subject: [tarantool-patches] [PATCH] sql: increment VIEW counter for tables within sub-select Date: Mon, 3 Dec 2018 18:00:37 +0300 Message-Id: <20181203150037.60513-1-korablev@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: v.shpilevoy@tarantool.org, Nikita Pettik VIEW can be created not only in form of AS SELECT but also in form of AS VALUES (...). In the latter case space's view reference counters were not incremented. Furthermore, if view came with sub-select, reference counters of tables within sub-select were not updated as well. These problems occurred due to simplified traverse over AST: only tables within FROM clause were accounted. This patch fixes them introducing complete pass over AST using walker. Closes #3815 --- Branch: https://github.com/tarantool/tarantool/commits/np/gh-3815-drop-view-as-values Issue: https://github.com/tarantool/tarantool/issues/3815 src/box/alter.cc | 18 +++++++---- src/box/sql.h | 29 ++++++++++------- src/box/sql/build.c | 3 -- src/box/sql/select.c | 85 ++++++++++++++++++++++++++++--------------------- src/box/sql/sqliteInt.h | 1 - test/sql/view.result | 58 +++++++++++++++++++++++++++++++++ test/sql/view.test.lua | 24 ++++++++++++++ 7 files changed, 160 insertions(+), 58 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index 029da029e..eca217511 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -1504,20 +1504,25 @@ update_view_references(struct Select *select, int update_value, bool suppress_error, const char **not_found_space) { assert(update_value == 1 || update_value == -1); - sql_select_expand_from_tables(select); - int from_tables_count = sql_select_from_table_count(select); + struct SrcList *list = sql_select_expand_from_tables(select); + if (list == NULL) + return -1; + int from_tables_count = sql_src_list_entry_count(list); for (int i = 0; i < from_tables_count; ++i) { - const char *space_name = sql_select_from_table_name(select, i); + const char *space_name = sql_src_list_entry_name(list, i); if (space_name == NULL) continue; uint32_t space_id; if (schema_find_id(BOX_SPACE_ID, 2, space_name, - strlen(space_name), &space_id) != 0) - return -1; + strlen(space_name), &space_id) != 0) { + sqlite3SrcListDelete(sql_get(), list); + return -1; + } if (space_id == BOX_ID_NIL) { if (! suppress_error) { assert(not_found_space != NULL); - *not_found_space = space_name; + *not_found_space = tt_sprintf("%s", space_name); + sqlite3SrcListDelete(sql_get(), list); return -1; } continue; @@ -1526,6 +1531,7 @@ update_view_references(struct Select *select, int update_value, assert(space->def->view_ref_count > 0 || update_value > 0); space->def->view_ref_count += update_value; } + sqlite3SrcListDelete(sql_get(), list); return 0; } diff --git a/src/box/sql.h b/src/box/sql.h index ec6d9d3cc..028a15245 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -374,35 +374,42 @@ void sql_select_delete(struct sqlite3 *db, struct Select *select); /** - * Expand all spaces names from 'FROM' clause, including - * ones from subqueries, and add those names to the original - * select. + * Collect all table names within given select, including + * ones from sub-queries, expressions, FROM clause etc. + * Return those names as a list. * - * @param select Select to be expanded. + * @param select Select to be investigated. + * @retval List containing all involved table names. + * NULL in case of OOM. */ -void +struct SrcList * sql_select_expand_from_tables(struct Select *select); /** * Temporary getter in order to avoid including sqliteInt.h * in alter.cc. * - * @param select Select to be examined. + * @param list List to be examined. * @retval Count of 'FROM' tables in given select. */ int -sql_select_from_table_count(const struct Select *select); +sql_src_list_entry_count(const struct SrcList *list); /** * Temporary getter in order to avoid including sqliteInt.h * in alter.cc. * - * @param select Select to be examined. - * @param i Ordinal number of 'FROM' table. - * @retval Name of i-th 'FROM' table. + * @param list List to be examined. + * @param i Ordinal number of entry. + * @retval Name of i-th entry. */ const char * -sql_select_from_table_name(const struct Select *select, int i); +sql_src_list_entry_name(const struct SrcList *list, int i); + +/** Delete an entire SrcList including all its substructure. */ +void +sqlite3SrcListDelete(struct sqlite3 *db, struct SrcList *list); + #if defined(__cplusplus) } /* extern "C" { */ diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 52f0bde15..7f2a2eca0 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3128,9 +3128,6 @@ sqlite3SrcListAssignCursors(Parse * pParse, SrcList * pList) } } -/* - * Delete an entire SrcList including all its substructure. - */ void sqlite3SrcListDelete(sqlite3 * db, SrcList * pList) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index ca709b44f..92ea0d14e 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -229,18 +229,18 @@ sql_select_delete(sqlite3 *db, Select *p) } int -sql_select_from_table_count(const struct Select *select) +sql_src_list_entry_count(const struct SrcList *list) { - assert(select != NULL && select->pSrc != NULL); - return select->pSrc->nSrc; + assert(list != NULL); + return list->nSrc; } const char * -sql_select_from_table_name(const struct Select *select, int i) +sql_src_list_entry_name(const struct SrcList *list, int i) { - assert(select != NULL && select->pSrc != NULL); - assert(i >= 0 && i < select->pSrc->nSrc); - return select->pSrc->a[i].zName; + assert(list != NULL); + assert(i >= 0 && i < list->nSrc); + return list->a[i].zName; } /* @@ -258,7 +258,9 @@ findRightmost(Select * p) /** * Work the same as sqlite3SrcListAppend(), but before adding to * list provide check on name duplicates: only values with unique - * names are appended. + * names are appended. Moreover, names of tables are not + * normalized: it is parser's business and in struct Select they + * are already in uppercased or unquoted form. * * @param db Database handler. * @param list List of entries. @@ -277,45 +279,54 @@ src_list_append_unique(struct sqlite3 *db, struct SrcList *list, if (name != NULL && strcmp(new_name, name) == 0) return list; } - struct Token token = { new_name, strlen(new_name), 0 }; - return sqlite3SrcListAppend(db, list, &token); + list = sqlite3SrcListEnlarge(db, list, 1, list->nSrc); + if (db->mallocFailed) { + sqlite3SrcListDelete(db, list); + return NULL; + } + struct SrcList_item *pItem = &list->a[list->nSrc - 1]; + pItem->zName = sqlite3DbStrNDup(db, new_name, strlen(new_name)); + if (pItem->zName == NULL) { + sqlite3SrcListDelete(db, list); + return NULL; + } + return list; } -/** - * This function is an inner call of recursive traverse through - * select AST starting from interface function - * sql_select_expand_from_tables(). - * - * @param top_select The root of AST. - * @param sub_select sub-select of current level recursion. - */ -static void -expand_names_sub_select(struct Select *top_select, struct Select *sub_select) +static int +select_collect_table_names(struct Walker *walker, struct Select *select) { - assert(top_select != NULL); - assert(sub_select != NULL); - struct SrcList_item *sub_src = sub_select->pSrc->a; - for (int i = 0; i < sub_select->pSrc->nSrc; ++i, ++sub_src) { - if (sub_src->zName == NULL) { - expand_names_sub_select(top_select, sub_src->pSelect); - } else { - top_select->pSrc = - src_list_append_unique(sql_get(), - top_select->pSrc, - sub_src->zName); - } + assert(walker != NULL); + assert(select != NULL); + for (int i = 0; i < select->pSrc->nSrc; ++i) { + if (select->pSrc->a[i].zName == NULL) + continue; + walker->u.pSrcList = + src_list_append_unique(sql_get(), walker->u.pSrcList, + select->pSrc->a[i].zName); + if (walker->u.pSrcList == NULL) + return WRC_Abort; } + return WRC_Continue; } -void +struct SrcList * sql_select_expand_from_tables(struct Select *select) { assert(select != NULL); - struct SrcList_item *src = select->pSrc->a; - for (int i = 0; i < select->pSrc->nSrc; ++i, ++src) { - if (select->pSrc->a[i].zName == NULL) - expand_names_sub_select(select, src->pSelect); + struct Walker walker; + struct SrcList *table_names = sql_alloc_src_list(sql_get()); + if (table_names == NULL) + return NULL; + memset(&walker, 0, sizeof(walker)); + walker.xExprCallback = sqlite3ExprWalkNoop; + walker.xSelectCallback = select_collect_table_names; + walker.u.pSrcList = table_names; + if (sqlite3WalkSelect(&walker, select) != 0) { + sqlite3SrcListDelete(sql_get(), walker.u.pSrcList); + return NULL; } + return walker.u.pSrcList; } /* diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index dbf58d967..1ec52b875 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3478,7 +3478,6 @@ int sqlite3IndexedByLookup(Parse *, struct SrcList_item *); void sqlite3SrcListShiftJoinType(SrcList *); void sqlite3SrcListAssignCursors(Parse *, SrcList *); void sqlite3IdListDelete(sqlite3 *, IdList *); -void sqlite3SrcListDelete(sqlite3 *, SrcList *); /** * Create a new index for an SQL table. name is the name of the diff --git a/test/sql/view.result b/test/sql/view.result index b211bcb2e..2effe4092 100644 --- a/test/sql/view.result +++ b/test/sql/view.result @@ -148,6 +148,64 @@ box.sql.execute("DROP VIEW v2;"); box.sql.execute("DROP TABLE t2;"); --- ... +-- gh-3815: AS VALUES syntax didn't incerement VIEW reference +-- counter. Moreover, tables within sub-select were not accounted +-- as well. +-- +box.sql.execute("CREATE TABLE b (s1 INT PRIMARY KEY);") +--- +... +box.sql.execute("CREATE VIEW bv (wombat) AS VALUES ((SELECT 'k' FROM b));") +--- +... +box.sql.execute("DROP TABLE b;") +--- +- error: 'Can''t drop space ''B'': other views depend on this space' +... +box.sql.execute("DROP VIEW bv;") +--- +... +box.sql.execute("DROP TABLE b;") +--- +... +box.sql.execute("CREATE TABLE b (s1 INT PRIMARY KEY);") +--- +... +box.sql.execute("CREATE TABLE c (s1 INT PRIMARY KEY);") +--- +... +box.sql.execute("CREATE VIEW bcv AS SELECT * FROM b WHERE s1 IN (SELECT * FROM c);") +--- +... +box.sql.execute("DROP TABLE c;") +--- +- error: 'Can''t drop space ''C'': other views depend on this space' +... +box.sql.execute("DROP VIEW bcv;") +--- +... +box.sql.execute("DROP TABLE c;") +--- +... +box.sql.execute("CREATE TABLE c (s1 INT PRIMARY KEY);") +--- +... +box.sql.execute("CREATE VIEW bcv(x, y) AS VALUES((SELECT 'k' FROM b), (VALUES((SELECT 1 FROM b WHERE s1 IN (VALUES((SELECT 1 + c.s1 FROM c)))))))") +--- +... +box.sql.execute("DROP TABLE c;") +--- +- error: 'Can''t drop space ''C'': other views depend on this space' +... +box.space.BCV:drop() +--- +... +box.sql.execute("DROP TABLE c;") +--- +... +box.sql.execute("DROP TABLE b;") +--- +... -- Cleanup box.sql.execute("DROP VIEW v1;"); --- diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua index a6269a1bf..cbca201cd 100644 --- a/test/sql/view.test.lua +++ b/test/sql/view.test.lua @@ -65,6 +65,30 @@ box.sql.execute("DROP TABLE t2;"); box.sql.execute("DROP VIEW v2;"); box.sql.execute("DROP TABLE t2;"); +-- gh-3815: AS VALUES syntax didn't incerement VIEW reference +-- counter. Moreover, tables within sub-select were not accounted +-- as well. +-- +box.sql.execute("CREATE TABLE b (s1 INT PRIMARY KEY);") +box.sql.execute("CREATE VIEW bv (wombat) AS VALUES ((SELECT 'k' FROM b));") +box.sql.execute("DROP TABLE b;") +box.sql.execute("DROP VIEW bv;") +box.sql.execute("DROP TABLE b;") + +box.sql.execute("CREATE TABLE b (s1 INT PRIMARY KEY);") +box.sql.execute("CREATE TABLE c (s1 INT PRIMARY KEY);") +box.sql.execute("CREATE VIEW bcv AS SELECT * FROM b WHERE s1 IN (SELECT * FROM c);") +box.sql.execute("DROP TABLE c;") +box.sql.execute("DROP VIEW bcv;") +box.sql.execute("DROP TABLE c;") + +box.sql.execute("CREATE TABLE c (s1 INT PRIMARY KEY);") +box.sql.execute("CREATE VIEW bcv(x, y) AS VALUES((SELECT 'k' FROM b), (VALUES((SELECT 1 FROM b WHERE s1 IN (VALUES((SELECT 1 + c.s1 FROM c)))))))") +box.sql.execute("DROP TABLE c;") +box.space.BCV:drop() +box.sql.execute("DROP TABLE c;") +box.sql.execute("DROP TABLE b;") + -- Cleanup box.sql.execute("DROP VIEW v1;"); box.sql.execute("DROP TABLE t1;"); -- 2.15.1