[tarantool-patches] Re: [PATCH v2 10/11] sql: move Triggers to server

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

Thanks for the patch! See 19 minor comments below.

On 09/06/2018 12: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                      | 124 +++++++++++++++
>   src/box/errcode.h                     |   2 +-
>   src/box/lua/schema.lua                |   6 +
>   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/sqliteInt.h               |   5 -
>   src/box/sql/tokenize.c                |  29 +++-
>   src/box/sql/trigger.c                 | 281 +++++++++++++++++-----------------
>   src/box/sql/vdbe.c                    |  76 ++-------
>   src/box/sql/vdbe.h                    |   1 -
>   src/box/sql/vdbeaux.c                 |   9 --
>   test/box/misc.result                  |   1 +
>   test/sql-tap/identifier_case.test.lua |   4 +-
>   test/sql-tap/trigger1.test.lua        |  14 +-
>   test/sql/triggers.result              | 260 +++++++++++++++++++++++++++++++
>   test/sql/triggers.test.lua            |  94 ++++++++++++
>   21 files changed, 737 insertions(+), 281 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..c683a51 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
> +triggers_task_rollback(struct trigger *trigger, void *event)

1. On_replace_dd_trigger works when a single trigger is created, dropped
or updated. So 'triggers' here is incorrect. Lets name it
on_replace_trigger_rollback. And on_replace_trigger_commit the second
function.

> @@ -3100,6 +3147,83 @@ 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(triggers_task_rollback, NULL);
> +	struct trigger *on_commit =
> +		txn_alter_trigger_new(triggers_task_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, 0, &trigger_name_len);

2. Please, define separate enums for _trigger field numbers in
schema_def.h like it is done for other system spaces. I think, the
previous patch is the perfect place for it.

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

3. Incorrect alignment.

> +		(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, 0, &trigger_name_len);
> +
> +		const char *space_opts =
> +			tuple_field_with_type_xc(new_tuple, 2, 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 (strncmp(trigger_name_src, trigger_name,
> +			    trigger_name_len) != 0) {

4. When you worked on CHECKs, I found a bug with strncmp() usage. Please,
fix it here too. Strncmp considers two strings be equal even if their
lengths are different.
> +		bool is_insert = (new_tuple != NULL && old_tuple == NULL);
> +		assert(!is_insert || old_trigger == NULL);

5. What is the point of is_insert? If it is true, then old_trigger == NULL,
so the line below is the same as just on_commit->data = old_trigger. It is
not?

> +
> +		on_commit->data = is_insert ? NULL : old_trigger;
> +		on_rollback->data = new_trigger;
> +	}
> +
> +	txn_on_rollback(txn, on_rollback);
> +	txn_on_commit(txn, on_commit);
>   }
>   
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index dd5ce0a..4996565 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -471,6 +472,11 @@ box.schema.space.drop = function(space_id, space_name, opts)
>           -- Delete automatically generated sequence.
>           box.schema.sequence.drop(sequence_tuple[2])
>       end
> +    local triggers = _trigger.index["space_id"]:select({space_id})
> +    for i = #triggers, 1, -1 do
> +        local v = triggers[i]
> +        _trigger:delete{v[1]}
> +    end

6. Why not just

	for _, t in _trigger.index.space_id:pairs({space_id}) do
	    _trigger:delete({t.name})
	end

?

> diff --git a/src/box/space.c b/src/box/space.c
> index b370b7c..d2aeecf 100644
> --- a/src/box/space.c
> +++ b/src/box/space.c
> @@ -209,6 +209,11 @@ space_delete(struct space *space)
>   	trigger_destroy(&space->on_replace);
>   	trigger_destroy(&space->on_stmt_begin);
>   	space_def_delete(space->def);
> +	/*
> +	 * SQL Triggers should be deleted with on_replace_dd_trigger
> +	 * initiated in LUA schema:delete.

7. What is schema:delete?

> +	 */
> +	assert(space->sql_triggers == NULL);
>   	space->vtab->destroy(space);
>   }
>   
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 3f59fc8..71a74d6 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -42,7 +42,6 @@
>   
>   #include "say.h"
>   #include "sqliteInt.h"
> -#include "tarantoolInt.h"
>   
>   /* Character classes for tokenizing
>    *
> @@ -123,6 +122,7 @@ static const char sql_ascii_class[] = {
>    * the #include below.
>    */
>   #include "keywordhash.h"
> +#include "tarantoolInt.h"

8. This and the previous diff are unnecessary.

> @@ -575,3 +580,23 @@ 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) {
> +		char *error = tt_static_buf();
> +		snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
> +		diag_set(ClientError, ER_SQL, error);
> +		sqlite3DbFree(db, sql_error);

9. Same as in the reviews for previous patches. Why not just

	diag_set(ClientError, EQ_SQL, sql_error);

?

> +	} else {
> +		trigger = parser.pNewTrigger;

10. You can remove

	if (! parse_only)
		delete(new_trigger)

from parser_destroy if here will nullify pNewTrigger like
sqlite3FinishTrigger. Parser_destroy is called much more
frequently, so lets optimize it to avoid branching when
possible.

> +	}
> +	sql_parser_destroy(&parser);
> +	return trigger;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index a9c686f..1b569bc 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -81,104 +82,128 @@ sqlite3BeginTrigger(Parse * pParse,
> -	/* Do not create a trigger on a system table */
> -	if (sqlite3StrNICmp(pTab->def->name, "sqlite_", 7) == 0) {
> -		sqlite3ErrorMsg(pParse,
> -				"cannot create trigger on system table");
> +	const char *table_name = pTableName->a[0].zName;
> +	uint32_t space_id;
> +	if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
> +			   &space_id) != 0) {
> +		pParse->rc = SQL_TARANTOOL_ERROR;

11. Forgot nErr++.

> +		goto trigger_cleanup;
> +	}
> +	if (space_id == BOX_ID_NIL) {
> +		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
> +		pParse->rc = SQL_TARANTOOL_ERROR;

12. Same. Please, check in other places yourself.

>   		goto trigger_cleanup;
>   	}
>   
> @@ -567,34 +555,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
> -/*
> - * Remove a trigger from the hash tables of the sqlite* pointer.
> - */
> -void
> -sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
> +int
> +sql_trigger_replace(struct sqlite3 *db, const char *name,
> +		    struct Trigger *trigger, struct Trigger **old_trigger)
> +	struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger;
> +	assert(src_trigger != NULL);
> +	struct space *space =
> +		space_cache_find(src_trigger->space_id);

13. Fits in one line.

> @@ -633,22 +646,17 @@ sqlite3TriggersExist(Table * pTab,	/* The table the contains the triggers */
>       )
>   {
>   	int mask = 0;
> -	Trigger *pList = 0;
> -	Trigger *p;
> +	struct Trigger *trigger_list = NULL;
>   	struct session *user_session = current_session();
> -
> -	if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0) {
> -		pList = pTab->pTrigger;
> -	}
> -	for (p = pList; p; p = p->pNext) {
> -		if (p->op == op && checkColumnOverlap(p->pColumns, pChanges)) {
> +	if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
> +		trigger_list = space_trigger_list(pTab->def->id);
> +	for (struct Trigger *p = trigger_list; p; p = p->pNext) {
> +		if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
>   			mask |= p->tr_tm;
> -		}
>   	}
> -	if (pMask) {
> +	if (pMask != 0)

14. Please, apply.

diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 1b569bc7b..10294455a 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -650,13 +650,14 @@ sqlite3TriggersExist(Table * pTab,	/* The table the contains the triggers */
  	struct session *user_session = current_session();
  	if ((user_session->sql_flags & SQLITE_EnableTrigger) != 0)
  		trigger_list = space_trigger_list(pTab->def->id);
-	for (struct Trigger *p = trigger_list; p; p = p->pNext) {
-		if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
+	for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) {
+		if (p->op == op && checkColumnOverlap(p->pColumns,
+						      pChanges) != 0)
  			mask |= p->tr_tm;
  	}
-	if (pMask != 0)
+	if (pMask != NULL)
  		*pMask = mask;
-	return (mask != 0) ? trigger_list : 0;
+	return mask != 0 ? trigger_list : NULL;
  }
  
  /*

>   }
>   
>   /*
> @@ -837,7 +845,7 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
>   	Parse *pSubParse;	/* Parse context for sub-vdbe */
>   	int iEndTrigger = 0;	/* Label to jump to if WHEN is false */
>   
> -	assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
> +	assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id);

15. != NULL. Please, check in other places yourself.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..2d1538e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4799,19 +4757,21 @@ case OP_RenameTable: {
>   		goto abort_due_to_error;
>   	}
>   
> -	pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> -	pTab->pTrigger = pTrig;
> +	space = space_by_id(space_id);

16. space is already got in this opcode above.

> +	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; ) {
> +		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;
>   	}

17. Why not this?

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2d1538e24..98b7d95a0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4761,15 +4761,14 @@ case OP_RenameTable: {
  	assert(space != NULL);
  
  	/* Rename all triggers created on this table. */
-	for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
-		struct Trigger *next_trigger = trigger->pNext;
+	for (struct Trigger *trigger = space->sql_triggers; trigger != NULL;
+	     trigger = trigger->pNext) {
  		rc = tarantoolSqlite3RenameTrigger(trigger->zName,
  						   zOldTableName, zNewTableName);
  		if (rc != SQLITE_OK) {
  			sqlite3ResetAllSchemasOfConnection(db);
  			goto abort_due_to_error;
  		}
-		trigger = next_trigger;
  	}

> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> new file mode 100644
> index 0000000..d214962
> --- /dev/null
> +++ b/test/sql/triggers.result
> @@ -0,0 +1,260 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
> +---
> +- true
> +...
> +-- get invariant part of the tuple
> + function inmutable_part(data) local r = {} for i, l in pairs(data) do r[#r+1]={l[1], l[3]} end return r end

18. 'inmutable' does not exist. Maybe 'immutable'?

In Lua you have tuple field access by name and table.insert() to append a value. So
lets use them to make the code more understandable:

	for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) 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; ]])
> +---
> +...
> +inmutable_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;"}}
> +---
> +...
> +inmutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_trigger'

19. Your tests looks failing. Or is it ok? Then why do you need 'immutable_part' here,
if :insert() fails before the call?