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 6A3B520FD7 for ; Sat, 9 Jun 2018 07:06:13 -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 xS_BwicOnYHy for ; Sat, 9 Jun 2018 07:06:13 -0400 (EDT) Received: from fallback.mail.ru (fallback5.mail.ru [94.100.181.253]) (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 19FAD20FE8 for ; Sat, 9 Jun 2018 07:06:13 -0400 (EDT) Received: from [10.161.100.15] (port=45572 helo=smtpng3.m.smailru.net) by fallback5.mail.ru with esmtp (envelope-from ) id 1fRaEO-00061q-7D for tarantool-patches@freelists.org; Sat, 09 Jun 2018 12:32:20 +0300 From: Kirill Shcherbatov 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> <4816bb71-cf0a-f508-6709-d2a526c532b9@tarantool.org> <3480efcd-5dbb-0580-b119-ddf734c32d4f@tarantool.org> Message-ID: Date: Sat, 9 Jun 2018 12:32:16 +0300 MIME-Version: 1.0 In-Reply-To: <3480efcd-5dbb-0580-b119-ddf734c32d4f@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" I'll send 2.0 patchset, > 1. It is not swap still. Swap of A and B means that before swap > A == value1, B = value2. After swap A == value2, B == value1. > Here you have done this: > A == B. > > Do you understand why exactly swap is needed here? Please, answer. > Do not skip the question. Yes, it is clear for me now: this function provides an ability to rollback. diff --git a/src/box/alter.cc b/src/box/alter.cc index 23cd4a3..03c2d91 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -552,7 +552,9 @@ space_swap_triggers(struct space *new_space, struct space *old_space) 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. */ + struct Trigger *old_value = new_space->sql_triggers; new_space->sql_triggers = old_space->sql_triggers; + old_space->sql_triggers = old_value; } > > 2. Space->sql_triggers leaks when I drop a space via Lua. I've reworked a tuple format in separate commit, I'll send a new patch set that includes it a bit later. 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 @@ -463,6 +463,7 @@ box.schema.space.drop = function(space_id, space_name, opts) check_param_table(opts, { if_exists = 'boolean' }) local _space = box.space[box.schema.SPACE_ID] local _index = box.space[box.schema.INDEX_ID] + local _trigger = box.space[box.schema.TRIGGER_ID] local _vindex = box.space[box.schema.VINDEX_ID] local _truncate = box.space[box.schema.TRUNCATE_ID] local _space_sequence = box.space[box.schema.SPACE_SEQUENCE_ID] @@ -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 local keys = _vindex:select(space_id) for i = #keys, 1, -1 do local v = keys[i] 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. + */ + assert(space->sql_triggers == NULL); space->vtab->destroy(space); } > > 3. After you fix the leak, try this test to simulate WAL error and you > will see a crash: > > box.cfg{} > 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) > box.sql.execute("CREATE INDEX t1a ON t1(a);") > box.error.injection.set("ERRINJ_WAL_IO", false) > box.sql.execute("INSERT INTO t1 VALUES (3, 3);") > There is no crash for now, I've included this test case: +-- 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) +--- +- 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;") > 4. Fits in 3 lines. + int rc = sql_trigger_replace(sql_get(), + sql_trigger_name(old_trigger), + NULL, &new_trigger); > 5. Extra new line. @@ -3131,7 +3130,6 @@ triggers_task_rollback(struct trigger *trigger, void *event) } } - > 6. Why do you need stmt and stmt->old_tuple here? As I see, > old_trigger != NULL is the same as stmt->old_tuple != NULL. static void triggers_task_commit(struct trigger *trigger, void *event) { - struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); struct Trigger *old_trigger = (struct Trigger *)trigger->data; - - if (stmt->old_tuple != NULL) { - /* DELETE, REPLACE trigger. */ - sql_trigger_delete(sql_get(), old_trigger); - } + /* DELETE, REPLACE trigger. */ + sql_trigger_delete(sql_get(), old_trigger); } > 7. Please, do not create two branches for each case. You can make this > function more compact. > 8. Two extra lines. > 9. Const char *. -char * +const char * sql_trigger_name(struct Trigger *trigger); > 10. Why? > 11. Same. Dropped. > 12. Still is not fixed from the previous review. 'Error' is unused. > And why do you need snprintf here? char *error = tt_static_buf(); snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error); diag_set(ClientError, ER_SQL, error); sqlite3DbFree(db, sql_error); I believe this should be like this. Done same in other places.