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 2307B2909F for ; Fri, 1 Jun 2018 16:24:46 -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 CvluQBnJsnRP for ; Fri, 1 Jun 2018 16:24:46 -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 47FAF29093 for ; Fri, 1 Jun 2018 16:24:45 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server References: From: Kirill Shcherbatov Message-ID: <7820dee1-47ea-9dbb-c185-9be4be94a488@tarantool.org> Date: Fri, 1 Jun 2018 23:24:41 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 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 Cc: "v.shpilevoy@tarantool.org" You better take a lock to the new patch version at the branch. I'll send it with next message. > /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)) { > ^~ - if (rc |= (node == NULL)) { + if (node == NULL) { + rc = -1; > 2. It does not work. > I do not think we should do in-memory non-persistent rename. Please, > investigate what other databases do. SQLite3, MySQL allows rename table with triggers. > 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. > 4. On rollback you must delete this trigger. I've introduce space_id instead of table name in Trigger object. Diff for this explicit change is at the end of this message. >> + * @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.I followed sql_expr_compile example. But it is ok for me to refactor this function this way. diff --git a/src/box/alter.cc b/src/box/alter.cc index 63f253b..487a497 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -3141,8 +3141,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) 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) + struct Trigger *trigger = + sql_trigger_compile(sql_get(), opts.sql); + if (trigger == NULL) diag_raise(); assert(trigger != NULL); const char *table_name = diff --git a/src/box/sql.h b/src/box/sql.h index f21b745..bd542c0 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -88,13 +88,12 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len, * 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. + * @retval NULL on error + * @retval not NULL Trigger AST pointer on success. */ -int -sql_trigger_compile(struct sqlite3 *db, const char *sql, - struct Trigger **trigger); +struct Trigger * +sql_trigger_compile(struct sqlite3 *db, const char *sql); > 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.)I've fellow sql_expr_compile logic, made same behavior. @@ -577,12 +579,16 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql) struct Parse parser; sql_parser_create(&parser, db); parser.parse_only = true; - char *unused; - if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) { + char *sql_error; + struct Trigger *trigger = NULL; + if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK) { + char *error = tt_static_buf(); + snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error); diag_set(ClientError, ER_SQL_EXECUTE, sql); - return NULL; + goto cleanup; } - struct Trigger *trigger = parser.pNewTrigger; + trigger = parser.pNewTrigger; +cleanup: sql_parser_destroy(&parser); return trigger; } I'll also change sql_expr_compile function in separate patch to follow this convention. >> + * 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. - * @param trigger_name to delete. + * @param trigger_name a name of trigger object to delete. >> +/** >> + * Get server triggers list by space_id. >> + * @param space_id Space ID. >> + * @retval NULL on error. > > 8. Are you sure? - * @retval NULL on error. - * @param not NULL on success. + * @retval trigger AST list. >> +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. txn_stmt doesn't contain valid space pointer as it is defined by trigger. > 10. sql_trigger_table_name. On getter methods you may omit 'get'.-sql_trigger_get_table_name(struct Trigger *trigger) +sql_trigger_table_name(struct Trigger *trigger) >> + /* 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. I believe that it is updated in sqlite3EndTable, below the only location of updating tnum. >> + /* 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. - /* Trigger is exporting with pNewTrigger field when - * parse_only flag is set. */ + /* + * Trigger is exporting with pNewTrigger field when + * parse_only flag is set. + */ > 14. Extra new line. dropped. >> +#include "box/box.h" >> +#include "box/tuple.h" > 15. Why do you need tuple.h? I removed it and nothing is changed. -#include "box/tuple.h" > 16. Why do you need extra () after & ? - if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) { + if (sqlite3HashFind(&db->pSchema->trigHash, zName)) { > 17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'. > Maybe it is the original SQLite bug. Can you please check? triggerStepAllocate(sqlite3 * db, /* Database connection */ u8 op, /* Trigger opcode */ Token * pName /* The target name */ ) { TriggerStep *pTriggerStep = sqlite3DbMallocZero(db, sizeof(TriggerStep) + pName->n + 1); ... char *z = (char *)&pTriggerStep[1]; ... pTriggerStep->zTarget = z; Memory for name is located in same allocation that object located is. >> + if ((pWhen != NULL && pTrigger->pWhen == NULL) || >> + (pColumns != NULL && pTrigger->pColumns == NULL)) >> + goto trigger_cleanup; > > 18. Why? We have deep-cloned pWhen into pTrigger->pWhen and pColumns into pTrigger->pColumns; There could be memory allocation error, so we ensure that action have been finished correctly. >> + struct Trigger *p; >> + for (p = trigger_list; p; p = p->pNext) { > > 19. Why not for (struct Trigger *p = ... ? - struct Trigger *p; - for (p = trigger_list; p; p = p->pNext) { + for (struct Trigger *p = trigger_list; p; p = p->pNext) { >> + return (mask ? trigger_list : 0); > > 20. Extra (). - return (mask ? trigger_list : 0); + return mask ? trigger_list : 0; > >> 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? > 23. Same as 21. I've assumed that it could be useful in future or for debug. -/* Opcode: ParseSchema3 P1 * * * * - * Synopsis: name=r[P1] sql=r[P1+1] - * - * Create trigger named r[P1] w/ DDL SQL stored in r[P1+1] - * in database P2 - */ -case OP_ParseSchema3: { - unreachable(); - break; -} - -/* Opcode: DropTrigger P1 * * P4 * - * - * Remove the internal (in-memory) data structures that describe - * the trigger named P4 in database P1. This is called after a trigger - * is dropped from disk (using the Destroy opcode) in order to keep - * the internal representation of the - * schema consistent with what is on disk. - */ -case OP_DropTrigger: { - unreachable(); - break; -} >> zOldTableName = sqlite3DbStrNDup(db, zOldTableName, >> sqlite3Strlen30(zOldTableName)); >> + > > 22. Garbage diff. dropped. > 24. Where are tests on triggers create/drop via Lua and direct _trigger > manipulations? diff --git a/src/box/alter.cc b/src/box/alter.cc index d3f9d42..6d63f96 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -736,21 +736,6 @@ 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) - diag_raise(); - } alter->new_space = NULL; /* for alter_space_delete(). */ /* * Delete the old version of the space, we are not @@ -3140,21 +3125,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) struct space_opts opts; struct region *region = &fiber()->gc; space_opts_decode(&opts, space_opts, region); - struct Trigger *trigger = sql_trigger_compile(sql_get(), opts.sql); if (trigger == NULL) diag_raise(); - assert(trigger != NULL); - const char *table_name = - sql_trigger_table_name(trigger); - if (table_name == NULL) - diag_raise(); - uint32_t space_id; - if (schema_find_id(BOX_SPACE_ID, 2, table_name, - strlen(table_name), &space_id) != 0) - diag_raise(); - if (sql_trigger_insert(space_id, trigger) != 0) + if (sql_trigger_insert(trigger) != 0) diag_raise(); } } diff --git a/src/box/sql.h b/src/box/sql.h index e6f5a02..bc96b75 100644 --- a/src/box/sql.h +++ b/src/box/sql.h @@ -114,33 +114,12 @@ 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); - -/** - * 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_table_name(struct Trigger *trigger); - -/** - * Rename specified trigger list. - * @param trigger containing a table name. - * @param new_table_name with new_name (optional). - * - * @retval Error code if any. - */ -int -sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, - const char *new_table_name); +sql_trigger_insert(struct Trigger *trigger); /** * Store duplicate of a parsed expression into @a parser. diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index 70ebef8..d5585cf 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -1440,7 +1440,6 @@ fkActionTrigger(Parse * pParse, /* Parse context */ } pStep->pTrig = pTrigger; pTrigger->pSchema = pTab->pSchema; - pTrigger->pTabSchema = pTab->pSchema; pFKey->apTrigger[iAction] = pTrigger; pTrigger->op = (pChanges ? TK_UPDATE : TK_DELETE); } diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 36695e3..06930c7 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3031,14 +3031,14 @@ struct Parse { */ struct Trigger { char *zName; /* The name of the trigger */ - char *table; /* The table or view to which the trigger applies */ + /** The ID of space the triggers is refer to. */ + uint32_t space_id; u8 op; /* One of TK_DELETE, TK_UPDATE, TK_INSERT */ u8 tr_tm; /* One of TRIGGER_BEFORE, TRIGGER_AFTER */ Expr *pWhen; /* The WHEN clause of the expression (may be NULL) */ IdList *pColumns; /* If this is an UPDATE OF trigger, the is stored here */ Schema *pSchema; /* Schema containing the trigger */ - Schema *pTabSchema; /* Schema containing the table */ TriggerStep *step_list; /* Link list of trigger program steps */ Trigger *pNext; /* Next trigger associated with the table */ }; diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c index 2d9d29e..9d613de 100644 --- a/src/box/sql/tokenize.c +++ b/src/box/sql/tokenize.c @@ -597,13 +597,13 @@ cleanup: } int -sql_trigger_insert(uint32_t space_id, struct Trigger *trigger) +sql_trigger_insert(struct Trigger *trigger) { - struct space *space = space_cache_find(space_id); + struct space *space = space_cache_find(trigger->space_id); assert(space != NULL); struct Hash *pHash = &trigger->pSchema->trigHash; - char *zName = trigger->zName; - void *ret = sqlite3HashInsert(pHash, zName, trigger); + char *trigger_name = trigger->zName; + void *ret = sqlite3HashInsert(pHash, trigger_name, trigger); if (ret != NULL) { /* Triggers couldn't present in hash. * So this is definitely a memory error. */ diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index a808d3a..2fd81c0 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -177,15 +177,14 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s goto trigger_cleanup; pTrigger->zName = zName; zName = 0; - pTrigger->table = strdup(pTableName->a[0].zName); - if (pTrigger->table == NULL) { - diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup", - "pTrigger->table"); + /* Initialize space_id on trigger insert. */ + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, + strlen(pTableName->a[0].zName), + &pTrigger->space_id) != 0) { pParse->rc = SQL_TARANTOOL_ERROR; goto trigger_cleanup; } pTrigger->pSchema = db->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); @@ -470,7 +469,6 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger) return; sqlite3DeleteTriggerStep(db, pTrigger->step_list); sqlite3DbFree(db, pTrigger->zName); - free(pTrigger->table); sql_expr_delete(db, pTrigger->pWhen, false); sqlite3IdListDelete(db, pTrigger->pColumns); sqlite3DbFree(db, pTrigger); @@ -525,16 +523,6 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr) } /* - * Return a pointer to the Table structure for the table that a trigger - * is set on. - */ -static Table * -tableOfTrigger(Trigger * pTrigger) -{ - return sqlite3HashFind(&pTrigger->pTabSchema->tblHash, pTrigger->table); -} - -/* * Drop a trigger given a pointer to that trigger. */ void @@ -569,20 +557,14 @@ sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name) struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); assert(trigger != NULL); - if (trigger->pSchema == trigger->pTabSchema) { - uint32_t space_id; - if (schema_find_id(BOX_SPACE_ID, 2, trigger->table, - strlen(trigger->table), &space_id) != 0) - return -1; - struct space *space = space_by_id(space_id); - /* Space could be already deleted. */ - if (space != NULL) { - struct Trigger **pp; - for (pp = &space->sql_triggers; - *pp != trigger; - pp = &((*pp)->pNext)); - *pp = (*pp)->pNext; - } + struct space *space = space_by_id(trigger->space_id); + /* Space could be already deleted. */ + if (space != NULL) { + struct Trigger **pp; + for (pp = &space->sql_triggers; + *pp != trigger; + pp = &((*pp)->pNext)); + *pp = (*pp)->pNext; } sqlite3DeleteTrigger(db, trigger); @@ -825,7 +807,7 @@ codeRowTrigger(Parse * pParse, /* Current parse context */ Parse *pSubParse; /* Parse context for sub-vdbe */ int iEndTrigger = 0; /* Label to jump to if WHEN is false */ - assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger)); + assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id); assert(pTop->pVdbe); /* Allocate the TriggerPrg and SubProgram objects. To ensure that they @@ -942,7 +924,7 @@ getRowTrigger(Parse * pParse, /* Current parse context */ Parse *pRoot = sqlite3ParseToplevel(pParse); TriggerPrg *pPrg; - assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger)); + assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id); /* It may be that this trigger has already been coded (or is in the * process of being coded). If this is the case, then an entry with @@ -1067,14 +1049,7 @@ sqlite3CodeRowTrigger(Parse * pParse, /* Parse context */ assert((op == TK_UPDATE) == (pChanges != 0)); for (p = pTrigger; p; p = p->pNext) { - - /* Sanity checking: The schema for the trigger and for the table are - * always defined. The trigger must be in the same schema as the table - * or else it must be a TEMP trigger. - */ assert(p->pSchema != 0); - assert(p->pTabSchema != 0); - assert(p->pSchema == p->pTabSchema); /* Determine whether we should code this trigger */ if (p->op == op @@ -1142,57 +1117,4 @@ sqlite3TriggerColmask(Parse * pParse, /* Parse context */ return mask; } -const char * -sql_trigger_table_name(struct Trigger *trigger) -{ - return trigger->table; -} - -int -sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger, - const char *new_table_name) -{ - struct names_list_t { - char *name; - struct names_list_t *next; - } *names_list = NULL; - - int rc = 0; - for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { - struct names_list_t *node = - region_alloc(&fiber()->gc, sizeof(*node)); - if (node == NULL) { - rc = -1; - diag_set(OutOfMemory, sizeof(*node), "region_alloc", - "node"); - break; - } - node->name = strdup(new_table_name); - if (node->name == NULL) { - rc = -1; - diag_set(OutOfMemory, strlen(new_table_name), "strdup", - "node->name"); - break; - } - node->next = names_list; - names_list = node; - } - if (rc != 0) - goto error; - - for (struct Trigger *t = trigger; t != NULL; t = t->pNext) { - free(t->table); - t->table = names_list->name; - names_list = names_list->next; - } - return 0; - -error: - while (names_list != NULL) { - free(names_list->name); - names_list = names_list->next; - } - return -1; -} - #endif /* !defined(SQLITE_OMIT_TRIGGER) */