[tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Jun 25 18:27:56 MSK 2018
> 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 <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) 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)
More information about the Tarantool-patches
mailing list