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 966702672F for ; Fri, 15 Jun 2018 12:21:38 -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 PkQ8pBy3ErUF for ; Fri, 15 Jun 2018 12:21:38 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 5397A26723 for ; Fri, 15 Jun 2018 12:21:38 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server References: <45ff716a-8295-50d8-f317-cc28f9d0dba4@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Fri, 15 Jun 2018 19:21:35 +0300 MIME-Version: 1.0 In-Reply-To: <45ff716a-8295-50d8-f317-cc28f9d0dba4@tarantool.org> 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" > 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. No, there is no problem. Maybe "old_trigger" is a little confusing name, but on_rollback->data = new_trigger; for insert and it is not NULL. New ErrInjection test validates this case. --- a/test/sql/errinj.test.lua +++ b/test/sql/errinj.test.lua @@ -44,3 +44,18 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false) box.sql.execute('DROP TABLE test') box.schema.user.revoke('guest', 'read,write,execute', 'universe') + + +-- 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.error.injection.set("ERRINJ_WAL_IO", true) +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;") +box.sql.execute("CREATE INDEX t1a ON t1(a);") +box.error.injection.set("ERRINJ_WAL_IO", false) +box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;") +box.sql.execute("INSERT INTO t1 VALUES (3, 3);") +box.sql.execute("SELECT * from t1"); +box.sql.execute("SELECT * from t2"); +box.sql.execute("DROP TABLE t1;") +box.sql.execute("DROP TABLE t2;") > >> + (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? Yes, it is. But old_trigger is NULL in this case and this comment is a legacy, there was a if-clause. - /* DELETE, REPLACE trigger. */ > >> + sql_trigger_delete(sql_get(), old_trigger); >> +} > > 3. Please, add comments to both on_replace_trigger functions. Similar to on_rollback_dd_sequence comments: +/** + * Trigger invoked on rollback in the _trigger space. + */ static void on_replace_trigger_rollback(struct trigger *trigger, void *event) +/** + * Trigger invoked on commit in the _trigger space. + */ static void on_replace_trigger_commit(struct trigger *trigger, void * /* event */) > 4. If this function raises, new_trigger will leak. Ok, I believe that now it is a time to use guard that was rejected earlier. It is common for alter.cc. @@ -3197,11 +3202,14 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) if (new_trigger == NULL) diag_raise(); + auto new_trigger_guard = make_scoped_guard([=] { + sql_trigger_delete(sql_get(), new_trigger); + }); + 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"); @@ -3210,7 +3218,6 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) tuple_field_u32_xc(new_tuple, BOX_TRIGGER_FIELD_SPACE_ID); 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"); @@ -3219,12 +3226,12 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event) 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; + new_trigger_guard.is_active = false; } txn_on_rollback(txn, on_rollback); > Good. Lets use the same way for parsed_expr as I mentioned in one of > the previous letters. Done in previous commit. > 5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you > ignore it and takes pNewTrigger. You should not. - if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK && - parser.rc != SQL_TARANTOOL_ERROR) { + if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK) { + if (parser.rc != SQL_TARANTOOL_ERROR) diag_set(ClientError, ER_SQL, sql_error); > 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(). @@ -4713,6 +4713,8 @@ case OP_RenameTable: { space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1); space = space_by_id(space_id); assert(space); + /* Rename space op doesn't change triggers. */ + struct Trigger *triggers = space->sql_triggers; zOldTableName = space_name(space); assert(zOldTableName); pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName); @@ -4757,16 +4759,13 @@ case OP_RenameTable: { goto abort_due_to_error; } - space = space_by_id(space_id); - assert(space != NULL); - for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) { + for (struct Trigger *trigger = triggers; trigger != NULL; ) { /* Store pointer as trigger will be destructed. */ struct Trigger *next_trigger = trigger->pNext; rc = tarantoolSqlite3RenameTrigger(trigger->zName, > 7. Why do you need this filter, if no one of errors > emerged from a lua file? Just copied from template. > 8. Same as point 19 on the previous review. Ok, let drop it. It is not really important test. > 9. Please, replace 'tuple trigger' with just 'trigger' in all new > error messages. Ok. > 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. Ok. > 11. Error injections are disabled in Release build. Please, move this test > case into a separate file. We have test/sql/errinj.test.lua. Moved to test/sql/errinj.test.lua