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 2A4F226B1E for ; Mon, 4 Jun 2018 09:27:37 -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 TJ4ylG4c-zTK for ; Mon, 4 Jun 2018 09:27:37 -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 B42E026AB9 for ; Mon, 4 Jun 2018 09:27:36 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server References: <7820dee1-47ea-9dbb-c185-9be4be94a488@tarantool.org> <59f11ba9-27a5-5b09-09de-416881ba3b89@tarantool.org> From: Vladislav Shpilevoy Message-ID: <4816bb71-cf0a-f508-6709-d2a526c532b9@tarantool.org> Date: Mon, 4 Jun 2018 16:27:33 +0300 MIME-Version: 1.0 In-Reply-To: <59f11ba9-27a5-5b09-09de-416881ba3b89@tarantool.org> 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! At first, please, do not mess the comments. Try to respond on them in the same order as I had wrote. At second, please, do not skip them. At third, check the travis - it fails on box/misc test. See 22 comments below. Most of them are one-line minors. On 01/06/2018 23:25, Kirill Shcherbatov wrote: > From c0775801fcdb475ee6e77dff483f4f6a84c1afc3 Mon Sep 17 00:00:00 2001 > Message-Id: > From: Kirill Shcherbatov > Date: Wed, 30 May 2018 16:03:43 +0300 > Subject: [PATCH] sql: move Triggers to server 1. Please, do not send SMTP headers in the body. > > Introduced sql_triggers field in space structure. > Changed parser logic to do not insert built 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 | 37 ++++- > src/box/errcode.h | 2 +- > src/box/space.h | 2 + > src/box/sql.c | 48 ++---- > src/box/sql.h | 44 ++++++ > src/box/sql/build.c | 8 +- > src/box/sql/fkey.c | 1 - > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 6 +- > src/box/sql/tokenize.c | 38 ++++- > src/box/sql/trigger.c | 287 +++++++++++++++++----------------- > src/box/sql/vdbe.c | 76 ++------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > test/sql-tap/identifier_case.test.lua | 4 +- > test/sql-tap/trigger1.test.lua | 4 +- > test/sql/triggers.test.lua | 72 +++++++++ > 17 files changed, 370 insertions(+), 275 deletions(-) > create mode 100644 test/sql/triggers.test.lua > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index f2bf85d..bf170a5 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -551,6 +551,9 @@ space_swap_triggers(struct space *new_space, struct space *old_space) > rlist_swap(&new_space->before_replace, &old_space->before_replace); > rlist_swap(&new_space->on_replace, &old_space->on_replace); > rlist_swap(&new_space->on_stmt_begin, &old_space->on_stmt_begin); > + /** Copy SQL Triggers pointer. */ > + new_space->sql_triggers = old_space->sql_triggers; > + old_space->sql_triggers = NULL; 2. I just have noticed it is not actual swap. Here new_space->sql_triggers are forgotten. It leads to bug on rollback, when swap_triggers_is_called again to swap new/old back. > } > > /** > @@ -732,7 +735,6 @@ alter_space_commit(struct trigger *trigger, void *event) > } > > trigger_run_xc(&on_alter_space, alter->new_space); > - > alter->new_space = NULL; /* for alter_space_delete(). */ 3. Garbage diff. > /* > * Delete the old version of the space, we are not > @@ -3100,6 +3102,39 @@ 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); > + if (sql_trigger_delete_by_name(sql_get(), trigger_name_src, > + trigger_name_len) != 0) > + diag_raise(); > + } 4. You still have not fixed the point '3.' from the previous review. If WAL write fails, you loss the deleted trigger, but must restore it back. > + if (stmt->new_tuple != NULL) { > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(stmt->new_tuple, 0, > + &trigger_name_len); > + 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 = > + sql_trigger_compile(sql_get(), opts.sql); > + if (trigger == NULL) > + diag_raise(); > + auto trigger_guard = make_scoped_guard([=] { > + sql_trigger_delete(sql_get(), trigger); > + }); > + if (sql_trigger_insert(trigger, trigger_name_src, > + trigger_name_len) != 0) > + diag_raise(); > + trigger_guard.is_active = false; 5. Why do you need the guard here? You can delete trigger explicitly before diag_raise() if insert() != 0. > + } > } > > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index ecbd573..4c07480 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -1935,7 +1935,6 @@ struct Table { > #ifndef SQLITE_OMIT_ALTERTABLE > int addColOffset; /* Offset in CREATE TABLE stmt to add a new column */ > #endif > - Trigger *pTrigger; /* List of triggers stored in pSchema */ > Schema *pSchema; /* Schema that contains this table */ > Table *pNextZombie; /* Next on the Parse.pZombieTab list */ > /** Space definition with Tarantool metadata. */ > @@ -3032,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. */ 6. triggers? There is one trigger. And 'is refer to' is not correct sentence. 'Refer' is a verb. > + 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 9b65064..0c851c9 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" > @@ -122,6 +123,7 @@ static const char sql_ascii_class[] = { > * the #include below. > */ > #include "keywordhash.h" > +#include "tarantoolInt.h" > > #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0) > > @@ -510,8 +512,13 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > } > if (pParse->rc != SQLITE_OK && pParse->rc != SQLITE_DONE > && pParse->zErrMsg == 0) { > - pParse->zErrMsg = > - sqlite3MPrintf(db, "%s", sqlite3ErrStr(pParse->rc)); > + const char *error; > + if (is_tarantool_error(pParse->rc) && tarantoolErrorMessage()) { 7. != NULL > + error = tarantoolErrorMessage(); > + } else { > + error = sqlite3ErrStr(pParse->rc); > + } 8. Do not use {} when both 'if' and 'else' are one lines. > + pParse->zErrMsg = sqlite3MPrintf(db, "%s", error); > } > assert(pzErrMsg != 0); > if (pParse->zErrMsg) { > @@ -528,7 +535,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg) > > if (pParse->pWithToFree) > sqlite3WithDelete(db, pParse->pWithToFree); > - sql_trigger_delete(db, pParse->pNewTrigger); > + /* > + * Trigger is exporting with pNewTrigger field when 9. is exported. > + * parse_only flag is set. > + */ > + if (!pParse->parse_only) > + sql_trigger_delete(db, pParse->pNewTrigger); > sqlite3DbFree(db, pParse->pVList); > while (pParse->pZombieTab) { > Table *p = pParse->pZombieTab; > @@ -569,3 +581,23 @@ cleanup: > sql_parser_destroy(&parser); > return expression; > } > + > +struct Trigger * > +sql_trigger_compile(struct sqlite3 *db, const char *sql) > +{ > + struct Parse parser; > + sql_parser_create(&parser, db); > + parser.parse_only = true; > + 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); 10. Same as in the previous patch: error and sql_error are unused. > + goto cleanup; > + } > + trigger = parser.pNewTrigger; > +cleanup: 11. You should avoid 'goto cleanup' here. > + sql_parser_destroy(&parser); > + return trigger; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index 053dadb..beaad11 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -33,6 +33,8 @@ > * This file contains the implementation for TRIGGERs > */ > > +#include "box/box.h" 12. Unused. > +#include "box/schema.h" > #include "sqliteInt.h" > #include "tarantoolInt.h" > #include "vdbeInt.h" > @@ -81,102 +83,121 @@ sqlite3BeginTrigger(Parse * pParse, /* The parse context of the CREATE TRIGGER s > ) > { > Trigger *pTrigger = 0; /* The new trigger */ > - Table *pTab; /* Table that the trigger fires off of */ > + /* Table that the trigger fires off of. */ > + struct Table *table = NULL; > char *zName = 0; /* Name of the trigger */ > sqlite3 *db = pParse->db; /* The database connection */ > DbFixer sFix; /* State vector for the DB fixer */ > > - /* Do not account nested operations: the count of such > - * operations depends on Tarantool data dictionary internals, > - * such as data layout in system spaces. > + /* > + * Do not account nested operations: the count of such > + * operations depends on Tarantool data dictionary > + * internals, such as data layout in system spaces. > */ > if (!pParse->nested) { > Vdbe *v = sqlite3GetVdbe(pParse); > if (v != NULL) > sqlite3VdbeCountChanges(v); > } > - assert(pName != 0); /* pName->z might be NULL, but not pName itself */ > + /* pName->z might be NULL, but not pName itself. */ > + assert(pName != 0); > assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE); > assert(op > 0 && op < 0xff); > > - if (!pTableName || db->mallocFailed) { > + if (!pTableName || db->mallocFailed) > goto trigger_cleanup; > - } > > - /* Ensure the table name matches database name and that the table exists */ > + /* > + * 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)) { > + if (sqlite3FixSrcList(&sFix, pTableName)) 13. != 0 > > - /* INSTEAD OF triggers can only appear on views and BEFORE triggers > + /* > + * INSTEAD OF triggers can only appear on views and BEFORE triggers > * cannot appear on views. So we might as well translate every > * INSTEAD OF trigger into a BEFORE trigger. It simplifies code > * elsewhere. > */ > - if (tr_tm == TK_INSTEAD) { > + if (tr_tm == TK_INSTEAD) > tr_tm = TK_BEFORE; > - } > > - /* Build the Trigger object */ > + /* Build the Trigger object. */ > pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger)); > if (pTrigger == 0) > goto trigger_cleanup; > pTrigger->zName = zName; > zName = 0; > - pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName); > + /* Initialize space_id on trigger insert. */ > + if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName, > + strlen(pTableName->a[0].zName), 14. Wrong indentation. > + &pTrigger->space_id) != 0) { > + pParse->rc = SQL_TARANTOOL_ERROR; > + goto trigger_cleanup; > + } > pTrigger->pSchema = db->pSchema; > - pTrigger->pTabSchema = pTab->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)) 15. Same. > + goto trigger_cleanup; > assert(pParse->pNewTrigger == 0); > pParse->pNewTrigger = pTrigger; > > @@ -227,10 +248,11 @@ sqlite3FinishTrigger(Parse * pParse, /* Parser context */ > goto triggerfinish_cleanup; > } > > - /* if we are not initializing, > - * generate byte code to insert a new trigger into Tarantool. > + /* > + * Generate byte code to insert a new trigger into > + * Tarantool for non-parsig mode or export trigger. 16. typo > */ > - if (!db->init.busy) { > + if (!pParse->parse_only) { > Vdbe *v; > int zOptsSz; > Table *pSysTrigger; > @@ -565,34 +550,68 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger) > sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > > sqlite3ChangeCookie(pParse); > - sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, pTrigger->zName, > - 0); > } > } > > -/* > - * Remove a trigger from the hash tables of the sqlite* pointer. > - */ > -void > -sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName) > +int > +sql_trigger_delete_by_name(struct sqlite3 *db, const char *name, int name_len) > { > - Trigger *pTrigger; > - Hash *pHash; > - struct session *user_session = current_session(); > + /* Name from Tarantool tuple may be not normalized. */ > + uint32_t used = region_used(&fiber()->gc); > + char *trigger_name = region_alloc(&fiber()->gc, name_len + 3); 17. Why +3? > + if (trigger_name == NULL) { > + diag_set(OutOfMemory, name_len + 3, "region_alloc", > + "trigger_name"); > + return -1; > + } > + memcpy(trigger_name, name, name_len); > + trigger_name[name_len] = 0; > + > + struct Hash *hash = &(db->pSchema->trigHash); > + struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL); > + assert(trigger != NULL); 18. Why != NULL? Can't sqlite3HashInsert fail? > + > + 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)); 19. Can't it fit in a single line? > + *pp = (*pp)->pNext; > + } > > +int > +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len) > +{ > + if (strncmp(name, trigger->zName, name_len) != 0) { > + diag_set(ClientError, ER_SQL, > + "tuple trigger name does not match extracted from SQL"); 20. There are no tuples. Please, do this check in alter.cc. For this you can declare trigger_name() helper. > + return -1; > + } > + > + struct space *space = space_cache_find(trigger->space_id); > + assert(space != NULL); > + struct Hash *hash = &trigger->pSchema->trigHash; > + if (sqlite3HashFind(hash, trigger->zName) != NULL) { > + diag_set(ClientError, ER_TRIGGER_EXISTS, trigger->zName);> + return -1; > + } > + > /* > @@ -631,22 +650,17 @@ 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); > + for (struct Trigger *p = trigger_list; p; p = p->pNext) { > + if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) > mask |= p->tr_tm; > - } > } > - if (pMask) { > + if (pMask) 21. != NULL > *pMask = mask; > - } > - return (mask ? pList : 0); > + return mask ? trigger_list : 0; > } > > /* > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..bba9bf1 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4799,19 +4757,21 @@ case OP_RenameTable: { > goto abort_due_to_error; > } > > - pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName); > - pTab->pTrigger = pTrig; > + space = space_by_id(space_id); > + assert(space != NULL); > > - /* Rename all trigger created on this table.*/ > - for (; pTrig; pTrig = pTrig->pNext) { > - sqlite3DbFree(db, pTrig->table); > - pTrig->table = sqlite3DbStrNDup(db, zNewTableName, > - sqlite3Strlen30(zNewTableName)); > - pTrig->pTabSchema = pTab->pSchema; > - rc = tarantoolSqlite3RenameTrigger(pTrig->zName, > + /* Rename all trigger created on this table. */ 22. triggers > + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) { > + struct Trigger *next_trigger = trigger->pNext; > + rc = tarantoolSqlite3RenameTrigger(trigger->zName, > zOldTableName, zNewTableName); > - if (rc) goto abort_due_to_error; > + if (rc != SQLITE_OK) { > + sqlite3ResetAllSchemasOfConnection(db); > + goto abort_due_to_error; > + } > + trigger = next_trigger; > } > + > sqlite3DbFree(db, (void*)zOldTableName); > sqlite3DbFree(db, (void*)zSqlStmt); > break;