[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server
Kirill Shcherbatov
kshcherbatov at tarantool.org
Fri Jun 15 19:21:35 MSK 2018
> 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
More information about the Tarantool-patches
mailing list