[tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server

[tarantool-patches] [PATCH v1 0/4] sql: remove Triggers to server

[Kirill Shcherbatov]

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers
Issue: https://github.com/tarantool/tarantool/issues/3273

As we are going to call parser on box.cfg() to recreate triggers
from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.
Inroduced box_space_id_by_name to get object id by name from custom space.
Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid
length in MsgPack after ALTER TABLE NAME.
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.

Kirill Shcherbatov (4):
  box: move db->pShchema init to sql_init
  sql: fix sql len in tarantoolSqlite3RenameTrigger
  box: introduce box_space_id_by_name
  sql: move Triggers to server

 src/box/alter.cc        |  57 +++++++++++
 src/box/box.cc          |  10 +-
 src/box/box.h           |  14 +++
 src/box/space.h         |   2 +
 src/box/sql.c           |  60 ++++-------
 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 --
 14 files changed, 374 insertions(+), 216 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v1 1/4] box: move db->pShchema init to sql_init

[Kirill Shcherbatov]

As we are going to call parser on box.cfg() to recreate triggers
from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.

Part of #3273.
---
 src/box/sql.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 7379cb4..47d1739 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -77,11 +77,18 @@ sql_init()
 		panic("failed to initialize SQL subsystem");
 
 	assert(db != NULL);
+	/* Initialize pSchema to use SQL parser. */
+	db->pSchema = sqlite3SchemaCreate(db);
+	if (db->pSchema == NULL) {
+		sqlite3_close(db);
+		panic("failed to initialize SQL Schema subsystem");
+	}
 }
 
 void
 sql_load_schema()
 {
+	assert(db->pSchema != NULL);
 	int rc;
 	struct session *user_session = current_session();
 	int commit_internal = !(user_session->sql_flags
@@ -89,7 +96,6 @@ sql_load_schema()
 
 	assert(db->init.busy == 0);
 	db->init.busy = 1;
-	db->pSchema = sqlite3SchemaCreate(db);
 	rc = sqlite3InitDatabase(db);
 	if (rc != SQLITE_OK) {
 		sqlite3SchemaClear(db);
-- 
2.7.4

[tarantool-patches] [PATCH v1 2/4] sql: fix sql len in tarantoolSqlite3RenameTrigger

[Kirill Shcherbatov]

Part of #3273.
---
 src/box/sql.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 47d1739..eacb288 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -686,8 +686,8 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
 	bool is_quoted = false;
 	trigger_stmt = rename_trigger(db, trigger_stmt, new_table_name, &is_quoted);
 
-	uint32_t trigger_stmt_new_len = trigger_stmt_len + old_table_name_len -
-					new_table_name_len + 2 * (!is_quoted);
+	uint32_t trigger_stmt_new_len = trigger_stmt_len + new_table_name_len -
+					old_table_name_len + 2 * (!is_quoted);
 	assert(trigger_stmt_new_len > 0);
 	key_len = mp_sizeof_array(2) + mp_sizeof_str(trig_name_len) +
 		  mp_sizeof_map(1) + mp_sizeof_str(3) +
-- 
2.7.4

[tarantool-patches] [PATCH v1 3/4] box: introduce box_space_id_by_name

[Kirill Shcherbatov]

Part of #3273.
---
 src/box/box.cc | 10 ++++++++--
 src/box/box.h  | 14 ++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 02722d6..9540d85 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -877,7 +877,7 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple)
 
 /* schema_find_id()-like method using only public API */
 uint32_t
-box_space_id_by_name(const char *name, uint32_t len)
+space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len)
 {
 	if (len > BOX_NAME_MAX)
 		return BOX_ID_NIL;
@@ -892,7 +892,7 @@ box_space_id_by_name(const char *name, uint32_t len)
 
 	/* NOTE: error and missing key cases are indistinguishable */
 	box_tuple_t *tuple;
-	if (box_index_get(BOX_VSPACE_ID, 2, begin, end, &tuple) != 0)
+	if (box_index_get(system_space_id, 2, begin, end, &tuple) != 0)
 		return BOX_ID_NIL;
 	if (tuple == NULL)
 		return BOX_ID_NIL;
@@ -902,6 +902,12 @@ box_space_id_by_name(const char *name, uint32_t len)
 }
 
 uint32_t
+box_space_id_by_name(const char *name, uint32_t len)
+{
+	return space_id_by_name(BOX_VSPACE_ID, name, len);
+}
+
+uint32_t
 box_index_id_by_name(uint32_t space_id, const char *name, uint32_t len)
 {
 	if (len > BOX_NAME_MAX)
diff --git a/src/box/box.h b/src/box/box.h
index bdd5d5c..ba33113 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -220,6 +220,20 @@ API_EXPORT int
 box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple);
 
 /**
+ * Find space id by name in specified system space.
+ *
+ * This function performs SELECT request to _vspace system space.
+ * \param system_space_id space to lookup name.
+ * \param name space name
+ * \param len length of \a name
+ * \retval BOX_ID_NIL on error or if not found (check box_error_last())
+ * \retval space_id otherwise
+ * \sa box_index_id_by_name
+ */
+uint32_t
+space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len);
+
+/**
  * Find space id by name.
  *
  * This function performs SELECT request to _vspace system space.
-- 
2.7.4

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

[Kirill Shcherbatov]

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
@@ -53,6 +53,7 @@
 #include "version.h"
 #include "sequence.h"
 #include "sql.h"
+#include "box.h"
 
 /**
  * chap-sha1 of empty string, i.e.
@@ -551,6 +552,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;
 }
 
 /**
@@ -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)
+			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();
+	}
+	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();
+	}
 }
 
 struct trigger alter_space_on_replace_space = {
diff --git a/src/box/space.h b/src/box/space.h
index b8d29ca..0413cd0 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -146,6 +146,8 @@ struct space {
 	struct rlist on_replace;
 	/** Triggers fired before space statement */
 	struct rlist on_stmt_begin;
+	/** SQL Trigger list. */
+	struct Trigger *sql_triggers;
 	/**
 	 * The number of *enabled* indexes in the space.
 	 *
diff --git a/src/box/sql.c b/src/box/sql.c
index eacb288..0ff6e73 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1219,9 +1219,6 @@ space_foreach_put_cb(struct space *space, void *udata)
 /* Load database schema from Tarantool. */
 void tarantoolSqlite3LoadSchema(InitData *init)
 {
-	box_iterator_t *it;
-	box_tuple_t *tuple;
-
 	sql_schema_put(
 		init, TARANTOOL_SYS_SCHEMA_NAME,
 		BOX_SCHEMA_ID, 0,
@@ -1290,42 +1287,6 @@ void tarantoolSqlite3LoadSchema(InitData *init)
 		init->rc = SQL_TARANTOOL_ERROR;
 		return;
 	}
-
-	/* Read _trigger */
-	it = box_index_iterator(BOX_TRIGGER_ID, 0, ITER_GE,
-				nil_key, nil_key + sizeof(nil_key));
-
-	if (it == NULL) {
-		init->rc = SQL_TARANTOOL_ITERATOR_FAIL;
-		return;
-	}
-
-	while (box_iterator_next(it, &tuple) == 0 && tuple != NULL) {
-		const char *field, *ptr;
-		char *name, *sql;
-		unsigned len;
-		assert(tuple_field_count(tuple) == 2);
-
-		field = tuple_field(tuple, 0);
-		assert (field != NULL);
-		ptr = mp_decode_str(&field, &len);
-		name = strndup(ptr, len);
-
-		field = tuple_field(tuple, 1);
-		assert (field != NULL);
-		mp_decode_array(&field);
-		ptr = mp_decode_str(&field, &len);
-		assert (strncmp(ptr, "sql", 3) == 0);
-
-		ptr = mp_decode_str(&field, &len);
-		sql = strndup(ptr, len);
-
-		sql_schema_put(init, name, 0, 0, sql);
-
-		free(name);
-		free(sql);
-	}
-	box_iterator_free(it);
 }
 
 /*********************************************************************
@@ -1733,6 +1694,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
 	return space->def->fields[fieldno].default_value_expr;
 }
 
+struct Trigger *
+space_trigger_list(uint32_t space_id)
+{
+	struct space *space = space_cache_find(space_id);
+	assert(space != NULL);
+	assert(space->def != NULL);
+	return space->sql_triggers;
+}
+
 struct space_def *
 sql_ephemeral_space_def_new(Parse *parser, const char *name)
 {
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.
+ */
+int
+sql_trigger_compile(struct sqlite3 *db, const char *sql,
+		    struct Trigger **trigger);
+
+/**
+ * Remove a trigger from the hash tables of the sqlite* pointer.
+ * @param db SQL handle.
+ * @param trigger_name to delete.
+ *
+ * @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.
+ * @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);
+
+/**
+ * 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);
+
+/**
+ * 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);
+
+/**
  * Store duplicate of a parsed expression into @a parser.
  * @param parser Parser context.
  * @param select Select to extract from.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 9cd1cde..475d8aa 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2289,16 +2289,14 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
-	 * _trigger. OP_DropTrigger will remove it from internal
-	 * SQL structures.
+	 * _trigger. on_replace_dd_trigger will remove it from
+	 * internal SQL structures.
 	 *
 	 * Do not account triggers deletion - they will be
 	 * accounted in DELETE from _space below.
 	 */
 	parse_context->nested++;
-	Table *table = sqlite3HashFind(&parse_context->db->pSchema->tblHash,
-				       space->def->name);
-	struct Trigger *trigger = table->pTrigger;
+	struct Trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
 		sqlite3DropTriggerPtr(parse_context, trigger);
 		trigger = trigger->pNext;
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)
+		return 0;
 	if (onError == ON_CONFLICT_ACTION_DEFAULT) {
 		if (pDest->iPKey >= 0)
 			onError = pDest->keyConf;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index b23cb1a..39393c5 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. */
@@ -4028,7 +4027,6 @@ TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
 				      u8);
 TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
 void sqlite3DeleteTrigger(sqlite3 *, Trigger *);
-void sqlite3UnlinkAndDeleteTrigger(sqlite3 *, const char *);
 u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
 			  int);
 #define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
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. */
+	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;
+	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;
+}
+
+
+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"
+#include "box/schema.h"
 #include "sqliteInt.h"
 #include "tarantoolInt.h"
 #include "vdbeInt.h"
@@ -81,7 +84,8 @@ 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 */
@@ -99,60 +103,65 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	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 */
 	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)) {
+			if (!noErr) {
+				sqlite3ErrorMsg(pParse,
+						"trigger %s already exists",
+						zName);
+			} else {
+				assert(!db->init.busy);
+			}
+			goto trigger_cleanup;
 		}
-		goto trigger_cleanup;
-	}
 
-	/* 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");
-		goto trigger_cleanup;
-	}
+		/* Do not create a trigger on a system table */
+		if (sqlite3StrNICmp(table->def->name, "sqlite_", 7) == 0) {
+			sqlite3ErrorMsg(pParse,
+					"cannot create trigger on system table");
+			goto trigger_cleanup;
+		}
 
-	/* INSTEAD of triggers are only for views and views only support INSTEAD
-	 * of triggers.
-	 */
-	if (space_is_view(pTab) && tr_tm != TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
-				(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
-				pTableName, 0);
-		goto trigger_cleanup;
-	}
-	if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF"
-				" trigger on table: %S", pTableName, 0);
-		goto trigger_cleanup;
+		/* INSTEAD of triggers are only for views and
+		 * views only support INSTEAD of triggers.
+		 */
+		if (table->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);
+			goto trigger_cleanup;
+		}
+		if (!table->def->opts.is_view && tr_tm == TK_INSTEAD) {
+			sqlite3ErrorMsg(pParse,
+					"cannot create INSTEAD OF trigger "
+					"on table: %S", pTableName, 0);
+			goto trigger_cleanup;
+		}
 	}
 
 	/* INSTEAD OF triggers can only appear on views and BEFORE triggers
@@ -160,9 +169,8 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	 * 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 */
 	pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger));
@@ -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);
+	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;
 	assert(pParse->pNewTrigger == 0);
 	pParse->pNewTrigger = pTrigger;
 
@@ -210,7 +227,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 	DbFixer sFix;		/* Fixer object */
 	Token nameToken;	/* Trigger name for error reporting */
 
-	pParse->pNewTrigger = 0;
+	pParse->pNewTrigger = NULL;
 	if (NEVER(pParse->nErr) || !pTrig)
 		goto triggerfinish_cleanup;
 	zName = pTrig->zName;
@@ -227,10 +244,9 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		goto triggerfinish_cleanup;
 	}
 
-	/* if we are not initializing,
-	 * generate byte code to insert a new trigger into Tarantool.
-	 */
-	if (!db->init.busy) {
+	/* Generate byte code to insert a new trigger into
+	 * Tarantool for non-parsig mode or export trigger. */
+	if (!pParse->parse_only) {
 		Vdbe *v;
 		int zOptsSz;
 		Table *pSysTrigger;
@@ -293,32 +309,10 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		pParse->nMem += 2;
 
 		sql_set_multi_write(pParse, false);
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol, 0,
-				  zName, P4_STATIC);
-
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol + 1, 0,
-				  zSql, P4_DYNAMIC);
 		sqlite3ChangeCookie(pParse);
-		sqlite3VdbeAddParseSchema3Op(v, iFirstCol);
-	}
-
-	if (db->init.busy) {
-		Trigger *pLink = pTrig;
-		Hash *pHash = &db->pSchema->trigHash;
-		pTrig = sqlite3HashInsert(pHash, zName, pTrig);
-		if (pTrig) {
-			sqlite3OomFault(db);
-		} else if (pLink->pSchema == pLink->pTabSchema) {
-			Table *pTab;
-			pTab =
-			    sqlite3HashFind(&pLink->pTabSchema->tblHash,
-					    pLink->table);
-			assert(pTab != 0);
-			pLink->pNext = pTab->pTrigger;
-			pTab->pTrigger = pLink;
-		}
+	} else {
+		pParse->pNewTrigger = pTrig;
+		pTrig = NULL;
 	}
 
  triggerfinish_cleanup:
@@ -329,7 +323,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		   alloc for it either wasn't called at all or failed.  */
 	}
 	sqlite3DeleteTrigger(db, pTrig);
-	assert(!pParse->pNewTrigger);
+	assert(!pParse->pNewTrigger || pParse->parse_only);
 	sqlite3DeleteTriggerStep(db, pStepList);
 }
 
@@ -481,7 +475,7 @@ sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger)
 		return;
 	sqlite3DeleteTriggerStep(db, pTrigger->step_list);
 	sqlite3DbFree(db, pTrigger->zName);
-	sqlite3DbFree(db, pTrigger->table);
+	free(pTrigger->table);
 	sql_expr_delete(db, pTrigger->pWhen, false);
 	sqlite3IdListDelete(db, pTrigger->pColumns);
 	sqlite3DbFree(db, pTrigger);
@@ -568,34 +562,35 @@ 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 *trigger_name)
 {
-	Trigger *pTrigger;
-	Hash *pHash;
 	struct session *user_session = current_session();
 
-	pHash = &(db->pSchema->trigHash);
-	pTrigger = sqlite3HashInsert(pHash, zName, 0);
-	if (ALWAYS(pTrigger)) {
-		if (pTrigger->pSchema == pTrigger->pTabSchema) {
-			Table *pTab = tableOfTrigger(pTrigger);
-			Trigger **pp;
-			for (pp = &pTab->pTrigger; *pp != pTrigger;
-			     pp = &((*pp)->pNext)) ;
+	struct Hash *hash = &(db->pSchema->trigHash);
+	struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL);
+	assert(trigger != NULL);
+
+	if (trigger->pSchema == trigger->pTabSchema) {
+		uint32_t space_id = space_id_by_name(BOX_SPACE_ID,
+			trigger->table, strlen(trigger->table));
+		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;
 		}
-		sqlite3DeleteTrigger(db, pTrigger);
-		user_session->sql_flags |= SQLITE_InternChanges;
 	}
+
+	sqlite3DeleteTrigger(db, trigger);
+	user_session->sql_flags |= SQLITE_InternChanges;
+	return 0;
 }
 
 /*
@@ -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) {
+		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);
 }
 
 /*
@@ -1155,4 +1146,55 @@ sqlite3TriggerColmask(Parse * pParse,	/* Parse context */
 	return mask;
 }
 
+const char *
+sql_trigger_get_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 (rc |= (node == NULL)) {
+			diag_set(OutOfMemory, sizeof(*node), "region_alloc",
+				 "node");
+			break;
+		}
+		node->name = strdup(new_table_name);
+		if (rc |= (node->name == NULL)) {
+			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) */
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: {
-	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;
 }
 
@@ -4745,7 +4716,6 @@ case OP_RenameTable: {
 	const char *zNewTableName;
 	Table *pTab;
 	FKey *pFKey;
-	Trigger *pTrig;
 	int iRootPage;
 	InitData initData;
 	char *argv[4] = {NULL, NULL, NULL, NULL};
@@ -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));
+
 	rc = tarantoolSqlite3RenameTable(pTab->tnum, zNewTableName,
 					 &zSqlStmt);
 	if (rc) goto abort_due_to_error;
@@ -4799,19 +4769,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. */
+	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;
@@ -4866,7 +4838,7 @@ case OP_DropIndex: {
  * schema consistent with what is on disk.
  */
 case OP_DropTrigger: {
-	sqlite3UnlinkAndDeleteTrigger(db, pOp->p4.z);
+	unreachable();
 	break;
 }
 #ifndef SQLITE_OMIT_TRIGGER
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 68d542c..77fa41a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -238,7 +238,6 @@ void sqlite3VdbeVerifyNoResultRow(Vdbe * p);
 VdbeOp *sqlite3VdbeAddOpList(Vdbe *, int nOp, VdbeOpList const *aOp,
 			     int iLineno);
 void sqlite3VdbeAddParseSchema2Op(Vdbe * p, int, int);
-void sqlite3VdbeAddParseSchema3Op(Vdbe * p, int);
 void sqlite3VdbeAddRenameTableOp(Vdbe * p, int, char *);
 void sqlite3VdbeChangeOpcode(Vdbe *, u32 addr, u8);
 void sqlite3VdbeChangeP1(Vdbe *, u32 addr, int P1);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 63f1978..7a5ac6f 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -410,15 +410,6 @@ sqlite3VdbeAddParseSchema2Op(Vdbe * p, int iRec, int n)
 	sqlite3VdbeAddOp3(p, OP_ParseSchema2, iRec, n, 0);
 }
 
-/*
- * Add an OP_ParseSchema3 opcode which in turn will create a trigger
- */
-void
-sqlite3VdbeAddParseSchema3Op(Vdbe * p, int iRec)
-{
-	sqlite3VdbeAddOp2(p, OP_ParseSchema3, iRec, 0);
-}
-
 void
 sqlite3VdbeAddRenameTableOp(Vdbe * p, int iTab, char* zNewName)
 {
-- 
2.7.4

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

[Vladislav Shpilevoy]

Hello. Thanks for the patchset!

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers
> Issue: https://github.com/tarantool/tarantool/issues/3273
> 
> As we are going to call parser on box.cfg() to recreate triggers
> from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.
> Inroduced box_space_id_by_name to get object id by name from custom space.

"Inroduced" - typo.

> Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid
> length in MsgPack after ALTER TABLE NAME.
> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert builded triggers, just only

"builded" - no such word.

> to do parsing. All triggers insertions and deletions are operated
> via on_replace_dd_trigger now.

Please, do not just copy-paste commit messages.

Out of the context I can not understand why "we are going to
call parser on box.cfg() to recreate triggers" is blocked by Schema and
a mystic function sqlite3BeginTrigger.

And why "Inroduced box_space_id_by_name to get object id by name
from custom space."? Box_space_id_by_name exists before you patch.

"Fixed bug in tarantoolSqlite3RenameTrigger" - this and the previous
hunks are minor patches and can be omitted here or just mentioned as
'helper patches'.

"Introduced sql_triggers field in space structure." - too minor
implementation detail.

> 
> Kirill Shcherbatov (4):
>    box: move db->pShchema init to sql_init
>    sql: fix sql len in tarantoolSqlite3RenameTrigger
>    box: introduce box_space_id_by_name
>    sql: move Triggers to server
> 
>   src/box/alter.cc        |  57 +++++++++++
>   src/box/box.cc          |  10 +-
>   src/box/box.h           |  14 +++
>   src/box/space.h         |   2 +
>   src/box/sql.c           |  60 ++++-------
>   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 --
>   14 files changed, 374 insertions(+), 216 deletions(-)
> 

[tarantool-patches] Re: [PATCH v1 1/4] box: move db->pShchema init to sql_init

[Vladislav Shpilevoy]

Thanks for the patch!

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> As we are going to call parser on box.cfg() to recreate triggers
> from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.
> 
> Part of #3273.
> ---
>   src/box/sql.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 7379cb4..47d1739 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -77,11 +77,18 @@ sql_init()
>   		panic("failed to initialize SQL subsystem");
>   
>   	assert(db != NULL);
> +	/* Initialize pSchema to use SQL parser. */

To use for what? Before what? I see it in the commit message, but lets
explain this in the comment as well.

> +	db->pSchema = sqlite3SchemaCreate(db);
> +	if (db->pSchema == NULL) {
> +		sqlite3_close(db);
> +		panic("failed to initialize SQL Schema subsystem");
> +	}
>   }
>   

[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Vladislav Shpilevoy]

Thanks for the patch! See 5 comments below.

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> Part of #3273.
> ---
>   src/box/box.cc | 10 ++++++++--
>   src/box/box.h  | 14 ++++++++++++++
>   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 02722d6..9540d85 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -877,7 +877,7 @@ box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple)
>   
>   /* schema_find_id()-like method using only public API */
>   uint32_t
> -box_space_id_by_name(const char *name, uint32_t len)
> +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len)

1. The comment now is wrong - space_id_by_name will be used exactly for
non-public API.

2. space_id_by_name must not take system_space_id as an argument. It must
always use BOX_SPACE_ID.

Do this:

/**
  * ...
  *
  * Return 0 on success, -1 on error.
  * @param[out] space_id BOX_ID_NIL - not found. Else space_id is
  *             saved.
  *
  * ...
  */
static inline int
space_id_by_name_impl(system_space_id, name, len, uint32_t *space_id);

uint32_t
space_id_by_name(name, len)
{
	return space_id_by_name_impl(BOX_SPACE_ID, name, len);
}

uint32_t
box_space_id_by_name(name, len)
{
	return space_id_by_name_impl(BOX_VSPACE_ID, name, len);
}

>   {
>   	if (len > BOX_NAME_MAX)
>   		return BOX_ID_NIL;
> diff --git a/src/box/box.h b/src/box/box.h
> index bdd5d5c..ba33113 100644
> --- a/src/box/box.h
> +++ b/src/box/box.h
> @@ -220,6 +220,20 @@ API_EXPORT int
>   box_return_tuple(box_function_ctx_t *ctx, box_tuple_t *tuple);
>   
>   /**
> + * Find space id by name in specified system space.
> + *
> + * This function performs SELECT request to _vspace system space.

3. Not _vspace.

> + * \param system_space_id space to lookup name.
> + * \param name space name
> + * \param len length of \a name
> + * \retval BOX_ID_NIL on error or if not found (check box_error_last())
> + * \retval space_id otherwise
> + * \sa box_index_id_by_name
> + */
> +uint32_t
> +space_id_by_name(uint32_t system_space_id, const char *name, uint32_t len);

4. space_id_by_name is not public function. You must not put it inside
/** \cond public */.

It must be in schema.h like other <something_by_something> functions.

5. Why can not you just port schema_find_id to C?

> +
> +/**
>    * Find space id by name.
>    *
>    * This function performs SELECT request to _vspace system space.
> 

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

[Vladislav Shpilevoy]

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?

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

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/31 14:26]:
> index b62f8ad..6e4f15d 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -53,6 +53,7 @@
>  #include "version.h"
>  #include "sequence.h"
>  #include "sql.h"
> +#include "box.h"
>  
>  /**

Whoa, if you had to include box.h something went terribly wrong.
It's a dependency loop.

Please read the SOP part about managing header file dependencies
and rethink dependencies in your code.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Kirill Shcherbatov]

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) */

[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Kirill Shcherbatov]

> 1. The comment now is wrong - space_id_by_name will be used exactly for
> non-public API.
> 2. space_id_by_name must not take system_space_id as an argument. It must
> always use BOX_SPACE_ID.
>   * Return 0 on success, -1 on error.
>   * @param[out] space_id BOX_ID_NIL - not found. Else space_id is
>   *             saved.
> 3. Not _vspace.
> 4. space_id_by_name is not public function. You must not put it inside
> /** \cond public */.
> 5. Why can not you just port schema_find_id to C?
I've ported schema_find_id to C. This required change it's signature.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2ddf920..c5055ee 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -224,28 +224,53 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
 
 uint32_t
 schema_find_id(uint32_t system_space_id, uint32_t index_id,
-	       const char *name, uint32_t len)
+	       const char *name, uint32_t len, uint32_t *space_id)
 {
-	if (len > BOX_NAME_MAX)
-		return BOX_ID_NIL;
-	struct space *space = space_cache_find_xc(system_space_id);
-	struct index *index = index_find_system_xc(space, index_id);
+	*space_id = BOX_ID_NIL;
+	if (len > BOX_NAME_MAX) {
+		diag_set(SystemError,
+			 "name length %d is greater than BOX_NAME_MAX", len);
+		return -1;
+	}
+	struct space *space = space_cache_find(system_space_id);
+	if (space == NULL)
+		return -1;
+	if (!space_is_memtx(space)) {
+		diag_set(ClientError, ER_UNSUPPORTED,
+			 space->engine->name, "system data");
+		return -1;
+	}
+	struct index *index = index_find(space, index_id);
+	if (index == NULL)
+		return -1;
 	uint32_t size = mp_sizeof_str(len);
 	struct region *region = &fiber()->gc;
 	uint32_t used = region_used(region);
-	char *key = (char *) region_alloc_xc(region, size);
-	auto guard = make_scoped_guard([=] { region_truncate(region, used); });
+	char *key = (char *)region_alloc(region, size);
+	if (key == NULL) {
+		diag_set(OutOfMemory, size, "region", "new slab");
+		return -1;
+	}
 	mp_encode_str(key, name, len);
-
-	struct iterator *it = index_create_iterator_xc(index, ITER_EQ, key, 1);
-	IteratorGuard iter_guard(it);
-
-	struct tuple *tuple = iterator_next_xc(it);
+	struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1);
+	if (it == NULL) {
+		region_truncate(region, used);
+		return -1;
+	}
+	int rc = 0;
+	struct tuple *tuple;
+	if (iterator_next(it, &tuple) != 0) {
+		rc = -1;
+		goto cleanup;
+	}
 	if (tuple) {
 		/* id is always field #1 */
-		return tuple_field_u32_xc(tuple, 0);
+		*space_id = tuple_field_u32_xc(tuple, 0);
 	}
-	return BOX_ID_NIL;
+cleanup:
+	iterator_delete(it);
+	region_truncate(region, used);
+	return rc;
 }
 
 /**
diff --git a/src/box/schema.h b/src/box/schema.h
index 1f7414f..099f9b0 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -102,6 +102,22 @@ space_is_system(struct space *space);
 struct sequence *
 sequence_by_id(uint32_t id);
 
+/**
+ * Find space id by name in specified system space with index.
+ *
+ * @param system_space_id identifier of the system space.
+ * @param index_id identifier of the index to lookup.
+ * @param name space to lookup name.
+ * @param len length of a name.
+ * @param space_id[out] space_id BOX_ID_NIL - not found.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+uint32_t
+schema_find_id(uint32_t system_space_id, uint32_t index_id,
+	       const char *name, uint32_t len, uint32_t *space_id);
+
 #if defined(__cplusplus)
 } /* extern "C" */
 
@@ -134,13 +150,6 @@ schema_free();
 
 struct space *schema_space(uint32_t id);
 
-/*
- * Find object id by object name.
- */
-uint32_t
-schema_find_id(uint32_t system_space_id, uint32_t index_id,
-	       const char *name, uint32_t len);
-
 /**
  * Insert a new function or update the old one.
  *
diff --git a/src/box/user.cc b/src/box/user.cc
index 7fa66da..3e7f466 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -450,7 +450,9 @@ user_find(uint32_t uid)
 struct user *
 user_find_by_name(const char *name, uint32_t len)
 {
-	uint32_t uid = schema_find_id(BOX_USER_ID, 2, name, len);
+	uint32_t uid;
+	if (schema_find_id(BOX_USER_ID, 2, name, len, &uid) != 0)
+		diag_raise();
 	struct user *user = user_by_id(uid);
 	if (user == NULL || user->def->type != SC_USER) {
 		diag_set(ClientError, ER_NO_SUCH_USER, tt_cstr(name, len));

[tarantool-patches] Re: [PATCH v1 1/4] box: move db->pShchema init to sql_init

[Kirill Shcherbatov]

> To use for what? Before what? I see it in the commit message, but lets
> explain this in the comment as well.

-       /* Initialize pSchema to use SQL parser. */
+       /*
+        * Initialize pSchema to use SQL parser on initialization:
+        * e.g. Trigger objects (compiled from SQL on tuple
+        * insertion in _trigger) need to refer it.
+        */

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

[Kirill Shcherbatov]

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

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;
 }
 
 /**
@@ -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(). */
 	/*
 	 * 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();
+	}
+	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;
+	}
 }
 
 struct trigger alter_space_on_replace_space = {
diff --git a/src/box/errcode.h b/src/box/errcode.h
index ba52880..eb05936 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -107,7 +107,7 @@ struct errcode_record {
 	/* 52 */_(ER_FUNCTION_EXISTS,		"Function '%s' already exists") \
 	/* 53 */_(ER_BEFORE_REPLACE_RET,	"Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
 	/* 54 */_(ER_FUNCTION_MAX,		"A limit on the total number of functions has been reached: %u") \
-	/* 55 */_(ER_UNUSED4,			"") \
+	/* 55 */_(ER_TRIGGER_EXISTS,		"Trigger '%s' already exists") \
 	/* 56 */_(ER_USER_MAX,			"A limit on the total number of users has been reached: %u") \
 	/* 57 */_(ER_NO_SUCH_ENGINE,		"Space engine '%s' does not exist") \
 	/* 58 */_(ER_RELOAD_CFG,		"Can't set option '%s' dynamically") \
diff --git a/src/box/space.h b/src/box/space.h
index b8d29ca..0413cd0 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -146,6 +146,8 @@ struct space {
 	struct rlist on_replace;
 	/** Triggers fired before space statement */
 	struct rlist on_stmt_begin;
+	/** SQL Trigger list. */
+	struct Trigger *sql_triggers;
 	/**
 	 * The number of *enabled* indexes in the space.
 	 *
diff --git a/src/box/sql.c b/src/box/sql.c
index 0eded6f..6ecb226 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1223,9 +1223,6 @@ space_foreach_put_cb(struct space *space, void *udata)
 /* Load database schema from Tarantool. */
 void tarantoolSqlite3LoadSchema(InitData *init)
 {
-	box_iterator_t *it;
-	box_tuple_t *tuple;
-
 	sql_schema_put(
 		init, TARANTOOL_SYS_SCHEMA_NAME,
 		BOX_SCHEMA_ID, 0,
@@ -1294,42 +1291,6 @@ void tarantoolSqlite3LoadSchema(InitData *init)
 		init->rc = SQL_TARANTOOL_ERROR;
 		return;
 	}
-
-	/* Read _trigger */
-	it = box_index_iterator(BOX_TRIGGER_ID, 0, ITER_GE,
-				nil_key, nil_key + sizeof(nil_key));
-
-	if (it == NULL) {
-		init->rc = SQL_TARANTOOL_ITERATOR_FAIL;
-		return;
-	}
-
-	while (box_iterator_next(it, &tuple) == 0 && tuple != NULL) {
-		const char *field, *ptr;
-		char *name, *sql;
-		unsigned len;
-		assert(tuple_field_count(tuple) == 2);
-
-		field = tuple_field(tuple, 0);
-		assert (field != NULL);
-		ptr = mp_decode_str(&field, &len);
-		name = strndup(ptr, len);
-
-		field = tuple_field(tuple, 1);
-		assert (field != NULL);
-		mp_decode_array(&field);
-		ptr = mp_decode_str(&field, &len);
-		assert (strncmp(ptr, "sql", 3) == 0);
-
-		ptr = mp_decode_str(&field, &len);
-		sql = strndup(ptr, len);
-
-		sql_schema_put(init, name, 0, 0, sql);
-
-		free(name);
-		free(sql);
-	}
-	box_iterator_free(it);
 }
 
 /*********************************************************************
@@ -1737,6 +1698,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
 	return space->def->fields[fieldno].default_value_expr;
 }
 
+struct Trigger *
+space_trigger_list(uint32_t space_id)
+{
+	struct space *space = space_cache_find(space_id);
+	assert(space != NULL);
+	assert(space->def != NULL);
+	return space->sql_triggers;
+}
+
 struct space_def *
 sql_ephemeral_space_def_new(Parse *parser, const char *name)
 {
diff --git a/src/box/sql.h b/src/box/sql.h
index 35aacc3..f4d909f 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -84,6 +84,17 @@ struct Expr *
 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.
+ *
+ * @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.
@@ -92,6 +103,39 @@ void
 sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
 
 /**
+ * Remove a trigger from the hash tables of the sqlite* pointer.
+ * @param db SQL handle.
+ * @param name a name of trigger object to delete.
+ * @param name_len length of the name.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_trigger_delete_by_name(struct sqlite3 *db, const char *name, int name_len);
+
+/**
+ * Get server triggers list by space_id.
+ * @param space_id Space ID.
+ *
+ * @retval trigger AST list.
+ */
+struct Trigger *
+space_trigger_list(uint32_t space_id);
+
+/**
+ * Perform insert trigger in appropriate space.
+ * @param trigger object to append.
+ * @param name  a name of trigger object to insert.
+ * @param name_len length of the name.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_trigger_insert(struct Trigger *trigger, const char *name, int name_len);
+
+/**
  * Store duplicate of a parsed expression into @a parser.
  * @param parser Parser context.
  * @param select Select to extract from.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28e4d7a..40f693b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2289,16 +2289,14 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
-	 * _trigger. OP_DropTrigger will remove it from internal
-	 * SQL structures.
+	 * _trigger. on_replace_dd_trigger will remove it from
+	 * internal SQL structures.
 	 *
 	 * Do not account triggers deletion - they will be
 	 * accounted in DELETE from _space below.
 	 */
 	parse_context->nested++;
-	Table *table = sqlite3HashFind(&parse_context->db->pSchema->tblHash,
-				       space->def->name);
-	struct Trigger *trigger = table->pTrigger;
+	struct Trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
 		sqlite3DropTriggerPtr(parse_context, trigger);
 		trigger = trigger->pNext;
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/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)
+		return 0;
 	if (onError == ON_CONFLICT_ACTION_DEFAULT) {
 		if (pDest->iPKey >= 0)
 			onError = pDest->keyConf;
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. */
+	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 */
 };
@@ -4027,7 +4026,6 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
 TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
 				      u8);
 TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
-void sqlite3UnlinkAndDeleteTrigger(sqlite3 *, const char *);
 u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
 			  int);
 #define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
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()) {
+			error = tarantoolErrorMessage();
+		} else {
+			error = sqlite3ErrStr(pParse->rc);
+		}
+		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
+	 * 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);
+		goto cleanup;
+	}
+	trigger = parser.pNewTrigger;
+cleanup:
+	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"
+#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))
 		goto trigger_cleanup;
-	}
-	pTab = sql_list_lookup_table(pParse, pTableName);
-	if (!pTab) {
-		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)) {
-		goto trigger_cleanup;
-	}
-	if (sqlite3HashFind(&(db->pSchema->trigHash), zName)) {
-		if (!noErr) {
-			sqlite3ErrorMsg(pParse, "trigger %s already exists",
-					zName);
-		} else {
-			assert(!db->init.busy);
-		}
+	if (zName == NULL)
 		goto trigger_cleanup;
-	}
 
-	/* 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");
+	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
 		goto trigger_cleanup;
-	}
 
-	/* INSTEAD of triggers are only for views and views only support INSTEAD
-	 * of triggers.
-	 */
-	if (space_is_view(pTab) && tr_tm != TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
-				(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
-				pTableName, 0);
-		goto trigger_cleanup;
-	}
-	if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF"
-				" trigger on table: %S", pTableName, 0);
-		goto trigger_cleanup;
+	/* FIXME: Move all checks in VDBE #3435. */
+	if (!pParse->parse_only) {
+		table = sql_list_lookup_table(pParse, pTableName);
+		if (table == NULL)
+			goto trigger_cleanup;
+
+		if (sqlite3HashFind(&db->pSchema->trigHash, zName)) {
+			if (!noErr) {
+				diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
+				pParse->rc = SQL_TARANTOOL_ERROR;
+			} else {
+				assert(!db->init.busy);
+			}
+			goto trigger_cleanup;
+		}
+
+		/* Do not create a trigger on a system table. */
+		if (sqlite3StrNICmp(table->def->name, "sqlite_", 7) == 0) {
+			sqlite3ErrorMsg(pParse,
+					"cannot create trigger on system table");
+			goto trigger_cleanup;
+		}
+
+		/*
+		 * INSTEAD of triggers are only for views and
+		 * views only support INSTEAD of triggers.
+		 */
+		if (table->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);
+			goto trigger_cleanup;
+		}
+		if (!table->def->opts.is_view && tr_tm == TK_INSTEAD) {
+			sqlite3ErrorMsg(pParse,
+					"cannot create INSTEAD OF trigger "
+					"on table: %S", pTableName, 0);
+			goto trigger_cleanup;
+		}
 	}
 
-	/* 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),
+			   &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))
+		goto trigger_cleanup;
 	assert(pParse->pNewTrigger == 0);
 	pParse->pNewTrigger = pTrigger;
 
@@ -210,7 +231,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 	DbFixer sFix;		/* Fixer object */
 	Token nameToken;	/* Trigger name for error reporting */
 
-	pParse->pNewTrigger = 0;
+	pParse->pNewTrigger = NULL;
 	if (NEVER(pParse->nErr) || !pTrig)
 		goto triggerfinish_cleanup;
 	zName = pTrig->zName;
@@ -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.
 	 */
-	if (!db->init.busy) {
+	if (!pParse->parse_only) {
 		Vdbe *v;
 		int zOptsSz;
 		Table *pSysTrigger;
@@ -288,37 +310,11 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 		sqlite3VdbeAddOp1(v, OP_Close, iCursor);
 
-		/* parseschema3(reg(iFirstCol), ref(iFirstCol)+1) */
-		iFirstCol = pParse->nMem + 1;
-		pParse->nMem += 2;
-
 		sql_set_multi_write(pParse, false);
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol, 0,
-				  zName, P4_STATIC);
-
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol + 1, 0,
-				  zSql, P4_DYNAMIC);
 		sqlite3ChangeCookie(pParse);
-		sqlite3VdbeAddParseSchema3Op(v, iFirstCol);
-	}
-
-	if (db->init.busy) {
-		Trigger *pLink = pTrig;
-		Hash *pHash = &db->pSchema->trigHash;
-		pTrig = sqlite3HashInsert(pHash, zName, pTrig);
-		if (pTrig) {
-			sqlite3OomFault(db);
-		} else if (pLink->pSchema == pLink->pTabSchema) {
-			Table *pTab;
-			pTab =
-			    sqlite3HashFind(&pLink->pTabSchema->tblHash,
-					    pLink->table);
-			assert(pTab != 0);
-			pLink->pNext = pTab->pTrigger;
-			pTab->pTrigger = pLink;
-		}
+	} else {
+		pParse->pNewTrigger = pTrig;
+		pTrig = NULL;
 	}
 
  triggerfinish_cleanup:
@@ -329,7 +325,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		   alloc for it either wasn't called at all or failed.  */
 	}
 	sql_trigger_delete(db, pTrig);
-	assert(!pParse->pNewTrigger);
+	assert(!pParse->pNewTrigger || pParse->parse_only);
 	sqlite3DeleteTriggerStep(db, pStepList);
 }
 
@@ -478,7 +474,6 @@ sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
 		return;
 	sqlite3DeleteTriggerStep(db, trigger->step_list);
 	sqlite3DbFree(db, trigger->zName);
-	sqlite3DbFree(db, trigger->table);
 	sql_expr_delete(db, trigger->pWhen, false);
 	sqlite3IdListDelete(db, trigger->pColumns);
 	sqlite3DbFree(db, trigger);
@@ -533,16 +528,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
@@ -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);
+	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);
+
+	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;
+	}
 
-	pHash = &(db->pSchema->trigHash);
-	pTrigger = sqlite3HashInsert(pHash, zName, 0);
-	if (ALWAYS(pTrigger)) {
-		if (pTrigger->pSchema == pTrigger->pTabSchema) {
-			Table *pTab = tableOfTrigger(pTrigger);
-			Trigger **pp;
-			for (pp = &pTab->pTrigger; *pp != pTrigger;
-			     pp = &((*pp)->pNext)) ;
-			*pp = (*pp)->pNext;
-		}
-		sql_trigger_delete(db, pTrigger);
-		user_session->sql_flags |= SQLITE_InternChanges;
+	sql_trigger_delete(db, trigger);
+	region_truncate(&fiber()->gc, used);
+	return 0;
+}
+
+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");
+		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;
+	}
+
+	void *ret = sqlite3HashInsert(hash, trigger->zName, trigger);
+	if (ret == trigger) {
+		diag_set(OutOfMemory, 0, "sqlite3HashInsert", "ret");
+		return -1;
 	}
+	assert(ret == NULL);
+	trigger->pNext = space->sql_triggers;
+	space->sql_triggers = trigger;
+	return 0;
 }
 
 /*
@@ -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)
 		*pMask = mask;
-	}
-	return (mask ? pList : 0);
+	return mask ? trigger_list : 0;
 }
 
 /*
@@ -835,7 +849,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
@@ -952,7 +966,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
@@ -1077,14 +1091,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
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
@@ -4686,46 +4686,6 @@ case OP_ParseSchema2: {
 	break;
 }
 
-/* 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: {
-	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;
-	}
-	break;
-}
-
 /* Opcode: RenameTable P1 * * P4 *
  * Synopsis: P1 = root, P4 = name
  *
@@ -4745,7 +4705,6 @@ case OP_RenameTable: {
 	const char *zNewTableName;
 	Table *pTab;
 	FKey *pFKey;
-	Trigger *pTrig;
 	int iRootPage;
 	InitData initData;
 	char *argv[4] = {NULL, NULL, NULL, NULL};
@@ -4758,7 +4717,6 @@ 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,
@@ -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. */
+	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;
@@ -4857,18 +4817,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
 
 /* Opcode: Program P1 P2 P3 P4 P5
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 68d542c..77fa41a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -238,7 +238,6 @@ void sqlite3VdbeVerifyNoResultRow(Vdbe * p);
 VdbeOp *sqlite3VdbeAddOpList(Vdbe *, int nOp, VdbeOpList const *aOp,
 			     int iLineno);
 void sqlite3VdbeAddParseSchema2Op(Vdbe * p, int, int);
-void sqlite3VdbeAddParseSchema3Op(Vdbe * p, int);
 void sqlite3VdbeAddRenameTableOp(Vdbe * p, int, char *);
 void sqlite3VdbeChangeOpcode(Vdbe *, u32 addr, u8);
 void sqlite3VdbeChangeP1(Vdbe *, u32 addr, int P1);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 679bd0b..7b3c13d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -410,15 +410,6 @@ sqlite3VdbeAddParseSchema2Op(Vdbe * p, int iRec, int n)
 	sqlite3VdbeAddOp3(p, OP_ParseSchema2, iRec, n, 0);
 }
 
-/*
- * Add an OP_ParseSchema3 opcode which in turn will create a trigger
- */
-void
-sqlite3VdbeAddParseSchema3Op(Vdbe * p, int iRec)
-{
-	sqlite3VdbeAddOp2(p, OP_ParseSchema3, iRec, 0);
-}
-
 void
 sqlite3VdbeAddRenameTableOp(Vdbe * p, int iTab, char* zNewName)
 {
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 3f6dc07..5e7573a 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -170,8 +170,8 @@ test:do_execsql_test(
 
 data = {
     { 1,  [[ trigger1 ]], {0}},
-    { 2,  [[ Trigger1 ]], {1, "trigger TRIGGER1 already exists"}},
-    { 3,  [["TRIGGER1"]], {1, "trigger TRIGGER1 already exists"}},
+    { 2,  [[ Trigger1 ]], {1, "Trigger 'TRIGGER1' already exists"}},
+    { 3,  [["TRIGGER1"]], {1, "Trigger 'TRIGGER1' already exists"}},
     { 4,  [["trigger1" ]], {0}}
 }
 
diff --git a/test/sql-tap/trigger1.test.lua b/test/sql-tap/trigger1.test.lua
index 40daba4..ed6ee36 100755
--- a/test/sql-tap/trigger1.test.lua
+++ b/test/sql-tap/trigger1.test.lua
@@ -101,7 +101,7 @@ test:do_catchsql_test(
          END
     ]], {
         -- <trigger1-1.2.1>
-        1, "trigger TR1 already exists"
+        1, "Trigger 'TR1' already exists"
         -- </trigger1-1.2.1>
     })
 
@@ -113,7 +113,7 @@ test:do_catchsql_test(
          END
     ]], {
         -- <trigger1-1.2.2>
-        1, [[trigger TR1 already exists]]
+        1, [[Trigger 'TR1' already exists]]
         -- </trigger1-1.2.2>
     })
 
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
new file mode 100644
index 0000000..23bdcfd
--- /dev/null
+++ b/test/sql/triggers.test.lua
@@ -0,0 +1,72 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+--
+-- 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; ]])
+box.space._trigger:select()
+
+-- checks for LUA tuples
+tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1T", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+box.space._trigger:select()
+
+
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+-- test duplicate index error
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+
+-- test triggers
+tuple = {"T2T", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+box.space._trigger:insert(tuple)
+tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+box.space._trigger:insert(tuple)
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(2);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- test t1t after t2t and t3t drop
+box.sql.execute("DROP TRIGGER T2T;")
+box.space._trigger:delete("T2T")
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(3);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- insert new SQL t2t and t3t
+box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
+box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+box.sql.execute("SELECT * FROM t2;")
+
+-- clean up
+box.space._trigger:delete("t1t")
+box.space._trigger:delete("t2t")
+box.space._trigger:delete("t3t")
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
+box.space._trigger:select()
+
+
+test_run:cmd("clear filter")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Vladislav Shpilevoy]

Hello. Thanks for the patch! Please, put the whole new patch at the end
of a letter after multiple '======'. It helps in review, thanks.

See 6 comments below.

1. Lets change commit title: the patch does not introduce
box_space_id_by_name. It was before the patch.

On 01/06/2018 23:24, Kirill Shcherbatov wrote:
>> 1. The comment now is wrong - space_id_by_name will be used exactly for
>> non-public API.
>> 2. space_id_by_name must not take system_space_id as an argument. It must
>> always use BOX_SPACE_ID.
>>    * Return 0 on success, -1 on error.
>>    * @param[out] space_id BOX_ID_NIL - not found. Else space_id is
>>    *             saved.
>> 3. Not _vspace.
>> 4. space_id_by_name is not public function. You must not put it inside
>> /** \cond public */.
>> 5. Why can not you just port schema_find_id to C?
> I've ported schema_find_id to C. This required change it's signature.
> 
> diff --git a/src/box/schema.cc b/src/box/schema.cc
> index 2ddf920..c5055ee 100644
> --- a/src/box/schema.cc
> +++ b/src/box/schema.cc
> @@ -224,28 +224,53 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
>   
>   uint32_t
>   schema_find_id(uint32_t system_space_id, uint32_t index_id,
> -	       const char *name, uint32_t len)
> +	       const char *name, uint32_t len, uint32_t *space_id)

2. How can you return -1, when the return type is unsigned?

3. It is not 'space_id' speaking in general. This function is used in
user.cc also to search user id by the name. Please, find another name
for the parameter.

>   {
> -	if (len > BOX_NAME_MAX)
> -		return BOX_ID_NIL;
> -	struct space *space = space_cache_find_xc(system_space_id);
> -	struct index *index = index_find_system_xc(space, index_id);
> +	*space_id = BOX_ID_NIL;
> +	if (len > BOX_NAME_MAX) {
> +		diag_set(SystemError,
> +			 "name length %d is greater than BOX_NAME_MAX", len);
> +		return -1;
> +	}
> +	struct space *space = space_cache_find(system_space_id);
> +	if (space == NULL)
> +		return -1;
> +	if (!space_is_memtx(space)) {
> +		diag_set(ClientError, ER_UNSUPPORTED,
> +			 space->engine->name, "system data");
> +		return -1;
> +	}
> +	struct index *index = index_find(space, index_id);
> +	if (index == NULL)
> +		return -1;
>   	uint32_t size = mp_sizeof_str(len);
>   	struct region *region = &fiber()->gc;
>   	uint32_t used = region_used(region);
> -	char *key = (char *) region_alloc_xc(region, size);
> -	auto guard = make_scoped_guard([=] { region_truncate(region, used); });
> +	char *key = (char *)region_alloc(region, size);
> +	if (key == NULL) {
> +		diag_set(OutOfMemory, size, "region", "new slab");

4. "new slab" -> "key". OutOfMemory takes size, alloc function and
the result variable name.

> +		return -1;
> +	}
>   	mp_encode_str(key, name, len);
> -
> -	struct iterator *it = index_create_iterator_xc(index, ITER_EQ, key, 1);
> -	IteratorGuard iter_guard(it);
> -
> -	struct tuple *tuple = iterator_next_xc(it);
> +	struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1);
> +	if (it == NULL) {
> +		region_truncate(region, used);
> +		return -1;
> +	}
> +	int rc = 0;
> +	struct tuple *tuple;
> +	if (iterator_next(it, &tuple) != 0) {
> +		rc = -1;
> +		goto cleanup;
> +	}
>   	if (tuple) {
>   		/* id is always field #1 */
> -		return tuple_field_u32_xc(tuple, 0);
> +		*space_id = tuple_field_u32_xc(tuple, 0);
>   	}
> -	return BOX_ID_NIL;
> +cleanup:
> +	iterator_delete(it);
> +	region_truncate(region, used);
> +	return rc;

5. You should avoid 'goto cleanup' here.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index c5055eefe..a08588aef 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -257,17 +257,12 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
  		region_truncate(region, used);
  		return -1;
  	}
-	int rc = 0;
  	struct tuple *tuple;
-	if (iterator_next(it, &tuple) != 0) {
-		rc = -1;
-		goto cleanup;
-	}
-	if (tuple) {
-		/* id is always field #1 */
+	int rc = iterator_next(it, &tuple);
+	if (rc == 0 && tuple != NULL) {
+		/* ID is always field #1. */
  		*space_id = tuple_field_u32_xc(tuple, 0);
  	}
-cleanup:
  	iterator_delete(it);
  	region_truncate(region, used);
  	return rc;

>   }
> diff --git a/src/box/schema.h b/src/box/schema.h
> index 1f7414f..099f9b0 100644
> --- a/src/box/schema.h
> +++ b/src/box/schema.h
> @@ -102,6 +102,22 @@ space_is_system(struct space *space);
>   struct sequence *
>   sequence_by_id(uint32_t id);
>   
> +/**
> + * Find space id by name in specified system space with index.
> + *
> + * @param system_space_id identifier of the system space.
> + * @param index_id identifier of the index to lookup.
> + * @param name space to lookup name.
> + * @param len length of a name.
> + * @param space_id[out] space_id BOX_ID_NIL - not found.

6. Same. Do not mention space. It is a function to find ID of
any system object. Not only space. This function is applicable
for _index, _space, _user, _func.

> + *
> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +uint32_t
> +schema_find_id(uint32_t system_space_id, uint32_t index_id,
> +	       const char *name, uint32_t len, uint32_t *space_id);
> +
>   #if defined(__cplusplus)
>   } /* extern "C" */

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

[Vladislav Shpilevoy]

Thanks for the patch! Why did not you send it? See 1 comment below.

> commit 7e6143c51e520f68db9a4fe96d6980ba65f653d8
> Author: Kirill Shcherbatov <kshcherbatov@gmail.com>
> Date:   Fri Jun 1 15:38:30 2018 +0300
> 
>     sql: refactor sql_expr_compile to return AST
> 
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 42c70a255..2555adbf4 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -539,9 +539,8 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
>  	return nErr;
>  }
>  
> -int
> -sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
> -		 struct Expr **result)
> +struct Expr *
> +sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
>  {
>  	const char *outer = "SELECT ";
>  	int len = strlen(outer) + expr_len;
> @@ -550,19 +549,23 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len,
>  	sql_parser_create(&parser, db);
>  	parser.parse_only = true;
>  
> +	struct Expr *expression = NULL;
>  	char *stmt = (char *)region_alloc(&parser.region, len + 1);
>  	if (stmt == NULL) {
>  		diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
> -		return -1;
> +		goto cleanup;
>  	}
>  	sprintf(stmt, "%s%.*s", outer, expr_len, expr);
>  
> -	char *unused;
> -	if (sqlite3RunParser(&parser, stmt, &unused) != SQLITE_OK) {
> -		diag_set(ClientError, ER_SQL_EXECUTE, expr);
> -		return -1;
> +	char *sql_error;
> +	if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK) {
> +		char *error = tt_static_buf();
> +		snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
> +		diag_set(ClientError, ER_SQL_EXECUTE, stmt);
> +		goto cleanup;

1. What is happening here? 'Error' variable is unused, as well as sql_error
as I can see.

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

[Vladislav Shpilevoy]

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;

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

[Kirill Shcherbatov]

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

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

> 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.
        new_space->sql_triggers = old_space->sql_triggers;
-       old_space->sql_triggers = NULL;

> 3. Garbage diff.
dropped

> 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.
The patch at the end of this message contains this changes.

> 5. Why do you need the guard here? You can delete trigger explicitly before
> diag_raise() if insert() != 0.
@@ -3127,13 +3127,11 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                        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)
+                                      trigger_name_len) != 0) {
+                       sql_trigger_delete(sql_get(), trigger);
                        diag_raise();
-               trigger_guard.is_active = false;
+               }
        }
 }


> 6. triggers? There is one trigger. And 'is refer to' is not correct sentence. 'Refer'
> is a verb.
-       /** The ID of space the triggers is refer to. */
+       /** The ID of space the trigger is refer to. */


> 7. != NULL
> 8. Do not use {} when both 'if' and 'else' are one lines.
-               if (is_tarantool_error(pParse->rc) && tarantoolErrorMessage()) {
+               if (is_tarantool_error(pParse->rc) &&
+                   tarantoolErrorMessage() != NULL)
                        error = tarantoolErrorMessage();
-               } else {
+               else
                        error = sqlite3ErrStr(pParse->rc);
-               }


> 9. is exported.
-        * Trigger is exporting with pNewTrigger field when
+        * Trigger is exported with pNewTrigger field when
> 10. Same as in the previous patch: error and sql_error are unused.
> 11. You should avoid 'goto cleanup' here.
-               goto cleanup;
+       } else {
+               trigger = parser.pNewTrigger;
        }
-       trigger = parser.pNewTrigger;
-cleanup:
        sql_parser_destroy(&parser);
        return trigger;
 }


>> +#include "box/box.h"
> 12. Unused.
-#include "box/box.h"


>> +	if (sqlite3FixSrcList(&sFix, pTableName))
> 
> 13. != 0
+       if (sqlite3FixSrcList(&sFix, pTableName) != 0)


>> +	if (schema_find_id(BOX_SPACE_ID, 2, pTableName->a[0].zName,
>> +			strlen(pTableName->a[0].zName),
> 
> 14. Wrong indentation.
This place is slightly different now:
+       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;
+               goto trigger_cleanup;
+       }
+       if (space_id == BOX_ID_NIL) {
+               diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+               pParse->rc = SQL_TARANTOOL_ERROR;
+               goto trigger_cleanup;
+       }


>> +	if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
>> +		(pColumns != NULL && pTrigger->pColumns == NULL))
> 
> 15. Same.
        if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
-               (pColumns != NULL && pTrigger->pColumns == NULL))
+           (pColumns != NULL && pTrigger->pColumns == NULL))

> 16. typo
-        * Tarantool for non-parsig mode or export trigger.
+        * Tarantool for non-parsing mode or export trigger.

>> +	uint32_t used = region_used(&fiber()->gc);
>> +	char *trigger_name = region_alloc(&fiber()->gc, name_len + 3);
> 
> 17. Why +3?
Some legacy... Fixed, this place is differ now.
+                       tuple_field_str_xc(old_tuple, 0, &trigger_name_len);
+               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 Hash *hash = &(db->pSchema->trigHash);
>> +	struct Trigger *trigger = sqlite3HashInsert(hash, trigger_name, NULL);
>> +	assert(trigger != NULL);
> 
> 18. Why != NULL? Can't sqlite3HashInsert fail?
It was assert for inconsistent tarantool state. This place is differ now, it is not actual.
> 
>> +
>> +	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?

Accounted, this place is differ now.
> 
>> +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.
Accounted, implemented in on_replace_dd_trigger.

>>   	int mask = 0;

>> -	if (pMask) {
>> +	if (pMask)
> 
> 21. != NULL
if (pMask != 0)
		*pMask = mask;
> 22. triggers
+       /* Rename all triggers created on this table. */



=======================================================================

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                      | 140 ++++++++++++++++++
 src/box/errcode.h                     |   2 +-
 src/box/space.h                       |   2 +
 src/box/sql.c                         |  48 ++-----
 src/box/sql.h                         |  42 ++++++
 src/box/sql/build.c                   |   8 +-
 src/box/sql/fkey.c                    |   2 -
 src/box/sql/insert.c                  |   6 +-
 src/box/sql/sqliteInt.h               |   7 +-
 src/box/sql/tokenize.c                |  28 +++-
 src/box/sql/trigger.c                 | 259 ++++++++++++++++------------------
 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              | 187 ++++++++++++++++++++++++
 test/sql/triggers.test.lua            |  69 +++++++++
 19 files changed, 629 insertions(+), 276 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..23cd4a3 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -551,6 +551,8 @@ 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;
 }
 
 /**
@@ -3091,6 +3093,55 @@ lock_before_dd(struct trigger *trigger, void *event)
 	latch_lock(&schema_lock);
 }
 
+static void
+triggers_task_rollback(struct trigger *trigger, void *event)
+{
+	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
+	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+	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);
+	}
+}
+
+
+static void
+triggers_task_commit(struct trigger *trigger, void *event)
+{
+	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
+	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+
+	if (stmt->old_tuple != NULL) {
+		/* DELETE, REPLACE trigger. */
+		sql_trigger_delete(sql_get(), old_trigger);
+	}
+}
+
 /**
  * A trigger invoked on replace in a space containing
  * SQL triggers.
@@ -3100,6 +3151,95 @@ 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);
+
+	char *trigger_name = NULL;
+	struct Trigger *new_trigger = NULL;
+	if (old_tuple != NULL) {
+		/* DROP, REPLACE trigger. */
+		uint32_t trigger_name_len;
+		const char *trigger_name_src =
+			tuple_field_str_xc(old_tuple, 0, &trigger_name_len);
+		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;
+	}
+	if (new_tuple != NULL) {
+		/* 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, 1, MP_MAP);
+		struct space_opts opts;
+		struct region *region = &fiber()->gc;
+		space_opts_decode(&opts, space_opts, region);
+		new_trigger = sql_trigger_compile(sql_get(), opts.sql);
+		if (new_trigger == NULL)
+			diag_raise();
+
+		if (strncmp(trigger_name_src, sql_trigger_name(new_trigger),
+			    trigger_name_len) != 0) {
+			sql_trigger_delete(sql_get(), new_trigger);
+			tnt_raise(ClientError, ER_SQL,
+				  "tuple trigger name does not match extracted "
+				  "from SQL");
+		}
+		trigger_name = sql_trigger_name(new_trigger);
+	}
+
+	if (old_tuple != NULL && new_tuple == NULL) {
+		/* DELETE trigger. */
+		struct Trigger *old_trigger;
+		int rc =
+			sql_trigger_replace(sql_get(), trigger_name, NULL,
+					    &old_trigger);
+		(void)rc;
+		assert(rc == 0);
+		assert(old_trigger != NULL);
+
+		on_commit->data = old_trigger;
+		on_rollback->data = old_trigger;
+	} else if (new_tuple != NULL && old_tuple == NULL) {
+		/* INSERT trigger. */
+		struct Trigger *old_trigger;
+		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
+					&old_trigger) != 0) {
+			sql_trigger_delete(sql_get(), new_trigger);
+			diag_raise();
+		}
+		assert(old_trigger == NULL);
+
+		on_commit->data = NULL;
+		on_rollback->data = new_trigger;
+	} else {
+		/* REPLACE trigger. */
+		struct Trigger *old_trigger;
+		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
+					&old_trigger) != 0) {
+			sql_trigger_delete(sql_get(), new_trigger);
+			diag_raise();
+		}
+		assert(old_trigger != NULL);
+
+		on_commit->data = old_trigger;
+		on_rollback->data = old_trigger;
+	}
+
+	txn_on_rollback(txn, on_rollback);
+	txn_on_commit(txn, on_commit);
+
+
 }
 
 struct trigger alter_space_on_replace_space = {
diff --git a/src/box/errcode.h b/src/box/errcode.h
index ba52880..eb05936 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -107,7 +107,7 @@ struct errcode_record {
 	/* 52 */_(ER_FUNCTION_EXISTS,		"Function '%s' already exists") \
 	/* 53 */_(ER_BEFORE_REPLACE_RET,	"Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
 	/* 54 */_(ER_FUNCTION_MAX,		"A limit on the total number of functions has been reached: %u") \
-	/* 55 */_(ER_UNUSED4,			"") \
+	/* 55 */_(ER_TRIGGER_EXISTS,		"Trigger '%s' already exists") \
 	/* 56 */_(ER_USER_MAX,			"A limit on the total number of users has been reached: %u") \
 	/* 57 */_(ER_NO_SUCH_ENGINE,		"Space engine '%s' does not exist") \
 	/* 58 */_(ER_RELOAD_CFG,		"Can't set option '%s' dynamically") \
diff --git a/src/box/space.h b/src/box/space.h
index b8d29ca..0413cd0 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -146,6 +146,8 @@ struct space {
 	struct rlist on_replace;
 	/** Triggers fired before space statement */
 	struct rlist on_stmt_begin;
+	/** SQL Trigger list. */
+	struct Trigger *sql_triggers;
 	/**
 	 * The number of *enabled* indexes in the space.
 	 *
diff --git a/src/box/sql.c b/src/box/sql.c
index 0eded6f..6ecb226 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1223,9 +1223,6 @@ space_foreach_put_cb(struct space *space, void *udata)
 /* Load database schema from Tarantool. */
 void tarantoolSqlite3LoadSchema(InitData *init)
 {
-	box_iterator_t *it;
-	box_tuple_t *tuple;
-
 	sql_schema_put(
 		init, TARANTOOL_SYS_SCHEMA_NAME,
 		BOX_SCHEMA_ID, 0,
@@ -1294,42 +1291,6 @@ void tarantoolSqlite3LoadSchema(InitData *init)
 		init->rc = SQL_TARANTOOL_ERROR;
 		return;
 	}
-
-	/* Read _trigger */
-	it = box_index_iterator(BOX_TRIGGER_ID, 0, ITER_GE,
-				nil_key, nil_key + sizeof(nil_key));
-
-	if (it == NULL) {
-		init->rc = SQL_TARANTOOL_ITERATOR_FAIL;
-		return;
-	}
-
-	while (box_iterator_next(it, &tuple) == 0 && tuple != NULL) {
-		const char *field, *ptr;
-		char *name, *sql;
-		unsigned len;
-		assert(tuple_field_count(tuple) == 2);
-
-		field = tuple_field(tuple, 0);
-		assert (field != NULL);
-		ptr = mp_decode_str(&field, &len);
-		name = strndup(ptr, len);
-
-		field = tuple_field(tuple, 1);
-		assert (field != NULL);
-		mp_decode_array(&field);
-		ptr = mp_decode_str(&field, &len);
-		assert (strncmp(ptr, "sql", 3) == 0);
-
-		ptr = mp_decode_str(&field, &len);
-		sql = strndup(ptr, len);
-
-		sql_schema_put(init, name, 0, 0, sql);
-
-		free(name);
-		free(sql);
-	}
-	box_iterator_free(it);
 }
 
 /*********************************************************************
@@ -1737,6 +1698,15 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
 	return space->def->fields[fieldno].default_value_expr;
 }
 
+struct Trigger *
+space_trigger_list(uint32_t space_id)
+{
+	struct space *space = space_cache_find(space_id);
+	assert(space != NULL);
+	assert(space->def != NULL);
+	return space->sql_triggers;
+}
+
 struct space_def *
 sql_ephemeral_space_def_new(Parse *parser, const char *name)
 {
diff --git a/src/box/sql.h b/src/box/sql.h
index 35aacc3..885f0a7 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -84,6 +84,17 @@ struct Expr *
 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.
+ *
+ * @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.
@@ -92,6 +103,37 @@ void
 sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
 
 /**
+ * Get server triggers list by space_id.
+ * @param space_id Space ID.
+ *
+ * @retval trigger AST list.
+ */
+struct Trigger *
+space_trigger_list(uint32_t space_id);
+
+/**
+ * Perform replace trigger in SQL internals with new AST object.
+ * @param db SQL handle.
+ * @param name a name of the trigger.
+ * @param trigger AST object to insert.
+ * @param[out] old trigger object from if exists.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_trigger_replace(struct sqlite3 *db, const char *name,
+		    struct Trigger *trigger, struct Trigger **old_trigger);
+
+/**
+ * Get trigger name by trigger AST object.
+ * @param trigger AST object.
+ * @return trigger name string.
+ */
+char *
+sql_trigger_name(struct Trigger *trigger);
+
+/**
  * Store duplicate of a parsed expression into @a parser.
  * @param parser Parser context.
  * @param select Select to extract from.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28e4d7a..40f693b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2289,16 +2289,14 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	/*
 	 * Drop all triggers associated with the table being
 	 * dropped. Code is generated to remove entries from
-	 * _trigger. OP_DropTrigger will remove it from internal
-	 * SQL structures.
+	 * _trigger. on_replace_dd_trigger will remove it from
+	 * internal SQL structures.
 	 *
 	 * Do not account triggers deletion - they will be
 	 * accounted in DELETE from _space below.
 	 */
 	parse_context->nested++;
-	Table *table = sqlite3HashFind(&parse_context->db->pSchema->tblHash,
-				       space->def->name);
-	struct Trigger *trigger = table->pTrigger;
+	struct Trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
 		sqlite3DropTriggerPtr(parse_context, trigger);
 		trigger = trigger->pNext;
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 70ebef8..5fcf6a5 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1439,8 +1439,6 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 			pStep->op = TK_UPDATE;
 		}
 		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/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)
+		return 0;
 	if (onError == ON_CONFLICT_ACTION_DEFAULT) {
 		if (pDest->iPKey >= 0)
 			onError = pDest->keyConf;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index ecbd573..276f881 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,13 @@ 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 trigger 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 */
 };
@@ -4027,7 +4025,6 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
 TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
 				      u8);
 TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
-void sqlite3UnlinkAndDeleteTrigger(sqlite3 *, const char *);
 u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
 			  int);
 #define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 817a282..d769d14 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"
@@ -123,6 +124,7 @@ static const char sql_ascii_class[] = {
  * the #include below.
  */
 #include "keywordhash.h"
+#include "tarantoolInt.h"
 
 #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0)
 
@@ -534,7 +536,12 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 
 	if (pParse->pWithToFree)
 		sqlite3WithDelete(db, pParse->pWithToFree);
-	sql_trigger_delete(db, pParse->pNewTrigger);
+	/*
+	 * Trigger is exported with pNewTrigger field when
+	 * 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;
@@ -575,3 +582,22 @@ 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, sql_error);
+	} else {
+		trigger = parser.pNewTrigger;
+	}
+	sql_parser_destroy(&parser);
+	return trigger;
+}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 053dadb..a027453 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,102 +82,127 @@ 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 */
 	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) != 0)
 		goto trigger_cleanup;
-	}
-	pTab = sql_list_lookup_table(pParse, pTableName);
-	if (!pTab) {
-		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 (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
+		goto trigger_cleanup;
+
+	if (!pParse->parse_only &&
+	    sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
 		if (!noErr) {
-			sqlite3ErrorMsg(pParse, "trigger %s already exists",
-					zName);
+			diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
+			pParse->rc = SQL_TARANTOOL_ERROR;
 		} else {
 			assert(!db->init.busy);
 		}
 		goto trigger_cleanup;
 	}
 
-	/* 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;
+		goto trigger_cleanup;
+	}
+	if (space_id == BOX_ID_NIL) {
+		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+		pParse->rc = SQL_TARANTOOL_ERROR;
 		goto trigger_cleanup;
 	}
 
-	/* INSTEAD of triggers are only for views and views only support INSTEAD
-	 * of triggers.
+	/* Do not create a trigger on a system table. */
+	if (sqlite3StrNICmp(zName, "sqlite_", 7) == 0) {
+		diag_set(ClientError, ER_SQL,
+			 "cannot create trigger on system table");
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		goto trigger_cleanup;
+	}
+
+	/*
+	 * INSTEAD of triggers are only for views and
+	 * views only support INSTEAD of triggers.
 	 */
-	if (space_is_view(pTab) && tr_tm != TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
-				(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
-				pTableName, 0);
+	struct space *space = space_cache_find(space_id);
+	assert(space != NULL);
+	if (space->def->opts.is_view && tr_tm != TK_INSTEAD) {
+		char *error = tt_static_buf();
+		snprintf(error, TT_STATIC_BUF_LEN,
+			 "cannot create %s trigger on view: %s",
+			 (tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER", table_name);
+		diag_set(ClientError, ER_SQL, error);
+		pParse->rc = SQL_TARANTOOL_ERROR;
 		goto trigger_cleanup;
 	}
-	if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) {
-		sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF"
-				" trigger on table: %S", pTableName, 0);
+	if (!space->def->opts.is_view && tr_tm == TK_INSTEAD) {
+		char *error = tt_static_buf();
+		snprintf(error, TT_STATIC_BUF_LEN,
+			 "cannot create INSTEAD OF trigger on table: %s",
+			 table_name);
+		diag_set(ClientError, ER_SQL, error);
+		pParse->rc = SQL_TARANTOOL_ERROR;
 		goto trigger_cleanup;
 	}
 
-	/* 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 */
-	pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger));
+	/* Build the Trigger object. */
+	pTrigger = (Trigger *)sqlite3DbMallocZero(db, sizeof(Trigger));
 	if (pTrigger == 0)
 		goto trigger_cleanup;
+	pTrigger->space_id = space_id;
 	pTrigger->zName = zName;
 	zName = 0;
-	pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
-	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))
+		goto trigger_cleanup;
 	assert(pParse->pNewTrigger == 0);
 	pParse->pNewTrigger = pTrigger;
 
@@ -185,11 +211,10 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	sqlite3SrcListDelete(db, pTableName);
 	sqlite3IdListDelete(db, pColumns);
 	sql_expr_delete(db, pWhen, false);
-	if (!pParse->pNewTrigger) {
+	if (pParse->pNewTrigger != NULL)
 		sql_trigger_delete(db, pTrigger);
-	} else {
+	else
 		assert(pParse->pNewTrigger == pTrigger);
-	}
 }
 
 /*
@@ -210,7 +235,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 	DbFixer sFix;		/* Fixer object */
 	Token nameToken;	/* Trigger name for error reporting */
 
-	pParse->pNewTrigger = 0;
+	pParse->pNewTrigger = NULL;
 	if (NEVER(pParse->nErr) || !pTrig)
 		goto triggerfinish_cleanup;
 	zName = pTrig->zName;
@@ -227,10 +252,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-parsing mode or export trigger.
 	 */
-	if (!db->init.busy) {
+	if (!pParse->parse_only) {
 		Vdbe *v;
 		int zOptsSz;
 		Table *pSysTrigger;
@@ -288,37 +314,11 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
 		sqlite3VdbeAddOp1(v, OP_Close, iCursor);
 
-		/* parseschema3(reg(iFirstCol), ref(iFirstCol)+1) */
-		iFirstCol = pParse->nMem + 1;
-		pParse->nMem += 2;
-
 		sql_set_multi_write(pParse, false);
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol, 0,
-				  zName, P4_STATIC);
-
-		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol + 1, 0,
-				  zSql, P4_DYNAMIC);
 		sqlite3ChangeCookie(pParse);
-		sqlite3VdbeAddParseSchema3Op(v, iFirstCol);
-	}
-
-	if (db->init.busy) {
-		Trigger *pLink = pTrig;
-		Hash *pHash = &db->pSchema->trigHash;
-		pTrig = sqlite3HashInsert(pHash, zName, pTrig);
-		if (pTrig) {
-			sqlite3OomFault(db);
-		} else if (pLink->pSchema == pLink->pTabSchema) {
-			Table *pTab;
-			pTab =
-			    sqlite3HashFind(&pLink->pTabSchema->tblHash,
-					    pLink->table);
-			assert(pTab != 0);
-			pLink->pNext = pTab->pTrigger;
-			pTab->pTrigger = pLink;
-		}
+	} else {
+		pParse->pNewTrigger = pTrig;
+		pTrig = NULL;
 	}
 
  triggerfinish_cleanup:
@@ -329,7 +329,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		   alloc for it either wasn't called at all or failed.  */
 	}
 	sql_trigger_delete(db, pTrig);
-	assert(!pParse->pNewTrigger);
+	assert(!pParse->pNewTrigger || pParse->parse_only);
 	sqlite3DeleteTriggerStep(db, pStepList);
 }
 
@@ -478,7 +478,6 @@ sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
 		return;
 	sqlite3DeleteTriggerStep(db, trigger->step_list);
 	sqlite3DbFree(db, trigger->zName);
-	sqlite3DbFree(db, trigger->table);
 	sql_expr_delete(db, trigger->pWhen, false);
 	sqlite3IdListDelete(db, trigger->pColumns);
 	sqlite3DbFree(db, trigger);
@@ -533,16 +532,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
@@ -565,34 +554,44 @@ 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_replace(struct sqlite3 *db, const char *name,
+		    struct Trigger *trigger, struct Trigger **old_trigger)
 {
-	Trigger *pTrigger;
-	Hash *pHash;
-	struct session *user_session = current_session();
-
-	pHash = &(db->pSchema->trigHash);
-	pTrigger = sqlite3HashInsert(pHash, zName, 0);
-	if (ALWAYS(pTrigger)) {
-		if (pTrigger->pSchema == pTrigger->pTabSchema) {
-			Table *pTab = tableOfTrigger(pTrigger);
-			Trigger **pp;
-			for (pp = &pTab->pTrigger; *pp != pTrigger;
-			     pp = &((*pp)->pNext)) ;
-			*pp = (*pp)->pNext;
-		}
-		sql_trigger_delete(db, pTrigger);
-		user_session->sql_flags |= SQLITE_InternChanges;
+	assert(db->pSchema != NULL);
+	assert(trigger == NULL || strcmp(name, trigger->zName) == 0);
+	struct Hash *hash = &db->pSchema->trigHash;
+	*old_trigger = sqlite3HashInsert(hash, name, trigger);
+	if (*old_trigger == trigger) {
+		diag_set(OutOfMemory, 0, "sqlite3HashInsert", "ret");
+		return -1;
+	}
+	struct Trigger *src_trigger = trigger != NULL ? trigger : *old_trigger;
+	assert(src_trigger != NULL);
+	struct space *space =
+		space_cache_find(src_trigger->space_id);
+	assert(space != NULL);
+
+	if (*old_trigger != NULL) {
+		struct Trigger **pp;
+		for (pp = &space->sql_triggers; *pp != *old_trigger;
+			pp = &((*pp)->pNext));
+		*pp = (*pp)->pNext;
 	}
+	if (trigger != NULL) {
+		trigger->pNext = space->sql_triggers;
+		space->sql_triggers = trigger;
+	}
+	return 0;
+}
+
+char *
+sql_trigger_name(struct Trigger *trigger)
+{
+	return trigger->zName;
 }
 
 /*
@@ -631,22 +630,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)
 		*pMask = mask;
-	}
-	return (mask ? pList : 0);
+	return mask ? trigger_list : 0;
 }
 
 /*
@@ -835,7 +829,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
@@ -952,7 +946,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
@@ -1077,15 +1071,6 @@ 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
 		    && p->tr_tm == tr_tm
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
@@ -4686,46 +4686,6 @@ case OP_ParseSchema2: {
 	break;
 }
 
-/* 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: {
-	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;
-	}
-	break;
-}
-
 /* Opcode: RenameTable P1 * * P4 *
  * Synopsis: P1 = root, P4 = name
  *
@@ -4745,7 +4705,6 @@ case OP_RenameTable: {
 	const char *zNewTableName;
 	Table *pTab;
 	FKey *pFKey;
-	Trigger *pTrig;
 	int iRootPage;
 	InitData initData;
 	char *argv[4] = {NULL, NULL, NULL, NULL};
@@ -4758,7 +4717,6 @@ 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,
@@ -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 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;
 	}
+
 	sqlite3DbFree(db, (void*)zOldTableName);
 	sqlite3DbFree(db, (void*)zSqlStmt);
 	break;
@@ -4857,18 +4817,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
 
 /* Opcode: Program P1 P2 P3 P4 P5
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 68d542c..77fa41a 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -238,7 +238,6 @@ void sqlite3VdbeVerifyNoResultRow(Vdbe * p);
 VdbeOp *sqlite3VdbeAddOpList(Vdbe *, int nOp, VdbeOpList const *aOp,
 			     int iLineno);
 void sqlite3VdbeAddParseSchema2Op(Vdbe * p, int, int);
-void sqlite3VdbeAddParseSchema3Op(Vdbe * p, int);
 void sqlite3VdbeAddRenameTableOp(Vdbe * p, int, char *);
 void sqlite3VdbeChangeOpcode(Vdbe *, u32 addr, u8);
 void sqlite3VdbeChangeP1(Vdbe *, u32 addr, int P1);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 679bd0b..7b3c13d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -410,15 +410,6 @@ sqlite3VdbeAddParseSchema2Op(Vdbe * p, int iRec, int n)
 	sqlite3VdbeAddOp3(p, OP_ParseSchema2, iRec, n, 0);
 }
 
-/*
- * Add an OP_ParseSchema3 opcode which in turn will create a trigger
- */
-void
-sqlite3VdbeAddParseSchema3Op(Vdbe * p, int iRec)
-{
-	sqlite3VdbeAddOp2(p, OP_ParseSchema3, iRec, 0);
-}
-
 void
 sqlite3VdbeAddRenameTableOp(Vdbe * p, int iTab, char* zNewName)
 {
diff --git a/test/box/misc.result b/test/box/misc.result
index 6f4028c..e8ee297 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -381,6 +381,7 @@ t;
   52: box.error.FUNCTION_EXISTS
   53: box.error.BEFORE_REPLACE_RET
   54: box.error.FUNCTION_MAX
+  55: box.error.TRIGGER_EXISTS
   56: box.error.USER_MAX
   57: box.error.NO_SUCH_ENGINE
   58: box.error.RELOAD_CFG
diff --git a/test/sql-tap/identifier_case.test.lua b/test/sql-tap/identifier_case.test.lua
index 3f6dc07..5e7573a 100755
--- a/test/sql-tap/identifier_case.test.lua
+++ b/test/sql-tap/identifier_case.test.lua
@@ -170,8 +170,8 @@ test:do_execsql_test(
 
 data = {
     { 1,  [[ trigger1 ]], {0}},
-    { 2,  [[ Trigger1 ]], {1, "trigger TRIGGER1 already exists"}},
-    { 3,  [["TRIGGER1"]], {1, "trigger TRIGGER1 already exists"}},
+    { 2,  [[ Trigger1 ]], {1, "Trigger 'TRIGGER1' already exists"}},
+    { 3,  [["TRIGGER1"]], {1, "Trigger 'TRIGGER1' already exists"}},
     { 4,  [["trigger1" ]], {0}}
 }
 
diff --git a/test/sql-tap/trigger1.test.lua b/test/sql-tap/trigger1.test.lua
index 40daba4..79111fd 100755
--- a/test/sql-tap/trigger1.test.lua
+++ b/test/sql-tap/trigger1.test.lua
@@ -43,7 +43,7 @@ test:do_catchsql_test(
         END;
     ]], {
         -- <trigger1-1.1.1>
-        1, "no such table: NO_SUCH_TABLE"
+        1, "Space 'NO_SUCH_TABLE' does not exist"
         -- </trigger1-1.1.1>
     })
 
@@ -55,7 +55,7 @@ test:do_catchsql_test(
         END;
     ]], {
         -- <trigger1-1.1.2>
-        1, "no such table: NO_SUCH_TABLE"
+        1, "Space 'NO_SUCH_TABLE' does not exist"
         -- </trigger1-1.1.2>
     })
 
@@ -101,7 +101,7 @@ test:do_catchsql_test(
          END
     ]], {
         -- <trigger1-1.2.1>
-        1, "trigger TR1 already exists"
+        1, "Trigger 'TR1' already exists"
         -- </trigger1-1.2.1>
     })
 
@@ -113,7 +113,7 @@ test:do_catchsql_test(
          END
     ]], {
         -- <trigger1-1.2.2>
-        1, [[trigger TR1 already exists]]
+        1, [[Trigger 'TR1' already exists]]
         -- </trigger1-1.2.2>
     })
 
@@ -251,7 +251,7 @@ test:do_catchsql_test(
         end;
     ]], {
         -- <trigger1-1.12>
-        1, "cannot create INSTEAD OF trigger on table: T1"
+        1, "SQL error: cannot create INSTEAD OF trigger on table: T1"
         -- </trigger1-1.12>
     })
 
@@ -265,7 +265,7 @@ test:do_catchsql_test(
         end;
     ]], {
         -- <trigger1-1.13>
-        1, "cannot create BEFORE trigger on view: V1"
+        1, "SQL error: cannot create BEFORE trigger on view: V1"
         -- </trigger1-1.13>
     })
 
@@ -280,7 +280,7 @@ test:do_catchsql_test(
         end;
     ]], {
         -- <trigger1-1.14>
-        1, "cannot create AFTER trigger on view: V1"
+        1, "SQL error: cannot create AFTER trigger on view: V1"
         -- </trigger1-1.14>
     })
 
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
new file mode 100644
index 0000000..353a3c2
--- /dev/null
+++ b/test/sql/triggers.result
@@ -0,0 +1,187 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+--
+-- 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; ]])
+---
+...
+box.space._trigger:select()
+---
+- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}]
+...
+-- checks for LUA tuples
+tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: Duplicate key exists in unique index 'primary' in space '_trigger'
+...
+tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: tuple trigger name does not match extracted from SQL'
+...
+tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- error: 'SQL error: tuple trigger name does not match extracted from SQL'
+...
+box.space._trigger:select()
+---
+- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}]
+...
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+---
+...
+-- test duplicate index error
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+---
+- error: Duplicate key exists in unique index 'sqlite_autoindex_T1_1' in space 'T1'
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- test triggers
+tuple = {"T2T", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+      END;'}]
+...
+tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+---
+...
+box.space._trigger:insert(tuple)
+---
+- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+      END;'}]
+...
+box.space._trigger:select()
+---
+- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}]
+  - ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+        END;'}]
+  - ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+        END;'}]
+...
+box.sql.execute("INSERT INTO t1 VALUES(2);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+  - [2]
+  - [3]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- test t1t after t2t and t3t drop
+box.sql.execute("DROP TRIGGER T2T;")
+---
+...
+box.space._trigger:delete("T3T")
+---
+- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+      END;'}]
+...
+box.space._trigger:select()
+---
+- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}]
+...
+box.sql.execute("INSERT INTO t1 VALUES(3);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- insert new SQL t2t and t3t
+box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
+---
+...
+box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
+---
+...
+box.space._trigger:select()
+---
+- - ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}]
+  - ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+        END; '}]
+  - ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+        END; '}]
+...
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+  - [2]
+  - [3]
+...
+-- clean up
+box.space._trigger:delete("T1T")
+---
+- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+      END; '}]
+...
+box.space._trigger:delete("T2T")
+---
+- ['T2T', {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+      END; '}]
+...
+box.space._trigger:delete("T3T")
+---
+- ['T3T', {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+      END; '}]
+...
+box.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("DROP TABLE t2;")
+---
+...
+box.space._trigger:select()
+---
+- []
+...
+test_run:cmd("clear filter")
+---
+- true
+...
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
new file mode 100644
index 0000000..a63a71b
--- /dev/null
+++ b/test/sql/triggers.test.lua
@@ -0,0 +1,69 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+--
+-- 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; ]])
+box.space._trigger:select()
+
+-- checks for LUA tuples
+tuple = {"T1T", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1t", {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+tuple = {"T1t", {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+box.space._trigger:insert(tuple)
+
+box.space._trigger:select()
+
+
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+-- test duplicate index error
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+
+-- test triggers
+tuple = {"T2T", {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+box.space._trigger:insert(tuple)
+tuple = {"T3T", {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+box.space._trigger:insert(tuple)
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(2);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- test t1t after t2t and t3t drop
+box.sql.execute("DROP TRIGGER T2T;")
+box.space._trigger:delete("T3T")
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(3);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+-- insert new SQL t2t and t3t
+box.sql.execute([[CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END; ]])
+box.sql.execute([[CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END; ]])
+box.space._trigger:select()
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+box.sql.execute("SELECT * FROM t2;")
+
+-- clean up
+box.space._trigger:delete("T1T")
+box.space._trigger:delete("T2T")
+box.space._trigger:delete("T3T")
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
+box.space._trigger:select()
+
+
+test_run:cmd("clear filter")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Kirill Shcherbatov]

> 1. Lets change commit title: the patch does not introduce
> box_space_id_by_name. It was before the patch.
    box: port schema_find_id to C
    
    Part of #3273.

> 2. How can you return -1, when the return type is unsigned?
> 3. It is not 'space_id' speaking in general. This function is used in
> user.cc also to search user id by the name. Please, find another name
> for the parameter.
> 4. "new slab" -> "key". OutOfMemory takes size, alloc function and
> the result variable name.
> 5. You should avoid 'goto cleanup' here.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index c5055ee..a85a09f 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -222,11 +222,11 @@ sc_space_new(uint32_t id, const char *name, struct key_def *key_def,
 	trigger_run_xc(&on_alter_space, space);
 }
 
-uint32_t
+int
 schema_find_id(uint32_t system_space_id, uint32_t index_id,
-	       const char *name, uint32_t len, uint32_t *space_id)
+	       const char *name, uint32_t len, uint32_t *object_id)
 {
-	*space_id = BOX_ID_NIL;
+	*object_id = BOX_ID_NIL;
 	if (len > BOX_NAME_MAX) {
 		diag_set(SystemError,
 			 "name length %d is greater than BOX_NAME_MAX", len);
@@ -248,7 +248,7 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
 	uint32_t used = region_used(region);
 	char *key = (char *)region_alloc(region, size);
 	if (key == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "key");
 		return -1;
 	}
 	mp_encode_str(key, name, len);
@@ -261,13 +261,10 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
 	struct tuple *tuple;
 	if (iterator_next(it, &tuple) != 0) {
 		rc = -1;
-		goto cleanup;
-	}
-	if (tuple) {
+	} else if (tuple) {
 		/* id is always field #1 */
-		*space_id = tuple_field_u32_xc(tuple, 0);
+		*object_id = tuple_field_u32_xc(tuple, 0);
 	}
-cleanup:
 	iterator_delete(it);
 	region_truncate(region, used);
 	return rc;

> 6. Same. Do not mention space. It is a function to find ID of
> any system object. Not only space. This function is applicable
> for _index, _space, _user, _func.

diff --git a/src/box/schema.h b/src/box/schema.h
index 099f9b0..cd9bb5b 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -107,16 +107,16 @@ sequence_by_id(uint32_t id);
*
* @param system_space_id identifier of the system space.
* @param index_id identifier of the index to lookup.
- * @param name space to lookup name.
+ * @param name of object to lookup.
* @param len length of a name.
- * @param space_id[out] space_id BOX_ID_NIL - not found.
+ * @param object_id[out] object_id or BOX_ID_NIL - not found.
*
* @retval 0 on success.
* @retval -1 on error.
*/
-uint32_t
+int
schema_find_id(uint32_t system_space_id, uint32_t index_id,
- const char *name, uint32_t len, uint32_t *space_id);
+ const char *name, uint32_t len, uint32_t *object_id);

#if defined(__cplusplus)
} /* extern "C" */

[tarantool-patches] Re: [PATCH v1 3/4] box: introduce box_space_id_by_name

[Vladislav Shpilevoy]

Hello. Thanks for the fixes!

The schema_find_id is still in C++ since it has tuple_field_u32_xc.
XC means exception. I removed it and slightly refactored other code.
The commit is force pushed and LGTM.

diff --git a/src/box/schema.cc b/src/box/schema.cc
index a85a09f9d..5d32e6153 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -226,7 +226,6 @@ int
  schema_find_id(uint32_t system_space_id, uint32_t index_id,
  	       const char *name, uint32_t len, uint32_t *object_id)
  {
-	*object_id = BOX_ID_NIL;
  	if (len > BOX_NAME_MAX) {
  		diag_set(SystemError,
  			 "name length %d is greater than BOX_NAME_MAX", len);
@@ -257,13 +256,14 @@ schema_find_id(uint32_t system_space_id, uint32_t index_id,
  		region_truncate(region, used);
  		return -1;
  	}
-	int rc = 0;
  	struct tuple *tuple;
-	if (iterator_next(it, &tuple) != 0) {
-		rc = -1;
-	} else if (tuple) {
+	int rc = iterator_next(it, &tuple);
+	if (rc == 0) {
  		/* id is always field #1 */
-		*object_id = tuple_field_u32_xc(tuple, 0);
+		if (tuple == NULL)
+			*object_id = BOX_ID_NIL;
+		else if (tuple_field_u32(tuple, 0, object_id) != 0)
+			return -1;
  	}
  	iterator_delete(it);
  	region_truncate(region, used);

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

[Vladislav Shpilevoy]

Thanks for the fixes! See 12 comments below. I have not reviewed
the whole patch since you did not fix some of the previous
comments, and I have found new bugs.

> 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                      | 140 ++++++++++++++++++
>   src/box/errcode.h                     |   2 +-
>   src/box/space.h                       |   2 +
>   src/box/sql.c                         |  48 ++-----
>   src/box/sql.h                         |  42 ++++++
>   src/box/sql/build.c                   |   8 +-
>   src/box/sql/fkey.c                    |   2 -
>   src/box/sql/insert.c                  |   6 +-
>   src/box/sql/sqliteInt.h               |   7 +-
>   src/box/sql/tokenize.c                |  28 +++-
>   src/box/sql/trigger.c                 | 259 ++++++++++++++++------------------
>   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              | 187 ++++++++++++++++++++++++
>   test/sql/triggers.test.lua            |  69 +++++++++
>   19 files changed, 629 insertions(+), 276 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..23cd4a3 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -551,6 +551,8 @@ 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;

1. It is not swap still. Swap of A and B means that before swap
A == value1, B = value2. After swap A == value2, B == value1.
Here you have done this:
A == B.

Do you understand why exactly swap is needed here? Please, answer.
Do not skip the question.

2. Space->sql_triggers leaks when I drop a space via Lua.

3. After you fix the leak, try this test to simulate WAL error and you
will see a crash:

	box.cfg{}
	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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
	box.error.injection.set("ERRINJ_WAL_IO", true)
	box.sql.execute("CREATE INDEX t1a ON t1(a);")
	box.error.injection.set("ERRINJ_WAL_IO", false)
	box.sql.execute("INSERT INTO t1 VALUES (3, 3);")

>   }
>   
>   /**
> @@ -3091,6 +3093,55 @@ lock_before_dd(struct trigger *trigger, void *event)
>   	latch_lock(&schema_lock);
>   }
>   
> +static void
> +triggers_task_rollback(struct trigger *trigger, void *event)
> +{
> +	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +	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);

4. Fits in 3 lines.

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

5. Extra new line.

> +static void
> +triggers_task_commit(struct trigger *trigger, void *event)
> +{
> +	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +
> +	if (stmt->old_tuple != NULL) {

6. Why do you need stmt and stmt->old_tuple here? As I see,
old_trigger != NULL is the same as stmt->old_tuple != NULL.

> +		/* DELETE, REPLACE trigger. */
> +		sql_trigger_delete(sql_get(), old_trigger);
> +	}
> +}
> +
> @@ -3100,6 +3151,95 @@ 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);
> +
> +	char *trigger_name = NULL;
> +	struct Trigger *new_trigger = NULL;

7. Please, do not create two branches for each case. You can make this
function more compact.
> +	if (new_tuple != NULL) {
> +		/* 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, 1, MP_MAP);
> +		struct space_opts opts;
> +		struct region *region = &fiber()->gc;
> +		space_opts_decode(&opts, space_opts, region);
> +		new_trigger = sql_trigger_compile(sql_get(), opts.sql);
> +		if (new_trigger == NULL)
> +			diag_raise();
> +
> +		if (strncmp(trigger_name_src, sql_trigger_name(new_trigger),
> +			    trigger_name_len) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			tnt_raise(ClientError, ER_SQL,
> +				  "tuple trigger name does not match extracted "
> +				  "from SQL");
> +		}
> +		trigger_name = sql_trigger_name(new_trigger);
> +	}
> +
> +	if (old_tuple != NULL && new_tuple == NULL) {
> +		/* DELETE trigger. */
> +		struct Trigger *old_trigger;
> +		int rc =
> +			sql_trigger_replace(sql_get(), trigger_name, NULL,
> +					    &old_trigger);
> +		(void)rc;
> +		assert(rc == 0);
> +		assert(old_trigger != NULL);
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = old_trigger;
> +	} else if (new_tuple != NULL && old_tuple == NULL) {
> +		/* INSERT trigger. */
> +		struct Trigger *old_trigger;
> +		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
> +					&old_trigger) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			diag_raise();
> +		}
> +		assert(old_trigger == NULL);
> +
> +		on_commit->data = NULL;
> +		on_rollback->data = new_trigger;
> +	} else {
> +		/* REPLACE trigger. */
> +		struct Trigger *old_trigger;
> +		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
> +					&old_trigger) != 0) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			diag_raise();
> +		}
> +		assert(old_trigger != NULL);
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = old_trigger;
> +	}
> +
> +	txn_on_rollback(txn, on_rollback);
> +	txn_on_commit(txn, on_commit);
> +
> +

8. Two extra lines.

>   }
>   
> diff --git a/src/box/sql.h b/src/box/sql.h
> index 35aacc3..885f0a7 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> +/**
> + * Get trigger name by trigger AST object.
> + * @param trigger AST object.
> + * @return trigger name string.
> + */
> +char *
> +sql_trigger_name(struct Trigger *trigger);

9. Const char *.


> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 817a282..d769d14 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"

10. Why?

>   
>   #include "say.h"
>   #include "sqliteInt.h"
> @@ -123,6 +124,7 @@ static const char sql_ascii_class[] = {
>    * the #include below.
>    */
>   #include "keywordhash.h"
> +#include "tarantoolInt.h"

11. Same.

>   
>   #define maybe_utf8(c) ((sqlite3CtypeMap[c] & 0x40) != 0)
>   
> @@ -575,3 +582,22 @@ 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, sql_error);

12. Still is not fixed from the previous review. 'Error' is unused.
And why do you need snprintf here?

Please, do another iteration of self-review before sending the patch
again. Remove all unused includes, extra lines, unused variables,
unnecessary calls, fix alignment etc.

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

[Vladislav Shpilevoy]

Please, in the commit message of 'box: sort error codes in misc.test'
explain why the patch is needed. It is not obvious for people out of
context.

On 31/05/2018 14:22, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers
> Issue: https://github.com/tarantool/tarantool/issues/3273
> 
> As we are going to call parser on box.cfg() to recreate triggers
> from SQL, we should initialize Schema as it used in sqlite3BeginTrigger.
> Inroduced box_space_id_by_name to get object id by name from custom space.
> Fixed bug in tarantoolSqlite3RenameTrigger: sql request have had invalid
> length in MsgPack after ALTER TABLE NAME.
> 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.
> 
> Kirill Shcherbatov (4):
>    box: move db->pShchema init to sql_init
>    sql: fix sql len in tarantoolSqlite3RenameTrigger
>    box: introduce box_space_id_by_name
>    sql: move Triggers to server
> 
>   src/box/alter.cc        |  57 +++++++++++
>   src/box/box.cc          |  10 +-
>   src/box/box.h           |  14 +++
>   src/box/space.h         |   2 +
>   src/box/sql.c           |  60 ++++-------
>   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 --
>   14 files changed, 374 insertions(+), 216 deletions(-)
> 

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

[Kirill Shcherbatov]

I'll send 2.0 patchset,

> 1. It is not swap still. Swap of A and B means that before swap
> A == value1, B = value2. After swap A == value2, B == value1.
> Here you have done this:
> A == B.
> 
> Do you understand why exactly swap is needed here? Please, answer.
> Do not skip the question.
Yes, it is clear for me now: this function provides an ability to rollback.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 23cd4a3..03c2d91 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -552,7 +552,9 @@ space_swap_triggers(struct space *new_space, struct space *old_space)
        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. */
+       struct Trigger *old_value = new_space->sql_triggers;
        new_space->sql_triggers = old_space->sql_triggers;
+       old_space->sql_triggers = old_value;
 }


> 
> 2. Space->sql_triggers leaks when I drop a space via Lua.

I've reworked a tuple format in separate commit, I'll send a new patch set that includes it a bit later.

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
@@ -463,6 +463,7 @@ box.schema.space.drop = function(space_id, space_name, opts)
     check_param_table(opts, { if_exists = 'boolean' })
     local _space = box.space[box.schema.SPACE_ID]
     local _index = box.space[box.schema.INDEX_ID]
+    local _trigger = box.space[box.schema.TRIGGER_ID]
     local _vindex = box.space[box.schema.VINDEX_ID]
     local _truncate = box.space[box.schema.TRUNCATE_ID]
     local _space_sequence = box.space[box.schema.SPACE_SEQUENCE_ID]
@@ -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
     local keys = _vindex:select(space_id)
     for i = #keys, 1, -1 do
         local v = keys[i]
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.
+        */
+       assert(space->sql_triggers == NULL);
        space->vtab->destroy(space);
 }
 


> 
> 3. After you fix the leak, try this test to simulate WAL error and you
> will see a crash:
> 
> 	box.cfg{}
> 	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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
> 	box.error.injection.set("ERRINJ_WAL_IO", true)
> 	box.sql.execute("CREATE INDEX t1a ON t1(a);")
> 	box.error.injection.set("ERRINJ_WAL_IO", false)
> 	box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
> 
There is no crash for now, I've included this test case:

+-- test crash on error.injection
+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.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+---
+...
+box.error.injection.set("ERRINJ_WAL_IO", true)
+---
+- ok
+...
+box.sql.execute("CREATE INDEX t1a ON t1(a);")
+---
+- error: Failed to write to disk
+...
+box.error.injection.set("ERRINJ_WAL_IO", false)
+---
+- ok
+...
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+---
+...
+box.sql.execute("SELECT * from t1");
+---
+- - [3, 3]
+...
+box.sql.execute("SELECT * from t2");
+---
+- - [1, 1]
+...
+box.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("DROP TABLE t2;")

> 4. Fits in 3 lines.
+               int rc = sql_trigger_replace(sql_get(),
+                                            sql_trigger_name(old_trigger),
+                                            NULL, &new_trigger);

> 5. Extra new line.
@@ -3131,7 +3130,6 @@ triggers_task_rollback(struct trigger *trigger, void *event)
        }
 }
 
-

> 6. Why do you need stmt and stmt->old_tuple here? As I see,
> old_trigger != NULL is the same as stmt->old_tuple != NULL.

 static void
 triggers_task_commit(struct trigger *trigger, void *event)
 {
-       struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
        struct Trigger *old_trigger = (struct Trigger *)trigger->data;
-
-       if (stmt->old_tuple != NULL) {
-               /* DELETE, REPLACE trigger. */
-               sql_trigger_delete(sql_get(), old_trigger);
-       }
+       /* DELETE, REPLACE trigger. */
+       sql_trigger_delete(sql_get(), old_trigger);
 }


> 7. Please, do not create two branches for each case. You can make this
> function more compact.
> 8. Two extra lines.


> 9. Const char *.
-char *
+const char *
 sql_trigger_name(struct Trigger *trigger);

> 10. Why?
> 11. Same.
Dropped.

> 12. Still is not fixed from the previous review. 'Error' is unused.
> And why do you need snprintf here?
char *error = tt_static_buf();
snprintf(error, TT_STATIC_BUF_LEN, "%s", sql_error);
diag_set(ClientError, ER_SQL, error);
sqlite3DbFree(db, sql_error);

I believe this should be like this.
Done same in other places.