[tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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)