[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 1. When you rollback insertion, that means you have no old_trigger.
> So sql_trigger_name(old_trigger) crashes. Please, add a test
> and fix the problem.
No, there is no problem. Maybe "old_trigger" is a little confusing name, but on_rollback->data = new_trigger; for insert and it is not NULL.
New ErrInjection test validates this case.

--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -44,3 +44,18 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
 
 box.sql.execute('DROP TABLE test')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
+
+-- 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.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)
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+box.sql.execute("SELECT * from t1");
+box.sql.execute("SELECT * from t2");
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")


> 
>> +		(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);
>> +	}
>> +}
>> +
>> +static void
>> +on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
>> +{
>> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
>> +	/* DELETE, REPLACE trigger. */
> 
> 2. Doesn't commit of insertion call this function?
Yes, it is. But old_trigger is NULL in this case and this comment is a legacy, there was a if-clause.
-       /* DELETE, REPLACE trigger. */

> 
>> +	sql_trigger_delete(sql_get(), old_trigger);
>> +}
> 
> 3. Please, add comments to both on_replace_trigger functions.
Similar to on_rollback_dd_sequence comments:
 
+/**
+ * Trigger invoked on rollback in the _trigger space.
+ */
 static void
 on_replace_trigger_rollback(struct trigger *trigger, void *event)

+/**
+ * Trigger invoked on commit in the _trigger space.
+ */
 static void
 on_replace_trigger_commit(struct trigger *trigger, void * /* event */)

> 4. If this function raises, new_trigger will leak.
Ok, I believe that now it is a time to use guard that was rejected earlier. It is common for alter.cc.

@@ -3197,11 +3202,14 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                if (new_trigger == NULL)
                        diag_raise();
 
+               auto new_trigger_guard = make_scoped_guard([=] {
+                   sql_trigger_delete(sql_get(), new_trigger);
+               });
+
                const char *trigger_name = sql_trigger_name(new_trigger);
                if (strlen(trigger_name) != trigger_name_len ||
                        memcmp(trigger_name_src, trigger_name,
                               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");
@@ -3210,7 +3218,6 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                        tuple_field_u32_xc(new_tuple,
                                           BOX_TRIGGER_FIELD_SPACE_ID);
                if (space_id != sql_trigger_space_id(new_trigger)) {
-                       sql_trigger_delete(sql_get(), new_trigger);
                        tnt_raise(ClientError, ER_SQL,
                                  "tuple space_id does not match the value "
                                  "resolved on AST building from SQL");
@@ -3219,12 +3226,12 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                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();
                }
 
                on_commit->data = old_trigger;
                on_rollback->data = new_trigger;
+               new_trigger_guard.is_active = false;
        }
 
        txn_on_rollback(txn, on_rollback);


> Good. Lets use the same way for parsed_expr as I mentioned in one of
> the previous letters.
Done in previous commit.

> 5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you
> ignore it and takes pNewTrigger. You should not.
-       if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
-           parser.rc != SQL_TARANTOOL_ERROR) {
+       if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK) {
+           if (parser.rc != SQL_TARANTOOL_ERROR)
                diag_set(ClientError, ER_SQL, sql_error);

> 6. You said that space * becomes invalid, but anyway its triggers are
> transferred with no changes, so you can save them into a local
> variable before space rename, and remove this space_by_id().
@@ -4713,6 +4713,8 @@ case OP_RenameTable: {
        space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1);
        space = space_by_id(space_id);
        assert(space);
+       /* Rename space op doesn't change triggers. */
+       struct Trigger *triggers = space->sql_triggers;
        zOldTableName = space_name(space);
        assert(zOldTableName);
        pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
@@ -4757,16 +4759,13 @@ case OP_RenameTable: {
                goto abort_due_to_error;
        }
 
-       space = space_by_id(space_id);
-       assert(space != NULL);

-       for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
+       for (struct Trigger *trigger = triggers; trigger != NULL; ) {
                /* Store pointer as trigger will be destructed. */
                struct Trigger *next_trigger = trigger->pNext;
                rc = tarantoolSqlite3RenameTrigger(trigger->zName,

> 7. Why do you need this filter, if no one of errors
> emerged from a lua file?
Just copied from template.

> 8. Same as point 19 on the previous review.
Ok, let drop it. It is not really important test.

> 9. Please, replace 'tuple trigger' with just 'trigger' in all new
> error messages.
Ok.

> 10. Instead of 'immutabling' result of insert you can just do
> 
>      _ = box.space._trigger:insert(tuple)
> 
> Anyway you are going to print that tuple from the next select(). Please
> do it in other places of insertions and deletions too.
Ok.
> 11. Error injections are disabled in Release build. Please, move this test
> case into a separate file. We have test/sql/errinj.test.lua.
Moved to test/sql/errinj.test.lua