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 ADF6424C12 for ; Tue, 5 Jun 2018 09:31:49 -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 orGs_2mRHR4c for ; Tue, 5 Jun 2018 09:31:49 -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 3B88A244FB for ; Tue, 5 Jun 2018 09:31:49 -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> <4816bb71-cf0a-f508-6709-d2a526c532b9@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3480efcd-5dbb-0580-b119-ddf734c32d4f@tarantool.org> Date: Tue, 5 Jun 2018 16:31:47 +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 12 comments below. I have not reviewed the whole patch since you did not fix some of the previous comments, and I have found new bugs. > 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 | 140 ++++++++++++++++++ > src/box/errcode.h | 2 +- > src/box/space.h | 2 + > src/box/sql.c | 48 ++----- > src/box/sql.h | 42 ++++++ > src/box/sql/build.c | 8 +- > src/box/sql/fkey.c | 2 - > src/box/sql/insert.c | 6 +- > src/box/sql/sqliteInt.h | 7 +- > src/box/sql/tokenize.c | 28 +++- > src/box/sql/trigger.c | 259 ++++++++++++++++------------------ > 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 | 187 ++++++++++++++++++++++++ > test/sql/triggers.test.lua | 69 +++++++++ > 19 files changed, 629 insertions(+), 276 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..23cd4a3 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -551,6 +551,8 @@ 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; 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. 2. Space->sql_triggers leaks when I drop a space via Lua. 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);") > } > > /** > @@ -3091,6 +3093,55 @@ lock_before_dd(struct trigger *trigger, void *event) > latch_lock(&schema_lock); > } > > +static void > +triggers_task_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); 4. Fits in 3 lines. > + (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); > + } > +} > + > + 5. Extra new line. > +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) { 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. > + /* DELETE, REPLACE trigger. */ > + sql_trigger_delete(sql_get(), old_trigger); > + } > +} > + > @@ -3100,6 +3151,95 @@ 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); > + > + char *trigger_name = NULL; > + struct Trigger *new_trigger = NULL; 7. Please, do not create two branches for each case. You can make this function more compact. > + if (new_tuple != NULL) { > + /* 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, 1, MP_MAP); > + struct space_opts opts; > + struct region *region = &fiber()->gc; > + space_opts_decode(&opts, space_opts, region); > + new_trigger = sql_trigger_compile(sql_get(), opts.sql); > + if (new_trigger == NULL) > + diag_raise(); > + > + if (strncmp(trigger_name_src, sql_trigger_name(new_trigger), > + 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"); > + } > + trigger_name = sql_trigger_name(new_trigger); > + } > + > + if (old_tuple != NULL && new_tuple == NULL) { > + /* DELETE trigger. */ > + 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 if (new_tuple != NULL && old_tuple == NULL) { > + /* INSERT trigger. */ > + 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(); > + } > + assert(old_trigger == NULL); > + > + on_commit->data = NULL; > + on_rollback->data = new_trigger; > + } else { > + /* REPLACE trigger. */ > + 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(); > + } > + assert(old_trigger != NULL); > + > + on_commit->data = old_trigger; > + on_rollback->data = old_trigger; > + } > + > + txn_on_rollback(txn, on_rollback); > + txn_on_commit(txn, on_commit); > + > + 8. Two extra lines. > } > > diff --git a/src/box/sql.h b/src/box/sql.h > index 35aacc3..885f0a7 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > +/** > + * Get trigger name by trigger AST object. > + * @param trigger AST object. > + * @return trigger name string. > + */ > +char * > +sql_trigger_name(struct Trigger *trigger); 9. Const char *. > diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c > index 817a282..d769d14 100644 > --- a/src/box/sql/tokenize.c > +++ b/src/box/sql/tokenize.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include "box/schema.h" 10. Why? > > #include "say.h" > #include "sqliteInt.h" > @@ -123,6 +124,7 @@ static const char sql_ascii_class[] = { > * the #include below. > */ > #include "keywordhash.h" > +#include "tarantoolInt.h" 11. Same. > > #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0) > > @@ -575,3 +582,22 @@ 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, sql_error); 12. Still is not fixed from the previous review. 'Error' is unused. And why do you need snprintf here? Please, do another iteration of self-review before sending the patch again. Remove all unused includes, extra lines, unused variables, unnecessary calls, fix alignment etc.