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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See 11 comments below.

Besides, I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> 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.
> 
> Resolves #3273.
> ---
>   src/box/alter.cc                      | 128 +++++++++++++++
>   src/box/errcode.h                     |   2 +-
>   src/box/lua/schema.lua                |   4 +
>   src/box/space.c                       |   5 +
>   src/box/space.h                       |   2 +
>   src/box/sql.c                         |  39 -----
>   src/box/sql.h                         |  50 ++++++
>   src/box/sql/build.c                   |   8 +-
>   src/box/sql/fkey.c                    |   2 -
>   src/box/sql/insert.c                  |   6 +-
>   src/box/sql/prepare.c                 |   1 +
>   src/box/sql/sqliteInt.h               |   5 -
>   src/box/sql/tokenize.c                |  20 ++-
>   src/box/sql/trigger.c                 | 282 +++++++++++++++++-----------------
>   src/box/sql/vdbe.c                    |  77 ++--------
>   src/box/sql/vdbe.h                    |   1 -
>   src/box/sql/vdbeaux.c                 |   9 --
>   test/box/misc.result                  |   1 +
>   test/engine/iterator.result           |   2 +-
>   test/sql-tap/identifier_case.test.lua |   4 +-
>   test/sql-tap/trigger1.test.lua        |  14 +-
>   test/sql/triggers.result              | 255 ++++++++++++++++++++++++++++++
>   test/sql/triggers.test.lua            |  92 +++++++++++
>   23 files changed, 731 insertions(+), 278 deletions(-)
>   create mode 100644 test/sql/triggers.result
>   create mode 100644 test/sql/triggers.test.lua
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index f2bf85d..4fc46fc 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
>   	latch_lock(&schema_lock);
>   }
>   
> +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;
> +	struct Trigger *new_trigger;
> +
> +	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
> +		/* DELETE 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(new_trigger == NULL);
> +	}  else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
> +		/* INSERT trigger. */
> +		int rc = sql_trigger_replace(sql_get(),
> +					     sql_trigger_name(old_trigger),
> +					     NULL, &new_trigger);

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.

> +		(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?

> +	sql_trigger_delete(sql_get(), old_trigger);
> +}

3. Please, add comments to both on_replace_trigger functions.

> @@ -3100,6 +3147,87 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
>   {
>   	struct txn *txn = (struct txn *) event;
>   	txn_check_singlestatement_xc(txn, "Space _trigger");
> +	struct txn_stmt *stmt = txn_current_stmt(txn);
> +	struct tuple *old_tuple = stmt->old_tuple;
> +	struct tuple *new_tuple = stmt->new_tuple;
> +
> +	struct trigger *on_rollback =
> +		txn_alter_trigger_new(on_replace_trigger_rollback, NULL);
> +	struct trigger *on_commit =
> +		txn_alter_trigger_new(on_replace_trigger_commit, NULL);
> +
> +	if (old_tuple != NULL && new_tuple == NULL) {
> +		/* DROP trigger. */
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
> +					   &trigger_name_len);
> +		char *trigger_name =
> +			(char *)region_alloc_xc(&fiber()->gc,
> +						trigger_name_len + 1);
> +		memcpy(trigger_name, trigger_name_src, trigger_name_len);
> +		trigger_name[trigger_name_len] = 0;
> +
> +		struct Trigger *old_trigger;
> +		int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
> +					     &old_trigger);
> +		(void)rc;
> +		assert(rc == 0);
> +		assert(old_trigger != NULL);
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = old_trigger;
> +	} else {
> +		/* INSERT, REPLACE trigger. */
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
> +					   &trigger_name_len);
> +
> +		const char *space_opts =
> +			tuple_field_with_type_xc(new_tuple,
> +						 BOX_TRIGGER_FIELD_OPTS,
> +						 MP_MAP);
> +		struct space_opts opts;
> +		struct region *region = &fiber()->gc;
> +		space_opts_decode(&opts, space_opts, region);
> +		struct Trigger *new_trigger =
> +			sql_trigger_compile(sql_get(), opts.sql);
> +		if (new_trigger == NULL)
> +			diag_raise();
> +
> +		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");
> +		}
> +		uint32_t space_id =
> +			tuple_field_u32_xc(new_tuple,
> +					   BOX_TRIGGER_FIELD_SPACE_ID);

4. If this function raises, new_trigger will leak.

> +		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");
> +		}
> +
> +		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;
> +	}
> +
> +	txn_on_rollback(txn, on_rollback);
> +	txn_on_commit(txn, on_commit);
>   }
>   
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e43fbcb..1fe6deb 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
>   	}
>   	parser->disableLookaside = 0;
>   	sqlite3DbFree(db, parser->zErrMsg);
> +	sql_trigger_delete(db, parser->pNewTrigger);

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

>   	region_destroy(&parser->region);
>   }
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index d22ce8c..e06cc0a 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -572,3 +571,22 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
>   	sql_parser_destroy(&parser);
>   	return expression;
>   }
> +
> +struct Trigger *
> +sql_trigger_compile(struct sqlite3 *db, const char *sql)
> +{
> +	struct Parse parser;
> +	sql_parser_create(&parser, db);
> +	parser.parse_only = true;
> +	char *sql_error;
> +	struct Trigger *trigger = NULL;
> +	if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
> +	    parser.rc != SQL_TARANTOOL_ERROR) {
> +		diag_set(ClientError, ER_SQL, sql_error);

5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you
ignore it and takes pNewTrigger. You should not.

> +	} else {
> +		trigger = parser.pNewTrigger;
> +		parser.pNewTrigger = NULL;
> +	}
> +	sql_parser_destroy(&parser);
> +	return trigger;
> +}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..b5f1f1c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4799,19 +4757,22 @@ case OP_RenameTable: {
>   		goto abort_due_to_error;
>   	}
>   
> -	pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> -	pTab->pTrigger = pTrig;
> +	space = space_by_id(space_id);

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

> +	assert(space != NULL);
>   
> -	/* Rename all trigger created on this table.*/
> -	for (; pTrig; pTrig = pTrig->pNext) {
> -		sqlite3DbFree(db, pTrig->table);
> -		pTrig->table = sqlite3DbStrNDup(db, zNewTableName,
> -						sqlite3Strlen30(zNewTableName));
> -		pTrig->pTabSchema = pTab->pSchema;
> -		rc = tarantoolSqlite3RenameTrigger(pTrig->zName,
> +	/* Rename all triggers created on this table. */
> +	for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
> +		/* Store pointer as trigger will be destructed. */
> +		struct Trigger *next_trigger = trigger->pNext;
> +		rc = tarantoolSqlite3RenameTrigger(trigger->zName,
>   						   zOldTableName, zNewTableName);
> -		if (rc) goto abort_due_to_error;
> +		if (rc != SQLITE_OK) {
> +			sqlite3ResetAllSchemasOfConnection(db);
> +			goto abort_due_to_error;
> +		}
> +		trigger = next_trigger;
>   	}
> +
>   	sqlite3DbFree(db, (void*)zOldTableName);
>   	sqlite3DbFree(db, (void*)zSqlStmt);
>   	break;
> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> new file mode 100644
> index 0000000..eb7de6d
> --- /dev/null
> +++ b/test/sql/triggers.result
> @@ -0,0 +1,255 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")

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

> +---
> +- true
> +...
> +-- get invariant part of the tuple
> + function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
> +---
> +...
> +--
> +-- gh-3273: Move Triggers to server
> +--
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +space_id = box.space._space.index["name"]:get('T1').id
> +---
> +...
> +-- checks for LUA tuples
> +tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})

8. Same as point 19 on the previous review.

> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_trigger'
> +...
> +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple trigger name does not match extracted from SQL'

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

> +...
> +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple trigger name does not match extracted from SQL'
> +...
> +tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple space_id does not match the value resolved on AST building
> +    from SQL'
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +box.sql.execute("DROP TABLE T1;")
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- []
> +...
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +space_id = box.space._space.index["name"]:get('T1').id
> +---
> +...
> +-- test, didn't trigger t1t degrade
> +box.sql.execute("INSERT INTO t1 VALUES(1);")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2;")
> +---
> +- - [1]
> +...
> +box.sql.execute("DELETE FROM t2;")
> +---
> +...
> +-- test triggers
> +tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- - - T2T
> +    - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
> +        END;'}

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.

> +...
> +tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END;'}
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +  - - T2T
> +    - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
> +        END;'}
> +  - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END;'}
> +...
> +box.sql.execute("INSERT INTO t1 VALUES(2);")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2;")
> +---
> +- - [1]
> +  - [2]
> +  - [3]
> +...
> +box.sql.execute("DELETE FROM t2;")
> +---
> +...
> +-- test t1t after t2t and t3t drop
> +box.sql.execute("DROP TRIGGER T2T;")
> +---
> +...
> +immutable_part({box.space._trigger:delete("T3T")})
> +---
> +- - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END;'}
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +box.sql.execute("INSERT INTO t1 VALUES(3);")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2;")
> +---
> +- - [1]
> +...
> +box.sql.execute("DELETE FROM t2;")
> +---
> +...
> +-- insert new SQL t2t and t3t
> +box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +  - - T2T
> +    - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
> +        END; '}
> +  - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END; '}
> +...
> +box.sql.execute("INSERT INTO t1 VALUES(4);")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2;")
> +---
> +- - [1]
> +  - [2]
> +  - [3]
> +...
> +-- clean up
> +box.sql.execute("DROP TABLE t1;")
> +---
> +...
> +box.sql.execute("DROP TABLE t2;")
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- []
> +...
> +-- 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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
> +---
> +...
> +box.error.injection.set("ERRINJ_WAL_IO", true)
> +---
> +- ok
> +...
> +box.sql.execute("CREATE INDEX t1a ON t1(a);")
> +---
> +- error: Failed to write to disk
> +...
> +box.error.injection.set("ERRINJ_WAL_IO", false)

11. Error injections are disabled in Release build. Please, move this test
case into a separate file. We have test/sql/errinj.test.lua.

> +---
> +- ok
> +...
> +box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
> +---
> +...
> +box.sql.execute("SELECT * from t1");
> +---
> +- - [3, 3]
> +...
> +box.sql.execute("SELECT * from t2");
> +---
> +- - [1, 1]
> +...
> +box.sql.execute("DROP TABLE t1;")
> +---
> +...
> +box.sql.execute("DROP TABLE t2;")
> +---
> +...
> +test_run:cmd("clear filter")
> +---
> +- true
> +...