[tarantool-patches] Re: [PATCH v4 5/6] sql: move Triggers to server

[n.pettik]

> On 20 Jun 2018, at 13:46, Kirill Shcherbatov <kshcherbatov at tarantool.org> 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.
> 

This patch in fact introduces a lot of changes. I would devote much more
informative commit message to describe them. I had to investigate
a lot of things which could be explained in comments/commit message.

For instance, why do we still have to hold triggers in a separate hash?
To invoke triggers on a particular space you can refer to space->triggers.
Deletion of trigger occurs at VDBE runtime, so you just need to make sure
that _trigger contains record with given name.

> /**
> @@ -3244,6 +3248,53 @@ lock_before_dd(struct trigger *trigger, void *event)
> }
> 
> /**
> + * Trigger invoked on rollback in the _trigger space.
> + */

Could you write more informative comment which would include information
concerning each action, i.e. why we delete tuple on insertion etc.
E.g.:

/* INSERT trigger. */
int rc = sql_trigger_replace(sql_get(),
                          sql_trigger_name(old_trigger),
                          NULL, &new_trigger);

It seems to be misleading since comment says ‘INSERT trigger’, 
but instead you delete trigger from hash.
I understand that you mean ‘on rollback of insertion we must delete
already inserted in hash trigger’, but lets make it more clear.

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

It is quite terrible: now we have struct Trigger and struct trigger,
which quite similar in their names. What do you think about renaming
struct Trigger to struct sql_trigger? 

> +	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);
> +		(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);
> +	}
> +}
> +
> 
> @@ -3252,6 +3303,88 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
> {
> +		const char *space_opts =
> +			tuple_field_with_type_xc(new_tuple,
> +						 BOX_TRIGGER_FIELD_OPTS,
> +						 MP_MAP);

Why do we still hold string of ‘CREATE TRIGGER’ statement as a map?
AFAIU trigger can’t feature any functional elements except for mentioned one.

> +		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();
> 
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 90bba1a..7dc3f80 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -95,6 +95,17 @@ struct Select *
> sql_view_compile(struct sqlite3 *db, const char *view_stmt);
> 
> /**
> + * Perform parsing of provided SQL request and construct trigger AST.
> + * @param db SQL context handle.
> + * @param sql request to parse.
> + *
> + * @retval NULL on error
> + * @retval not NULL Trigger AST pointer on success.
> + */
> +struct Trigger *
> +sql_trigger_compile(struct sqlite3 *db, const char *sql);
> +
> +/**
>  * Free AST pointed by trigger.
>  * @param db SQL handle.
>  * @param trigger AST object.
> @@ -103,6 +114,45 @@ void
> sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
> 
> /**
> + * Get server triggers list by space_id.

I would mention that this function must take valid space id.

> + * @param space_id Space ID.
> + *
> + * @retval trigger AST list.
> + */
> +struct Trigger *
> +space_trigger_list(uint32_t space_id);
> +
> 
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 122c283..e4c936d 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -33,6 +33,7 @@
>  * This file contains the implementation for TRIGGERs
>  */
> 
> +#include "box/schema.h"
> #include "sqliteInt.h"
> #include "tarantoolInt.h"
> #include "vdbeInt.h"
> @@ -81,115 +82,141 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>     )
> {

I see that you have slightly refactored this function. I propose to make
an effort to completely refactor this function. Moreover, comment to this
function is no longer relevant:

>A Trigger
>* structure is generated based on the information available and stored
>* in pParse->pNewTrigger.

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

You are repeating these two lines 6 times across the code:

> +		pParse->rc = SQL_TARANTOOL_ERROR;
> +		pParse->nErr++;

I suggest to add another one exit label, which would perform exactly
these actions. 

> +		goto trigger_cleanup;
> +	}
> +
> +	/* Do not create a trigger on a system table. */
> +	if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {

I suggest to substitute this obsolete check with Tarantool’s one:
you can use space_is_system() function.

> +		diag_set(ClientError, ER_SQL,
> +			 "cannot create trigger on system table");
> +		pParse->rc = SQL_TARANTOOL_ERROR;
> +		pParse->nErr++;
> 		goto trigger_cleanup;
> 	}
> 
> -	/* INSTEAD of triggers are only for views and views only support INSTEAD
> -	 * of triggers.
> +	/*
> +	 * INSTEAD of triggers are only for views and
> +	 * views only support INSTEAD of triggers.
> 	 */
> -	if (pTab->def->opts.is_view && tr_tm != TK_INSTEAD) {
> -		sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
> -				(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
> -				pTableName, 0);

Well, personally I would move these checks (i.e. testing that trigger created on VIEW
must be <INSTEAD OF>, and trigger created on table can’t be <INSTEAD OF>’) to
on_replace_dd_trigger. Otherwise, you may catch severe errors after creation of such
‘Inconsistent’ triggers from Lua.

> @@ -566,34 +559,59 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)

Lets rename this function and fix comment for it. I suggest to call it vdbe_code_drop_trigger(),
vdbe_emit_drop_trigger() or whatever meaningful name you prefer.

> 			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> 
> 		sqlite3ChangeCookie(pParse);
> -		sqlite3VdbeAddOp4(v, OP_DropTrigger, 0, 0, 0, pTrigger->zName,
> -				  0);
> 	}
> }
> 
> -/*
> - * 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)

I would give it a name like sql_trigger_hash_replace().

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 5b5cf83..c1df880 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4710,46 +4710,6 @@ case OP_ParseSchema2: {
> 	break;
> }
> 
> /* Opcode: RenameTable P1 * * P4 *
>  * Synopsis: P1 = root, P4 = name
>  *
> @@ -4823,18 +4783,22 @@ case OP_RenameTable: {
> 		goto abort_due_to_error;
> 	}
> 
> -	pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> -	pTab->pTrigger = pTrig;
> -
> -	/* 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,
> +	/*
> +	 * Rebuild 'CREATE TRIGGER' expressions of all triggers
> +	 * created on this table. Sure, this action is not atomic
> +	 * due to lack of transactional DDL, but just do the best
> +	 * effort.
> +	 */
> +	for (struct Trigger *trigger = 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);

Are you sure that in a case of error we must delete all triggers from hash?

> +			goto abort_due_to_error;
> +		}
> +		trigger = next_trigger;
> 	}
> 	sqlite3DbFree(db, (void*)zOldTableName);
> 	sqlite3DbFree(db, (void*)zSqlStmt);
> @@ -4881,18 +4845,6 @@ case OP_DropIndex: {
> 	break;
> }
> 
> -/* Opcode: DropTrigger P1 * * P4 *
> - *
> - * Remove the internal (in-memory) data structures that describe
> - * the trigger named P4 in database P1.  This is called after a trigger
> - * is dropped from disk (using the Destroy opcode) in order to keep
> - * the internal representation of the
> - * schema consistent with what is on disk.
> - */
> -case OP_DropTrigger: {
> -	sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z);
> -	break;
> -}
> #ifndef SQLITE_OMIT_TRIGGER

I would also delete all mentions of SQLITE_OMIT_TRIGGER (within a separate patch),
since triggers are always enabled in our SQL.

> +...
> diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
> index 63d3063..f559099 100644
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -44,3 +44,19 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
> 
> box.sql.execute('DROP TABLE test')
> box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> +
> +----
> +---- gh-3273: Move SQL TRIGGERs into server.
> +----
> +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)

I would also add the same tests with failed drop of trigger.

> diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
> new file mode 100644
> index 0000000..8e3f0c5
> --- /dev/null
> +++ b/test/sql/triggers.test.lua
> @@ -0,0 +1,74 @@
> +env = require('test_run')
> +test_run = env.new()
> +
> +-- get invariant part of the tuple

Lets start comments with capital letter and end with dots.
Also, it doesn’t seem to be obvious what you mean by ‘invariant’.

> + 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())
> +
> +space_id = box.space._space.index["name"]:get('T1').id
> +assert(box.space.T1.id == space_id)

How could they mismatch? I guess there are a lot of tests which
check such things.