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 2614925B1A for ; Wed, 13 Jun 2018 14:53:43 -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 LDvtrrG5NOgG for ; Wed, 13 Jun 2018 14:53:43 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 B5C64259A5 for ; Wed, 13 Jun 2018 14:53:42 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 13 Jun 2018 21:53:38 +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: tarantool-patches@freelists.org, Kirill Shcherbatov Thanks for the patch! See 19 minor comments below. On 09/06/2018 12:32, Kirill Shcherbatov wrote: > 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 | 124 +++++++++++++++ > src/box/errcode.h | 2 +- > src/box/lua/schema.lua | 6 + > src/box/space.c | 5 + > src/box/space.h | 2 + > src/box/sql.c | 39 ----- > src/box/sql.h | 50 ++++++ > src/box/sql/build.c | 8 +- > src/box/sql/fkey.c | 2 - > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 5 - > src/box/sql/tokenize.c | 29 +++- > src/box/sql/trigger.c | 281 +++++++++++++++++----------------- > src/box/sql/vdbe.c | 76 ++------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > test/box/misc.result | 1 + > test/sql-tap/identifier_case.test.lua | 4 +- > test/sql-tap/trigger1.test.lua | 14 +- > test/sql/triggers.result | 260 +++++++++++++++++++++++++++++++ > test/sql/triggers.test.lua | 94 ++++++++++++ > 21 files changed, 737 insertions(+), 281 deletions(-) > create mode 100644 test/sql/triggers.result > create mode 100644 test/sql/triggers.test.lua > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index f2bf85d..c683a51 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event) > latch_lock(&schema_lock); > } > > +static void > +triggers_task_rollback(struct trigger *trigger, void *event) 1. On_replace_dd_trigger works when a single trigger is created, dropped or updated. So 'triggers' here is incorrect. Lets name it on_replace_trigger_rollback. And on_replace_trigger_commit the second function. > @@ -3100,6 +3147,83 @@ 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); > + struct tuple *old_tuple = stmt->old_tuple; > + struct tuple *new_tuple = stmt->new_tuple; > + > + struct trigger *on_rollback = > + txn_alter_trigger_new(triggers_task_rollback, NULL); > + struct trigger *on_commit = > + txn_alter_trigger_new(triggers_task_commit, NULL); > + > + if (old_tuple != NULL && new_tuple == NULL) { > + /* DROP trigger. */ > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(old_tuple, 0, &trigger_name_len); 2. Please, define separate enums for _trigger field numbers in schema_def.h like it is done for other system spaces. I think, the previous patch is the perfect place for it. > + char *trigger_name = > + (char *)region_alloc_xc(&fiber()->gc, > + trigger_name_len + 1); > + memcpy(trigger_name, trigger_name_src, trigger_name_len); > + trigger_name[trigger_name_len] = 0; > + > + struct Trigger *old_trigger; > + int rc = sql_trigger_replace(sql_get(), trigger_name, NULL, > + &old_trigger); 3. Incorrect alignment. > + (void)rc; > + assert(rc == 0); > + assert(old_trigger != NULL); > + > + on_commit->data = old_trigger; > + on_rollback->data = old_trigger; > + } else { > + /* INSERT, REPLACE trigger. */ > + uint32_t trigger_name_len; > + const char *trigger_name_src = > + tuple_field_str_xc(new_tuple, 0, &trigger_name_len); > + > + const char *space_opts = > + tuple_field_with_type_xc(new_tuple, 2, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + struct Trigger *new_trigger = > + sql_trigger_compile(sql_get(), opts.sql); > + if (new_trigger == NULL) > + diag_raise(); > + > + const char *trigger_name = sql_trigger_name(new_trigger); > + if (strncmp(trigger_name_src, trigger_name, > + trigger_name_len) != 0) { 4. When you worked on CHECKs, I found a bug with strncmp() usage. Please, fix it here too. Strncmp considers two strings be equal even if their lengths are different. > + bool is_insert = (new_tuple != NULL && old_tuple == NULL); > + assert(!is_insert || old_trigger == NULL); 5. What is the point of is_insert? If it is true, then old_trigger == NULL, so the line below is the same as just on_commit->data = old_trigger. It is not? > + > + on_commit->data = is_insert ? NULL : old_trigger; > + on_rollback->data = new_trigger; > + } > + > + txn_on_rollback(txn, on_rollback); > + txn_on_commit(txn, on_commit); > } > > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index dd5ce0a..4996565 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -471,6 +472,11 @@ box.schema.space.drop = function(space_id, space_name, opts) > -- Delete automatically generated sequence. > box.schema.sequence.drop(sequence_tuple[2]) > end > + local triggers = _trigger.index["space_id"]:select({space_id}) > + for i = #triggers, 1, -1 do > + local v = triggers[i] > + _trigger:delete{v[1]} > + end 6. Why not just for _, t in _trigger.index.space_id:pairs({space_id}) do _trigger:delete({t.name}) end ? > diff --git a/src/box/space.c b/src/box/space.c > index b370b7c..d2aeecf 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -209,6 +209,11 @@ space_delete(struct space *space) > trigger_destroy(&space->on_replace); > trigger_destroy(&space->on_stmt_begin); > space_def_delete(space->def); > + /* > + * SQL Triggers should be deleted with on_replace_dd_trigger > + * initiated in LUA schema:delete. 7. What is schema:delete? > + */ > + assert(space->sql_triggers == NULL); > space->vtab->destroy(space); > } > > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 3f59fc8..71a74d6 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -42,7 +42,6 @@ > > #include "say.h" > #include "sqliteInt.h" > -#include "tarantoolInt.h" > > /* Character classes for tokenizing > * > @@ -123,6 +122,7 @@ static const char sql_ascii_class[] = { > * the #include below. > */ > #include "keywordhash.h" > +#include "tarantoolInt.h" 8. This and the previous diff are unnecessary. > @@ -575,3 +580,23 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len) > 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, error); > + sqlite3DbFree(db, sql_error); 9. Same as in the reviews for previous patches. Why not just diag_set(ClientError, EQ_SQL, sql_error); ? > + } else { > + trigger = parser.pNewTrigger; 10. You can remove if (! parse_only) delete(new_trigger) from parser_destroy if here will nullify pNewTrigger like sqlite3FinishTrigger. Parser_destroy is called much more frequently, so lets optimize it to avoid branching when possible. > + } > + sql_parser_destroy(&parser); > + return trigger; > +} > diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c > index a9c686f..1b569bc 100644 > --- a/src/box/sql/trigger.c > +++ b/src/box/sql/trigger.c > @@ -81,104 +82,128 @@ sqlite3BeginTrigger(Parse * pParse, > - /* Do not create a trigger on a system table */ > - if (sqlite3StrNICmp(pTab->def->name, "sqlite_", 7) == 0) { > - sqlite3ErrorMsg(pParse, > - "cannot create trigger on system table"); > + const char *table_name = pTableName->a[0].zName; > + uint32_t space_id; > + if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name), > + &space_id) != 0) { > + pParse->rc = SQL_TARANTOOL_ERROR; 11. Forgot nErr++. > + goto trigger_cleanup; > + } > + if (space_id == BOX_ID_NIL) { > + diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); > + pParse->rc = SQL_TARANTOOL_ERROR; 12. Same. Please, check in other places yourself. > goto trigger_cleanup; > } > > @@ -567,34 +555,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger) > -/* > - * Remove a trigger from the hash tables of the sqlite* pointer. > - */ > -void > -sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName) > +int > +sql_trigger_replace(struct sqlite3 *db, const char *name, > + struct Trigger *trigger, struct Trigger **old_trigger) > + struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger; > + assert(src_trigger != NULL); > + struct space *space = > + space_cache_find(src_trigger->space_id); 13. Fits in one line. > @@ -633,22 +646,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 != 0) 14. Please, apply. diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c index 1b569bc7b..10294455a 100644 --- a/src/box/sql/trigger.c +++ b/src/box/sql/trigger.c @@ -650,13 +650,14 @@ sqlite3TriggersExist(Table * pTab, /* The table the contains the triggers */ struct session *user_session = current_session(); 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)) + for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) { + if (p->op == op && checkColumnOverlap(p->pColumns, + pChanges) != 0) mask |= p->tr_tm; } - if (pMask != 0) + if (pMask != NULL) *pMask = mask; - return (mask != 0) ? trigger_list : 0; + return mask != 0 ? trigger_list : NULL; } /* > } > > /* > @@ -837,7 +845,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); 15. != NULL. Please, check in other places yourself. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..2d1538e 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); 16. space is already got in this opcode above. > + 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 triggers created on this table. */ > + 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; > } 17. Why not this? diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 2d1538e24..98b7d95a0 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -4761,15 +4761,14 @@ case OP_RenameTable: { assert(space != NULL); /* Rename all triggers created on this table. */ - for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) { - struct Trigger *next_trigger = trigger->pNext; + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; + trigger = trigger->pNext) { rc = tarantoolSqlite3RenameTrigger(trigger->zName, zOldTableName, zNewTableName); if (rc != SQLITE_OK) { sqlite3ResetAllSchemasOfConnection(db); goto abort_due_to_error; } - trigger = next_trigger; } > diff --git a/test/sql/triggers.result b/test/sql/triggers.result > new file mode 100644 > index 0000000..d214962 > --- /dev/null > +++ b/test/sql/triggers.result > @@ -0,0 +1,260 @@ > +env = require('test_run') > +--- > +... > +test_run = env.new() > +--- > +... > +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:: '") > +--- > +- true > +... > +-- get invariant part of the tuple > + function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end 18. 'inmutable' does not exist. Maybe 'immutable'? In Lua you have tuple field access by name and table.insert() to append a value. So lets use them to make the code more understandable: for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end > +--- > +... > +-- > +-- gh-3273: Move Triggers to server > +-- > +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; ]]) > +--- > +... > +inmutable_part(box.space._trigger:select()) > +--- > +- - - T1T > + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); > + END; '} > +... > +space_id = box.space._space.index["name"]:get('T1').id > +--- > +... > +-- checks for LUA tuples > +tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} > +--- > +... > +inmutable_part({box.space._trigger:insert(tuple)}) > +--- > +- error: Duplicate key exists in unique index 'primary' in space '_trigger' 19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here, if :insert() fails before the call?