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

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See 3 comments below.

On 15/06/2018 19:21, Kirill Shcherbatov wrote:
>> 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')
> +
> +

1. Please, start a sentence with capital letter and finish
with dot.

2. Put here link to the issue of the form:
--
-- gh-####: description.
--

3. I have pushed some fixes in a separate commit. Please,
look and squash. After the patch will be LGTM.

> +-- 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;")
> 
>