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

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

Thanks for the patch! See 24 comments below.

1.

/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]
                 if (rc |= (node == NULL)) {
                     ~~~^~~~~~~~~~~~~~~~~
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: place parentheses around the assignment to silence this warning
                 if (rc |= (node == NULL)) {
                        ^
                     (                   )
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1168:10: note: use '!=' to turn this compound assignment into an inequality comparison
                 if (rc |= (node == NULL)) {
                        ^~
                        !=
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]
                 if (rc |= (node->name == NULL)) {
                     ~~~^~~~~~~~~~~~~~~~~~~~~~~
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: place parentheses around the assignment to silence this warning
                 if (rc |= (node->name == NULL)) {
                        ^
                     (                         )
/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/trigger.c:1174:10: note: use '!=' to turn this compound assignment into an inequality comparison
                 if (rc |= (node->name == NULL)) {
                        ^~
                        !=

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert builded 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        |  57 +++++++++++
>   src/box/space.h         |   2 +
>   src/box/sql.c           |  48 ++-------
>   src/box/sql.h           |  62 ++++++++++++
>   src/box/sql/build.c     |   8 +-
>   src/box/sql/insert.c    |   6 +-
>   src/box/sql/sqliteInt.h |   2 -
>   src/box/sql/tokenize.c  |  43 +++++++-
>   src/box/sql/trigger.c   | 258 ++++++++++++++++++++++++++++--------------------
>   src/box/sql/vdbe.c      |  58 +++--------
>   src/box/sql/vdbe.h      |   1 -
>   src/box/sql/vdbeaux.c   |   9 --
>   12 files changed, 343 insertions(+), 211 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index b62f8ad..6e4f15d 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -733,6 +737,20 @@ alter_space_commit(struct trigger *trigger, void *event)
>   
>   	trigger_run_xc(&on_alter_space, alter->new_space);
>   
> +	if (alter->new_space->sql_triggers != NULL &&
> +	    strcmp(alter->new_space->def->name,
> +		   alter->old_space->def->name) != 0) {
> +		/*
> +		 * The function below either changes the name of
> +		 * all triggers, or does not change any of them.
> +		 * It should be last action in alter_space_commit
> +		 * as it is difficult to guarantee its rollback.
> +		 */
> +		if (sql_trigger_list_alter_table_name_transactional(
> +			alter->new_space->sql_triggers,
> +			alter->new_space->def->name) != 0)

2. It does not work.

box.cfg{}
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;
]])
box.space.T1:rename('T3')
box.space._trigger:select{}

I still see the trigger with the old table name.

I do not think we should do in-memory non-persistent rename. Please,
investigate what other databases do.

> +			diag_raise();
> +	}
>   	alter->new_space = NULL; /* for alter_space_delete(). */
>   	/*
>   	 * Delete the old version of the space, we are not
> @@ -3100,6 +3118,45 @@ 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);
> +		/* Can't use static buff as TT_STATIC_BUF_LEN
> +		 * is not enough for identifier max len test. */
> +		char *trigger_name =
> +			(char *)region_alloc_xc(&fiber()->gc, trigger_name_len);
> +		sprintf(trigger_name, "%.*s", trigger_name_len,
> +			trigger_name_src);
> +		if (sql_trigger_delete_by_name(sql_get(), trigger_name) != 0)
> +			diag_raise();

3. If WAL write fails, you could not rollback because the trigger is
already deleted now. Please, implement on_rollback/on_commit triggers.
You must delete the old trigger on commit only.

> +	}
> +	if (stmt->new_tuple != NULL) {
> +		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;
> +		if (sql_trigger_compile(sql_get(), opts.sql, &trigger) != 0)
> +			diag_raise();
> +		assert(trigger != NULL);
> +		const char *table_name =
> +			sql_trigger_get_table_name(trigger);
> +		if (table_name == NULL)
> +			diag_raise();
> +		uint32_t space_id =
> +			space_id_by_name(BOX_SPACE_ID, table_name,
> +					 strlen(table_name));
> +		if (space_id == BOX_ID_NIL)
> +			diag_raise();
> +		if (sql_trigger_insert(space_id, trigger) != 0)
> +			diag_raise();

4. On rollback you must delete this trigger.

> +	}
>   }
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 23021e5..f21b745 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -66,6 +66,7 @@ struct Expr;
>   struct Parse;
>   struct Select;
>   struct Table;
> +struct Trigger;
>   
>   /**
>    * Perform parsing of provided expression. This is done by
> @@ -84,6 +85,67 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
>   		 struct Expr **result);
>   
>   /**
> + * Perform parsing of provided SQL request and construct trigger AST.
> + * @param db SQL context handle.
> + * @param sql request to parse.
> + * @param[out] trigger Result: AST structure.
> + *
> + * @retval Error code if any.

5. More specifically?

> + */
> +int
> +sql_trigger_compile(struct sqlite3 *db, const char *sql,
> +		    struct Trigger **trigger);

6. Why can not you just return Trigger *? Not NULL on success,
NULL on error. It is a common practice.

> +
> +/**
> + * Remove a trigger from the hash tables of the sqlite* pointer.
> + * @param db SQL handle.
> + * @param trigger_name to delete.

7. You delete not the trigger name, but the trigger object.

> + *
> + * @retval Error code if any.
> + */
> +int
> +sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name);
> +
> +/**
> + * Get server triggers list by space_id.
> + * @param space_id Space ID.
> + * @retval NULL on error.

8. Are you sure?

> + * @param not NULL on success.
> + */
> +struct Trigger *
> +space_trigger_list(uint32_t space_id);
> +
> +/**
> + * Perform insert trigger in appropriate space.
> + * @param space_id id of the space to append trigger.
> + * @param trigger object to append.
> + *
> + * @retval Error code if any.
> + */
> +int
> +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger);

9. Why can not you use space *? In alter.cc you have it in txn_stmt.

> +
> +/**
> + * Get table name of specified trigger.
> + * @param trigger containing a table name.
> + * @param new_table_name with new_name (optional).
> + * @retval table name string.
> + */
> +const char *
> +sql_trigger_get_table_name(struct Trigger *trigger);

10. sql_trigger_table_name. On getter methods you may omit 'get'.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 59c61c7..023e6b0 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1766,9 +1766,9 @@ xferOptimization(Parse * pParse,	/* Parser context */
>   		 */
>   		return 0;
>   	}
> -	if (pDest->pTrigger) {
> -		return 0;	/* tab1 must not have triggers */
> -	}
> +	/* The pDest must not have triggers. */
> +	if (space_trigger_list(pDest->def->id) != NULL)

11. As far as I remember, in Table.def id now is always 0. It is
not updated after insertion into _space, so you can not use it.
Use tnum for a while. Or please make a patch, that updates def.id
in the same place where tnum.

> +		return 0;
>   	if (onError == ON_CONFLICT_ACTION_DEFAULT) {
>   		if (pDest->iPKey >= 0)
>   			onError = pDest->keyConf;
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 59df75f..24db578 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"
> @@ -528,7 +529,10 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>   
>   	if (pParse->pWithToFree)
>   		sqlite3WithDelete(db, pParse->pWithToFree);
> -	sqlite3DeleteTrigger(db, pParse->pNewTrigger);
> +	/* Trigger is exporting with pNewTrigger field when
> +	 * parse_only flag is set. */

12. Please, obey Tarantool comment style. In other places too. I will not
comment each explicitly from now.

> +	if (!pParse->parse_only)
> +		sqlite3DeleteTrigger(db, pParse->pNewTrigger);
>   	sqlite3DbFree(db, pParse->pVList);
>   	while (pParse->pZombieTab) {
>   		Table *p = pParse->pZombieTab;
> @@ -564,3 +568,40 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
>   	sql_parser_destroy(&parser);
>   	return 0;
>   }
> +
> +int
> +sql_trigger_compile(struct sqlite3 *db, const char *sql,
> +		    struct Trigger **trigger)
> +{
> +	struct Parse parser;
> +	sql_parser_create(&parser, db);
> +	parser.parse_only = true;
> +	char *unused;

13. Why have you skipped the error message?

(Please, do not respond with just a diff. Try to answer on
the questions. This and others.)

> +	if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) {
> +		diag_set(ClientError, ER_SQL_EXECUTE, sql);
> +		return -1;
> +	}
> +	*trigger = parser.pNewTrigger;
> +	sql_parser_destroy(&parser);
> +	return 0;
> +}
> +
> +

14. Extra new line.

> +int
> +sql_trigger_insert(uint32_t space_id, struct Trigger *trigger)
> +{
> +	struct space *space = space_cache_find(space_id);
> +	assert(space != NULL);
> +	struct Hash *pHash = &trigger->pSchema->trigHash;
> +	char *zName = trigger->zName;
> +	void *ret = sqlite3HashInsert(pHash, zName, trigger);
> +	if (ret != NULL) {
> +		/* Triggers couldn't present in hash.
> +		 * So this is definitely a memory error. */
> +		diag_set(OutOfMemory, 0, "sqlite3HashInsert", "hash");
> +		return -1;
> +	}
> +	trigger->pNext = space->sql_triggers;
> +	space->sql_triggers = trigger;
> +	return 0;
> +}
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index ea35211..00f161e 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -33,6 +33,9 @@
>    * This file contains the implementation for TRIGGERs
>    */
>   
> +#include "box/box.h"
> +#include "box/tuple.h"

15. Why do you need tuple.h? I removed it and nothing is changed.

> +#include "box/schema.h"
>   #include "sqliteInt.h"
>   #include "tarantoolInt.h"
>   #include "vdbeInt.h"
> @@ -99,60 +103,65 @@ sqlite3BeginTrigger(Parse * pParse
>   	/* 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)) {
> -		goto trigger_cleanup;
> -	}
> -	pTab = sql_list_lookup_table(pParse, pTableName);
> -	if (!pTab) {
> +	if (sqlite3FixSrcList(&sFix, pTableName))
>   		goto trigger_cleanup;
> -	}
>   
> -	/* Check that the trigger name is not reserved and that no trigger of the
> -	 * specified name exists
> -	 */
>   	zName = sqlite3NameFromToken(db, pName);
> -	if (!zName || SQLITE_OK != sqlite3CheckIdentifierName(pParse, zName)) {
> +	if (zName == NULL)
>   		goto trigger_cleanup;
> -	}
> -	if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) {
> -		if (!noErr) {
> -			sqlite3ErrorMsg(pParse, "trigger %s already exists",
> -					zName);
> -		} else {
> -			assert(!db->init.busy);
> +
> +	/* FIXME: Move all checks in VDBE #3435. */
> +	if (!pParse->parse_only) {
> +		if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
> +			goto trigger_cleanup;
> +
> +		table = sql_list_lookup_table(pParse, pTableName);
> +		if (table == NULL)
> +			goto trigger_cleanup;
> +
> +		if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) {

16. Why do you need extra () after & ?

> +			if (!noErr) {
> +				sqlite3ErrorMsg(pParse,
> +						"trigger %s already exists",
> +						zName);
> +			} else {
> +				assert(!db->init.busy);
> +			}
> +			goto trigger_cleanup;
>   		}
> -		goto trigger_cleanup;
> @@ -170,13 +178,22 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>   		goto trigger_cleanup;
>   	pTrigger->zName = zName;
>   	zName = 0;
> -	pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
> +	pTrigger->table = strdup(pTableName->a[0].zName);

17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'.
Maybe it is the original SQLite bug. Can you please check?

> +	if (pTrigger->table == NULL) {
> +		diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup",
> +			 "pTrigger->table");
> +		pParse->rc = SQL_TARANTOOL_ERROR;
> +		goto trigger_cleanup;
> +	}
>   	pTrigger->pSchema = db->pSchema;
> -	pTrigger->pTabSchema = pTab->pSchema;
> +	pTrigger->pTabSchema = db->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))
> +		goto trigger_cleanup;

18. Why?

> @@ -634,22 +629,18 @@ 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);
> +	struct Trigger *p;
> +	for (p = trigger_list; p; p = p->pNext) {

19. Why not for (struct Trigger *p = ... ?

> +		if (p->op == op && checkColumnOverlap(p->pColumns, pChanges))
>   			mask |= p->tr_tm;
> -		}
>   	}
> -	if (pMask) {
> +	if (pMask)
>   		*pMask = mask;
> -	}
> -	return (mask ? pList : 0);
> +	return (mask ? trigger_list : 0);

20. Extra ().

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..f60c122 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4693,36 +4693,7 @@ case OP_ParseSchema2: {
>    * in database P2
>    */
>   case OP_ParseSchema3: {

21. Why have not you deleted it?

> -	InitData initData;
> -	Mem *pRec;
> -	char zPgnoBuf[16];
> -	char *argv[4] = {NULL, zPgnoBuf, NULL, NULL};
> -	assert(db->pSchema != NULL);
> -
> -	initData.db = db;
> -	initData.pzErrMsg = &p->zErrMsg;
> -
> -	assert(db->init.busy==0);
> -	db->init.busy = 1;
> -	initData.rc = SQLITE_OK;
> -	assert(!db->mallocFailed);
> -
> -	pRec = &aMem[pOp->p1];
> -	argv[0] = pRec[0].z;
> -	argv[1] = "0";
> -	argv[2] = pRec[1].z;
> -	sqlite3InitCallback(&initData, 3, argv, NULL);
> -
> -	rc = initData.rc;
> -	db->init.busy = 0;
> -
> -	if (rc) {
> -		sqlite3ResetAllSchemasOfConnection(db);
> -		if (rc==SQLITE_NOMEM) {
> -			goto no_mem;
> -		}
> -		goto abort_due_to_error;
> -	}
> +	unreachable();
>   	break;
>   }
> @@ -4758,11 +4728,11 @@ case OP_RenameTable: {
>   	assert(zOldTableName);
>   	pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
>   	assert(pTab);
> -	pTrig = pTab->pTrigger;
>   	iRootPage = pTab->tnum;
>   	zNewTableName = pOp->p4.z;
>   	zOldTableName = sqlite3DbStrNDup(db, zOldTableName,
>   					 sqlite3Strlen30(zOldTableName));
> +

22. Garbage diff.

>   	rc = tarantoolSqlite3RenameTable(pTab->tnum, zNewTableName,
>   					 &zSqlStmt);
>   	if (rc) goto abort_due_to_error;
> @@ -4866,7 +4838,7 @@ case OP_DropIndex: {
>    * schema consistent with what is on disk.
>    */
>   case OP_DropTrigger: {
> -	sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z);
> +	unreachable();

23. Same as 21.

>   	break;
>   }
>   #ifndef SQLITE_OMIT_TRIGGER

24. Where are tests on triggers create/drop via Lua and direct _trigger
manipulations?