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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

You better take a lock to the new patch version at the branch. I'll send it with next message.

> /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)) {
>                         ^~
-               if (rc |= (node == NULL)) {
+               if (node == NULL) {
+                       rc = -1;

> 2. It does not work.
> I do not think we should do in-memory non-persistent rename. Please,
> investigate what other databases do.
SQLite3, MySQL allows rename table with triggers.

> 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.
> 4. On rollback you must delete this trigger.
I've introduce space_id instead of table name in Trigger object.
Diff for this explicit change is at the end of this message.

>> + * @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.I followed sql_expr_compile example. But it is ok for me to refactor this function this way.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 63f253b..487a497 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3141,8 +3141,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		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)
+		struct Trigger *trigger =
+			sql_trigger_compile(sql_get(), opts.sql);
+		if (trigger == NULL)
 			diag_raise();
 		assert(trigger != NULL);
 		const char *table_name =
diff --git a/src/box/sql.h b/src/box/sql.h
index f21b745..bd542c0 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -88,13 +88,12 @@ sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
  * 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.
+ * @retval NULL on error
+ * @retval not NULL Trigger AST pointer on success.
  */
-int
-sql_trigger_compile(struct sqlite3 *db, const char *sql,
-		    struct Trigger **trigger);
+struct Trigger *
+sql_trigger_compile(struct sqlite3 *db, const char *sql);

> 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.)I've fellow sql_expr_compile logic, made same behavior.

@@ -577,12 +579,16 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql)
struct Parse parser;
sql_parser_create(&parser, db);
parser.parse_only = true;
- char *unused;
- if (sqlite3RunParser(&parser, sql, &unused) != SQLITE_OK) {
+ 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);
- return NULL;
+ goto cleanup;
}
- struct Trigger *trigger = parser.pNewTrigger;
+ trigger = parser.pNewTrigger;
+cleanup:
sql_parser_destroy(&parser);
return trigger;
}

I'll also change sql_expr_compile function in separate patch to follow this convention.
 
>> + * 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.
- * @param trigger_name to delete.
+ * @param trigger_name a name of trigger object to delete.

>> +/**
>> + * Get server triggers list by space_id.
>> + * @param space_id Space ID.
>> + * @retval NULL on error.
> 
> 8. Are you sure?
- * @retval NULL on error.
- * @param not NULL on success.
+ * @retval trigger AST list.

>> +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.
txn_stmt doesn't contain valid space pointer as it is defined by trigger.
> 10. sql_trigger_table_name. On getter methods you may omit 'get'.-sql_trigger_get_table_name(struct Trigger *trigger)
+sql_trigger_table_name(struct Trigger *trigger)

>> +	/* 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.
I believe that it is updated in sqlite3EndTable, below the only location of updating tnum.

>> +	/* 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.

-       /* Trigger is exporting with pNewTrigger field when
-        * parse_only flag is set. */
+       /*
+        * Trigger is exporting with pNewTrigger field when
+        * parse_only flag is set.
+        */


> 14. Extra new line.
dropped.

>> +#include "box/box.h"
>> +#include "box/tuple.h"
> 15. Why do you need tuple.h? I removed it and nothing is changed.
-#include "box/tuple.h"

> 16. Why do you need extra () after & ?
-               if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) {
+               if (sqlite3HashFind(&db->pSchema->trigHash, zName)) {


> 17. Looks like sqlite3DeleteTriggerStep() leaks. It does not delete 'table'.
> Maybe it is the original SQLite bug. Can you please check?
triggerStepAllocate(sqlite3 * db,	/* Database connection */
		    u8 op,	/* Trigger opcode */
		    Token * pName	/* The target name */
    )
{
	TriggerStep *pTriggerStep =
	    sqlite3DbMallocZero(db, sizeof(TriggerStep) + pName->n + 1);
...
char *z = (char *)&pTriggerStep[1];
...
pTriggerStep->zTarget = z;

Memory for name is located in same allocation that object located is.  

>> +	if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
>> +		(pColumns != NULL && pTrigger->pColumns == NULL))
>> +		goto trigger_cleanup;
> 
> 18. Why?
We have deep-cloned pWhen into pTrigger->pWhen and pColumns into pTrigger->pColumns;
There could be memory allocation error, so we ensure that action have been finished correctly.

>> +	struct Trigger *p;
>> +	for (p = trigger_list; p; p = p->pNext) {
> 
> 19. Why not for (struct Trigger *p = ... ?
-       struct Trigger *p;
-       for (p = trigger_list; p; p = p->pNext) {
+       for (struct Trigger *p = trigger_list; p; p = p->pNext) {

>> +	return (mask ? trigger_list : 0);
> 
> 20. Extra ().
-       return (mask ? trigger_list : 0);
+       return mask ? trigger_list : 0;

> 
>> 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?
> 23. Same as 21.
I've  assumed that it could be useful in future or for debug.

-/* Opcode: ParseSchema3 P1 * * * *
- * Synopsis: name=r[P1] sql=r[P1+1]
- *
- * Create trigger named r[P1] w/ DDL SQL stored in r[P1+1]
- * in database P2
- */
-case OP_ParseSchema3: {
-       unreachable();
-       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: {
-       unreachable();
-       break;
-}

>>   	zOldTableName = sqlite3DbStrNDup(db, zOldTableName,
>>   					 sqlite3Strlen30(zOldTableName));
>> +
> 
> 22. Garbage diff.
dropped.

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



diff --git a/src/box/alter.cc b/src/box/alter.cc
index d3f9d42..6d63f96 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -736,21 +736,6 @@ 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)
-			diag_raise();
-	}
 	alter->new_space = NULL; /* for alter_space_delete(). */
 	/*
 	 * Delete the old version of the space, we are not
@@ -3140,21 +3125,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		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();
-		assert(trigger != NULL);
-		const char *table_name =
-			sql_trigger_table_name(trigger);
-		if (table_name == NULL)
-			diag_raise();
-		uint32_t space_id;
-		if (schema_find_id(BOX_SPACE_ID, 2, table_name,
-				   strlen(table_name), &space_id) != 0)
-			diag_raise();
-		if (sql_trigger_insert(space_id, trigger) != 0)
+		if (sql_trigger_insert(trigger) != 0)
 			diag_raise();
 	}
 }
diff --git a/src/box/sql.h b/src/box/sql.h
index e6f5a02..bc96b75 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -114,33 +114,12 @@ 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);
-
-/**
- * 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_table_name(struct Trigger *trigger);
-
-/**
- * Rename specified trigger list.
- * @param trigger containing a table name.
- * @param new_table_name with new_name (optional).
- *
- * @retval Error code if any.
- */
-int
-sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger,
-						const char *new_table_name);
+sql_trigger_insert(struct Trigger *trigger);
 
 /**
  * Store duplicate of a parsed expression into @a parser.
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 70ebef8..d5585cf 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1440,7 +1440,6 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 		}
 		pStep->pTrig = pTrigger;
 		pTrigger->pSchema = pTab->pSchema;
-		pTrigger->pTabSchema = pTab->pSchema;
 		pFKey->apTrigger[iAction] = pTrigger;
 		pTrigger->op = (pChanges ? TK_UPDATE : TK_DELETE);
 	}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 36695e3..06930c7 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3031,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. */
+	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 2d9d29e..9d613de 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -597,13 +597,13 @@ cleanup:
 }
 
 int
-sql_trigger_insert(uint32_t space_id, struct Trigger *trigger)
+sql_trigger_insert(struct Trigger *trigger)
 {
-	struct space *space = space_cache_find(space_id);
+	struct space *space = space_cache_find(trigger->space_id);
 	assert(space != NULL);
 	struct Hash *pHash = &trigger->pSchema->trigHash;
-	char *zName = trigger->zName;
-	void *ret = sqlite3HashInsert(pHash, zName, trigger);
+	char *trigger_name = trigger->zName;
+	void *ret = sqlite3HashInsert(pHash, trigger_name, trigger);
 	if (ret != NULL) {
 		/* Triggers couldn't present in hash.
 		 * So this is definitely a memory error. */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index a808d3a..2fd81c0 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -177,15 +177,14 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 		goto trigger_cleanup;
 	pTrigger->zName = zName;
 	zName = 0;
-	pTrigger->table = strdup(pTableName->a[0].zName);
-	if (pTrigger->table == NULL) {
-		diag_set(OutOfMemory, strlen(pTableName->a[0].zName), "strdup",
-			 "pTrigger->table");
+	/* Initialize space_id on trigger insert. */
+	if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName,
+			strlen(pTableName->a[0].zName),
+			   &pTrigger->space_id) != 0) {
 		pParse->rc = SQL_TARANTOOL_ERROR;
 		goto trigger_cleanup;
 	}
 	pTrigger->pSchema = db->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);
@@ -470,7 +469,6 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger)
 		return;
 	sqlite3DeleteTriggerStep(db, pTrigger->step_list);
 	sqlite3DbFree(db, pTrigger->zName);
-	free(pTrigger->table);
 	sql_expr_delete(db, pTrigger->pWhen, false);
 	sqlite3IdListDelete(db, pTrigger->pColumns);
 	sqlite3DbFree(db, pTrigger);
@@ -525,16 +523,6 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
 }
 
 /*
- * Return a pointer to the Table structure for the table that a trigger
- * is set on.
- */
-static Table *
-tableOfTrigger(Trigger * pTrigger)
-{
-	return sqlite3HashFind(&pTrigger->pTabSchema->tblHash, pTrigger->table);
-}
-
-/*
  * Drop a trigger given a pointer to that trigger.
  */
 void
@@ -569,20 +557,14 @@ sql_trigger_delete_by_name(struct sqlite3 *db, const char *trigger_name)
 	struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL);
 	assert(trigger != NULL);
 
-	if (trigger->pSchema == trigger->pTabSchema) {
-		uint32_t space_id;
-		if (schema_find_id(BOX_SPACE_ID, 2, trigger->table,
-				   strlen(trigger->table), &space_id) != 0)
-			return -1;
-		struct space *space = space_by_id(space_id);
-		/* Space could be already deleted. */
-		if (space != NULL) {
-			struct Trigger **pp;
-			for (pp = &space->sql_triggers;
-			     *pp != trigger;
-			     pp = &((*pp)->pNext));
-			*pp = (*pp)->pNext;
-		}
+	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));
+		*pp = (*pp)->pNext;
 	}
 
 	sqlite3DeleteTrigger(db, trigger);
@@ -825,7 +807,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);
 	assert(pTop->pVdbe);
 
 	/* Allocate the TriggerPrg and SubProgram objects. To ensure that they
@@ -942,7 +924,7 @@ getRowTrigger(Parse * pParse,	/* Current parse context */
 	Parse *pRoot = sqlite3ParseToplevel(pParse);
 	TriggerPrg *pPrg;
 
-	assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
+	assert(pTrigger->zName == 0 || pTab->def->id == pTrigger->space_id);
 
 	/* It may be that this trigger has already been coded (or is in the
 	 * process of being coded). If this is the case, then an entry with
@@ -1067,14 +1049,7 @@ sqlite3CodeRowTrigger(Parse * pParse,	/* Parse context */
 	assert((op == TK_UPDATE) == (pChanges != 0));
 
 	for (p = pTrigger; p; p = p->pNext) {
-
-		/* Sanity checking:  The schema for the trigger and for the table are
-		 * always defined.  The trigger must be in the same schema as the table
-		 * or else it must be a TEMP trigger.
-		 */
 		assert(p->pSchema != 0);
-		assert(p->pTabSchema != 0);
-		assert(p->pSchema == p->pTabSchema);
 
 		/* Determine whether we should code this trigger */
 		if (p->op == op
@@ -1142,57 +1117,4 @@ sqlite3TriggerColmask(Parse * pParse,	/* Parse context */
 	return mask;
 }
 
-const char *
-sql_trigger_table_name(struct Trigger *trigger)
-{
-	return trigger->table;
-}
-
-int
-sql_trigger_list_alter_table_name_transactional(struct Trigger *trigger,
-						const char *new_table_name)
-{
-	struct names_list_t {
-	    char *name;
-	    struct names_list_t *next;
-	} *names_list = NULL;
-
-	int rc = 0;
-	for (struct Trigger *t = trigger; t != NULL; t = t->pNext) {
-		struct names_list_t *node =
-			region_alloc(&fiber()->gc, sizeof(*node));
-		if (node == NULL) {
-			rc = -1;
-			diag_set(OutOfMemory, sizeof(*node), "region_alloc",
-				 "node");
-			break;
-		}
-		node->name = strdup(new_table_name);
-		if (node->name == NULL) {
-			rc = -1;
-			diag_set(OutOfMemory, strlen(new_table_name), "strdup",
-				 "node->name");
-			break;
-		}
-		node->next = names_list;
-		names_list = node;
-	}
-	if (rc != 0)
-		goto error;
-
-	for (struct Trigger *t = trigger; t != NULL; t = t->pNext) {
-		free(t->table);
-		t->table = names_list->name;
-		names_list = names_list->next;
-	}
-	return 0;
-
-error:
-	while (names_list != NULL) {
-		free(names_list->name);
-		names_list = names_list->next;
-	}
-	return -1;
-}
-
 #endif				/* !defined(SQLITE_OMIT_TRIGGER) */