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 DD43F261ED for ; Thu, 14 Jun 2018 15:27:26 -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 RvXscnDDrES9 for ; Thu, 14 Jun 2018 15:27:26 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 744ED25F69 for ; Thu, 14 Jun 2018 15:27:26 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server References: From: Vladislav Shpilevoy Message-ID: <45ff716a-8295-50d8-f317-cc28f9d0dba4@tarantool.org> Date: Thu, 14 Jun 2018 22:27:24 +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 fixes! See 11 comments below. Besides, I have pushed other review fixes as a separate commit. Please, look and squash. On 14/06/2018 20: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 | 128 +++++++++++++++ > src/box/errcode.h | 2 +- > src/box/lua/schema.lua | 4 + > 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/prepare.c | 1 + > src/box/sql/sqliteInt.h | 5 - > src/box/sql/tokenize.c | 20 ++- > src/box/sql/trigger.c | 282 +++++++++++++++++----------------- > src/box/sql/vdbe.c | 77 ++-------- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeaux.c | 9 -- > test/box/misc.result | 1 + > test/engine/iterator.result | 2 +- > test/sql-tap/identifier_case.test.lua | 4 +- > test/sql-tap/trigger1.test.lua | 14 +- > test/sql/triggers.result | 255 ++++++++++++++++++++++++++++++ > test/sql/triggers.test.lua | 92 +++++++++++ > 23 files changed, 731 insertions(+), 278 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..4fc46fc 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 > +on_replace_trigger_rollback(struct trigger *trigger, void *event) > +{ > + struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); > + struct Trigger *old_trigger = (struct Trigger *)trigger->data; > + struct Trigger *new_trigger; > + > + if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) { > + /* DELETE trigger. */ > + if (sql_trigger_replace(sql_get(), > + sql_trigger_name(old_trigger), > + old_trigger, &new_trigger) != 0) > + panic("Out of memory on insertion into trigger hash"); > + assert(new_trigger == NULL); > + } else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) { > + /* INSERT trigger. */ > + int rc = sql_trigger_replace(sql_get(), > + sql_trigger_name(old_trigger), > + NULL, &new_trigger); 1. When you rollback insertion, that means you have no old_trigger. So sql_trigger_name(old_trigger) crashes. Please, add a test and fix the problem. > + (void)rc; > + assert(rc == 0); > + assert(new_trigger == old_trigger); > + sql_trigger_delete(sql_get(), new_trigger); > + } else { > + /* REPLACE trigger. */ > + if (sql_trigger_replace(sql_get(), > + sql_trigger_name(old_trigger), > + old_trigger, &new_trigger) != 0) > + panic("Out of memory on insertion into trigger hash"); > + assert(old_trigger != new_trigger); > + > + sql_trigger_delete(sql_get(), new_trigger); > + } > +} > + > +static void > +on_replace_trigger_commit(struct trigger *trigger, void * /* event */) > +{ > + struct Trigger *old_trigger = (struct Trigger *)trigger->data; > + /* DELETE, REPLACE trigger. */ 2. Doesn't commit of insertion call this function? > + sql_trigger_delete(sql_get(), old_trigger); > +} 3. Please, add comments to both on_replace_trigger functions. > @@ -3100,6 +3147,87 @@ 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(on_replace_trigger_rollback, NULL); > + struct trigger *on_commit = > + txn_alter_trigger_new(on_replace_trigger_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, BOX_TRIGGER_FIELD_NAME, > + &trigger_name_len); > + 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); > + (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, BOX_TRIGGER_FIELD_NAME, > + &trigger_name_len); > + > + const char *space_opts = > + tuple_field_with_type_xc(new_tuple, > + BOX_TRIGGER_FIELD_OPTS, > + 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 (strlen(trigger_name) != trigger_name_len || > + memcmp(trigger_name_src, trigger_name, > + trigger_name_len) != 0) { > + sql_trigger_delete(sql_get(), new_trigger); > + tnt_raise(ClientError, ER_SQL, > + "tuple trigger name does not match extracted " > + "from SQL"); > + } > + uint32_t space_id = > + tuple_field_u32_xc(new_tuple, > + BOX_TRIGGER_FIELD_SPACE_ID); 4. If this function raises, new_trigger will leak. > + if (space_id != sql_trigger_space_id(new_trigger)) { > + sql_trigger_delete(sql_get(), new_trigger); > + tnt_raise(ClientError, ER_SQL, > + "tuple space_id does not match the value " > + "resolved on AST building from SQL"); > + } > + > + struct Trigger *old_trigger; > + if (sql_trigger_replace(sql_get(), trigger_name, new_trigger, > + &old_trigger) != 0) { > + sql_trigger_delete(sql_get(), new_trigger); > + diag_raise(); > + } > + > + on_commit->data = old_trigger; > + on_rollback->data = new_trigger; > + } > + > + txn_on_rollback(txn, on_rollback); > + txn_on_commit(txn, on_commit); > } > > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index e43fbcb..1fe6deb 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser) > } > parser->disableLookaside = 0; > sqlite3DbFree(db, parser->zErrMsg); > + sql_trigger_delete(db, parser->pNewTrigger); Good. Lets use the same way for parsed_expr as I mentioned in one of the previous letters. > region_destroy(&parser->region); > } > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index d22ce8c..e06cc0a 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -572,3 +571,22 @@ 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 && > + parser.rc != SQL_TARANTOOL_ERROR) { > + diag_set(ClientError, ER_SQL, sql_error); 5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you ignore it and takes pNewTrigger. You should not. > + } else { > + trigger = parser.pNewTrigger; > + parser.pNewTrigger = NULL; > + } > + sql_parser_destroy(&parser); > + return trigger; > +} > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3fe5875..b5f1f1c 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4799,19 +4757,22 @@ case OP_RenameTable: { > goto abort_due_to_error; > } > > - pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName); > - pTab->pTrigger = pTrig; > + space = space_by_id(space_id); 6. You said that space * becomes invalid, but anyway its triggers are transferred with no changes, so you can save them into a local variable before space rename, and remove this space_by_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 triggers created on this table. */ > + for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) { > + /* Store pointer as trigger will be destructed. */ > + 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; > diff --git a/test/sql/triggers.result b/test/sql/triggers.result > new file mode 100644 > index 0000000..eb7de6d > --- /dev/null > +++ b/test/sql/triggers.result > @@ -0,0 +1,255 @@ > +env = require('test_run') > +--- > +... > +test_run = env.new() > +--- > +... > +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:: '") 7. Why do you need this filter, if no one of errors emerged from a lua file? > +--- > +- true > +... > +-- get invariant part of the tuple > + function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r 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; ]]) > +--- > +... > +immutable_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;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) 8. Same as point 19 on the previous review. > +--- > +- error: Duplicate key exists in unique index 'primary' in space '_trigger' > +... > +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) > +--- > +- error: 'SQL error: tuple trigger name does not match extracted from SQL' 9. Please, replace 'tuple trigger' with just 'trigger' in all new error messages. > +... > +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) > +--- > +- error: 'SQL error: tuple trigger name does not match extracted from SQL' > +... > +tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) > +--- > +- error: 'SQL error: tuple space_id does not match the value resolved on AST building > + from SQL' > +... > +immutable_part(box.space._trigger:select()) > +--- > +- - - T1T > + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); > + END; '} > +... > +box.sql.execute("DROP TABLE T1;") > +--- > +... > +immutable_part(box.space._trigger:select()) > +--- > +- [] > +... > +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);") > +--- > +... > +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]]) > +--- > +... > +immutable_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 > +--- > +... > +-- test, didn't trigger t1t degrade > +box.sql.execute("INSERT INTO t1 VALUES(1);") > +--- > +... > +box.sql.execute("SELECT * FROM t2;") > +--- > +- - [1] > +... > +box.sql.execute("DELETE FROM t2;") > +--- > +... > +-- test triggers > +tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) > +--- > +- - - T2T > + - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); > + END;'} 10. Instead of 'immutabling' result of insert you can just do _ = box.space._trigger:insert(tuple) Anyway you are going to print that tuple from the next select(). Please do it in other places of insertions and deletions too. > +... > +tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}} > +--- > +... > +immutable_part({box.space._trigger:insert(tuple)}) > +--- > +- - - T3T > + - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); > + END;'} > +... > +immutable_part(box.space._trigger:select()) > +--- > +- - - T1T > + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); > + END; '} > + - - T2T > + - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); > + END;'} > + - - T3T > + - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); > + END;'} > +... > +box.sql.execute("INSERT INTO t1 VALUES(2);") > +--- > +... > +box.sql.execute("SELECT * FROM t2;") > +--- > +- - [1] > + - [2] > + - [3] > +... > +box.sql.execute("DELETE FROM t2;") > +--- > +... > +-- test t1t after t2t and t3t drop > +box.sql.execute("DROP TRIGGER T2T;") > +--- > +... > +immutable_part({box.space._trigger:delete("T3T")}) > +--- > +- - - T3T > + - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); > + END;'} > +... > +immutable_part(box.space._trigger:select()) > +--- > +- - - T1T > + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); > + END; '} > +... > +box.sql.execute("INSERT INTO t1 VALUES(3);") > +--- > +... > +box.sql.execute("SELECT * FROM t2;") > +--- > +- - [1] > +... > +box.sql.execute("DELETE FROM t2;") > +--- > +... > +-- insert new SQL t2t and t3t > +box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]]) > +--- > +... > +box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]]) > +--- > +... > +immutable_part(box.space._trigger:select()) > +--- > +- - - T1T > + - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); > + END; '} > + - - T2T > + - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); > + END; '} > + - - T3T > + - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); > + END; '} > +... > +box.sql.execute("INSERT INTO t1 VALUES(4);") > +--- > +... > +box.sql.execute("SELECT * FROM t2;") > +--- > +- - [1] > + - [2] > + - [3] > +... > +-- clean up > +box.sql.execute("DROP TABLE t1;") > +--- > +... > +box.sql.execute("DROP TABLE t2;") > +--- > +... > +immutable_part(box.space._trigger:select()) > +--- > +- [] > +... > +-- test crash on error.injection > +box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY, a INTEGER);"); > +--- > +... > +box.sql.execute("CREATE TABLE t2(id INTEGER PRIMARY KEY, a INTEGER);"); > +--- > +... > +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;") > +--- > +... > +box.error.injection.set("ERRINJ_WAL_IO", true) > +--- > +- ok > +... > +box.sql.execute("CREATE INDEX t1a ON t1(a);") > +--- > +- error: Failed to write to disk > +... > +box.error.injection.set("ERRINJ_WAL_IO", false) 11. Error injections are disabled in Release build. Please, move this test case into a separate file. We have test/sql/errinj.test.lua. > +--- > +- ok > +... > +box.sql.execute("INSERT INTO t1 VALUES (3, 3);") > +--- > +... > +box.sql.execute("SELECT * from t1"); > +--- > +- - [3, 3] > +... > +box.sql.execute("SELECT * from t2"); > +--- > +- - [1, 1] > +... > +box.sql.execute("DROP TABLE t1;") > +--- > +... > +box.sql.execute("DROP TABLE t2;") > +--- > +... > +test_run:cmd("clear filter") > +--- > +- true > +...