[tarantool-patches] Re: [PATCH v1 4/4] sql: move Triggers to server

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

Thanks for the patch!

At first, please, do not mess the comments. Try to respond on them
in the same order as I had wrote.

At second, please, do not skip them.

At third, check the travis - it fails on box/misc test.

See 22 comments below. Most of them are one-line minors.

On 01/06/2018 23:25, Kirill Shcherbatov wrote:
>  From c0775801fcdb475ee6e77dff483f4f6a84c1afc3 Mon Sep 17 00:00:00 2001
> Message-Id: <c0775801fcdb475ee6e77dff483f4f6a84c1afc3.1527884700.git.kshcherbatov@gmail.com>
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Wed, 30 May 2018 16:03:43 +0300
> Subject: [PATCH] sql: move Triggers to server

1. Please, do not send SMTP headers in the body.

> 
> 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                      |  37 ++++-
>   src/box/errcode.h                     |   2 +-
>   src/box/space.h                       |   2 +
>   src/box/sql.c                         |  48 ++----
>   src/box/sql.h                         |  44 ++++++
>   src/box/sql/build.c                   |   8 +-
>   src/box/sql/fkey.c                    |   1 -
>   src/box/sql/insert.c                  |   6 +-
>   src/box/sql/sqliteInt.h               |   6 +-
>   src/box/sql/tokenize.c                |  38 ++++-
>   src/box/sql/trigger.c                 | 287 +++++++++++++++++-----------------
>   src/box/sql/vdbe.c                    |  76 ++-------
>   src/box/sql/vdbe.h                    |   1 -
>   src/box/sql/vdbeaux.c                 |   9 --
>   test/sql-tap/identifier_case.test.lua |   4 +-
>   test/sql-tap/trigger1.test.lua        |   4 +-
>   test/sql/triggers.test.lua            |  72 +++++++++
>   17 files changed, 370 insertions(+), 275 deletions(-)
>   create mode 100644 test/sql/triggers.test.lua
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index f2bf85d..bf170a5 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -551,6 +551,9 @@ space_swap_triggers(struct space *new_space, struct space *old_space)
>   	rlist_swap(&new_space->before_replace, &old_space->before_replace);
>   	rlist_swap(&new_space->on_replace, &old_space->on_replace);
>   	rlist_swap(&new_space->on_stmt_begin, &old_space->on_stmt_begin);
> +	/** Copy SQL Triggers pointer. */
> +	new_space->sql_triggers = old_space->sql_triggers;
> +	old_space->sql_triggers = NULL;

2. I just have noticed it is not actual swap. Here new_space->sql_triggers are
forgotten. It leads to bug on rollback, when swap_triggers_is_called again
to swap new/old back.

>   }
>   
>   /**
> @@ -732,7 +735,6 @@ alter_space_commit(struct trigger *trigger, void *event)
>   	}
>   
>   	trigger_run_xc(&on_alter_space, alter->new_space);
> -
>   	alter->new_space = NULL; /* for alter_space_delete(). */

3. Garbage diff.

>   	/*
>   	 * Delete the old version of the space, we are not
> @@ -3100,6 +3102,39 @@ 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);
> +	if (stmt->old_tuple != NULL) {
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(stmt->old_tuple, 0,
> +					   &trigger_name_len);
> +		if (sql_trigger_delete_by_name(sql_get(), trigger_name_src,
> +					       trigger_name_len) != 0)
> +			diag_raise();
> +	}

4. You still have not fixed the point '3.' from the previous review.
If WAL write fails, you loss the deleted trigger, but must restore it back.

> +	if (stmt->new_tuple != NULL) {
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(stmt->new_tuple, 0,
> +					   &trigger_name_len);
> +		const char *space_opts =
> +			tuple_field_with_type_xc(stmt->new_tuple, 1, MP_MAP);
> +		struct space_opts opts;
> +		struct region *region = &fiber()->gc;
> +		space_opts_decode(&opts, space_opts, region);
> +		struct Trigger *trigger =
> +			sql_trigger_compile(sql_get(), opts.sql);
> +		if (trigger == NULL)
> +			diag_raise();
> +		auto trigger_guard = make_scoped_guard([=] {
> +		    sql_trigger_delete(sql_get(), trigger);
> +		});
> +		if (sql_trigger_insert(trigger, trigger_name_src,
> +				       trigger_name_len) != 0)
> +			diag_raise();
> +		trigger_guard.is_active = false;

5. Why do you need the guard here? You can delete trigger explicitly before
diag_raise() if insert() != 0.

> +	}
>   }
>   
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index ecbd573..4c07480 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1935,7 +1935,6 @@ struct Table {
>   #ifndef SQLITE_OMIT_ALTERTABLE
>   	int addColOffset;	/* Offset in CREATE TABLE stmt to add a new column */
>   #endif
> -	Trigger *pTrigger;	/* List of triggers stored in pSchema */
>   	Schema *pSchema;	/* Schema that contains this table */
>   	Table *pNextZombie;	/* Next on the Parse.pZombieTab list */
>   	/** Space definition with Tarantool metadata. */
> @@ -3032,14 +3031,14 @@ struct Parse {
>    */
>   struct Trigger {
>   	char *zName;		/* The name of the trigger                        */
> -	char *table;		/* The table or view to which the trigger applies */
> +	/** The ID of space the triggers is refer to. */

6. triggers? There is one trigger. And 'is refer to' is not correct sentence. 'Refer'
is a verb.

> +	uint32_t space_id;
>   	u8 op;			/* One of TK_DELETE, TK_UPDATE, TK_INSERT         */
>   	u8 tr_tm;		/* One of TRIGGER_BEFORE, TRIGGER_AFTER */
>   	Expr *pWhen;		/* The WHEN clause of the expression (may be NULL) */
>   	IdList *pColumns;	/* If this is an UPDATE OF <column-list> trigger,
>   				   the <column-list> is stored here */
>   	Schema *pSchema;	/* Schema containing the trigger */
> -	Schema *pTabSchema;	/* Schema containing the table */
>   	TriggerStep *step_list;	/* Link list of trigger program steps             */
>   	Trigger *pNext;		/* Next trigger associated with the table */
>   };
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 9b65064..0c851c9 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -39,6 +39,7 @@
>   #include <stdlib.h>
>   #include <unicode/utf8.h>
>   #include <unicode/uchar.h>
> +#include "box/schema.h"
>   
>   #include "say.h"
>   #include "sqliteInt.h"
> @@ -122,6 +123,7 @@ static const char sql_ascii_class[] = {
>    * the #include below.
>    */
>   #include "keywordhash.h"
> +#include "tarantoolInt.h"
>   
>   #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0)
>   
> @@ -510,8 +512,13 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>   	}
>   	if (pParse->rc != SQLITE_OK && pParse->rc != SQLITE_DONE
>   	    && pParse->zErrMsg == 0) {
> -		pParse->zErrMsg =
> -		    sqlite3MPrintf(db, "%s", sqlite3ErrStr(pParse->rc));
> +		const char *error;
> +		if (is_tarantool_error(pParse->rc) && tarantoolErrorMessage()) {

7. != NULL

> +			error = tarantoolErrorMessage();
> +		} else {
> +			error = sqlite3ErrStr(pParse->rc);
> +		}

8. Do not use {} when both 'if' and 'else' are one lines.

> +		pParse->zErrMsg = sqlite3MPrintf(db, "%s", error);
>   	}
>   	assert(pzErrMsg != 0);
>   	if (pParse->zErrMsg) {
> @@ -528,7 +535,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>   
>   	if (pParse->pWithToFree)
>   		sqlite3WithDelete(db, pParse->pWithToFree);
> -	sql_trigger_delete(db, pParse->pNewTrigger);
> +	/*
> +	 * Trigger is exporting with pNewTrigger field when

9. is exported.

> +	 * parse_only flag is set.
> +	 */
> +	if (!pParse->parse_only)
> +		sql_trigger_delete(db, pParse->pNewTrigger);
>   	sqlite3DbFree(db, pParse->pVList);
>   	while (pParse->pZombieTab) {
>   		Table *p = pParse->pZombieTab;
> @@ -569,3 +581,23 @@ cleanup:
>   	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_EXECUTE, sql);

10. Same as in the previous patch: error and sql_error are unused.

> +		goto cleanup;
> +	}
> +	trigger = parser.pNewTrigger;
> +cleanup:

11. You should avoid 'goto cleanup' here.

> +	sql_parser_destroy(&parser);
> +	return trigger;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 053dadb..beaad11 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -33,6 +33,8 @@
>    * This file contains the implementation for TRIGGERs
>    */
>   
> +#include "box/box.h"

12. Unused.

> +#include "box/schema.h"
>   #include "sqliteInt.h"
>   #include "tarantoolInt.h"
>   #include "vdbeInt.h"
> @@ -81,102 +83,121 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>       )
>   {
>   	Trigger *pTrigger = 0;	/* The new trigger */
> -	Table *pTab;		/* Table that the trigger fires off of */
> +	/* Table that the trigger fires off of. */
> +	struct Table *table = NULL;
>   	char *zName = 0;	/* Name of the trigger */
>   	sqlite3 *db = pParse->db;	/* The database connection */
>   	DbFixer sFix;		/* State vector for the DB fixer */
>   
> -	/* Do not account nested operations: the count of such
> -	 * operations depends on Tarantool data dictionary internals,
> -	 * such as data layout in system spaces.
> +	/*
> +	 * Do not account nested operations: the count of such
> +	 * operations depends on Tarantool data dictionary
> +	 * internals, such as data layout in system spaces.
>   	 */
>   	if (!pParse->nested) {
>   		Vdbe *v = sqlite3GetVdbe(pParse);
>   		if (v != NULL)
>   			sqlite3VdbeCountChanges(v);
>   	}
> -	assert(pName != 0);	/* pName->z might be NULL, but not pName itself */
> +	/* pName->z might be NULL, but not pName itself. */
> +	assert(pName != 0);
>   	assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE);
>   	assert(op > 0 && op < 0xff);
>   
> -	if (!pTableName || db->mallocFailed) {
> +	if (!pTableName || db->mallocFailed)
>   		goto trigger_cleanup;
> -	}
>   
> -	/* Ensure the table name matches database name and that the table exists */
> +	/*
> +	 * Ensure the table name matches database name and that
> +	 * the table exists.
> +	 */
>   	if (db->mallocFailed)
>   		goto trigger_cleanup;
>   	assert(pTableName->nSrc == 1);
>   	sqlite3FixInit(&sFix, pParse, "trigger", pName);
> -	if (sqlite3FixSrcList(&sFix, pTableName)) {
> +	if (sqlite3FixSrcList(&sFix, pTableName))

13. != 0

>   
> -	/* INSTEAD OF triggers can only appear on views and BEFORE triggers
> +	/*
> +	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
>   	 * cannot appear on views.  So we might as well translate every
>   	 * INSTEAD OF trigger into a BEFORE trigger.  It simplifies code
>   	 * elsewhere.
>   	 */
> -	if (tr_tm == TK_INSTEAD) {
> +	if (tr_tm == TK_INSTEAD)
>   		tr_tm = TK_BEFORE;
> -	}
>   
> -	/* Build the Trigger object */
> +	/* Build the Trigger object. */
>   	pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger));
>   	if (pTrigger == 0)
>   		goto trigger_cleanup;
>   	pTrigger->zName = zName;
>   	zName = 0;
> -	pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
> +	/* Initialize space_id on trigger insert. */
> +	if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName,
> +			strlen(pTableName->a[0].zName),

14. Wrong indentation.

> +			   &pTrigger->space_id) != 0) {
> +		pParse->rc = SQL_TARANTOOL_ERROR;
> +		goto trigger_cleanup;
> +	}
>   	pTrigger->pSchema = db->pSchema;
> -	pTrigger->pTabSchema = pTab->pSchema;
>   	pTrigger->op = (u8) op;
>   	pTrigger->tr_tm = tr_tm == TK_BEFORE ? TRIGGER_BEFORE : TRIGGER_AFTER;
>   	pTrigger->pWhen = sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE);
>   	pTrigger->pColumns = sqlite3IdListDup(db, pColumns);
> +	if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
> +		(pColumns != NULL && pTrigger->pColumns == NULL))

15. Same.

> +		goto trigger_cleanup;
>   	assert(pParse->pNewTrigger == 0);
>   	pParse->pNewTrigger = pTrigger;
>   
> @@ -227,10 +248,11 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
>   		goto triggerfinish_cleanup;
>   	}
>   
> -	/* if we are not initializing,
> -	 * generate byte code to insert a new trigger into Tarantool.
> +	/*
> +	 * Generate byte code to insert a new trigger into
> +	 * Tarantool for non-parsig mode or export trigger.

16. typo

>   	 */
> -	if (!db->init.busy) {
> +	if (!pParse->parse_only) {
>   		Vdbe *v;
>   		int zOptsSz;
>   		Table *pSysTrigger;
> @@ -565,34 +550,68 @@ sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
>   			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_delete_by_name(struct sqlite3 *db, const char *name, int name_len)
>   {
> -	Trigger *pTrigger;
> -	Hash *pHash;
> -	struct session *user_session = current_session();
> +	/* Name from Tarantool tuple may be not normalized. */
> +	uint32_t used = region_used(&fiber()->gc);
> +	char *trigger_name = region_alloc(&fiber()->gc, name_len + 3);

17. Why +3?

> +	if (trigger_name == NULL) {
> +		diag_set(OutOfMemory, name_len + 3, "region_alloc",
> +			 "trigger_name");
> +		return -1;
> +	}
> +	memcpy(trigger_name, name, name_len);
> +	trigger_name[name_len] = 0;
> +
> +	struct Hash *hash = &(db->pSchema->trigHash);
> +	struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL);
> +	assert(trigger != NULL);

18. Why != NULL? Can't sqlite3HashInsert fail?

> +
> +	struct space *space = space_by_id(trigger->space_id);
> +	/* Space could be already deleted. */
> +	if (space != NULL) {
> +		struct Trigger **pp;
> +		for (pp = &space->sql_triggers;
> +		     *pp != trigger;
> +		     pp = &((*pp)->pNext));

19. Can't it fit in a single line?

> +		*pp = (*pp)->pNext;
> +	}
>   


> +int
> +sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len)
> +{
> +	if (strncmp(name, trigger->zName, name_len) != 0) {
> +		diag_set(ClientError, ER_SQL,
> +			 "tuple trigger name does not match extracted from SQL");

20. There are no tuples. Please, do this check in alter.cc. For this you can
declare trigger_name() helper.

> +		return -1;
> +	}
> +
> +	struct space *space = space_cache_find(trigger->space_id);
> +	assert(space != NULL);
> +	struct Hash *hash = &trigger->pSchema->trigHash;
> +	if (sqlite3HashFind(hash, trigger->zName) != NULL) {
> +		diag_set(ClientError, ER_TRIGGER_EXISTS, trigger->zName);> +		return -1;
> +	}
> +
>   /*
> @@ -631,22 +650,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)

21. != NULL

>   		*pMask = mask;
> -	}
> -	return (mask ? pList : 0);
> +	return mask ? trigger_list : 0;
>   }
>   
>   /*
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..bba9bf1 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);
> +	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 trigger created on this table. */

22. triggers

> +	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;
>   	}
> +
>   	sqlite3DbFree(db, (void*)zOldTableName);
>   	sqlite3DbFree(db, (void*)zSqlStmt);
>   	break;