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 13A9624F73 for ; Thu, 31 May 2018 13:37:02 -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 Hmnyu1rWHTd4 for ; Thu, 31 May 2018 13:37:02 -0400 (EDT) Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 44B3624FBA for ; Thu, 31 May 2018 13:36:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 31 May 2018 20:36:53 +0300 MIME-Version: 1.0 In-Reply-To: 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: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! See 24 comments below. 1. /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] if (rc |= (node == NULL)) { ~~~^~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: place parentheses around the assignment to silence this warning if (rc |= (node == NULL)) { ^ ( ) /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: use '!=' to turn this compound assignment into an inequality comparison if (rc |= (node == NULL)) { ^~ != /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses] if (rc |= (node->name == NULL)) { ~~~^~~~~~~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: place parentheses around the assignment to silence this warning if (rc |= (node->name == NULL)) { ^ ( ) /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: use '!=' to turn this compound assignment into an inequality comparison if (rc |= (node->name == NULL)) { ^~ != On 31/05/2018 14:22, Kirill Shcherbatov wrote: > Introduced sql_triggers field in space structure. > Changed parser logic to do not insert builded triggers, just only > to do parsing. All triggers insertions and deletions are operated > via on_replace_dd_trigger now. > > Resolves #3273. > --- > src/box/alter.cc | 57 +++++++++++ > src/box/space.h | 2 + > src/box/sql.c | 48 ++------- > src/box/sql.h | 62 ++++++++++++ > src/box/sql/build.c | 8 +- > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 2 - > src/box/sql/tokenize.c | 43 +++++++- > src/box/sql/trigger.c | 258 ++++++++++++++++++++++++++++-------------------- > src/box/sql/vdbe.c | 58 +++-------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > 12 files changed, 343 insertions(+), 211 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index b62f8ad..6e4f15d 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -733,6 +737,20 @@ alter_space_commit(struct trigger *trigger, void *event) > > trigger_run_xc(&on_alter_space, alter->new_space); > > + if (alter->new_space->sql_triggers != NULL && > + strcmp(alter->new_space->def->name, > + alter->old_space->def->name) != 0) { > + /* > + * The function below either changes the name of > + * all triggers, or does not change any of them. > + * It should be last action in alter_space_commit > + * as it is difficult to guarantee its rollback. > + */ > + if (sql_trigger_list_alter_table_name_transactional( > + alter->new_space->sql_triggers, > + alter->new_space->def->name) != 0) 2. It does not work. box.cfg{} box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);") box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);") box.sql.execute([[ CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]]) box.space.T1:rename('T3') box.space._trigger:select{} I still see the trigger with the old table name. I do not think we should do in-memory non-persistent rename. Please, investigate what other databases do. > + diag_raise(); > + } > alter->new_space = NULL; /* for alter_space_delete(). */ > /* > * Delete the old version of the space, we are not > @@ -3100,6 +3118,45 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) > { > struct txn *txn = (struct txn *) event; > txn_check_singlestatement_xc(txn, "Space _trigger"); > + > + struct txn_stmt *stmt = txn_current_stmt(txn); > + if (stmt->old_tuple != NULL) { > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(stmt->old_tuple, 0, > + &trigger_name_len); > + /* Can't use static buff as TT_STATIC_BUF_LEN > + * is not enough for identifier max len test. */ > + char *trigger_name = > + (char *)region_alloc_xc(&fiber()->gc, trigger_name_len); > + sprintf(trigger_name, "%.*s", trigger_name_len, > + trigger_name_src); > + if (sql_trigger_delete_by_name(sql_get(), trigger_name) != 0) > + diag_raise(); 3. If WAL write fails, you could not rollback because the trigger is already deleted now. Please, implement on_rollback/on_commit triggers. You must delete the old trigger on commit only. > + } > + if (stmt->new_tuple != NULL) { > + const char *space_opts = > + tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + > + struct Trigger *trigger; > + if (sql_trigger_compile(sql_get(), opts.sql, &trigger) != 0) > + diag_raise(); > + assert(trigger != NULL); > + const char *table_name = > + sql_trigger_get_table_name(trigger); > + if (table_name == NULL) > + diag_raise(); > + uint32_t space_id = > + space_id_by_name(BOX_SPACE_ID, table_name, > + strlen(table_name)); > + if (space_id == BOX_ID_NIL) > + diag_raise(); > + if (sql_trigger_insert(space_id, trigger) != 0) > + diag_raise(); 4. On rollback you must delete this trigger. > + } > } > diff --git a/src/box/sql.h b/src/box/sql.h > index 23021e5..f21b745 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -66,6 +66,7 @@ struct Expr; > struct Parse; > struct Select; > struct Table; > +struct Trigger; > > /** > * Perform parsing of provided expression. This is done by > @@ -84,6 +85,67 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, > struct Expr **result); > > /** > + * Perform parsing of provided SQL request and construct trigger AST. > + * @param db SQL context handle. > + * @param sql request to parse. > + * @param[out] trigger Result: AST structure. > + * > + * @retval Error code if any. 5. More specifically? > + */ > +int > +sql_trigger_compile(struct sqlite3 *db, const char *sql, > + struct Trigger **trigger); 6. Why can not you just return Trigger *? Not NULL on success, NULL on error. It is a common practice. > + > +/** > + * Remove a trigger from the hash tables of the sqlite* pointer. > + * @param db SQL handle. > + * @param trigger_name to delete. 7. You delete not the trigger name, but the trigger object. > + * > + * @retval Error code if any. > + */ > +int > +sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name); > + > +/** > + * Get server triggers list by space_id. > + * @param space_id Space ID. > + * @retval NULL on error. 8. Are you sure? > + * @param not NULL on success. > + */ > +struct Trigger * > +space_trigger_list(uint32_t space_id); > + > +/** > + * Perform insert trigger in appropriate space. > + * @param space_id id of the space to append trigger. > + * @param trigger object to append. > + * > + * @retval Error code if any. > + */ > +int > +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger); 9. Why can not you use space *? In alter.cc you have it in txn_stmt. > + > +/** > + * Get table name of specified trigger. > + * @param trigger containing a table name. > + * @param new_table_name with new_name (optional). > + * @retval table name string. > + */ > +const char * > +sql_trigger_get_table_name(struct Trigger *trigger); 10. sql_trigger_table_name. On getter methods you may omit 'get'. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 59c61c7..023e6b0 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1766,9 +1766,9 @@ xferOptimization(Parse * pParse, /* Parser context */ > */ > return 0; > } > - if (pDest->pTrigger) { > - return 0; /* tab1 must not have triggers */ > - } > + /* The pDest must not have triggers. */ > + if (space_trigger_list(pDest->def->id) != NULL) 11. As far as I remember, in Table.def id now is always 0. It is not updated after insertion into _space, so you can not use it. Use tnum for a while. Or please make a patch, that updates def.id in the same place where tnum. > + return 0; > if (onError == ON_CONFLICT_ACTION_DEFAULT) { > if (pDest->iPKey >= 0) > onError = pDest->keyConf; > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 59df75f..24db578 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include "box/schema.h" > > #include "say.h" > #include "sqliteInt.h" > @@ -528,7 +529,10 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > > if (pParse->pWithToFree) > sqlite3WithDelete(db, pParse->pWithToFree); > - sqlite3DeleteTrigger(db, pParse->pNewTrigger); > + /* Trigger is exporting with pNewTrigger field when > + * parse_only flag is set. */ 12. Please, obey Tarantool comment style. In other places too. I will not comment each explicitly from now. > + if (!pParse->parse_only) > + sqlite3DeleteTrigger(db, pParse->pNewTrigger); > sqlite3DbFree(db, pParse->pVList); > while (pParse->pZombieTab) { > Table *p = pParse->pZombieTab; > @@ -564,3 +568,40 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len, > sql_parser_destroy(&parser); > return 0; > } > + > +int > +sql_trigger_compile(struct sqlite3 *db, const char *sql, > + struct Trigger **trigger) > +{ > + struct Parse parser; > + sql_parser_create(&parser, db); > + parser.parse_only = true; > + char *unused; 13. Why have you skipped the error message? (Please, do not respond with just a diff. Try to answer on the questions. This and others.) > + if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) { > + diag_set(ClientError, ER_SQL_EXECUTE, sql); > + return -1; > + } > + *trigger = parser.pNewTrigger; > + sql_parser_destroy(&parser); > + return 0; > +} > + > + 14. Extra new line. > +int > +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger) > +{ > + struct space *space = space_cache_find(space_id); > + assert(space != NULL); > + struct Hash *pHash = &trigger->pSchema->trigHash; > + char *zName = trigger->zName; > + void *ret = sqlite3HashInsert(pHash, zName, trigger); > + if (ret != NULL) { > + /* Triggers couldn't present in hash. > + * So this is definitely a memory error. */ > + diag_set(OutOfMemory, 0, "sqlite3HashInsert", "hash"); > + return -1; > + } > + trigger->pNext = space->sql_triggers; > + space->sql_triggers = trigger; > + return 0; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index ea35211..00f161e 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -33,6 +33,9 @@ > * This file contains the implementation for TRIGGERs > */ > > +#include "box/box.h" > +#include "box/tuple.h" 15. Why do you need tuple.h? I removed it and nothing is changed. > +#include "box/schema.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > #include "vdbeInt.h" > @@ -99,60 +103,65 @@ sqlite3BeginTrigger(Parse * pParse > /* Ensure the table name matches database name and that the table exists */ > if (db->mallocFailed) > goto trigger_cleanup; > assert(pTableName->nSrc == 1); > sqlite3FixInit(&sFix, pParse, "trigger", pName); > - if (sqlite3FixSrcList(&sFix, pTableName)) { > - goto trigger_cleanup; > - } > - pTab = sql_list_lookup_table(pParse, pTableName); > - if (!pTab) { > + if (sqlite3FixSrcList(&sFix, pTableName)) > goto trigger_cleanup; > - } > > - /* Check that the trigger name is not reserved and that no trigger of the > - * specified name exists > - */ > zName = sqlite3NameFromToken(db, pName); > - if (!zName || SQLITE_OK != sqlite3CheckIdentifierName(pParse, zName)) { > + if (zName == NULL) > goto trigger_cleanup; > - } > - if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { > - if (!noErr) { > - sqlite3ErrorMsg(pParse, "trigger %s already exists", > - zName); > - } else { > - assert(!db->init.busy); > + > + /* FIXME: Move all checks in VDBE #3435. */ > + if (!pParse->parse_only) { > + if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK) > + goto trigger_cleanup; > + > + table = sql_list_lookup_table(pParse, pTableName); > + if (table == NULL) > + goto trigger_cleanup; > + > + if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { 16. Why do you need extra () after & ? > + if (!noErr) { > + sqlite3ErrorMsg(pParse, > + "trigger %s already exists", > + zName); > + } else { > + assert(!db->init.busy); > + } > + goto trigger_cleanup; > } > - goto trigger_cleanup; > @@ -170,13 +178,22 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s > goto trigger_cleanup; > pTrigger->zName = zName; > zName = 0; > - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); > + pTrigger->table = strdup(pTableName->a[0].zName); 17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'. Maybe it is the original SQLite bug. Can you please check? > + if (pTrigger->table == NULL) { > + diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup", > + "pTrigger->table"); > + pParse->rc = SQL_TARANTOOL_ERROR; > + goto trigger_cleanup; > + } > pTrigger->pSchema = db->pSchema; > - pTrigger->pTabSchema = pTab->pSchema; > + pTrigger->pTabSchema = db->pSchema; > pTrigger->op = (u8) op; > pTrigger->tr_tm = tr_tm == TK_BEFORE ? TRIGGER_BEFORE : TRIGGER_AFTER; > pTrigger->pWhen = sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE); > pTrigger->pColumns = sqlite3IdListDup(db, pColumns); > + if ((pWhen != NULL && pTrigger->pWhen == NULL) || > + (pColumns != NULL && pTrigger->pColumns == NULL)) > + goto trigger_cleanup; 18. Why? > @@ -634,22 +629,18 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */ > ) > { > int mask = 0; > - Trigger *pList = 0; > - Trigger *p; > + struct Trigger *trigger_list = NULL; > struct session *user_session = current_session(); > - > - if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) { > - pList = pTab->pTrigger; > - } > - for (p = pList; p; p = p->pNext) { > - if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) { > + if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) > + trigger_list = space_trigger_list(pTab->def->id); > + struct Trigger *p; > + for (p = trigger_list; p; p = p->pNext) { 19. Why not for (struct Trigger *p = ... ? > + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) > mask |= p->tr_tm; > - } > } > - if (pMask) { > + if (pMask) > *pMask = mask; > - } > - return (mask ? pList : 0); > + return (mask ? trigger_list : 0); 20. Extra (). > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..f60c122 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4693,36 +4693,7 @@ case OP_ParseSchema2: { > * in database P2 > */ > case OP_ParseSchema3: { 21. Why have not you deleted it? > - InitData initData; > - Mem *pRec; > - char zPgnoBuf[16]; > - char *argv[4] = {NULL, zPgnoBuf, NULL, NULL}; > - assert(db->pSchema != NULL); > - > - initData.db = db; > - initData.pzErrMsg = &p->zErrMsg; > - > - assert(db->init.busy==0); > - db->init.busy = 1; > - initData.rc = SQLITE_OK; > - assert(!db->mallocFailed); > - > - pRec = &aMem[pOp->p1]; > - argv[0] = pRec[0].z; > - argv[1] = "0"; > - argv[2] = pRec[1].z; > - sqlite3InitCallback(&initData, 3, argv, NULL); > - > - rc = initData.rc; > - db->init.busy = 0; > - > - if (rc) { > - sqlite3ResetAllSchemasOfConnection(db); > - if (rc==SQLITE_NOMEM) { > - goto no_mem; > - } > - goto abort_due_to_error; > - } > + unreachable(); > break; > } > @@ -4758,11 +4728,11 @@ case OP_RenameTable: { > assert(zOldTableName); > pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName); > assert(pTab); > - pTrig = pTab->pTrigger; > iRootPage = pTab->tnum; > zNewTableName = pOp->p4.z; > zOldTableName = sqlite3DbStrNDup(db, zOldTableName, > sqlite3Strlen30(zOldTableName)); > + 22. Garbage diff. > rc = tarantoolSqlite3RenameTable(pTab->tnum, zNewTableName, > &zSqlStmt); > if (rc) goto abort_due_to_error; > @@ -4866,7 +4838,7 @@ case OP_DropIndex: { > * schema consistent with what is on disk. > */ > case OP_DropTrigger: { > - sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z); > + unreachable(); 23. Same as 21. > break; > } > #ifndef SQLITE_OMIT_TRIGGER 24. Where are tests on triggers create/drop via Lua and direct _trigger manipulations?