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 1DF8424CFF for ; Tue, 5 Jun 2018 07:30:24 -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 mbhOs89I3KXF for ; Tue, 5 Jun 2018 07:30:24 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 4125824CFC for ; Tue, 5 Jun 2018 07:30:23 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] sql: rework VIEW internals References: <1528129571.147907906@f369.i.mail.ru> From: Vladislav Shpilevoy Message-ID: Date: Tue, 5 Jun 2018 14:30:19 +0300 MIME-Version: 1.0 In-Reply-To: <1528129571.147907906@f369.i.mail.ru> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Nikita Pettik Hello. Thanks for the patch! > Firstly, I don't like code duplication in view on_commit/on_rollback > triggers. But I didn't manage to find any 'satisfactory' solution, > except for adding auxilary function with enum/bool arg which in turn > will determine action on view reference counter. I think it is better than code duplication. Lets do special function (maybe with another name) sql_view_update_references(Select *, int update_value) { expand_names() for (int i = 0; i < select->pSrc->nSrc; ++i) { const char *space_name = select->pSrc->a[i].zName; if (space_name == NULL) continue; uint32_t space_id = schema_find_id(BOX_SPACE_ID, 0, space_name, strlen(space_name)); assert(space_id != BOX_ID_NIL); space = space_by_id(space_id); space->def->view_ref_count += update_value; } } on_drop/create_commit just do update_references(+1/-1) + delete. > Secondly, (I guess) including in alter.cc sqliteInt.h header seems to be > total mess, but again - I failed to come up with kind of sinsible> resolution. Is it time for dividing SQL code base into source/header files? Yes, sqliteInt.h in alter.cc looks ugly. Just implement temporary getters/setters taking Select * and defined in sql.h. Look for example on Expr * - it is used in alter.cc, but does not require include anything except sql.h. Also maybe the suggestion above can help you. You can declare sql_view_update_references in sql.h, and do not access select->pSrc->a[i].zName directly in alter.cc. > At least, including "select.h" looks less annoying. > Finally, I temporary changed system space in box_space_id_by_name() > from _vspace to _space, in order to find space by name in on_dd_replace trigger. _vspace is needed in box_space_id_by_name because it is called by external API and hides from the user the spaces he has no access. So _space is not acceptable here. > I have noticed that Kirill Sh. has almost done the same thing, so this > hack is going to be substituted with his patch. In alter.cc you can use schema_find_id(). Kirill in his patch was not able to use this function because it throws exceptions, but alter.cc is C++. See my 22 comments and recommendations below. 1. Travis is failed on the branch on box/func_reload. It is very strange, but on the previous commit on your branch it is not failed. > > src/box/alter.cc | 84 ++++++++++++ > src/box/box.cc | 4 +- > src/box/space_def.c | 1 + > src/box/space_def.h | 2 + > src/box/sql.h | 47 +++++++ > src/box/sql/build.c | 289 ++++++++++++----------------------------- > src/box/sql/delete.c | 6 +- > src/box/sql/expr.c | 5 +- > src/box/sql/fkey.c | 4 +- > src/box/sql/insert.c | 4 +- > src/box/sql/parse.y | 19 +-- > src/box/sql/select.c | 61 +++++++-- > src/box/sql/sqliteInt.h | 31 ++++- > src/box/sql/tokenize.c | 18 +++ > src/box/sql/trigger.c | 8 +- > src/box/sql/vdbe.c | 24 ++++ > test/sql-tap/colname.test.lua | 4 +- > test/sql-tap/drop_all.test.lua | 4 +- > test/sql-tap/fkey2.test.lua | 1 + > test/sql-tap/trigger1.test.lua | 1 + > test/sql-tap/triggerC.test.lua | 1 + > test/sql-tap/view.test.lua | 137 ++++++++++++++----- > test/sql/view.result | 23 +++- > test/sql/view.test.lua | 11 +- > 24 files changed, 502 insertions(+), 287 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index b62f8adea..1fbb54d1a 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -29,6 +29,7 @@ > * SUCH DAMAGE. > */ > #include "alter.h" > +#include "box.h" 2. As I said above, you can remove box.h. It must not be included anywhere in internal files when possible. So consider this diff, please. After this fix and after others I have not run tests. So please check them before applying. If they fail after me, then sorry %) @@ -29,7 +29,6 @@ * SUCH DAMAGE. */ #include "alter.h" -#include "box.h" #include "schema.h" @@ -1465,8 +1464,8 @@ on_create_view_commit(struct trigger *trigger, void *event) const char *space_name = select->pSrc->a[i].zName; if (space_name == NULL) continue; - uint32_t space_id = box_space_id_by_name(space_name, - strlen(space_name)); + uint32_t space_id = schema_find_id(BOX_SPACE_ID, 2, space_name, + strlen(space_name)); assert(space_id != BOX_ID_NIL); @@ -1497,8 +1496,8 @@ on_drop_view_commit(struct trigger *trigger, void *event) const char *space_name = select->pSrc->a[i].zName; if (space_name == NULL) continue; - uint32_t space_id = box_space_id_by_name(space_name, - strlen(space_name)); + uint32_t space_id = schema_find_id(BOX_SPACE_ID, 2, space_name, + strlen(space_name)); assert(space_id != BOX_ID_NIL); > @@ -1440,6 +1442,70 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, > } > } > > +/** > + * Trigger which is fired on creation of new SQL view. > + * Its purpose is to increment view reference counters of > + * dependent spaces. > + */ > +static void > +on_create_view_commit(struct trigger *trigger, void *event) > +{ > + (void) event; > + struct space *space = (struct space *)trigger->data; > + struct space_def *def = space->def; > + struct Select *select; > + if (sql_view_compile(sql_get(), def->opts.sql, &select) != 0) { > + diag_log();> + return; 3. On commit trigger must never fail. So please, do compile in on_replace_dd_space, and here just use it. I propose this. - Compile sql in on_replace_dd_space; - Put compiled Select * in trigger->data (as well as you pass space * now); - In on_drop/create_view_commit you take Select * from trigger data. Space * as I can see is used in these functions to take def->sql and create Select * only so it is ok to replace it with already compiled Select *. Then do not forget to add on_rollback triggers to delete Select *. > @@ -1575,6 +1652,13 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) > struct trigger *on_commit = > txn_alter_trigger_new(on_drop_space_commit, space); > txn_on_commit(txn, on_commit); > + if (old_space->def->opts.is_view && > + old_space->def->opts.sql != NULL) { 4. How is it possible that is_view == true, but sql == NULL? > if (def->opts.is_view != old_space->def->opts.is_view) > tnt_raise(ClientError, ER_ALTER_SPACE, > space_name(old_space), > "can not convert a space to " > "a view and vice versa"); 5. Please, use {} for 'if' body consisting of multiple lines. 6. I do not see where do you set def->view_ref_count on space alter. See this test: tarantool> box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY);") tarantool> box.sql.execute("CREATE VIEW v1 AS SELECT * FROM t1") tarantool> box.sql.execute("DROP TABLE t1") --- - error: 'Can''t drop table T1: other views depend on this space' ... It is ok. But then I do this: tarantool> s = box.space._space:get{box.space.T1.id} tarantool> box.space._space:replace(s) Nothing is really changed, only space * and space_def * are recreated. But now I am able to do this: tarantool> box.sql.execute("DROP TABLE t1") --- ... You need to update class ModifySpace to move view refs from the old to the new def. > diff --git a/src/box/sql.h b/src/box/sql.h > index 23021e56b..9a006e5cb 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -83,6 +83,19 @@ int > sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, > struct Expr **result); > > +/** > + * This routine executes parser on 'CREATE VIEW ...' statement > + * and loads content of SELECT into internal structs as result. > + * > + * @param db Current SQL context. > + * @param view_stmt String containing 'CREATE VIEW' statement. > + * @param[out] select Fetched SELECT statement. > + * @retval 0 on success, -1 otherwise. > + */ > +int > +sql_view_compile(struct sqlite3 *db, const char *view_stmt, > + struct Select **select); 7. You need an out parameter only when returned value is occupied for another output. Here it is enough to just return Select * on success and NULL on error. I see that sql_expr_compile uses out parameter but it is wrong too. Kirill in his patch fixes it. > @@ -293,6 +306,40 @@ sql_parser_create(struct Parse *parser, struct sqlite3 *db); > +/** > + * Work the same as sqlite3SrcListAppend(), but before adding to > + * list provide check on name duplicates: only values with unique > + * names are appended. > + * > + * @param db Database handler. > + * @param list List of entries. > + * @param new_name Name of entity to be added. > + * @retval @list with new element on success, old one otherwise. > + */ > +struct SrcList * > +sql_src_list_append_unique(struct sqlite3 *db, struct SrcList *list, > + const char *new_name); 8. Do you really need this declaration in sql.h? It is used in select.c only. Maybe it can be static inside this file? > + > +/** > + * Expand all spaces names from 'FROM' clause, including > + * ones from subqueries, and add those names to the original > + * select. > + * > + * @param select Select to be expanded. > + */ > +void > +sql_select_expand_from_tables(struct Select *select); 9. If you agree with my suggestion to create sql_view_update_references(), you can move this function into select.c together with the former and make it static. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 28e4d7a4d..053cf83b7 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2009,72 +2007,74 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ > } > > #ifndef SQLITE_OMIT_VIEW 10. Maybe it is worth to delete SQLITE_OMIT_VIEW alongside? > -/* > - * The parser calls this routine in order to create a new VIEW > - */ > void > -sqlite3CreateView(Parse * pParse, /* The parsing context */ > - Token * pBegin, /* The CREATE token that begins the statement */ > - Token * pName, /* The token that holds the name of the view */ > - ExprList * pCNames, /* Optional list of view column names */ > - Select * pSelect, /* A SELECT statement that will become the new view */ > - int noErr /* Suppress error messages if VIEW already exists */ > - ) > +sql_create_view(struct Parse *parse_context, struct Token *begin, > + struct Token *name, struct ExprList *aliases, > + struct Select *select, bool if_exists) 11. 'if_exists' in build.c, but 'is_exists' in sqliteInt.h. 12. Please, consider this diff: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 053cf83b7..935054540 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2020,16 +2020,16 @@ sql_create_view(struct Parse *parse_context, struct Token *begin, } sqlite3StartTable(parse_context, name, if_exists); struct Table *p = parse_context->pNewTable; - if (p == NULL || parse_context->nErr) + if (p == NULL || parse_context->nErr != 0) goto create_view_fail; struct Table *sel_tab = sqlite3ResultSetOfSelect(parse_context, select); if (sel_tab == NULL) goto create_view_fail; if (aliases != NULL) { if ((int)sel_tab->def->field_count != aliases->nExpr) { - sqlite3ErrorMsg(parse_context, - "expected %d columns for '%s' but got %d", - aliases->nExpr, p->def->name, + sqlite3ErrorMsg(parse_context, "expected %d columns "\ + "for '%s' but got %d", aliases->nExpr, + p->def->name, sel_tab->def->field_count); goto create_view_fail; } @@ -2053,21 +2053,17 @@ sql_create_view(struct Parse *parse_context, struct Token *begin, */ struct Token end = parse_context->sLastToken; assert(end.z[0] != 0); - if (end.z[0] != ';') { + if (end.z[0] != ';') end.z += end.n; - } end.n = 0; - int n = (int)(end.z - begin->z); + int n = end.z - begin->z; assert(n > 0); const char *z = begin->z; - while (sqlite3Isspace(z[n - 1])) { + while (sqlite3Isspace(z[n - 1])) n--; - } end.z = &z[n - 1]; end.n = 1; - p->def->opts.sql = malloc(n + 1); - memcpy(p->def->opts.sql, begin->z, n); - p->def->opts.sql[n] = '\0'; + p->def->opts.sql = strndup(begin->z, n); > + p->def->opts.sql = malloc(n + 1); 13. No error checking. Here you should to set diag OOM, increment parser.nErr etc. > @@ -2084,155 +2084,22 @@ sql_view_column_names(struct Parse *parse, struct Table *table) > { > assert(table != NULL); > assert(space_is_view(table)); > - /* A positive nCol means the columns names for this view > - * are already known. > - */ > - if (table->def->field_count > 0) > - return 0; > - > - /* A negative nCol is a special marker meaning that we are > - * currently trying to compute the column names. If we > - * enter this routine with a negative nCol, it means two > - * or more views form a loop, like this: > - * > - * CREATE VIEW one AS SELECT * FROM two; > - * CREATE VIEW two AS SELECT * FROM one; > - * > - * Actually, the error above is now caught prior to > - * reaching this point. But the following test is still > - * important as it does come up in the following: > - * > - * CREATE TABLE main.ex1(a); > - * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1; > - * SELECT * FROM temp.ex1; > - */ > - if ((int)table->def->field_count < 0) { > - sqlite3ErrorMsg(parse, "view %s is circularly defined", > - table->def->name); > + struct sqlite3 *db = parse->db; > + struct Select *select; > + if (sql_view_compile(db, table->def->opts.sql, &select) != 0) { > + diag_log(); 14. You should not log each error. sql_view_column_names can be called by the parser only, so user will receive the error anyway. Log is needed when 1) the error is not logged itself, see LoggedError, 2) when it emerges with no possibility to be raised up to a user. > /** > * Remove entries from the _sql_stat1 and _sql_stat4 > @@ -2283,7 +2150,6 @@ static void > sql_code_drop_table(struct Parse *parse_context, struct space *space, > bool is_view) > { > - struct sqlite3 *db = parse_context->db; > struct Vdbe *v = sqlite3GetVdbe(parse_context); > assert(v != NULL); > /* > @@ -2313,6 +2179,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space, > int space_id_reg = ++parse_context->nMem; > int space_id = space->def->id; > sqlite3VdbeAddOp2(v, OP_Integer, space_id, space_id_reg); > + sqlite3VdbeAddOp1(v, OP_CheckViewReferences, space_id_reg); 15. Why do you need separate opcode here? As far as I can understand you can rely on Tarantool core. SQL can just delete the view from _space, and in alter.cc the refs will be checked. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 2aa35a114..0b64aee74 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -228,6 +225,43 @@ findRightmost(Select * p) > return p; > } > > +/** > + * 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) > +{ 16. Out of 80 below, and declaration fits in one line. Please, consider this diff: diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 0b64aee74..7cec4baa2 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -234,20 +234,19 @@ findRightmost(Select * p) * @param sub_select sub-select of current level recursion. */ static void -expand_names_sub_select(struct Select *top_select, - struct Select *sub_select) +expand_names_sub_select(struct Select *top_select, struct Select *sub_select) { assert(top_select != NULL); assert(sub_select != NULL); - for (int i = 0; i < sub_select->pSrc->nSrc; ++i) { - if (sub_select->pSrc->a[i].zName == NULL) { - expand_names_sub_select(top_select, - sub_select->pSrc->a[i].pSelect); + 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 = sql_src_list_append_unique(sql_get(), top_select->pSrc, - sub_select->pSrc->a[i].zName); + sub_src->zName); } } } > +void > +sql_select_expand_from_tables(struct Select *select) > +{ > + assert(select != NULL); > + for (int i = 0; i < select->pSrc->nSrc; ++i) > + if (select->pSrc->a[i].zName == NULL) > + expand_names_sub_select(select, > + select->pSrc->a[i].pSelect); > +} 17. Multi-line 'if' and 'for' must have {}: @@ -256,10 +255,11 @@ void sql_select_expand_from_tables(struct Select *select) { assert(select != NULL); - for (int i = 0; i < select->pSrc->nSrc; ++i) - if (select->pSrc->a[i].zName == NULL) - expand_names_sub_select(select, - select->pSrc->a[i].pSelect); + struct SrcList_item *src = select->pSrc->a; + for (int i = 0; i < select->pSrc->nSrc; ++i, ++src) { + if (src->zName == NULL) + expand_names_sub_select(select, src->pSelect); + } } > @@ -4738,13 +4772,16 @@ selectExpander(Walker * pWalker, Select * p) > } > #if !defined(SQLITE_OMIT_VIEW) > if (space_is_view(pTab)) { > - if (sql_view_column_names(pParse, pTab) != 0) > + struct Select *select; > + if (sql_view_compile(db, pTab->def->opts.sql, > + &select) != 0) { > + diag_log(); > return WRC_Abort; > + } > + sqlite3SrcListAssignCursors(pParse, > + select->pSrc); 18. Looks exactly like sql_view_column_names(). Why have not you used it? > assert(pFrom->pSelect == 0); > - assert(pTab->def->opts.is_view == > - (pTab->pSelect != NULL)); > - pFrom->pSelect = > - sqlite3SelectDup(db, pTab->pSelect, 0); > + pFrom->pSelect = select; > sqlite3SelectSetName(pFrom->pSelect, > pTab->def->name); > int columns = pTab->def->field_count; > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 01351a183..781f5d0a0 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2962,6 +2959,8 @@ struct Parse { > bool parse_only; > /** If parse_only is set to true, store parsed expression. */ > struct Expr *parsed_expr; > + /** If parse_only is set to true, store parsed SELECT. */ > + struct Select *parsed_select; 19. Is it possible to make union of parsed_expr, parsed_select? Like this: union { struct Expr *expr; struct Select *select; } ast; > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 42c70a255..25d8dc59b 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -566,3 +566,21 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len, > sql_parser_destroy(&parser); > return 0; > } > + > +int > +sql_view_compile(struct sqlite3 *db, const char *view_stmt, > + struct Select **select) > +{ > + assert(select != NULL); > + struct Parse parser; > + sql_parser_create(&parser, db); > + parser.parse_only = true; > + char *unused; > + if (sqlite3RunParser(&parser, view_stmt, &unused) != SQLITE_OK) { > + diag_set(ClientError, ER_SQL_EXECUTE, view_stmt); 20. ER_SQL_EXECUTE expects parser error message in the argument, it is not? SQL statement in error looks useless. I see the fact of an error, but can not understand what is wrong. > + return -1; 21. Must not you destroy the parser regardless of result? > + } > + *select = parser.parsed_select; > + sql_parser_destroy(&parser); > + return 0; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index ea3521133..32f40044f 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -758,7 +758,7 @@ codeTriggerProgram(Parse * pParse, /* The parser context */ > sqlite3SelectDup(db, pStep->pSelect, 0); > sqlite3SelectDestInit(&sDest, SRT_Discard, 0); > sqlite3Select(pParse, pSelect, &sDest); > - sqlite3SelectDelete(db, pSelect); > + sql_select_delete(db, pSelect); 22. Something is wrong with indentation. > break; > } > } >