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 AD476217AB for ; Mon, 25 Jun 2018 11:27:58 -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 v7xqjPuusV2q for ; Mon, 25 Jun 2018 11:27:58 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 3F6B2216FC for ; Mon, 25 Jun 2018 11:27:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server References: <8d28de228babb13b962c8ffed9399caf45586447.1529490955.git.kshcherbatov@tarantool.org> <4FD641D5-2133-4135-ADC6-72900826F458@tarantool.org> From: Kirill Shcherbatov Message-ID: <5704ba48-ea6b-4995-c405-4ea7c1ade87c@tarantool.org> Date: Mon, 25 Jun 2018 18:27:56 +0300 MIME-Version: 1.0 In-Reply-To: <4FD641D5-2133-4135-ADC6-72900826F458@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik@corp.mail.ru" > For instance, why do we still have to hold triggers in a separate hash? > To invoke triggers on a particular space you can refer to space->triggers. > Deletion of trigger occurs at VDBE runtime, so you just need to make sure > that _trigger contains record with given name. 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. Global DB hash has been kept as we still ned to get Trigger AST by name on trigger deletion. > Could you write more informative comment which would include information > concerning each action, i.e. why we delete tuple on insertion etc. /** * Trigger invoked on rollback in the _trigger space. + * Insert old sql_trigger AST stored in trigger->data, + * destruct new object in any. */ static void on_replace_trigger_rollback(struct trigger *trigger, void *event) @@ -3286,6 +3288,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event) /** * Trigger invoked on commit in the _trigger space. + * Drop useless old sql_trigger AST object if any. */ > E.g.: > > /* INSERT trigger. */ > int rc = sql_trigger_replace(sql_get(), > sql_trigger_name(old_trigger), > NULL, &new_trigger); > > It seems to be misleading since comment says ‘INSERT trigger’, > but instead you delete trigger from hash. > I understand that you mean ‘on rollback of insertion we must delete > already inserted in hash trigger’, but lets make it more clear. I've changed comment to contain "Rollback" word; /* Rollback DELETE trigger. */ /* Rollback INSERT trigger. */ /* Rollback REPLACE trigger. */ > >> +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; > > It is quite terrible: now we have struct Trigger and struct trigger, > which quite similar in their names. What do you think about renaming > struct Trigger to struct sql_trigger? Ok, done code refactoring in separate commit. > Why do we still hold string of ‘CREATE TRIGGER’ statement as a map? > AFAIU trigger can’t feature any functional elements except for mentioned one. Vlad already answered this in this thread question. > I would mention that this function must take valid space id. - * @param space_id Space ID. + * @param space_id valid Space ID. > I see that you have slightly refactored this function. I propose to make > an effort to completely refactor this function. Moreover, comment to this > function is no longer relevant: Ok, as a part on separate refactoring commit. > You are repeating these two lines 6 times across the code: > > I suggest to add another one exit label, which would perform exactly > these actions. +set_tarantool_error_and_cleanup: + pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; + goto trigger_cleanup; and so on. > >> + goto trigger_cleanup; >> + } >> + >> + /* Do not create a trigger on a system table. */ >> + if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) { > > I suggest to substitute this obsolete check with Tarantool’s one: > you can use space_is_system() function. - if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) { + if (space_is_system(space_id)) { > Well, personally I would move these checks (i.e. testing that trigger created on VIEW > must be , and trigger created on table can’t be ’) to > on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such > ‘Inconsistent’ triggers from Lua. Good idea, moved to sql_trigger_replace. I've also added some tests: +++ b/test/sql/triggers.test.lua @@ -71,3 +71,37 @@ box.sql.execute("DROP TABLE t1;") box.sql.execute("DROP TABLE t2;") immutable_part(box.space._trigger:select()) +-- Test target tables restricts. +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY,b);") +space_id = box.space.T1.id + +tuple = {"T1T", space_id, {sql = [[ +create trigger t1t instead of update on t1 for each row begin + delete from t1 WHERE a=old.a+2; +end;]]}} +box.space._trigger:insert(tuple) + +box.sql.execute("CREATE VIEW V1 AS SELECT * FROM t1;") +space_id = box.space.V1.id + +tuple = {"V1T", space_id, {sql = [[ +create trigger v1t before update on v1 for each row begin + delete from t1 WHERE a=old.a+2; +end;]]}} +box.space._trigger:insert(tuple) + +tuple = {"V1T", space_id, {sql = [[ +create trigger v1t AFTER update on v1 for each row begin + delete from t1 WHERE a=old.a+2; +end;]]}} +box.space._trigger:insert(tuple) + +space_id = box.space._sql_stat1.id +tuple = {"T1T", space_id, {sql = [[ +create trigger t1t instead of update on "_sql_stat1" for each row begin + delete from t1 WHERE a=old.a+2; +end;]]}} +box.space._trigger:insert(tuple) + +box.sql.execute("DROP VIEW V1;") +box.sql.execute("DROP TABLE T1;") > >> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger) > > Lets rename this function and fix comment for it. I suggest to call it vdbe_code_drop_trigger(), > vdbe_emit_drop_trigger() or whatever meaningful name you prefer. Ok, as a part of next refactoring commit. > I would give it a name like sql_trigger_hash_replace(). I can't agree. This function not only updates the cache, but also insert AST into space trigger list. > Are you sure that in a case of error we must delete all triggers from hash?Hm, can't remember, how I've wrote this code. Yes, a non-trivial place. Like this: if (rc != SQLITE_OK) { sqlite3CommitInternalChanges(); goto abort_due_to_error; } > I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch), > since triggers are always enabled in our SQL. Ok, as a part of next refactoring commit. > >> +... >> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua >> index 63d3063..f559099 100644 >> --- a/test/sql/errinj.test.lua >> +++ b/test/sql/errinj.test.lua >> @@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false) >> >> box.sql.execute('DROP TABLE test') >> box.schema.user.revoke('guest', 'read,write,execute', 'universe') >> + >> +---- >> +---- gh-3273: Move SQL TRIGGERs into server. >> +---- >> +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) > > I would also add the same tests with failed drop of trigger. Good idea, +box.error.injection.set("ERRINJ_WAL_IO", true) +box.sql.execute("DROP TRIGGER t1t;") +box.error.injection.set("ERRINJ_WAL_IO", false) +box.sql.execute("DELETE FROM t1;") +box.sql.execute("DELETE FROM t2;") +box.sql.execute("INSERT INTO t1 VALUES (3, 3);") +box.sql.execute("SELECT * from t1"); +box.sql.execute("SELECT * from t2"); > >> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua >> new file mode 100644 >> index 0000000..8e3f0c5 >> --- /dev/null >> +++ b/test/sql/triggers.test.lua >> @@ -0,0 +1,74 @@ >> +env = require('test_run') >> +test_run = env.new() >> + >> +-- get invariant part of the tuple > > Lets start comments with capital letter and end with dots. +-- Test triggers. .. so on. > Also, it doesn’t seem to be obvious what you mean by ‘invariant’. --- get invariant part of the tuple +-- Get invariant part of the tuple; name and opts don't change. > How could they mismatch? I guess there are a lot of tests which > check such things. -assert(box.space.T1.id == space_id)