[tarantool-patches] [PATCH v3 00/10] sql: remove Triggers to server

[tarantool-patches] [PATCH v3 00/10] 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

Kirill Shcherbatov (10):
  box: move db->pShchema init to sql_init
  sql: fix leak on CREATE TABLE and resolve self ref
  sql: fix sql len in tarantoolSqlite3RenameTrigger
  box: port schema_find_id to C
  sql: refactor sql_expr_compile to return AST
  sql: move sqlite3DeleteTrigger to sql.h
  box: sort error codes in misc.test
  sql: new  _trigger space format with space_id
  sql: move Triggers to server
  sql: VDBE tests for trigger existence

 src/box/alter.cc                                   | 134 ++++++++-
 src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
 src/box/errcode.h                                  |   2 +-
 src/box/lua/schema.lua                             |   4 +
 src/box/lua/upgrade.lua                            |   4 +
 src/box/schema.cc                                  |  54 +++-
 src/box/schema.h                                   |  23 +-
 src/box/schema_def.h                               |   6 +
 src/box/space.c                                    |   5 +
 src/box/space.h                                    |   2 +
 src/box/sql.c                                      |  85 ++----
 src/box/sql.h                                      |  66 ++++-
 src/box/sql/build.c                                |  59 +++-
 src/box/sql/callback.c                             |   7 +-
 src/box/sql/fkey.c                                 |   2 -
 src/box/sql/insert.c                               |   6 +-
 src/box/sql/main.c                                 |  10 +-
 src/box/sql/prepare.c                              |   2 +
 src/box/sql/sqliteInt.h                            |  30 +-
 src/box/sql/status.c                               |   6 +-
 src/box/sql/tokenize.c                             |  50 +++-
 src/box/sql/trigger.c                              | 320 ++++++++++----------
 src/box/sql/vdbe.c                                 |  87 ++----
 src/box/sql/vdbe.h                                 |   3 +-
 src/box/sql/vdbeapi.c                              |   5 +-
 src/box/sql/vdbeaux.c                              |  13 +-
 src/box/user.cc                                    |   4 +-
 src/diag.h                                         |   3 +
 test/app-tap/tarantoolctl.test.lua                 |   2 +-
 test/box-py/bootstrap.result                       |   5 +-
 test/box/access_misc.result                        |   4 +-
 test/box/access_sysview.result                     |   4 +-
 test/box/alter.result                              |   1 +
 test/box/misc.result                               | 326 +++++++++++----------
 test/box/misc.test.lua                             |   4 +-
 test/engine/iterator.result                        |   2 +-
 test/sql-tap/identifier_case.test.lua              |   4 +-
 test/sql-tap/trigger1.test.lua                     |  14 +-
 test/sql/gh2141-delete-trigger-drop-table.result   |   4 +-
 test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +-
 test/sql/persistency.result                        |   8 +-
 test/sql/persistency.test.lua                      |   8 +-
 test/sql/triggers.result                           | 255 ++++++++++++++++
 test/sql/triggers.test.lua                         |  92 ++++++
 test/sql/upgrade.result                            |  37 ++-
 test/sql/upgrade.test.lua                          |   9 +-
 46 files changed, 1205 insertions(+), 570 deletions(-)
 create mode 100644 test/sql/triggers.result
 create mode 100644 test/sql/triggers.test.lua

-- 
2.7.4

[tarantool-patches] [PATCH v3 01/10] 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 | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 7379cb4..93fca68 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -77,11 +77,22 @@ sql_init()
 		panic("failed to initialize SQL subsystem");
 
 	assert(db != NULL);
+	/*
+	 * Initialize pSchema to use SQL parser on initialization:
+	 * e.g. Trigger objects (compiled from SQL on tuple
+	 * insertion in _trigger) need to refer it.
+	 */
+	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 +100,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 v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

Trigger presence in system should be tested on each VDBE
execution attempt, not on Parser iteration.

Part of #3435, #3273
---
 src/box/sql/build.c     | 37 +++++++++++++++++++++++++++++++++++++
 src/box/sql/main.c      | 10 +++++++---
 src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++
 src/box/sql/trigger.c   | 23 +++++++++++------------
 src/box/sql/vdbe.c      | 10 +++++++---
 src/box/sql/vdbe.h      |  2 ++
 src/box/sql/vdbeapi.c   |  5 +++--
 src/box/sql/vdbeaux.c   |  4 ++++
 src/diag.h              |  3 +++
 9 files changed, 94 insertions(+), 20 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 67cdf11..0a731df 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -4171,4 +4171,41 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
 		sqlite3DbFree(db, pWith);
 	}
 }
+
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				     int index_id, const char *name,
+				     struct error *error, bool no_error)
+{
+	error_ref(error);
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	assert(v != NULL);
+
+	struct sqlite3 *db = parser->db;
+	name = sqlite3DbStrDup(db, name);
+	if (name == NULL) {
+		diag_set(OutOfMemory, strlen(name) + 1, "sqlite3DbStrDup",
+			 "name");
+		return -1;
+	}
+	int cursor = parser->nTab++;
+	int entity_id =
+		SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id, index_id);
+	emit_open_cursor(parser, cursor, entity_id);
+
+	int name_reg = parser->nMem++;
+	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
+				      P4_DYNAMIC);
+	sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
+	if (no_error) {
+		error_unref(error);
+		sqlite3VdbeAddOp0(v, OP_Halt);
+	} else {
+		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+				  ON_CONFLICT_ACTION_FAIL, 0, (void *)error,
+				  P4_ERROROBJ);
+	}
+	sqlite3VdbeAddOp1(v, OP_Close, cursor);
+	return 0;
+}
 #endif				/* !defined(SQLITE_OMIT_CTE) */
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0acf7bc..e1de815 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1454,10 +1454,14 @@ sqlite3_errmsg(sqlite3 * db)
 		z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
 	} else {
 		testcase(db->pErr == 0);
-		z = (char *)sqlite3_value_text(db->pErr);
 		assert(!db->mallocFailed);
-		if (z == 0) {
-			z = sqlite3ErrStr(db->errCode);
+		if (db->errCode != SQL_TARANTOOL_ERROR) {
+			assert(!db->mallocFailed);
+			z = (char *)sqlite3_value_text(db->pErr);
+			if (z == NULL)
+				z = sqlite3ErrStr(db->errCode);
+		} else {
+			z = diag_last_error(diag_get())->errmsg;
 		}
 	}
 	return z;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 332986f..d86ace5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4537,4 +4537,24 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
+/**
+ * Generate VDBE code to halt execution with correct error if
+ * the object with specified name is already present in
+ * specified space.
+ *
+ * @param parser Parsing context.
+ * @param space_id Space to lookup identifier.
+ * @param index_id Index identifier containing string primary key.
+ * @param name Of object to test existence.
+ * @param error object to raise on target exists.
+ * @param no_error Do not raise error flag.
+ *
+ * @retval -1 on memory allocation error.
+ * @retval 0 on success.
+ */
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				     int index_id, const char *name,
+				     struct error *error, bool no_error);
+
 #endif				/* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 9ca4262..4cb89fe 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -122,18 +122,6 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
 		goto trigger_cleanup;
 
-	if (!pParse->parse_only &&
-	    sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
-		if (!noErr) {
-			diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
-			pParse->rc = SQL_TARANTOOL_ERROR;
-			pParse->nErr++;
-		} else {
-			assert(!db->init.busy);
-		}
-		goto trigger_cleanup;
-	}
-
 	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),
@@ -184,6 +172,17 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 		pParse->nErr++;
 		goto trigger_cleanup;
 	}
+	if (!pParse->parse_only) {
+		struct error *error =
+			error_object(ClientError, ER_TRIGGER_EXISTS, zName);
+		if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
+							 0, zName, error,
+							 (noErr != 0)) != 0) {
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			goto trigger_cleanup;
+		}
+	}
 
 	/*
 	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b5f1f1c..c3109ab 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1005,9 +1005,10 @@ case OP_Halt: {
 	p->rc = pOp->p1;
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
-	assert(pOp->p5<=4);
 	if (p->rc) {
-		if (pOp->p5) {
+		if (p->rc == SQL_TARANTOOL_ERROR) {
+			diag_add_error(diag_get(), (struct error *)pOp->p4.z);
+		} else if (pOp->p5 != 0) {
 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
 							       "FOREIGN KEY" };
 			testcase( pOp->p5==1);
@@ -1029,7 +1030,10 @@ case OP_Halt: {
 		p->rc = SQLITE_BUSY;
 	} else {
 		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
-		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
+		if (p->rc != SQL_TARANTOOL_ERROR)
+			rc = (p->rc != SQLITE_OK) ? SQLITE_ERROR : SQLITE_DONE;
+		else
+			rc = SQL_TARANTOOL_ERROR;
 	}
 	goto vdbe_return;
 }
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 77fa41a..66e6954 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -149,6 +149,8 @@ typedef struct VdbeOpList VdbeOpList;
 #define P4_PTR      (-18)	/* P4 is a generic pointer */
 #define P4_KEYDEF   (-19)       /* P4 is a pointer to key_def structure. */
 #define P4_SPACEPTR (-20)       /* P4 is a space pointer */
+/** P4 is an error object. */
+#define P4_ERROROBJ (-21)
 
 /* Error message codes for OP_Halt */
 #define P5_ConstraintNotNull 1
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index d35338a..0aa5c4a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -598,8 +598,9 @@ sqlite3Step(Vdbe * p)
 	 * contains the value that would be returned if sqlite3_finalize()
 	 * were called on statement p.
 	 */
-	assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
-	       || (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
+	assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR ||
+	       (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE ||
+	       rc == SQL_TARANTOOL_ERROR);
 	if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
 		/* If this statement was prepared using sqlite3_prepare_v2(), and an
 		 * error has occurred, then return the error code in p->rc to the
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 7b3c13d..8d89894 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -981,6 +981,10 @@ freeP4(sqlite3 * db, int p4type, void *p4)
 			}
 			break;
 		}
+	case P4_ERROROBJ: {
+		error_unref(p4);
+		break;
+	}
 	}
 }
 
diff --git a/src/diag.h b/src/diag.h
index 0ccf86d..502b294 100644
--- a/src/diag.h
+++ b/src/diag.h
@@ -261,6 +261,9 @@ struct error *
 BuildSocketError(const char *file, unsigned line, const char *socketname,
 		 const char *format, ...);
 
+#define error_object(class, ...) \
+	Build##class(__FILE__, __LINE__, ##__VA_ARGS__);
+
 #define diag_set(class, ...) do {					\
 	/* Preserve the original errno. */                              \
 	int save_errno = errno;                                         \
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy]

Thanks for the fixes! See 1 comment below.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> Trigger presence in system should be tested on each VDBE
> execution attempt, not on Parser iteration.
> 
> Part of #3435, #3273
> ---
>   src/box/sql/build.c     | 37 +++++++++++++++++++++++++++++++++++++
>   src/box/sql/main.c      | 10 +++++++---
>   src/box/sql/sqliteInt.h | 20 ++++++++++++++++++++
>   src/box/sql/trigger.c   | 23 +++++++++++------------
>   src/box/sql/vdbe.c      | 10 +++++++---
>   src/box/sql/vdbe.h      |  2 ++
>   src/box/sql/vdbeapi.c   |  5 +++--
>   src/box/sql/vdbeaux.c   |  4 ++++
>   src/diag.h              |  3 +++
>   9 files changed, 94 insertions(+), 20 deletions(-)
> 
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index 9ca4262..4cb89fe 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -184,6 +172,17 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>   		pParse->nErr++;
>   		goto trigger_cleanup;
>   	}
> +	if (!pParse->parse_only) {
> +		struct error *error =
> +			error_object(ClientError, ER_TRIGGER_EXISTS, zName);

Unfortunately we have the problem with error objects creating. ClientError in
the constructor increments error counter in statistics visible to user. So this
still not happened error affects the public API. We need to find another way.

I think, we may assume that this way of error setting will be used for
ClientErrors only. Indeed, OOM is set explicitly in the place where emerged. Other
errors are not used in VDBE. The only hindrance is that ClientErrors have
different argument count. But I have found function box_error_set() in error.h.
It is a public API to set ClientError with any message.

So lets return to the previous implementation, but instead of passing
(error code; code arguments) lets pass (error code; the full error message) and
use box_error_set() instead of direct diag_set(ClientError, ...).

To repeat the same error message format as from the code you can use

     box_error_set(..., code, tnt_errcode_desc(code), parameters).

> +		if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
> +							 0, zName, error,
> +							 (noErr != 0)) != 0) {
> +			pParse->rc = SQL_TARANTOOL_ERROR;
> +			pParse->nErr++;
> +			goto trigger_cleanup;
> +		}
> +	}
>   

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> Unfortunately we have the problem with error objects creating. ClientError in
> the constructor increments error counter in statistics visible to user. So this
> still not happened error affects the public API. We need to find another way.
> 
> I think, we may assume that this way of error setting will be used for
> ClientErrors only. Indeed, OOM is set explicitly in the place where emerged. Other
> errors are not used in VDBE. The only hindrance is that ClientErrors have
> different argument count. But I have found function box_error_set() in error.h.
> It is a public API to set ClientError with any message.
> 
> So lets return to the previous implementation, but instead of passing
> (error code; code arguments) lets pass (error code; the full error message) and
> use box_error_set() instead of direct diag_set(ClientError, ...).
> 
> To repeat the same error message format as from the code you can use

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0edbda1..8db9231 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -4145,4 +4145,51 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
 		sqlite3DbFree(db, pWith);
 	}
 }
+
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				     int index_id, const char *name_src,
+				     int tarantool_error_code,
+				     const char *error_src, bool no_error)
+{
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	assert(v != NULL);
+
+	struct sqlite3 *db = parser->db;
+	char *name = NULL;
+	char *error = NULL;
+	name = sqlite3DbStrDup(db, name_src);
+	if (name != NULL)
+		error = sqlite3DbStrDup(db, error_src);
+	if (name == NULL || error == NULL) {
+		size_t size =
+			(name != NULL ? strlen(error_src) :
+			 strlen(name_src)) + 1;
+		const char *var_name = name != NULL ? "error" : "name";
+		diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);
+		sqlite3DbFree(db, name);
+		sqlite3DbFree(db, error);
+		return -1;
+	}
+
+	int cursor = parser->nTab++;
+	int entity_id =
+		SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id, index_id);
+	emit_open_cursor(parser, cursor, entity_id);
+
+	int name_reg = parser->nMem++;
+	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
+				      P4_DYNAMIC);
+	sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
+	if (no_error) {
+		sqlite3VdbeAddOp0(v, OP_Halt);
+	} else {
+		error = sqlite3DbStrDup(db, error);
+		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+				  ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+		sqlite3VdbeChangeP5(v, tarantool_error_code);
+	}
+	sqlite3VdbeAddOp1(v, OP_Close, cursor);
+	return 0;
+}
 #endif				/* !defined(SQLITE_OMIT_CTE) */
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0acf7bc..e1de815 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1454,10 +1454,14 @@ sqlite3_errmsg(sqlite3 * db)
 		z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
 	} else {
 		testcase(db->pErr == 0);
-		z = (char *)sqlite3_value_text(db->pErr);
 		assert(!db->mallocFailed);
-		if (z == 0) {
-			z = sqlite3ErrStr(db->errCode);
+		if (db->errCode != SQL_TARANTOOL_ERROR) {
+			assert(!db->mallocFailed);
+			z = (char *)sqlite3_value_text(db->pErr);
+			if (z == NULL)
+				z = sqlite3ErrStr(db->errCode);
+		} else {
+			z = diag_last_error(diag_get())->errmsg;
 		}
 	}
 	return z;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 7f1a201..fc6b858 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4594,4 +4594,28 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
+/**
+ * Generate VDBE code to halt execution with correct error if
+ * the object with specified name is already present in
+ * specified space.
+ * The function does not begin to hold the passed error pointer
+ * to memory.
+ *
+ * @param parser Parsing context.
+ * @param space_id Space to lookup identifier.
+ * @param index_id Index identifier containing string primary key.
+ * @param name Of object to test existence.
+ * @param tarantool_error_code to set on halt.
+ * @param error string to notify on halt.
+ * @param no_error Do not raise error flag.
+ *
+ * @retval -1 on memory allocation error.
+ * @retval 0 on success.
+ */
+int
+parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				     int index_id, const char *name,
+				     int tarantool_error_code,
+				     const char *error, bool no_error);
+
 #endif				/* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index ec420ea..eadef53 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -122,18 +122,6 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
 		goto trigger_cleanup;
 
-	if (!pParse->parse_only &&
-	    sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
-		if (!noErr) {
-			diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
-			pParse->rc = SQL_TARANTOOL_ERROR;
-			pParse->nErr++;
-		} else {
-			assert(!db->init.busy);
-		}
-		goto trigger_cleanup;
-	}
-
 	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),
@@ -184,6 +172,20 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 		pParse->nErr++;
 		goto trigger_cleanup;
 	}
+	if (!pParse->parse_only) {
+		char error[DIAG_ERRMSG_MAX];
+		snprintf(error, DIAG_ERRMSG_MAX,
+			 tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);
+		if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
+							 0, zName,
+							 ER_TRIGGER_EXISTS,
+							 error,
+							 (noErr != 0)) != 0) {
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			goto trigger_cleanup;
+		}
+	}
 
 	/*
 	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c035ffe..5f88e67 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -960,7 +960,9 @@ case OP_HaltIfNull: {      /* in3 */
  *
  * If P4 is not null then it is an error message string.
  *
- * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
+ * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
+ * P4 is error message to set. Else P5 is a value between 0 and 4,
+ * inclusive, that modifies the P4 string.
  *
  *    0:  (no change)
  *    1:  NOT NULL contraint failed: P4
@@ -968,8 +970,8 @@ case OP_HaltIfNull: {      /* in3 */
  *    3:  CHECK constraint failed: P4
  *    4:  FOREIGN KEY constraint failed: P4
  *
- * If P5 is not zero and P4 is NULL, then everything after the ":" is
- * omitted.
+ * If P5 is not zero and  P4 is  NULL, then everything after the
+ * ":" is omitted.
  *
  * There is an implied "Halt 0 0 0" instruction inserted at the very end of
  * every program.  So a jump past the last instruction of the program
@@ -1005,9 +1007,11 @@ case OP_Halt: {
 	p->rc = pOp->p1;
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
-	assert(pOp->p5<=4);
 	if (p->rc) {
-		if (pOp->p5) {
+		if (p->rc == SQL_TARANTOOL_ERROR) {
+			assert(pOp->p4.z != NULL);
+			box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+		} else if (pOp->p5 != 0) {
 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
 							       "FOREIGN KEY" };
 			testcase( pOp->p5==1);
@@ -1029,7 +1033,10 @@ case OP_Halt: {
 		p->rc = SQLITE_BUSY;
 	} else {
 		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
-		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
+		if (p->rc != SQL_TARANTOOL_ERROR)
+			rc = (p->rc != SQLITE_OK) ? SQLITE_ERROR : SQLITE_DONE;
+		else
+			rc = SQL_TARANTOOL_ERROR;
 	}
 	goto vdbe_return;
 }
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index d35338a..0aa5c4a 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -598,8 +598,9 @@ sqlite3Step(Vdbe * p)
 	 * contains the value that would be returned if sqlite3_finalize()
 	 * were called on statement p.
 	 */
-	assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR
-	       || (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE);
+	assert(rc == SQLITE_ROW || rc == SQLITE_DONE || rc == SQLITE_ERROR ||
+	       (rc & 0xff) == SQLITE_BUSY || rc == SQLITE_MISUSE ||
+	       rc == SQL_TARANTOOL_ERROR);
 	if (p->isPrepareV2 && rc != SQLITE_ROW && rc != SQLITE_DONE) {
 		/* If this statement was prepared using sqlite3_prepare_v2(), and an
 		 * error has occurred, then return the error code in p->rc to the

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy]

Thanks for the patch! See 6 comments below.

On 15/06/2018 19:21, Kirill Shcherbatov wrote:
>> Unfortunately we have the problem with error objects creating. ClientError in
>> the constructor increments error counter in statistics visible to user. So this
>> still not happened error affects the public API. We need to find another way.
>>
>> I think, we may assume that this way of error setting will be used for
>> ClientErrors only. Indeed, OOM is set explicitly in the place where emerged. Other
>> errors are not used in VDBE. The only hindrance is that ClientErrors have
>> different argument count. But I have found function box_error_set() in error.h.
>> It is a public API to set ClientError with any message.
>>
>> So lets return to the previous implementation, but instead of passing
>> (error code; code arguments) lets pass (error code; the full error message) and
>> use box_error_set() instead of direct diag_set(ClientError, ...).
>>
>> To repeat the same error message format as from the code you can use
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0edbda1..8db9231 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -4145,4 +4145,51 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
>   		sqlite3DbFree(db, pWith);
>   	}
>   }
> +
> +int
> +parser_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
> +				     int index_id, const char *name_src,
> +				     int tarantool_error_code,
> +				     const char *error_src, bool no_error)

1. Sorry, after discussion with Nikita we have decided to
use 'vdbe_' prefix for emitters even if they take parser in
first argument.

2. Mismatch of parameter names: error in header, error_src in source.

> +{
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	assert(v != NULL);
> +
> +	struct sqlite3 *db = parser->db;
> +	char *name = NULL;
> +	char *error = NULL;
> +	name = sqlite3DbStrDup(db, name_src);
> +	if (name != NULL)
> +		error = sqlite3DbStrDup(db, error_src);
> +	if (name == NULL || error == NULL) {
> +		size_t size =
> +			(name != NULL ? strlen(error_src) :
> +			 strlen(name_src)) + 1;
> +		const char *var_name = name != NULL ? "error" : "name";
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);

3. Please, reduce the mess. Separate these OOM handlers.

> +		sqlite3DbFree(db, name);
> +		sqlite3DbFree(db, error);
> +		return -1;
> +	}
> +
> +	int cursor = parser->nTab++;
> +	int entity_id =
> +		SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id, index_id);
> +	emit_open_cursor(parser, cursor, entity_id);
> +
> +	int name_reg = parser->nMem++;
> +	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
> +				      P4_DYNAMIC);
> +	sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
> +	if (no_error) {
> +		sqlite3VdbeAddOp0(v, OP_Halt);
> +	} else {
> +		error = sqlite3DbStrDup(db, error);

4. This error is duplicated already above, it is not?

> +		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +				  ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
> +		sqlite3VdbeChangeP5(v, tarantool_error_code);
> +	}
> +	sqlite3VdbeAddOp1(v, OP_Close, cursor);
> +	return 0;
> +}
>   #endif				/* !defined(SQLITE_OMIT_CTE) */
> diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
> index ec420ea..eadef53 100644
> --- a/src/box/sql/trigger.c
> +++ b/src/box/sql/trigger.c
> @@ -184,6 +172,20 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
>   		pParse->nErr++;
>   		goto trigger_cleanup;
>   	}
> +	if (!pParse->parse_only) {
> +		char error[DIAG_ERRMSG_MAX];
> +		snprintf(error, DIAG_ERRMSG_MAX,
> +			 tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);

5. Please, use tt_sprintf. Do not declare big arrays on the
stack when possible.

> +		if (parser_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
> +							 0, zName,
> +							 ER_TRIGGER_EXISTS,
> +							 error,
> +							 (noErr != 0)) != 0) {
> +			pParse->rc = SQL_TARANTOOL_ERROR;
> +			pParse->nErr++;
> +			goto trigger_cleanup;
> +		}
> +	}
>   
>   	/*
>   	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c035ffe..5f88e67 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1029,7 +1033,10 @@ case OP_Halt: {
>   		p->rc = SQLITE_BUSY;
>   	} else {
>   		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
> -		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
> +		if (p->rc != SQL_TARANTOOL_ERROR)
> +			rc = (p->rc != SQLITE_OK) ? SQLITE_ERROR : SQLITE_DONE;
> +		else
> +			rc = SQL_TARANTOOL_ERROR;

6. Earlier this function could return either DONE or ERROR. Now
it can return SQL_TARANTOOL_ERROR, and I am not sure that the
behavior should be changed in such way. Please, explain why do you
need SQL_TARANTOOL_ERROR here.

>   	}
>   	goto vdbe_return;
>   }

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> 1. Sorry, after discussion with Nikita we have decided to
> use 'vdbe_' prefix for emitters even if they take parser in
> first argument.
Tnx, squashed.

> 2. Mismatch of parameter names: error in header, error_src in source.
Fixed.

> 3. Please, reduce the mess. Separate these OOM handlers.
        name = sqlite3DbStrDup(db, name_src);
-       if (name == NULL)
+       if (name == NULL) {
+               size_t size = strlen(name_src) + 1;
+               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
                return -1;
+       }
        error = sqlite3DbStrDup(db, error_src);
        if (error == NULL) {
                sqlite3DbFree(db, name);
-               return -1;
-       }
-       if (name == NULL || error == NULL) {
-               size_t size =
-                       (name != NULL ? strlen(error_src) :
-                        strlen(name_src)) + 1;
-               const char *var_name = name != NULL ? "error" : "name";
-               diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);
-               sqlite3DbFree(db, name);
-               sqlite3DbFree(db, error);
+               size_t size = strlen(error_src) + 1;
+               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
                return -1;
        }

> 4. This error is duplicated already above, it is not?
Damn, you are right.
-               error = sqlite3DbStrDup(db, error);

> 5. Please, use tt_sprintf. Do not declare big arrays on the
> stack when possible.
-               char error[DIAG_ERRMSG_MAX];
-               snprintf(error, DIAG_ERRMSG_MAX,
-                        tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);
+               const char *error_msg =
+                       tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);

> 6. Earlier this function could return either DONE or ERROR. Now
> it can return SQL_TARANTOOL_ERROR, and I am not sure that the
> behavior should be changed in such way. Please, explain why do you
> need SQL_TARANTOOL_ERROR here.
This is the sane behavior that parser has. I'd like to distinguish tarantool errors and out them from diag like Parser does. 
Take a look to the changes in sqlite3_errmsg.

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See 4 comments below.

On 18/06/2018 22:22, Kirill Shcherbatov wrote:
>> 1. Sorry, after discussion with Nikita we have decided to
>> use 'vdbe_' prefix for emitters even if they take parser in
>> first argument.
> Tnx, squashed.
> 
>> 2. Mismatch of parameter names: error in header, error_src in source.
> Fixed.

1. Still is not fixed. Now mismatch of the parameter name in the
declaration and the name in the comment.

> 
>> 3. Please, reduce the mess. Separate these OOM handlers.
>          name = sqlite3DbStrDup(db, name_src);
> -       if (name == NULL)
> +       if (name == NULL) {
> +               size_t size = strlen(name_src) + 1;
> +               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
>                  return -1;
> +       }
>          error = sqlite3DbStrDup(db, error_src);
>          if (error == NULL) {
>                  sqlite3DbFree(db, name);
> -               return -1;
> -       }
> -       if (name == NULL || error == NULL) {
> -               size_t size =
> -                       (name != NULL ? strlen(error_src) :
> -                        strlen(name_src)) + 1;
> -               const char *var_name = name != NULL ? "error" : "name";
> -               diag_set(OutOfMemory, size, "sqlite3DbStrDup", var_name);
> -               sqlite3DbFree(db, name);
> -               sqlite3DbFree(db, error);
> +               size_t size = strlen(error_src) + 1;
> +               diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
>                  return -1;
>          }
> 
> 
>> 6. Earlier this function could return either DONE or ERROR. Now
>> it can return SQL_TARANTOOL_ERROR, and I am not sure that the
>> behavior should be changed in such way. Please, explain why do you
>> need SQL_TARANTOOL_ERROR here.
> This is the sane behavior that parser has. I'd like to distinguish tarantool errors and out them from diag like Parser does.
> Take a look to the changes in sqlite3_errmsg.

2. I see the changes, but they are not linked with VdbeExec return value. The only
place where you have used SQL_TARANTOOL_ERROR is an assertion in sqlite3Step.

db->errCode finally is set not to VdbeExec return value, but to Vdbe->rc, that
is SQL_TARANTOOL_ERROR already.

I have reverted your changes at the end of OP_Halt:

@@ -1033,10 +1033,7 @@ case OP_Halt: {
  		p->rc = SQLITE_BUSY;
  	} else {
  		assert(rc==SQLITE_OK || (p->rc&0xff)==SQLITE_CONSTRAINT);
-		if (p->rc != SQL_TARANTOOL_ERROR)
-			rc = p->rc != SQLITE_OK ? SQLITE_ERROR : SQLITE_DONE;
-		else
-			rc = SQL_TARANTOOL_ERROR;
+		rc = p->rc ? SQLITE_ERROR : SQLITE_DONE;
  	}
  	goto vdbe_return;

And nothing is changed. The tests passed ok.

Why?


3. Please, put a new patch version at the end of letter.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0edbda1d9..4588b1c6d 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -4145,4 +4145,50 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
>  		sqlite3DbFree(db, pWith);
>  	}
>  }
> +
> +int
> +vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
> +				   int index_id, const char *name_src,
> +				   int tarantool_error_code,
> +				   const char *error_src, bool no_error)
> +{
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	assert(v != NULL);
> +
> +	struct sqlite3 *db = parser->db;
> +	char *name = NULL;
> +	char *error = NULL;
> +	name = sqlite3DbStrDup(db, name_src);
> +	if (name == NULL) {
> +		size_t size = strlen(name_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
> +		return -1;
> +	}
> +	error = sqlite3DbStrDup(db, error_src);
> +	if (error == NULL) {
> +		sqlite3DbFree(db, name);
> +		size_t size = strlen(error_src) + 1;
> +		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
> +		return -1;
> +	}

4. Why do you need to separate char *name and *error announcements from
assignments?

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

Trigger presence in system should be tested on each VDBE
execution attempt, not on Parser iteration.

Part of #3435, #3273
---
 src/box/sql/build.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/box/sql/main.c         | 11 ++++++++---
 src/box/sql/sqliteInt.h    | 24 ++++++++++++++++++++++++
 src/box/sql/trigger.c      | 25 +++++++++++++------------
 src/box/sql/vdbe.c         | 20 +++++++++++++-------
 test/sql-tap/view.test.lua |  8 ++++----
 test/sql/view.result       |  4 ++--
 7 files changed, 106 insertions(+), 28 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c8bfad7..fff7c19 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3971,4 +3971,46 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
 		sqlite3DbFree(db, pWith);
 	}
 }
+
+int
+vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				   int index_id, const char *name_src,
+				   int tarantool_error_code,
+				   const char *error_src, bool no_error)
+{
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	assert(v != NULL);
+
+	struct sqlite3 *db = parser->db;
+	char *name = sqlite3DbStrDup(db, name_src);
+	if (name == NULL) {
+		size_t size = strlen(name_src) + 1;
+		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "name");
+		return -1;
+	}
+	char *error = sqlite3DbStrDup(db, error_src);
+	if (error == NULL) {
+		sqlite3DbFree(db, name);
+		size_t size = strlen(error_src) + 1;
+		diag_set(OutOfMemory, size, "sqlite3DbStrDup", "error");
+		return -1;
+	}
+
+	int cursor = parser->nTab++;
+	vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
+
+	int name_reg = parser->nMem++;
+	int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
+				      P4_DYNAMIC);
+	sqlite3VdbeAddOp4Int(v, OP_NoConflict, cursor, label + 3, name_reg, 1);
+	if (no_error) {
+		sqlite3VdbeAddOp0(v, OP_Halt);
+	} else {
+		sqlite3VdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+				  ON_CONFLICT_ACTION_FAIL, 0, error, P4_DYNAMIC);
+		sqlite3VdbeChangeP5(v, tarantool_error_code);
+	}
+	sqlite3VdbeAddOp1(v, OP_Close, cursor);
+	return 0;
+}
 #endif				/* !defined(SQLITE_OMIT_CTE) */
diff --git a/src/box/sql/main.c b/src/box/sql/main.c
index 0acf7bc..e435c01 100644
--- a/src/box/sql/main.c
+++ b/src/box/sql/main.c
@@ -1454,11 +1454,16 @@ sqlite3_errmsg(sqlite3 * db)
 		z = sqlite3ErrStr(SQLITE_NOMEM_BKPT);
 	} else {
 		testcase(db->pErr == 0);
-		z = (char *)sqlite3_value_text(db->pErr);
 		assert(!db->mallocFailed);
-		if (z == 0) {
-			z = sqlite3ErrStr(db->errCode);
+		if (db->errCode != SQL_TARANTOOL_ERROR) {
+			assert(!db->mallocFailed);
+			z = (char *)sqlite3_value_text(db->pErr);
+			if (z == NULL)
+				z = sqlite3ErrStr(db->errCode);
+		} else {
+			z = diag_last_error(diag_get())->errmsg;
 		}
+		assert(z != NULL);
 	}
 	return z;
 }
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 72803fa..831faf0 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4627,4 +4627,28 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
+/**
+ * Generate VDBE code to halt execution with correct error if
+ * the object with specified name is already present in
+ * specified space.
+ * The function does not begin to hold the passed error pointer
+ * to memory.
+ *
+ * @param parser Parsing context.
+ * @param space_id Space to lookup identifier.
+ * @param index_id Index identifier containing string primary key.
+ * @param name Of object to test existence.
+ * @param tarantool_error_code to set on halt.
+ * @param error_src string to notify on halt.
+ * @param no_error Do not raise error flag.
+ *
+ * @retval -1 on memory allocation error.
+ * @retval 0 on success.
+ */
+int
+vdbe_emit_execution_halt_on_exists(struct Parse *parser, int space_id,
+				   int index_id, const char *name,
+				   int tarantool_error_code,
+				   const char *error_src, bool no_error);
+
 #endif				/* SQLITEINT_H */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index e4c936d..8ccb646 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -122,18 +122,6 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
 		goto trigger_cleanup;
 
-	if (!pParse->parse_only &&
-	    sqlite3HashFind(&db->pSchema->trigHash, zName) != NULL) {
-		if (!noErr) {
-			diag_set(ClientError, ER_TRIGGER_EXISTS, zName);
-			pParse->rc = SQL_TARANTOOL_ERROR;
-			pParse->nErr++;
-		} else {
-			assert(!db->init.busy);
-		}
-		goto trigger_cleanup;
-	}
-
 	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),
@@ -179,6 +167,19 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 		pParse->nErr++;
 		goto trigger_cleanup;
 	}
+	if (!pParse->parse_only) {
+		const char *error_msg =
+			tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS), zName);
+		if (vdbe_emit_execution_halt_on_exists(pParse, BOX_TRIGGER_ID,
+						       0, zName,
+						       ER_TRIGGER_EXISTS,
+						       error_msg,
+						       (noErr != 0)) != 0) {
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			goto trigger_cleanup;
+		}
+	}
 
 	/*
 	 * INSTEAD OF triggers can only appear on views and BEFORE triggers
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c1df880..9f7f50d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -960,7 +960,9 @@ case OP_HaltIfNull: {      /* in3 */
  *
  * If P4 is not null then it is an error message string.
  *
- * P5 is a value between 0 and 4, inclusive, that modifies the P4 string.
+ * If P1 is SQL_TARANTOOL_ERROR then P5 is a ClientError code and
+ * P4 is error message to set. Else P5 is a value between 0 and 4,
+ * inclusive, that modifies the P4 string.
  *
  *    0:  (no change)
  *    1:  NOT NULL contraint failed: P4
@@ -968,8 +970,8 @@ case OP_HaltIfNull: {      /* in3 */
  *    3:  CHECK constraint failed: P4
  *    4:  FOREIGN KEY constraint failed: P4
  *
- * If P5 is not zero and P4 is NULL, then everything after the ":" is
- * omitted.
+ * If P5 is not zero and  P4 is  NULL, then everything after the
+ * ":" is omitted.
  *
  * There is an implied "Halt 0 0 0" instruction inserted at the very end of
  * every program.  So a jump past the last instruction of the program
@@ -1005,9 +1007,11 @@ case OP_Halt: {
 	p->rc = pOp->p1;
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
-	assert(pOp->p5<=4);
 	if (p->rc) {
-		if (pOp->p5) {
+		if (p->rc == SQL_TARANTOOL_ERROR) {
+			assert(pOp->p4.z != NULL);
+			box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+		} else if (pOp->p5 != 0) {
 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
 							       "FOREIGN KEY" };
 			testcase( pOp->p5==1);
@@ -2999,8 +3003,10 @@ case OP_CheckViewReferences: {
 	struct space *space = space_by_id(space_id);
 	assert(space != NULL);
 	if (space->def->view_ref_count > 0) {
-		sqlite3VdbeError(p,"Can't drop table %s: other views depend "
-				 "on this space",  space->def->name);
+		box_error_set(__FILE__, __LINE__, ER_DROP_SPACE,
+			      tnt_errcode_desc(ER_DROP_SPACE),
+			      space->def->name,
+			      "other views depend on this space");
 		rc = SQL_TARANTOOL_ERROR;
 		goto abort_due_to_error;
 	}
diff --git a/test/sql-tap/view.test.lua b/test/sql-tap/view.test.lua
index 5dc9503..ac1a27d 100755
--- a/test/sql-tap/view.test.lua
+++ b/test/sql-tap/view.test.lua
@@ -127,7 +127,7 @@ test:do_catchsql_test(
         DROP TABLE t1;
     ]], {
         -- <view-1.6>
-        1, "Can't drop table T1: other views depend on this space"
+        1, "Can't drop space 'T1': other views depend on this space"
         -- </view-1.6>
     })
 
@@ -1185,7 +1185,7 @@ test:do_catchsql_test(
         DROP TABLE t11;
     ]], {
         -- <view-23.2>
-        1, "Can't drop table T11: other views depend on this space"
+        1, "Can't drop space 'T11': other views depend on this space"
         -- </view-23.2>
     })
 
@@ -1195,7 +1195,7 @@ test:do_catchsql_test(
         DROP TABLE t12;
     ]], {
         -- <view-23.3>
-        1, "Can't drop table T12: other views depend on this space"
+        1, "Can't drop space 'T12': other views depend on this space"
         -- </view-23.3>
     })
 
@@ -1205,7 +1205,7 @@ test:do_catchsql_test(
         DROP TABLE t13;
     ]], {
         -- <view-23.4>
-        1, "Can't drop table T13: other views depend on this space"
+        1, "Can't drop space 'T13': other views depend on this space"
         -- </view-23.4>
     })
 
diff --git a/test/sql/view.result b/test/sql/view.result
index 0b9dc55..b033f19 100644
--- a/test/sql/view.result
+++ b/test/sql/view.result
@@ -124,7 +124,7 @@ box.sql.execute("CREATE VIEW v2 AS SELECT * FROM t2;");
 ...
 box.sql.execute("DROP TABLE t2;");
 ---
-- error: 'Can''t drop table T2: other views depend on this space'
+- error: 'Can''t drop space ''T2'': other views depend on this space'
 ...
 sp = box.space._space:get{box.space.T2.id};
 ---
@@ -134,7 +134,7 @@ sp = box.space._space:replace(sp);
 ...
 box.sql.execute("DROP TABLE t2;");
 ---
-- error: 'Can''t drop table T2: other views depend on this space'
+- error: 'Can''t drop space ''T2'': other views depend on this space'
 ...
 box.sql.execute("DROP VIEW v2;");
 ---
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy]

Thanks for the fixes!

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c1df880..9f7f50d 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2999,8 +3003,10 @@ case OP_CheckViewReferences: {
>   	struct space *space = space_by_id(space_id);
>   	assert(space != NULL);
>   	if (space->def->view_ref_count > 0) {
> -		sqlite3VdbeError(p,"Can't drop table %s: other views depend "
> -				 "on this space",  space->def->name);
> +		box_error_set(__FILE__, __LINE__, ER_DROP_SPACE,
> +			      tnt_errcode_desc(ER_DROP_SPACE),
> +			      space->def->name,
> +			      "other views depend on this space");

As I told you verbally, here you can use directly diag_set since
you know the concrete error code and parameters.

So please, apply this diff:

@@ -3003,10 +3003,8 @@ case OP_CheckViewReferences: {
  	struct space *space = space_by_id(space_id);
  	assert(space != NULL);
  	if (space->def->view_ref_count > 0) {
-		box_error_set(__FILE__, __LINE__, ER_DROP_SPACE,
-			      tnt_errcode_desc(ER_DROP_SPACE),
-			      space->def->name,
-			      "other views depend on this space");
+		diag_set(ClientError, ER_DROP_SPACE, space->def->name,
+			 "other views depend on this space");
  		rc = SQL_TARANTOOL_ERROR;
  		goto abort_due_to_error;
  	}

>   		rc = SQL_TARANTOOL_ERROR;
>   		goto abort_due_to_error;
>   	}

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> As I told you verbally, here you can use directly diag_set since
> you know the concrete error code and parameters.
> 
> So please, apply this diff:
> 
> @@ -3003,10 +3003,8 @@ case OP_CheckViewReferences: {
>   	struct space *space = space_by_id(space_id);
>   	assert(space != NULL);
>   	if (space->def->view_ref_count > 0) {
> -		box_error_set(__FILE__, __LINE__, ER_DROP_SPACE,
> -			      tnt_errcode_desc(ER_DROP_SPACE),
> -			      space->def->name,
> -			      "other views depend on this space");
> +		diag_set(ClientError, ER_DROP_SPACE, space->def->name,
> +			 "other views depend on this space");
>   		rc = SQL_TARANTOOL_ERROR;
>   		goto abort_due_to_error;
>   	}
> 
>>   		rc = SQL_TARANTOOL_ERROR;
>>   		goto abort_due_to_error;
>>   	}
> 
Thank you for review. Applied.

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Vladislav Shpilevoy]

Hello. Thanks for the fixes!

Now the same comment as a pair reviews before:
parameter name mismatch in header and source.

On 20/06/2018 09:38, Kirill Shcherbatov wrote:
>> As I told you verbally, here you can use directly diag_set since
>> you know the concrete error code and parameters.
>>
>> So please, apply this diff:
>>
>> @@ -3003,10 +3003,8 @@ case OP_CheckViewReferences: {
>>    	struct space *space = space_by_id(space_id);
>>    	assert(space != NULL);
>>    	if (space->def->view_ref_count > 0) {
>> -		box_error_set(__FILE__, __LINE__, ER_DROP_SPACE,
>> -			      tnt_errcode_desc(ER_DROP_SPACE),
>> -			      space->def->name,
>> -			      "other views depend on this space");
>> +		diag_set(ClientError, ER_DROP_SPACE, space->def->name,
>> +			 "other views depend on this space");
>>    		rc = SQL_TARANTOOL_ERROR;
>>    		goto abort_due_to_error;
>>    	}
>>
>>>    		rc = SQL_TARANTOOL_ERROR;
>>>    		goto abort_due_to_error;
>>>    	}
>>
> Thank you for review. Applied.
> 

[tarantool-patches] Re: [PATCH v3 10/10] sql: VDBE tests for trigger existence

[Kirill Shcherbatov]

> Now the same comment as a pair reviews before:
> parameter name mismatch in header and source.
> 
Damn, another one name. Ok.

[tarantool-patches] [PATCH v3 02/10] sql: fix leak on CREATE TABLE and resolve self ref

[Kirill Shcherbatov]

---
 src/box/sql.c         |  8 ++++----
 src/box/sql/build.c   | 14 +++++---------
 src/box/sql/prepare.c |  1 +
 src/box/sql/trigger.c |  1 +
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 93fca68..509e581 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1856,13 +1856,13 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
 
 	sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
 				   expr_list);
-
-	sql_parser_destroy(&parser);
+	int rc = 0;
 	if (parser.rc != SQLITE_OK) {
 		/* Tarantool error may be already set with diag. */
 		if (parser.rc != SQL_TARANTOOL_ERROR)
 			diag_set(ClientError, ER_SQL, parser.zErrMsg);
-		return -1;
+		rc = -1;
 	}
-	return 0;
+	sql_parser_destroy(&parser);
+	return rc;
 }
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 62d687b..606bb69 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -565,7 +565,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 	 */
 	if (!pParse->nested) {
 		if ((v = sqlite3GetVdbe(pParse)) == NULL)
-			goto begin_table_error;
+			goto cleanup;
 		sqlite3VdbeCountChanges(v);
 	}
 
@@ -575,7 +575,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 	if (zName == 0)
 		return;
 	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
-		goto begin_table_error;
+		goto cleanup;
 
 	assert(db->pSchema != NULL);
 	pTable = sqlite3HashFind(&db->pSchema->tblHash, zName);
@@ -587,12 +587,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 		} else {
 			assert(!db->init.busy || CORRUPT_DB);
 		}
-		goto begin_table_error;
+		goto cleanup;
 	}
 
 	pTable = sql_table_new(pParse, zName);
 	if (pTable == NULL)
-		goto begin_table_error;
+		goto cleanup;
 
 	assert(pParse->pNewTable == 0);
 	pParse->pNewTable = pTable;
@@ -608,11 +608,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
 	if (!db->init.busy && (v = sqlite3GetVdbe(pParse)) != 0)
 		sql_set_multi_write(pParse, true);
 
-	/* Normal (non-error) return. */
-	return;
-
-	/* If an error occurs, we jump here */
- begin_table_error:
+ cleanup:
 	sqlite3DbFree(db, zName);
 	return;
 }
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index 3042cb1..e43fbcb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -453,5 +453,6 @@ sql_parser_destroy(Parse *parser)
 		db->lookaside.bDisable -= parser->disableLookaside;
 	}
 	parser->disableLookaside = 0;
+	sqlite3DbFree(db, parser->zErrMsg);
 	region_destroy(&parser->region);
 }
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index ea35211..dc9dc46 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -815,6 +815,7 @@ transferParseError(Parse * pTo, Parse * pFrom)
 	} else {
 		sqlite3DbFree(pFrom->db, pFrom->zErrMsg);
 	}
+	pFrom->zErrMsg = NULL;
 }
 
 /*
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 02/10] sql: fix leak on CREATE TABLE and resolve self ref

[n.pettik]

Nitpicking: if you can’t fit commit subject into 50 chars,
you are able to shrink it and instead write about it in commit message.
Like:

sql: fix memory leaks

This patch fixes several memory leaks. One of them appeared during
introducing space_def in struct Table. Name of table wasn’t released
in case of successful returning from function sqlite3StartTable().
Second one - bla-bla-bla.

It is only advise, tho. But I stick to the point that describing provided changes
aren’t less important then code itself.

Patch itself looks OK to me.

[tarantool-patches] Re: [PATCH v3 02/10] sql: fix leak on CREATE TABLE and resolve self ref

[Vladislav Shpilevoy]

Thanks for the review!

I have fixed it and pushed into 2.0.

On 15/06/2018 01:46, n.pettik wrote:
> Nitpicking: if you can’t fit commit subject into 50 chars,
> you are able to shrink it and instead write about it in commit message.
> Like:
> 
> sql: fix memory leaks
> 
> This patch fixes several memory leaks. One of them appeared during
> introducing space_def in struct Table. Name of table wasn’t released
> in case of successful returning from function sqlite3StartTable().
> Second one - bla-bla-bla.
> 
> It is only advise, tho. But I stick to the point that describing provided changes
> aren’t less important then code itself.
> 
> Patch itself looks OK to me.
> 

[tarantool-patches] [PATCH v3 03/10] 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 509e581..85de926 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -690,8 +690,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 v3 04/10] box: port schema_find_id to C

[Kirill Shcherbatov]

Part of #3273.
---
 src/box/schema.cc | 54 ++++++++++++++++++++++++++++++++++++++----------------
 src/box/schema.h  | 23 ++++++++++++++++-------
 src/box/user.cc   |  4 +++-
 3 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2ddf920..ccdc744 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -222,30 +222,52 @@ 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)
+	       const char *name, uint32_t len, uint32_t *object_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);
+	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", "key");
+		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);
-	if (tuple) {
+	struct iterator *it = index_create_iterator(index, ITER_EQ, key, 1);
+	if (it == NULL) {
+		region_truncate(region, used);
+		return -1;
+	}
+	struct tuple *tuple;
+	int rc = iterator_next(it, &tuple);
+	if (rc == 0) {
 		/* id is always field #1 */
-		return tuple_field_u32_xc(tuple, 0);
+		if (tuple == NULL)
+			*object_id = BOX_ID_NIL;
+		else if (tuple_field_u32(tuple, 0, object_id) != 0)
+			rc = -1;
 	}
-	return BOX_ID_NIL;
+	iterator_delete(it);
+	region_truncate(region, used);
+	return rc;
 }
 
 /**
diff --git a/src/box/schema.h b/src/box/schema.h
index 1f7414f..3e76604 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 object id by name in specified system space with index.
+ *
+ * @param system_space_id identifier of the system object.
+ * @param index_id identifier of the index to lookup.
+ * @param name of object to lookup.
+ * @param len length of a name.
+ * @param object_id[out] object_id or BOX_ID_NIL - not found.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+schema_find_id(uint32_t system_space_id, uint32_t index_id, const char *name,
+	       uint32_t len, uint32_t *object_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));
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 04/10] box: port schema_find_id to C

[Vladislav Shpilevoy]

Hello, Nikita. Please, review this and the previous 3 commits so as to
push the patchset part by part and simplify rebase of next versions.


On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> Part of #3273.
> ---
>   src/box/schema.cc | 54 ++++++++++++++++++++++++++++++++++++++----------------
>   src/box/schema.h  | 23 ++++++++++++++++-------
>   src/box/user.cc   |  4 +++-
>   3 files changed, 57 insertions(+), 24 deletions(-)
> 

[tarantool-patches] Re: [PATCH v3 04/10] box: port schema_find_id to C

[n.pettik]

First four patches LGTM as well.

> Hello, Nikita. Please, review this and the previous 3 commits so as to
> push the patchset part by part and simplify rebase of next versions.

[tarantool-patches] Re: [PATCH v3 04/10] box: port schema_find_id to C

[Vladislav Shpilevoy]

Thanks. First 4 commits are pushed to 2.0.

Kirill, please, rebase.

On 15/06/2018 01:46, n.pettik wrote:
> First four patches LGTM as well.
> 
>> Hello, Nikita. Please, review this and the previous 3 commits so as to
>> push the patchset part by part and simplify rebase of next versions.
> 

[tarantool-patches] [PATCH v3 05/10] sql: refactor sql_expr_compile to return AST

[Kirill Shcherbatov]

---
 src/box/alter.cc       |  6 +++---
 src/box/sql.c          |  5 +++--
 src/box/sql.h          |  9 ++++-----
 src/box/sql/tokenize.c | 32 +++++++++++++++++++-------------
 4 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index b62f8ad..f2bf85d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -406,9 +406,9 @@ field_def_decode(struct field_def *field, const char **data,
 	}
 
 	if (field->default_value != NULL &&
-	    sql_expr_compile(sql_get(), field->default_value,
-			     strlen(field->default_value),
-			     &field->default_value_expr) != 0)
+		(field->default_value_expr =
+			sql_expr_compile(sql_get(), field->default_value,
+					 strlen(field->default_value))) == NULL)
 		diag_raise();
 }
 
diff --git a/src/box/sql.c b/src/box/sql.c
index 85de926..d18f379 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1813,8 +1813,9 @@ sql_check_list_item_init(struct ExprList *expr_list, int column,
 			return -1;
 		}
 	}
-	if (expr_str != NULL && sql_expr_compile(db, expr_str, expr_str_len,
-						 &item->pExpr) != 0) {
+	if (expr_str != NULL &&
+	    (item->pExpr = sql_expr_compile(db, expr_str, expr_str_len)) ==
+		NULL) {
 		sqlite3DbFree(db, item->zName);
 		return -1;
 	}
diff --git a/src/box/sql.h b/src/box/sql.h
index 23021e5..3f6cf22 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -75,13 +75,12 @@ struct Table;
  * @param db SQL context handle.
  * @param expr Expression to parse.
  * @param expr_len Length of @an expr.
- * @param[out] result Result: AST structure.
  *
- * @retval Error code if any.
+ * @retval NULL on error.
+ * @retval not NULL Expr AST pointer on success.
  */
-int
-sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len,
-		 struct Expr **result);
+struct Expr *
+sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len);
 
 /**
  * Store duplicate of a parsed expression into @a parser.
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index 42c70a2..d84a840 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -42,6 +42,7 @@
 
 #include "say.h"
 #include "sqliteInt.h"
+#include "tarantoolInt.h"
 
 /* Character classes for tokenizing
  *
@@ -510,8 +511,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() != NULL)
+			error = tarantoolErrorMessage();
+		else
+			error = sqlite3ErrStr(pParse->rc);
+		pParse->zErrMsg = sqlite3MPrintf(db, "%s", error);
 	}
 	assert(pzErrMsg != 0);
 	if (pParse->zErrMsg) {
@@ -539,9 +545,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 +555,20 @@ 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;
+		return NULL;
 	}
 	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;
-	}
-	*result = parser.parsed_expr;
+	char *sql_error;
+	if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK &&
+		parser.rc != SQL_TARANTOOL_ERROR)
+		diag_set(ClientError, ER_SQL, sql_error);
+	else
+		expression = parser.parsed_expr;
 	sql_parser_destroy(&parser);
-	return 0;
+	return expression;
 }
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 05/10] sql: refactor sql_expr_compile to return AST

[Vladislav Shpilevoy]

Thanks for the fixes! See 2 comments below.

Besides, I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> ---
>   src/box/alter.cc       |  6 +++---
>   src/box/sql.c          |  5 +++--
>   src/box/sql.h          |  9 ++++-----
>   src/box/sql/tokenize.c | 32 +++++++++++++++++++-------------
>   4 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index 42c70a2..d84a840 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -550,19 +555,20 @@ 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;
> +		return NULL;

1. Here parser.region leaks.

>   	}
>   	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;
> -	}
> -	*result = parser.parsed_expr;
> +	char *sql_error;
> +	if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK &&
> +		parser.rc != SQL_TARANTOOL_ERROR)
> +		diag_set(ClientError, ER_SQL, sql_error);
> +	else
> +		expression = parser.parsed_expr;

2. If a TARANTOOL_ERROR has occurred and parsed_expr had been set before it,
then you return not NULL, but should return NULL.

And you should destroy the expression. Looks like it was a leak before your
patch. Lets always delete all the parsed expressions in parser_destroy (here
only parsed_expr is). And in this function on success nullify this member
before destroy.

>   	sql_parser_destroy(&parser);
> -	return 0;
> +	return expression;
>   }
> 

[tarantool-patches] Re: [PATCH v3 05/10] sql: refactor sql_expr_compile to return AST

[Kirill Shcherbatov]

> 1. Here parser.region leaks.
> 2. If a TARANTOOL_ERROR has occurred and parsed_expr had been set before it,
> then you return not NULL, but should return NULL.
> 
> And you should destroy the expression. Looks like it was a leak before your
> patch. Lets always delete all the parsed expressions in parser_destroy (here
> only parsed_expr is). And in this function on success nullify this member
> before destroy.

diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e43fbcb..34ee8e3 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
        }
        parser->disableLookaside = 0;
        sqlite3DbFree(db, parser->zErrMsg);
+       sql_expr_delete(db, parser->parsed_expr, false);
        region_destroy(&parser->region);
 }
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index d6f5a43..9e4576b 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -559,16 +559,20 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
        char *stmt = (char *)region_alloc(&parser.region, len + 1);
        if (stmt == NULL) {
                diag_set(OutOfMemory, len + 1, "region_alloc", "stmt");
-               return NULL;
+               goto end;
        }
        sprintf(stmt, "%s%.*s", outer, expr_len, expr);
 
        char *sql_error;
-       if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK &&
-           parser.rc != SQL_TARANTOOL_ERROR)
-               diag_set(ClientError, ER_SQL, sql_error);
-       else
+       if (sqlite3RunParser(&parser, stmt, &sql_error) != SQLITE_OK) {
+               if (parser.rc != SQL_TARANTOOL_ERROR)
+                       diag_set(ClientError, ER_SQL, sql_error);
+       } else {
                expression = parser.parsed_expr;
+               parser.parsed_expr = NULL;
+       }
+
+end:
        sql_parser_destroy(&parser);
        return expression;
 }

[tarantool-patches] [PATCH v3 06/10] sql: move sqlite3DeleteTrigger to sql.h

[Kirill Shcherbatov]

As we are going to port triggers to server, we need
an instrument to release allocated memory in alter.cc.

Part of #3273.
---
 src/box/sql.h           |  9 +++++++++
 src/box/sql/callback.c  |  7 +++----
 src/box/sql/sqliteInt.h |  3 +--
 src/box/sql/status.c    |  6 +++---
 src/box/sql/tokenize.c  |  2 +-
 src/box/sql/trigger.c   | 30 +++++++++++++-----------------
 6 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/src/box/sql.h b/src/box/sql.h
index 3f6cf22..35aacc3 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
@@ -83,6 +84,14 @@ struct Expr *
 sql_expr_compile(struct sqlite3 *db, const char *expr, int expr_len);
 
 /**
+ * Free AST pointed by trigger.
+ * @param db SQL handle.
+ * @param trigger AST object.
+ */
+void
+sql_trigger_delete(struct sqlite3 *db, 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/callback.c b/src/box/sql/callback.c
index 8289c05..8bc6a32 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -290,10 +290,9 @@ sqlite3SchemaClear(sqlite3 * db)
 	temp1 = pSchema->tblHash;
 	temp2 = pSchema->trigHash;
 	sqlite3HashInit(&pSchema->trigHash);
-	for (pElem = sqliteHashFirst(&temp2); pElem;
-	     pElem = sqliteHashNext(pElem)) {
-		sqlite3DeleteTrigger(0, (Trigger *) sqliteHashData(pElem));
-	}
+	for (pElem = sqliteHashFirst(&temp2); pElem != NULL;
+		pElem = sqliteHashNext(pElem))
+		sql_trigger_delete(NULL, (Trigger *) sqliteHashData(pElem));
 	sqlite3HashClear(&temp2);
 	sqlite3HashInit(&pSchema->tblHash);
 	for (pElem = sqliteHashFirst(&temp1); pElem;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a1..ecbd573 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4027,7 +4027,6 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
 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);
@@ -4035,7 +4034,7 @@ u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
 #define sqlite3IsToplevel(p) ((p)->pToplevel==0)
 #else
 #define sqlite3TriggersExist(C,D,E,F) 0
-#define sqlite3DeleteTrigger(A,B)
+#define sql_trigger_delete(A,B)
 #define sqlite3DropTriggerPtr(A,B)
 #define sqlite3UnlinkAndDeleteTrigger(A,B,C)
 #define sqlite3CodeRowTrigger(A,B,C,D,E,F,G,H,I)
diff --git a/src/box/sql/status.c b/src/box/sql/status.c
index 84f07cc..dda91c5 100644
--- a/src/box/sql/status.c
+++ b/src/box/sql/status.c
@@ -256,9 +256,9 @@ sqlite3_db_status(sqlite3 * db,	/* The database connection whose status is desir
 
 				for (p = sqliteHashFirst(&pSchema->trigHash); p;
 				     p = sqliteHashNext(p)) {
-					sqlite3DeleteTrigger(db,
-							     (Trigger *)
-							     sqliteHashData(p));
+					sql_trigger_delete(db,
+							   (Trigger *)
+							   sqliteHashData(p));
 				}
 				for (p = sqliteHashFirst(&pSchema->tblHash); p;
 				     p = sqliteHashNext(p)) {
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index d84a840..d22ce8c 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -534,7 +534,7 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 
 	if (pParse->pWithToFree)
 		sqlite3WithDelete(db, pParse->pWithToFree);
-	sqlite3DeleteTrigger(db, pParse->pNewTrigger);
+	sql_trigger_delete(db, pParse->pNewTrigger);
 	sqlite3DbFree(db, pParse->pVList);
 	while (pParse->pZombieTab) {
 		Table *p = pParse->pZombieTab;
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index dc9dc46..84ea8db 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -185,11 +185,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) {
-		sqlite3DeleteTrigger(db, pTrigger);
-	} else {
+	if (pParse->pNewTrigger == NULL)
+		sql_trigger_delete(db, pTrigger);
+	else
 		assert(pParse->pNewTrigger == pTrigger);
-	}
 }
 
 /*
@@ -328,7 +327,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		/* No need to free zName sinceif we reach this point
 		   alloc for it either wasn't called at all or failed.  */
 	}
-	sqlite3DeleteTrigger(db, pTrig);
+	sql_trigger_delete(db, pTrig);
 	assert(!pParse->pNewTrigger);
 	sqlite3DeleteTriggerStep(db, pStepList);
 }
@@ -471,20 +470,17 @@ sqlite3TriggerDeleteStep(sqlite3 * db,	/* Database connection */
 	return pTriggerStep;
 }
 
-/*
- * Recursively delete a Trigger structure
- */
 void
-sqlite3DeleteTrigger(sqlite3 * db, Trigger * pTrigger)
+sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
 {
-	if (pTrigger == 0)
+	if (trigger == NULL)
 		return;
-	sqlite3DeleteTriggerStep(db, pTrigger->step_list);
-	sqlite3DbFree(db, pTrigger->zName);
-	sqlite3DbFree(db, pTrigger->table);
-	sql_expr_delete(db, pTrigger->pWhen, false);
-	sqlite3IdListDelete(db, pTrigger->pColumns);
-	sqlite3DbFree(db, pTrigger);
+	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);
 }
 
 /*
@@ -593,7 +589,7 @@ sqlite3UnlinkAndDeleteTrigger(sqlite3 * db, const char *zName)
 			     pp = &((*pp)->pNext)) ;
 			*pp = (*pp)->pNext;
 		}
-		sqlite3DeleteTrigger(db, pTrigger);
+		sql_trigger_delete(db, pTrigger);
 		user_session->sql_flags |= SQLITE_InternChanges;
 	}
 }
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 06/10] sql: move sqlite3DeleteTrigger to sql.h

[Vladislav Shpilevoy]

Thanks for the fixes!

I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> As we are going to port triggers to server, we need
> an instrument to release allocated memory in alter.cc.
> 
> Part of #3273.
> ---
>   src/box/sql.h           |  9 +++++++++
>   src/box/sql/callback.c  |  7 +++----
>   src/box/sql/sqliteInt.h |  3 +--
>   src/box/sql/status.c    |  6 +++---
>   src/box/sql/tokenize.c  |  2 +-
>   src/box/sql/trigger.c   | 30 +++++++++++++-----------------
>   6 files changed, 30 insertions(+), 27 deletions(-)
> 

[tarantool-patches] [PATCH v3 07/10] box: sort error codes in misc.test

[Kirill Shcherbatov]

As error codes were not sorted, changing any of error
constants significantly change test case output. This
cause unnecessary changes on each such commit.
---
 test/box/misc.result   | 325 +++++++++++++++++++++++++------------------------
 test/box/misc.test.lua |   4 +-
 2 files changed, 166 insertions(+), 163 deletions(-)

diff --git a/test/box/misc.result b/test/box/misc.result
index a2bbdca..930d0a8 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -319,173 +319,174 @@ type(require('yaml').encode(box.slab.info()));
 ----------------
 t = {}
 for k,v in pairs(box.error) do
-   table.insert(t, 'box.error.'..tostring(k)..' : '..tostring(v))
+   if type(v) == 'number' then
+    t[v] = 'box.error.'..tostring(k)
+   end
 end;
 ---
 ...
 t;
 ---
-- - 'box.error.UNKNOWN_REPLICA : 62'
-  - 'box.error.WRONG_INDEX_RECORD : 106'
-  - 'box.error.NO_SUCH_TRIGGER : 34'
-  - 'box.error.SEQUENCE_EXISTS : 146'
-  - 'box.error.CHECKPOINT_IN_PROGRESS : 120'
-  - 'box.error.FIELD_TYPE : 23'
-  - 'box.error.SQL_BIND_PARAMETER_MAX : 156'
-  - 'box.error.WRONG_SPACE_FORMAT : 141'
-  - 'box.error.SQL_BIND_TYPE : 155'
-  - 'box.error.UNKNOWN_UPDATE_OP : 28'
-  - 'box.error.WRONG_COLLATION_OPTIONS : 151'
-  - 'box.error.CURSOR_NO_TRANSACTION : 80'
-  - 'box.error.TUPLE_REF_OVERFLOW : 86'
-  - 'box.error.ALTER_SEQUENCE : 143'
-  - 'box.error.INVALID_XLOG_NAME : 75'
-  - 'box.error.SAVEPOINT_EMPTY_TX : 60'
-  - 'box.error.NO_SUCH_FUNCTION : 51'
-  - 'box.error.ROLE_LOOP : 87'
-  - 'box.error.TUPLE_NOT_FOUND : 4'
-  - 'box.error.LOADING : 116'
-  - 'box.error.VIEW_MISSING_SQL : 161'
-  - 'box.error.BACKUP_IN_PROGRESS : 129'
-  - 'box.error.DROP_USER : 44'
-  - 'box.error.MODIFY_INDEX : 14'
-  - 'box.error.PASSWORD_MISMATCH : 47'
-  - 'box.error.UNSUPPORTED_ROLE_PRIV : 98'
-  - 'box.error.ACCESS_DENIED : 42'
-  - 'box.error.CANT_CREATE_COLLATION : 150'
-  - 'box.error.USER_EXISTS : 46'
-  - 'box.error.WAL_IO : 40'
-  - 'box.error.PROC_RET : 21'
-  - 'box.error.PRIV_GRANTED : 89'
-  - 'box.error.CREATE_SPACE : 9'
-  - 'box.error.GRANT : 88'
-  - 'box.error.INVALID_INDEX_FILE : 131'
-  - 'box.error.UNKNOWN_SCHEMA_OBJECT : 49'
-  - 'box.error.WRONG_DD_VERSION : 140'
-  - 'box.error.CREATE_ROLE : 84'
-  - 'box.error.VINYL_MAX_TUPLE_SIZE : 139'
-  - 'box.error.LOAD_FUNCTION : 99'
-  - 'box.error.INVALID_XLOG : 74'
-  - 'box.error.PRIV_NOT_GRANTED : 91'
-  - 'box.error.TRANSACTION_CONFLICT : 97'
-  - 'box.error.GUEST_USER_PASSWORD : 96'
-  - 'box.error.PROC_C : 102'
-  - 'box.error.INVALID_RUN_FILE : 132'
-  - 'box.error.NONMASTER : 6'
-  - 'box.error.MEMTX_MAX_TUPLE_SIZE : 110'
-  - 'box.error.DROP_FUNCTION : 71'
-  - 'box.error.CFG : 59'
-  - 'box.error.NO_SUCH_FIELD : 37'
-  - 'box.error.CONNECTION_TO_SELF : 117'
-  - 'box.error.FUNCTION_MAX : 54'
-  - 'box.error.ILLEGAL_PARAMS : 1'
-  - 'box.error.PARTIAL_KEY : 136'
-  - 'box.error.SAVEPOINT_NO_TRANSACTION : 114'
-  - 'box.error.LOAD_MODULE : 138'
-  - 'box.error.FUNCTION_LANGUAGE : 100'
-  - 'box.error.ROLE_GRANTED : 90'
-  - 'box.error.CHECKPOINT_ROLLBACK : 134'
-  - 'box.error.NO_SUCH_USER : 45'
-  - 'box.error.CANT_UPDATE_PRIMARY_KEY : 94'
-  - 'box.error.EXACT_MATCH : 19'
-  - 'box.error.ROLE_EXISTS : 83'
-  - 'box.error.REPLICASET_UUID_IS_RO : 65'
-  - 'box.error.INDEX_TYPE : 13'
-  - 'box.error.NO_SUCH_PROC : 33'
-  - 'box.error.MEMORY_ISSUE : 2'
-  - 'box.error.KEY_PART_TYPE : 18'
-  - 'box.error.CREATE_FUNCTION : 50'
-  - 'box.error.ALREADY_RUNNING : 126'
-  - 'box.error.SQL_BIND_VALUE : 154'
-  - 'box.error.NO_SUCH_INDEX : 35'
-  - 'box.error.UNKNOWN_RTREE_INDEX_DISTANCE_TYPE : 103'
-  - 'box.error.TUPLE_FOUND : 3'
-  - 'box.error.VIEW_IS_RO : 113'
-  - 'box.error.LOCAL_INSTANCE_ID_IS_READ_ONLY : 128'
-  - 'box.error.FUNCTION_EXISTS : 52'
-  - 'box.error.UPDATE_ARG_TYPE : 26'
-  - 'box.error.FOREIGN_KEY_CONSTRAINT : 162'
-  - 'box.error.CROSS_ENGINE_TRANSACTION : 81'
-  - 'box.error.ACTION_MISMATCH : 160'
-  - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
-  - 'box.error.injection : table: <address>
-  - 'box.error.FUNCTION_TX_ACTIVE : 30'
-  - 'box.error.SQL_BIND_NOT_FOUND : 159'
-  - 'box.error.RELOAD_CFG : 58'
-  - 'box.error.NO_SUCH_ENGINE : 57'
-  - 'box.error.COMMIT_IN_SUB_STMT : 122'
-  - 'box.error.SQL_EXECUTE : 157'
-  - 'box.error.NULLABLE_MISMATCH : 153'
-  - 'box.error.LAST_DROP : 15'
-  - 'box.error.NO_SUCH_ROLE : 82'
-  - 'box.error.DECOMPRESSION : 124'
-  - 'box.error.CREATE_SEQUENCE : 142'
-  - 'box.error.CREATE_USER : 43'
-  - 'box.error.SPACE_FIELD_IS_DUPLICATE : 149'
-  - 'box.error.INSTANCE_UUID_MISMATCH : 66'
-  - 'box.error.SEQUENCE_OVERFLOW : 147'
-  - 'box.error.SYSTEM : 115'
-  - 'box.error.KEY_PART_IS_TOO_LONG : 118'
-  - 'box.error.TUPLE_FORMAT_LIMIT : 16'
-  - 'box.error.BEFORE_REPLACE_RET : 53'
-  - 'box.error.NO_SUCH_SAVEPOINT : 61'
-  - 'box.error.TRUNCATE_SYSTEM_SPACE : 137'
-  - 'box.error.VY_QUOTA_TIMEOUT : 135'
-  - 'box.error.WRONG_INDEX_OPTIONS : 108'
-  - 'box.error.INVALID_VYLOG_FILE : 133'
-  - 'box.error.INDEX_FIELD_COUNT_LIMIT : 127'
-  - 'box.error.READ_VIEW_ABORTED : 130'
-  - 'box.error.USER_MAX : 56'
-  - 'box.error.PROTOCOL : 104'
-  - 'box.error.TUPLE_NOT_ARRAY : 22'
-  - 'box.error.KEY_PART_COUNT : 31'
-  - 'box.error.ALTER_SPACE : 12'
-  - 'box.error.ACTIVE_TRANSACTION : 79'
-  - 'box.error.EXACT_FIELD_COUNT : 38'
-  - 'box.error.DROP_SEQUENCE : 144'
-  - 'box.error.INVALID_MSGPACK : 20'
-  - 'box.error.MORE_THAN_ONE_TUPLE : 41'
-  - 'box.error.RTREE_RECT : 101'
-  - 'box.error.SUB_STMT_MAX : 121'
-  - 'box.error.UNKNOWN_REQUEST_TYPE : 48'
-  - 'box.error.SPACE_EXISTS : 10'
-  - 'box.error.PROC_LUA : 32'
-  - 'box.error.ROLE_NOT_GRANTED : 92'
-  - 'box.error.NO_SUCH_SPACE : 36'
-  - 'box.error.WRONG_INDEX_PARTS : 107'
-  - 'box.error.DROP_SPACE : 11'
-  - 'box.error.MIN_FIELD_COUNT : 39'
-  - 'box.error.REPLICASET_UUID_MISMATCH : 63'
-  - 'box.error.UPDATE_FIELD : 29'
-  - 'box.error.COMPRESSION : 119'
-  - 'box.error.INVALID_ORDER : 68'
-  - 'box.error.INDEX_EXISTS : 85'
-  - 'box.error.SPLICE : 25'
-  - 'box.error.UNKNOWN : 0'
-  - 'box.error.DROP_PRIMARY_KEY : 17'
-  - 'box.error.NULLABLE_PRIMARY : 152'
-  - 'box.error.NO_SUCH_SEQUENCE : 145'
-  - 'box.error.SQL : 158'
-  - 'box.error.INVALID_UUID : 64'
-  - 'box.error.INJECTION : 8'
-  - 'box.error.TIMEOUT : 78'
-  - 'box.error.IDENTIFIER : 70'
-  - 'box.error.ITERATOR_TYPE : 72'
-  - 'box.error.REPLICA_MAX : 73'
-  - 'box.error.MISSING_REQUEST_FIELD : 69'
-  - 'box.error.MISSING_SNAPSHOT : 93'
-  - 'box.error.WRONG_SPACE_OPTIONS : 111'
-  - 'box.error.READONLY : 7'
-  - 'box.error.UNSUPPORTED : 5'
-  - 'box.error.UPDATE_INTEGER_OVERFLOW : 95'
-  - 'box.error.NO_CONNECTION : 77'
-  - 'box.error.INVALID_XLOG_ORDER : 76'
-  - 'box.error.UPSERT_UNIQUE_SECONDARY_KEY : 105'
-  - 'box.error.ROLLBACK_IN_SUB_STMT : 123'
-  - 'box.error.WRONG_SCHEMA_VERSION : 109'
-  - 'box.error.UNSUPPORTED_INDEX_FEATURE : 112'
-  - 'box.error.INDEX_PART_TYPE_MISMATCH : 24'
-  - 'box.error.INVALID_XLOG_TYPE : 125'
+- 0: box.error.UNKNOWN
+  1: box.error.ILLEGAL_PARAMS
+  2: box.error.MEMORY_ISSUE
+  3: box.error.TUPLE_FOUND
+  4: box.error.TUPLE_NOT_FOUND
+  5: box.error.UNSUPPORTED
+  6: box.error.NONMASTER
+  7: box.error.READONLY
+  8: box.error.INJECTION
+  9: box.error.CREATE_SPACE
+  10: box.error.SPACE_EXISTS
+  11: box.error.DROP_SPACE
+  12: box.error.ALTER_SPACE
+  13: box.error.INDEX_TYPE
+  14: box.error.MODIFY_INDEX
+  15: box.error.LAST_DROP
+  16: box.error.TUPLE_FORMAT_LIMIT
+  17: box.error.DROP_PRIMARY_KEY
+  18: box.error.KEY_PART_TYPE
+  19: box.error.EXACT_MATCH
+  20: box.error.INVALID_MSGPACK
+  21: box.error.PROC_RET
+  22: box.error.TUPLE_NOT_ARRAY
+  23: box.error.FIELD_TYPE
+  24: box.error.INDEX_PART_TYPE_MISMATCH
+  25: box.error.SPLICE
+  26: box.error.UPDATE_ARG_TYPE
+  27: box.error.FORMAT_MISMATCH_INDEX_PART
+  28: box.error.UNKNOWN_UPDATE_OP
+  29: box.error.UPDATE_FIELD
+  30: box.error.FUNCTION_TX_ACTIVE
+  31: box.error.KEY_PART_COUNT
+  32: box.error.PROC_LUA
+  33: box.error.NO_SUCH_PROC
+  34: box.error.NO_SUCH_TRIGGER
+  35: box.error.NO_SUCH_INDEX
+  36: box.error.NO_SUCH_SPACE
+  37: box.error.NO_SUCH_FIELD
+  38: box.error.EXACT_FIELD_COUNT
+  39: box.error.MIN_FIELD_COUNT
+  40: box.error.WAL_IO
+  41: box.error.MORE_THAN_ONE_TUPLE
+  42: box.error.ACCESS_DENIED
+  43: box.error.CREATE_USER
+  44: box.error.DROP_USER
+  45: box.error.NO_SUCH_USER
+  46: box.error.USER_EXISTS
+  47: box.error.PASSWORD_MISMATCH
+  48: box.error.UNKNOWN_REQUEST_TYPE
+  49: box.error.UNKNOWN_SCHEMA_OBJECT
+  50: box.error.CREATE_FUNCTION
+  51: box.error.NO_SUCH_FUNCTION
+  52: box.error.FUNCTION_EXISTS
+  53: box.error.BEFORE_REPLACE_RET
+  54: box.error.FUNCTION_MAX
+  56: box.error.USER_MAX
+  57: box.error.NO_SUCH_ENGINE
+  58: box.error.RELOAD_CFG
+  59: box.error.CFG
+  60: box.error.SAVEPOINT_EMPTY_TX
+  61: box.error.NO_SUCH_SAVEPOINT
+  62: box.error.UNKNOWN_REPLICA
+  63: box.error.REPLICASET_UUID_MISMATCH
+  64: box.error.INVALID_UUID
+  65: box.error.REPLICASET_UUID_IS_RO
+  66: box.error.INSTANCE_UUID_MISMATCH
+  68: box.error.INVALID_ORDER
+  69: box.error.MISSING_REQUEST_FIELD
+  70: box.error.IDENTIFIER
+  71: box.error.DROP_FUNCTION
+  72: box.error.ITERATOR_TYPE
+  73: box.error.REPLICA_MAX
+  74: box.error.INVALID_XLOG
+  75: box.error.INVALID_XLOG_NAME
+  76: box.error.INVALID_XLOG_ORDER
+  77: box.error.NO_CONNECTION
+  78: box.error.TIMEOUT
+  79: box.error.ACTIVE_TRANSACTION
+  80: box.error.CURSOR_NO_TRANSACTION
+  81: box.error.CROSS_ENGINE_TRANSACTION
+  82: box.error.NO_SUCH_ROLE
+  83: box.error.ROLE_EXISTS
+  84: box.error.CREATE_ROLE
+  85: box.error.INDEX_EXISTS
+  86: box.error.TUPLE_REF_OVERFLOW
+  87: box.error.ROLE_LOOP
+  88: box.error.GRANT
+  89: box.error.PRIV_GRANTED
+  90: box.error.ROLE_GRANTED
+  91: box.error.PRIV_NOT_GRANTED
+  92: box.error.ROLE_NOT_GRANTED
+  93: box.error.MISSING_SNAPSHOT
+  94: box.error.CANT_UPDATE_PRIMARY_KEY
+  95: box.error.UPDATE_INTEGER_OVERFLOW
+  96: box.error.GUEST_USER_PASSWORD
+  97: box.error.TRANSACTION_CONFLICT
+  98: box.error.UNSUPPORTED_ROLE_PRIV
+  99: box.error.LOAD_FUNCTION
+  100: box.error.FUNCTION_LANGUAGE
+  101: box.error.RTREE_RECT
+  102: box.error.PROC_C
+  103: box.error.UNKNOWN_RTREE_INDEX_DISTANCE_TYPE
+  104: box.error.PROTOCOL
+  105: box.error.UPSERT_UNIQUE_SECONDARY_KEY
+  106: box.error.WRONG_INDEX_RECORD
+  107: box.error.WRONG_INDEX_PARTS
+  108: box.error.WRONG_INDEX_OPTIONS
+  109: box.error.WRONG_SCHEMA_VERSION
+  110: box.error.MEMTX_MAX_TUPLE_SIZE
+  111: box.error.WRONG_SPACE_OPTIONS
+  112: box.error.UNSUPPORTED_INDEX_FEATURE
+  113: box.error.VIEW_IS_RO
+  114: box.error.SAVEPOINT_NO_TRANSACTION
+  115: box.error.SYSTEM
+  116: box.error.LOADING
+  117: box.error.CONNECTION_TO_SELF
+  118: box.error.KEY_PART_IS_TOO_LONG
+  119: box.error.COMPRESSION
+  120: box.error.CHECKPOINT_IN_PROGRESS
+  121: box.error.SUB_STMT_MAX
+  122: box.error.COMMIT_IN_SUB_STMT
+  123: box.error.ROLLBACK_IN_SUB_STMT
+  124: box.error.DECOMPRESSION
+  125: box.error.INVALID_XLOG_TYPE
+  126: box.error.ALREADY_RUNNING
+  127: box.error.INDEX_FIELD_COUNT_LIMIT
+  128: box.error.LOCAL_INSTANCE_ID_IS_READ_ONLY
+  129: box.error.BACKUP_IN_PROGRESS
+  130: box.error.READ_VIEW_ABORTED
+  131: box.error.INVALID_INDEX_FILE
+  132: box.error.INVALID_RUN_FILE
+  133: box.error.INVALID_VYLOG_FILE
+  134: box.error.CHECKPOINT_ROLLBACK
+  135: box.error.VY_QUOTA_TIMEOUT
+  136: box.error.PARTIAL_KEY
+  137: box.error.TRUNCATE_SYSTEM_SPACE
+  138: box.error.LOAD_MODULE
+  139: box.error.VINYL_MAX_TUPLE_SIZE
+  140: box.error.WRONG_DD_VERSION
+  141: box.error.WRONG_SPACE_FORMAT
+  142: box.error.CREATE_SEQUENCE
+  143: box.error.ALTER_SEQUENCE
+  144: box.error.DROP_SEQUENCE
+  145: box.error.NO_SUCH_SEQUENCE
+  146: box.error.SEQUENCE_EXISTS
+  147: box.error.SEQUENCE_OVERFLOW
+  149: box.error.SPACE_FIELD_IS_DUPLICATE
+  150: box.error.CANT_CREATE_COLLATION
+  151: box.error.WRONG_COLLATION_OPTIONS
+  152: box.error.NULLABLE_PRIMARY
+  153: box.error.NULLABLE_MISMATCH
+  154: box.error.SQL_BIND_VALUE
+  155: box.error.SQL_BIND_TYPE
+  156: box.error.SQL_BIND_PARAMETER_MAX
+  157: box.error.SQL_EXECUTE
+  158: box.error.SQL
+  159: box.error.SQL_BIND_NOT_FOUND
+  160: box.error.ACTION_MISMATCH
+  161: box.error.VIEW_MISSING_SQL
+  162: box.error.FOREIGN_KEY_CONSTRAINT
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 073a748..e24228a 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -111,7 +111,9 @@ type(require('yaml').encode(box.slab.info()));
 ----------------
 t = {}
 for k,v in pairs(box.error) do
-   table.insert(t, 'box.error.'..tostring(k)..' : '..tostring(v))
+   if type(v) == 'number' then
+    t[v] = 'box.error.'..tostring(k)
+   end
 end;
 t;
 
-- 
2.7.4

[tarantool-patches] [PATCH v3 08/10] sql: new _trigger space format with space_id

[Kirill Shcherbatov]

As we would like to lookup triggers by space_id in future
on space deletion to delete associated _trigger tuples we
need to introduce new field space_id as secondary key.

Part of #3273.
---
 src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
 src/box/lua/upgrade.lua                            |   4 +++
 src/box/schema_def.h                               |   6 ++++
 src/box/sql.c                                      |  21 +++++++-----
 src/box/sql/sqliteInt.h                            |   2 ++
 src/box/sql/trigger.c                              |   8 +++--
 test/app-tap/tarantoolctl.test.lua                 |   2 +-
 test/box-py/bootstrap.result                       |   5 +--
 test/box/access_misc.result                        |   4 +--
 test/box/access_sysview.result                     |   4 +--
 test/box/alter.result                              |   1 +
 test/sql/gh2141-delete-trigger-drop-table.result   |   4 +--
 test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +--
 test/sql/persistency.result                        |   8 ++---
 test/sql/persistency.test.lua                      |   8 ++---
 test/sql/upgrade.result                            |  37 ++++++++++++++++++---
 test/sql/upgrade.test.lua                          |   9 ++++-
 17 files changed, 91 insertions(+), 36 deletions(-)

diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index d271dca7bc6030f6a05c28c6385ed01a0a02f700..a8a00ec29e7106afbd06294cf6ffe291f90a2e10 100644
GIT binary patch
delta 1694
zcmV;P24VT44X6!}9)B}5W;ZrBISNT`b97;DV`VxZVq`I4G%`73EjBVRG%YkYH!&?Z
zW-vD`GG;kpI50CYF*i6k3RXjGZ)0mZAbWiZ3e~y`y3GbK0M4rK@!O>U00000D77#B
z08l+K04hVU4oT1$Hvs@&JireJkQ`J#QC_IkDFq@CiO~a`bblj<f!dUr5pAcW$;}j#
zB<X3(6|NP4kJNOv>$ELmxPvu%zAcout22V@@+qYh(*Vl=;{f7}eD(eMXFOG}!_0MA
zQ{^T>M0(5}i{c1=M)gI&wbZ2YVhW35sXv~Nz7(<Rd}$mrUr!|VEv#js&b#y3ldmod
zf)$}az4fgVw||}m@{d+sDw2{7Ccw4Sd~Mvyo20RNsDF1}Gm^p2$O5>Qnnjr8NZ}$@
zUp0bRm$XW>z1(Z5NvqXk=KEc=B-c_isUoTJ2ZUR*dM!1-%Q<nNXZjQlS2A&)SMqXl
zOeK)R!6gtY@J}Yw2}73sotGj=gKaLA(km`OP_%4Waet%=mK`eqEDZ`TuDnT<gKMez
z;fvXoH(B&`@Q0+vqyMpMyhEBRZ}K~<*I;NrFFOX;Qj^t7JhY#e-&r}AB%m%=u5VUt
z7F<isvOiA(>T>m+wMGHamSAJ@%rPRMSLyuCDh+}quFt5>9Ozi}J*gB7TuV)jUmVAj
z2)>>LVt?@L8+U#JVVzsC{o!|=Y2<e<TcW@8dHxkunF2qbUHrvqu8&q@2;jZ(iCcSI
z%|a}$&aTT5{8ZxS)sfDj{+W^oG0DGKg&A-yHA&ox@lK915rbnvcwj(U)G7-cD|%E^
zo)kPNs?Ls%4(bHT1gZpVW<3(r8*wc)FARw%qJKkmUU%X`a3DC7O^ywYkWE*^)qJ>?
znnE=<TM4##py>dk;V@$}Q!}%H#W9P52AK>p7-TNQScutTXsm^)P+>w-D$EQFg<4-^
zuBGP71PT+P@}*DK<;=+c?lS69k%%@L3D;6{V-bU73-g&u)G?peF(Oltn9gNDsvHwj
zJ%849-1#T^@Xx3Mfl+<O`}Kz(&rXh@R2&hmr6#OnP?x)ZMYRXQwbZO33B$bmSF1b^
zuBB!P`)XAuo_FfH9L~e26;A2It-MlqI+;o)4kttjMB$ilr%GYSQem5pkP1O65S{zO
z_V_^c=y=%d*mTU~s4+OMrKWwUNT<C~^?#T<sLNH&)}hU_k(88lrPjEXnlSYFepl9I
zjmEXq1Yl_$+PoS2$&#P7f($T=<nN<;W4t(S8JNXXNzSSr+Bg_Gw^aD7^~JT+ED^3)
z<co_tFIa#bYWVn{myFJlFMi&|a!Khhi<d~u0*j37=cTOkFT=Cap^fXgmYO>`V1LBw
zxYGG%KQC`?fW}mmT8Fk87Xe^M0)a@V;aFr43CsWhfB+}}@dXDd=ZXdrfUr1<!!VA4
z7=}Xtj6xGfKp-dx2{b^pU^HXwVBKS4p%&1X|4*aC=rB5r4iKkj+pXF1yo`v%b|Iu9
zdofA^fdyFzLvnw7cVbd<_StLc@_#R^s5l<^p4W;`8)7qS9>v{|TchhZB_P^);=ymZ
zIl7E^Uvlc<%`Fj-7xknjySbbQ4f!DO#f(gG#AT(F2pzK&f)RTwwyKFODV_57>)62c
zcU8a_L{GIZbY%Pz#>pe6#^}gK1ydGiF#eR)Gjp;Gt+E+g;bR)c>^vjr(tmsM89goz
z1A&B6?YN%{OOg{CnHyC3+<)`5__2GC1PwVg^uSGQF#eR)Gjp;GO~6}!wtV)haNl!i
zV3weV8pz9g_D>aT=wIklyeHtL&G749Jsmxug;UC;VbMR~u&zFKv>o>O!QR-KmFgwy
z50A^=RP+dFcJSHQk?!c?`F}z%LBKyYQbqk3bpDiyd|?vZ^C0TiDxb<tl1w`#F_kZ@
zj|Ta;4hk`H!92Ly7#DV+#&F9_ZH8^mDu2j1dRS2>UXm?dxrK1*3vmvJgznO%+@MU9
z&FbjovR)z~e@Aib@JDa0jX`Rx;muN>Oq+mTOc!e{6IVo}y#OjS&VNwbdcc{Hozr7r
zK=WtO9rs}jIa>?olPm@&{Ada=x%f*h`1(<QBQE@pu!7r`tUXO;d_=|Ip@}+^Y_!rV
zxw&QFHhIOe`ahl%;E{JMQbFlmJc$@rD}kiHR_%KIk*T>JWoBLv1#B$^hO@mwpbE8P
oJi~Ha`5lx^HFfBzk{A9#Sl5<&4hy7?2*p>)dXg|iIn@xY?G8vRL;wH)

delta 1688
zcmV;J250%G4WbQ@9)CA7F*0H`F*r5~Np5p=VQyn(Iv{3aVKy^mG-EAfIAUZiG-EX~
zEn+h@W-VnnF)%n{F)}n_H#iDbLu_wjYdRo%F*+bHeF_TIx(m9^1|tB@X~^8mr2qf`
z001bpFZ}>e)i411LZl8!&{$0YfIW;-52I0K5Pk_XKqcbbL4TXX{Q$^sCLS<!$HYcN
zrlck%?V2P)zDy=66lajMGo8{c399v6>?$}HOBd`M$VQ%~lv2_F$pGX4H4)bLYoM_F
zUb8aS<L%2!got#Q`vt)ecSh7jfUT39@}PG?aMT^oM?Zr1Rl@Y0k+2ssd)L)8md?BL
z+KaGG6N(L)L4V!#uJXFRg?VSAEfnbpR|;V3Bw_oy@)m8a&eFd-zY)o}GpYb=o#YTD
zI8eBl(f>l6^=P9+oQt(}l9N^)X1@PrGh*u`lPJ=cyP)f;QClZ@Z*){1>T_(q9F%FL
zljE6A_=ID!QT@tvJ?z<_R2T!6{hf~>w#B#>I;WKup?@S7vOr0qa)qY~fI5W&d5bCs
zwodX{59U|iVo}%e2l2wA|FG-3S+-Z+Vpq~s1KEB)q8QjZ$yOh)Y(F2nlAc<OU_Gv0
zU5(N#uyvASe{KZpan+S{Mgb9*U|+G!GoqmXbna?Y1_6>+XGCKT^!s&9C<OzyPEy|o
z!_WgU?0;MM;<E4Cc^PzdcFp#)zUyogzjGQA{o~K`u86`E0Q&smFm8KwHu^%~?tRPa
z+Tp6ky5OqpdJJ**fuC1LI<xfGjy%31?`pI(z}88M*EQo^3~AuSAs9ed#D1)}Q6CjP
zDSA+}I6-%Kc6L-IR3%g-VzrtYF;gOJourKwNq>bvsZc4D&MD^v;)FwULt`^UgPJ+b
ze6V$rLo+p;DaLt_=@66Q5JM9)6SJYkZ;OHkm<%u&U@p8^c;Q}TYAscuu^}n66jP!=
zsSAv)lXRIdV?$GZ{K<N(5&7SpHd`nX5ho+T)=BOQUM#vUuYI7N{k)zL;Q)E*oEG%u
z8Gq>Y8LHpTJMf2hM)XJN`+oQ9XLUR?IV$3W`b4mGlCgfV9(V7GXb%KiC&^;v-MbrY
z@xazej<N1WcU)cpDIE{T2IZwrsMMQQAZ|R94G4^5`n=R<!Z?4EbMXoT78>JrfK>QF
zeeBpCwTFjl2TBJ`4h~0*4}G~|>m;iSMSm)*4XVT3u^v~qN@ZNCMp9Ce6-vX_NtWjG
z{jaRY+YDPL37}3Y<5DvAiz6>-g%~)4;_oABVLTXa8qT0MiZxng-06kRuJ4wV_SiZ}
z60B8AgmIAP#R}Y^Mvn)A$>=BX1<=1*uPCV7;wJI7a1oLHe57^$BderS#+_W)I)6zX
z957}yT<Ki1pN};)L814AQW-bXB0#K2fkUX_SY!|h%m4s@04M+v1_vp}iUtyZusDjt
zFpPm1hGPJXLIX!YASeh4G(fdrG$SlvT|;7_7Qm4EO(KKHATo#yVAEsm)_AK7Gb$3>
zC6gNKVl4^83lbp=-T(O3#iSJ3$A7HF$X~Uh;uz#)u2r8lgl6Wrk2@AyOZRCSK~7uM
z8$U_)nFHdHKZX;08u1@rJKYX)NY>k3oEKG_B~q0v=#;`(!Pa9YNfM{b`_(mY`A-$}
z1<~_Xf*rb*hH+Bl7BE_-QC}nl4hTGi<FFVh0*M&LHgHaNmCP9qI(koV9)BdwfIA?N
zZ;bSFGLlGSl5>kVQF_`iE$e&_$E6`_1|3L&1_U0$aafELfe;}4pUpXYH+svoBAh#P
z@dgrmFJqZ*`1%)yE8f3x#FG1UA^0m+L$`uR>aa-u*&eP2ar7N_#ALTfgH?K#gg?kF
ze^a3!qGf?*Vh-wjTpt$XLw|CdekL=j`lI~c`SthmqPlZpGpu>+RNndo^%05{*RnB2
zFO7q%8RNhP&=_jtJYg6`dMQfI(FYbO;yfkH?`|Oo{6Yv#k>qY&%Nvw|f>|B8+~_3|
zhVLkk4}WCVS{_LK8r}@$lW7Cs#dNOLFma;t)C)|ZaU8W>2Amn$Ie$I|1{9wb-EsNF
zklWfhpY&pIktb6C&Bb?W!Of5Q9&w?5gxQ?FWDwKz$4GP=9`2|!!?sp>C6rqRrY5g=
zslUN<5+2!(#TS%Lizk_JmHd$KZ`E$DKmVEmQD)}$P@7gzU^wj80=lYQ(=+Yk+TKB%
itF48e>+~O}byP9oun~gm$Q^t|sV4~omjl%ht?dd`FccL4

diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 7258f47..f112a93 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -471,12 +471,16 @@ local function upgrade_to_2_1_0()
 
     log.info("create space _trigger")
     local format = {{name='name', type='string'},
+                    {name='space_id', type='unsigned'},
                     {name='opts', type='map'}}
     _space:insert{_trigger.id, ADMIN, '_trigger', 'memtx', 0, MAP, format}
 
     log.info("create index primary on _trigger")
     _index:insert{_trigger.id, 0, 'primary', 'tree', { unique = true },
                   {{0, 'string'}}}
+    log.info("create index secondary on _trigger")
+    _index:insert{_trigger.id, 1, 'space_id', 'tree', { unique = false },
+                  {{1, 'unsigned'}}}
 
     local stat1_ft = {{name='tbl', type='string'},
                       {name='idx', type='string'},
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index eb07332..9b003c4 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -217,6 +217,12 @@ enum {
 	BOX_SPACE_SEQUENCE_FIELD_IS_GENERATED = 2,
 };
 
+enum {
+	BOX_TRIGGER_FIELD_NAME = 0,
+	BOX_TRIGGER_FIELD_SPACE_ID = 1,
+	BOX_TRIGGER_FIELD_OPTS = 2,
+};
+
 /*
  * Different objects which can be subject to access
  * control.
diff --git a/src/box/sql.c b/src/box/sql.c
index d18f379..564e5bf 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -669,8 +669,11 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
 	if (box_index_get(BOX_TRIGGER_ID, 0, key_begin, key, &tuple) != 0)
 		return SQL_TARANTOOL_ERROR;
 	assert(tuple != NULL);
-	assert(tuple_field_count(tuple) == 2);
-	const char *field = box_tuple_field(tuple, 1);
+	assert(tuple_field_count(tuple) == 3);
+	const char *field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_SPACE_ID);
+	assert(mp_typeof(*field) == MP_UINT);
+	uint32_t space_id = mp_decode_uint(&field);
+	field = box_tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
 	assert(mp_typeof(*field) == MP_MAP);
 	mp_decode_map(&field);
 	const char *sql_str = mp_decode_str(&field, &key_len);
@@ -693,16 +696,18 @@ int tarantoolSqlite3RenameTrigger(const char *trig_name,
 	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) +
+	key_len = mp_sizeof_array(3) + mp_sizeof_str(trig_name_len) +
 		  mp_sizeof_map(1) + mp_sizeof_str(3) +
-		  mp_sizeof_str(trigger_stmt_new_len);
+		  mp_sizeof_str(trigger_stmt_new_len) +
+		  mp_sizeof_uint(space_id);
 	char *new_tuple = (char*)region_alloc(&fiber()->gc, key_len);
 	if (new_tuple == NULL) {
 		diag_set(OutOfMemory, key_len, "region_alloc", "new_tuple");
 		return SQL_TARANTOOL_ERROR;
 	}
-	char *new_tuple_end = mp_encode_array(new_tuple, 2);
+	char *new_tuple_end = mp_encode_array(new_tuple, 3);
 	new_tuple_end = mp_encode_str(new_tuple_end, trig_name, trig_name_len);
+	new_tuple_end = mp_encode_uint(new_tuple_end, space_id);
 	new_tuple_end = mp_encode_map(new_tuple_end, 1);
 	new_tuple_end = mp_encode_str(new_tuple_end, "sql", 3);
 	new_tuple_end = mp_encode_str(new_tuple_end, trigger_stmt,
@@ -1253,7 +1258,7 @@ void tarantoolSqlite3LoadSchema(InitData *init)
 		init, TARANTOOL_SYS_TRIGGER_NAME,
 		BOX_TRIGGER_ID, 0,
 		"CREATE TABLE \""TARANTOOL_SYS_TRIGGER_NAME"\" ("
-		"\"name\" TEXT PRIMARY KEY, \"opts\")"
+		"\"name\" TEXT PRIMARY KEY, \"space_id\" INT, \"opts\")"
 	);
 
 	sql_schema_put(
@@ -1308,14 +1313,14 @@ void tarantoolSqlite3LoadSchema(InitData *init)
 		const char *field, *ptr;
 		char *name, *sql;
 		unsigned len;
-		assert(tuple_field_count(tuple) == 2);
+		assert(tuple_field_count(tuple) == 3);
 
 		field = tuple_field(tuple, 0);
 		assert (field != NULL);
 		ptr = mp_decode_str(&field, &len);
 		name = strndup(ptr, len);
 
-		field = tuple_field(tuple, 1);
+		field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
 		assert (field != NULL);
 		mp_decode_array(&field);
 		ptr = mp_decode_str(&field, &len);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index ecbd573..19f5b86 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3032,6 +3032,8 @@ struct Parse {
  */
 struct Trigger {
 	char *zName;		/* The name of the trigger                        */
+	/** The ID of space the trigger refers to. */
+	uint32_t space_id;
 	char *table;		/* The table or view to which the trigger applies */
 	u8 op;			/* One of TK_DELETE, TK_UPDATE, TK_INSERT         */
 	u8 tr_tm;		/* One of TRIGGER_BEFORE, TRIGGER_AFTER */
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 84ea8db..b83c096 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -169,6 +169,7 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	if (pTrigger == 0)
 		goto trigger_cleanup;
 	pTrigger->zName = zName;
+	pTrigger->space_id = pTab->def->id;
 	zName = 0;
 	pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
 	pTrigger->pSchema = db->pSchema;
@@ -255,7 +256,7 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 
 		/* makerecord(cursor(iRecord), [reg(iFirstCol), reg(iFirstCol+1)])  */
 		iFirstCol = pParse->nMem + 1;
-		pParse->nMem += 2;
+		pParse->nMem += 3;
 		iRecord = ++pParse->nMem;
 
 		zOpts = sqlite3DbMallocRaw(pParse->db,
@@ -275,9 +276,10 @@ sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
 		sqlite3VdbeAddOp4(v,
 				  OP_String8, 0, iFirstCol, 0,
 				  zName, P4_DYNAMIC);
-		sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 1,
+		sqlite3VdbeAddOp2(v, OP_Integer, pTrig->space_id, iFirstCol + 1);
+		sqlite3VdbeAddOp4(v, OP_Blob, zOptsSz, iFirstCol + 2,
 				  MSGPACK_SUBTYPE, zOpts, P4_DYNAMIC);
-		sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 2, iRecord);
+		sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 3, iRecord);
 		sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
 		/* Do not account nested operations: the count of such
 		 * operations depends on Tarantool data dictionary internals,
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index cee9164..02ef5ea 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -339,7 +339,7 @@ do
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 1 --replica 2", "\n", 3)
             check_ctlcat_xlog(test_i, dir, "--from=3 --to=6 --format=json --show-system --replica 2", "\n", 0)
             check_ctlcat_snap(test_i, dir, "--space=280", "---\n", 21)
-            check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 46)
+            check_ctlcat_snap(test_i, dir, "--space=288", "---\n", 47)
         end)
     end)
 
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index ad9501b..b5c16c0 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -65,8 +65,8 @@ box.space._space:select{}
       {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
-  - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
-        'type': 'map'}]]
+  - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+        'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
         'type': 'unsigned'}]]
   - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
@@ -122,6 +122,7 @@ box.space._index:select{}
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
+  - [328, 1, 'space_id', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]]
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index 19e82bb..5a2563d 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result
@@ -798,8 +798,8 @@ box.space._space:select()
       {'name': 'object_id', 'type': 'unsigned'}, {'name': 'privilege', 'type': 'unsigned'}]]
   - [320, 1, '_cluster', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'uuid',
         'type': 'string'}]]
-  - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
-        'type': 'map'}]]
+  - [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+        'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
   - [330, 1, '_truncate', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'}, {'name': 'count',
         'type': 'unsigned'}]]
   - [340, 1, '_space_sequence', 'memtx', 0, {}, [{'name': 'id', 'type': 'unsigned'},
diff --git a/test/box/access_sysview.result b/test/box/access_sysview.result
index 3f93b5e..ae04266 100644
--- a/test/box/access_sysview.result
+++ b/test/box/access_sysview.result
@@ -234,7 +234,7 @@ box.session.su('guest')
 ...
 #box.space._vindex:select{}
 ---
-- 47
+- 48
 ...
 #box.space._vuser:select{}
 ---
@@ -262,7 +262,7 @@ box.session.su('guest')
 ...
 #box.space._vindex:select{}
 ---
-- 47
+- 48
 ...
 #box.space._vuser:select{}
 ---
diff --git a/test/box/alter.result b/test/box/alter.result
index 217c41d..c41b52f 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -224,6 +224,7 @@ _index:select{}
   - [320, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [320, 1, 'uuid', 'tree', {'unique': true}, [[1, 'string']]]
   - [328, 0, 'primary', 'tree', {'unique': true}, [[0, 'string']]]
+  - [328, 1, 'space_id', 'tree', {'unique': false}, [[1, 'unsigned']]]
   - [330, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [340, 0, 'primary', 'tree', {'unique': true}, [[0, 'unsigned']]]
   - [340, 1, 'sequence', 'tree', {'unique': false}, [[1, 'unsigned']]]
diff --git a/test/sql/gh2141-delete-trigger-drop-table.result b/test/sql/gh2141-delete-trigger-drop-table.result
index ba7016c..ec5a380 100644
--- a/test/sql/gh2141-delete-trigger-drop-table.result
+++ b/test/sql/gh2141-delete-trigger-drop-table.result
@@ -24,7 +24,7 @@ box.sql.execute("CREATE TRIGGER tt_ad AFTER DELETE ON t BEGIN SELECT 1; END")
 ---
 ...
 -- check that these triggers exist
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 ---
 - - ['TT_AD', !!binary gaNzcWzZOkNSRUFURSBUUklHR0VSIHR0X2FkIEFGVEVSIERFTEVURSBPTiB0IEJFR0lOIFNFTEVDVCAxOyBFTkQ=]
   - ['TT_AI', !!binary gaNzcWzZOkNSRUFURSBUUklHR0VSIHR0X2FpIEFGVEVSIElOU0VSVCBPTiB0IEJFR0lOIFNFTEVDVCAxOyBFTkQ=]
@@ -38,7 +38,7 @@ box.sql.execute("DROP TABLE t")
 ---
 ...
 -- check that triggers were dropped with deleted table
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 ---
 - []
 ...
diff --git a/test/sql/gh2141-delete-trigger-drop-table.test.lua b/test/sql/gh2141-delete-trigger-drop-table.test.lua
index e6a030c..87110a4 100644
--- a/test/sql/gh2141-delete-trigger-drop-table.test.lua
+++ b/test/sql/gh2141-delete-trigger-drop-table.test.lua
@@ -11,10 +11,10 @@ box.sql.execute("CREATE TRIGGER tt_bd BEFORE DELETE ON t BEGIN SELECT 1; END")
 box.sql.execute("CREATE TRIGGER tt_ad AFTER DELETE ON t BEGIN SELECT 1; END")
 
 -- check that these triggers exist
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 
 -- drop table
 box.sql.execute("DROP TABLE t")
 
 -- check that triggers were dropped with deleted table
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
diff --git a/test/sql/persistency.result b/test/sql/persistency.result
index 7a7f6b8..d85d7cc 100644
--- a/test/sql/persistency.result
+++ b/test/sql/persistency.result
@@ -140,7 +140,7 @@ box.sql.execute("INSERT INTO barfoo VALUES ('foobar', 1000)")
 box.sql.execute("CREATE TRIGGER tfoobar AFTER INSERT ON foobar BEGIN INSERT INTO barfoo VALUES ('trigger test', 9999); END")
 ---
 ...
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
 ---
 - - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
 ...
@@ -166,7 +166,7 @@ box.sql.execute("SELECT a FROM t1 ORDER BY b, a LIMIT 10 OFFSET 20;");
 ...
 test_run:cmd('restart server default');
 -- prove that trigger survived
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
 ---
 - - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
 ...
@@ -179,7 +179,7 @@ box.sql.execute("SELECT * FROM barfoo WHERE foo = 9999");
 - - ['trigger test', 9999]
 ...
 -- and still persistent
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 ---
 - - ['TFOOBAR', !!binary gaNzcWzZaUNSRUFURSBUUklHR0VSIHRmb29iYXIgQUZURVIgSU5TRVJUIE9OIGZvb2JhciBCRUdJTiBJTlNFUlQgSU5UTyBiYXJmb28gVkFMVUVTICgndHJpZ2dlciB0ZXN0JywgOTk5OSk7IEVORA==]
 ...
@@ -193,7 +193,7 @@ box.sql.execute("DROP TRIGGER tfoobar")
 - error: 'no such trigger: TFOOBAR'
 ...
 -- Should be empty
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 ---
 - []
 ...
diff --git a/test/sql/persistency.test.lua b/test/sql/persistency.test.lua
index bd05545..e994a62 100644
--- a/test/sql/persistency.test.lua
+++ b/test/sql/persistency.test.lua
@@ -49,7 +49,7 @@ box.sql.execute("INSERT INTO barfoo VALUES ('foobar', 1000)")
 
 -- create a trigger
 box.sql.execute("CREATE TRIGGER tfoobar AFTER INSERT ON foobar BEGIN INSERT INTO barfoo VALUES ('trigger test', 9999); END")
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
 
 -- Many entries
 box.sql.execute("CREATE TABLE t1(a,b,c,PRIMARY KEY(b,c));")
@@ -59,21 +59,21 @@ box.sql.execute("SELECT a FROM t1 ORDER BY b, a LIMIT 10 OFFSET 20;");
 test_run:cmd('restart server default');
 
 -- prove that trigger survived
-box.sql.execute("SELECT * FROM \"_trigger\"");
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
 
 -- ... functional
 box.sql.execute("INSERT INTO foobar VALUES ('foobar trigger test', 8888)")
 box.sql.execute("SELECT * FROM barfoo WHERE foo = 9999");
 
 -- and still persistent
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 
 -- and can be dropped just once
 box.sql.execute("DROP TRIGGER tfoobar")
 -- Should error
 box.sql.execute("DROP TRIGGER tfoobar")
 -- Should be empty
-box.sql.execute("SELECT * FROM \"_trigger\"")
+box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
 
 -- prove barfoo2 still exists
 box.sql.execute("INSERT INTO barfoo VALUES ('xfoo', 1)")
diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result
index dcdb689..882bdd0 100644
--- a/test/sql/upgrade.result
+++ b/test/sql/upgrade.result
@@ -19,8 +19,8 @@ test_run:switch('upgrade')
 -- test system tables
 box.space._space.index['name']:get('_trigger')
 ---
-- [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'opts',
-      'type': 'map'}]]
+- [328, 1, '_trigger', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'}, {'name': 'space_id',
+      'type': 'unsigned'}, {'name': 'opts', 'type': 'map'}]]
 ...
 box.space._space.index['name']:get('_sql_stat1')
 ---
@@ -69,6 +69,9 @@ box.sql.execute("CREATE TABLE t_out(x INTEGER PRIMARY KEY);")
 box.sql.execute("CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1); END;")
 ---
 ...
+box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2); END;")
+---
+...
 box.space._space.index['name']:get('T')
 ---
 - [513, 1, 'T', 'memtx', 1, {'sql': 'CREATE TABLE t(x INTEGER PRIMARY KEY)'}, [{'type': 'integer',
@@ -79,10 +82,33 @@ box.space._space.index['name']:get('T_OUT')
 - [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
   [{'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
 ...
-box.space._trigger:get('T1T')
+t1t = box.space._trigger:get('T1T')
+---
+...
+t2t = box.space._trigger:get('T2T')
+---
+...
+t1t.name
+---
+- T1T
+...
+t1t.opts
 ---
-- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
-      END;'}]
+- {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
+    END;'}
+...
+t2t.name
+---
+- T2T
+...
+t2t.opts
+---
+- {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2);
+    END;'}
+...
+assert(t1t.id == t2t.id)
+---
+- true
 ...
 box.sql.execute("INSERT INTO T VALUES(1);")
 ---
@@ -94,6 +120,7 @@ box.space.T:select()
 box.space.T_OUT:select()
 ---
 - - [1]
+  - [2]
 ...
 box.sql.execute("SELECT * FROM T")
 ---
diff --git a/test/sql/upgrade.test.lua b/test/sql/upgrade.test.lua
index d0add86..6560710 100644
--- a/test/sql/upgrade.test.lua
+++ b/test/sql/upgrade.test.lua
@@ -25,9 +25,16 @@ box.space._index:get({box.space._space.index['name']:get('T1').id, 0})
 box.sql.execute("CREATE TABLE t(x INTEGER PRIMARY KEY);")
 box.sql.execute("CREATE TABLE t_out(x INTEGER PRIMARY KEY);")
 box.sql.execute("CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1); END;")
+box.sql.execute("CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2); END;")
 box.space._space.index['name']:get('T')
 box.space._space.index['name']:get('T_OUT')
-box.space._trigger:get('T1T')
+t1t = box.space._trigger:get('T1T')
+t2t = box.space._trigger:get('T2T')
+t1t.name
+t1t.opts
+t2t.name
+t2t.opts
+assert(t1t.id == t2t.id)
 
 box.sql.execute("INSERT INTO T VALUES(1);")
 box.space.T:select()
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 08/10] sql: new _trigger space format with space_id

[Vladislav Shpilevoy]

Thanks for the fixes! See 2 comments below.

Besides, I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> As we would like to lookup triggers by space_id in future
> on space deletion to delete associated _trigger tuples we
> need to introduce new field space_id as secondary key.
> 
> Part of #3273.
> ---
>   src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
>   src/box/lua/upgrade.lua                            |   4 +++
>   src/box/schema_def.h                               |   6 ++++
>   src/box/sql.c                                      |  21 +++++++-----
>   src/box/sql/sqliteInt.h                            |   2 ++
>   src/box/sql/trigger.c                              |   8 +++--
>   test/app-tap/tarantoolctl.test.lua                 |   2 +-
>   test/box-py/bootstrap.result                       |   5 +--
>   test/box/access_misc.result                        |   4 +--
>   test/box/access_sysview.result                     |   4 +--
>   test/box/alter.result                              |   1 +
>   test/sql/gh2141-delete-trigger-drop-table.result   |   4 +--
>   test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +--
>   test/sql/persistency.result                        |   8 ++---
>   test/sql/persistency.test.lua                      |   8 ++---
>   test/sql/upgrade.result                            |  37 ++++++++++++++++++---
>   test/sql/upgrade.test.lua                          |   9 ++++-
>   17 files changed, 91 insertions(+), 36 deletions(-)
> 
> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index d271dca7bc6030f6a05c28c6385ed01a0a02f700..a8a00ec29e7106afbd06294cf6ffe291f90a2e10 100644
> GIT binary patch
> delta 1694
> zcmV;P24VT44X6!}9)B}5W;ZrBISNT`b97;DV`VxZVq`I4G%`73EjBVRG%YkYH!&?Z
> zW-vD`GG;kpI50CYF*i6k3RXjGZ)0mZAbWiZ3e~y`y3GbK0M4rK@!O>U00000D77#B
> z08l+K04hVU4oT1$Hvs@&JireJkQ`J#QC_IkDFq@CiO~a`bblj<f!dUr5pAcW$;}j#
> zB<X3(6|NP4kJNOv>$ELmxPvu%zAcout22V@@+qYh(*Vl=;{f7}eD(eMXFOG}!_0MA
> zQ{^T>M0(5}i{c1=M)gI&wbZ2YVhW35sXv~Nz7(<Rd}$mrUr!|VEv#js&b#y3ldmod
> zf)$}az4fgVw||}m@{d+sDw2{7Ccw4Sd~Mvyo20RNsDF1}Gm^p2$O5>Qnnjr8NZ}$@
> zUp0bRm$XW>z1(Z5NvqXk=KEc=B-c_isUoTJ2ZUR*dM!1-%Q<nNXZjQlS2A&)SMqXl
> zOeK)R!6gtY@J}Yw2}73sotGj=gKaLA(km`OP_%4Waet%=mK`eqEDZ`TuDnT<gKMez
> z;fvXoH(B&`@Q0+vqyMpMyhEBRZ}K~<*I;NrFFOX;Qj^t7JhY#e-&r}AB%m%=u5VUt
> z7F<isvOiA(>T>m+wMGHamSAJ@%rPRMSLyuCDh+}quFt5>9Ozi}J*gB7TuV)jUmVAj
> z2)>>LVt?@L8+U#JVVzsC{o!|=Y2<e<TcW@8dHxkunF2qbUHrvqu8&q@2;jZ(iCcSI
> z%|a}$&aTT5{8ZxS)sfDj{+W^oG0DGKg&A-yHA&ox@lK915rbnvcwj(U)G7-cD|%E^
> zo)kPNs?Ls%4(bHT1gZpVW<3(r8*wc)FARw%qJKkmUU%X`a3DC7O^ywYkWE*^)qJ>?
> znnE=<TM4##py>dk;V@$}Q!}%H#W9P52AK>p7-TNQScutTXsm^)P+>w-D$EQFg<4-^
> zuBGP71PT+P@}*DK<;=+c?lS69k%%@L3D;6{V-bU73-g&u)G?peF(Oltn9gNDsvHwj
> zJ%849-1#T^@Xx3Mfl+<O`}Kz(&rXh@R2&hmr6#OnP?x)ZMYRXQwbZO33B$bmSF1b^
> zuBB!P`)XAuo_FfH9L~e26;A2It-MlqI+;o)4kttjMB$ilr%GYSQem5pkP1O65S{zO
> z_V_^c=y=%d*mTU~s4+OMrKWwUNT<C~^?#T<sLNH&)}hU_k(88lrPjEXnlSYFepl9I
> zjmEXq1Yl_$+PoS2$&#P7f($T=<nN<;W4t(S8JNXXNzSSr+Bg_Gw^aD7^~JT+ED^3)
> z<co_tFIa#bYWVn{myFJlFMi&|a!Khhi<d~u0*j37=cTOkFT=Cap^fXgmYO>`V1LBw
> zxYGG%KQC`?fW}mmT8Fk87Xe^M0)a@V;aFr43CsWhfB+}}@dXDd=ZXdrfUr1<!!VA4
> z7=}Xtj6xGfKp-dx2{b^pU^HXwVBKS4p%&1X|4*aC=rB5r4iKkj+pXF1yo`v%b|Iu9
> zdofA^fdyFzLvnw7cVbd<_StLc@_#R^s5l<^p4W;`8)7qS9>v{|TchhZB_P^);=ymZ
> zIl7E^Uvlc<%`Fj-7xknjySbbQ4f!DO#f(gG#AT(F2pzK&f)RTwwyKFODV_57>)62c
> zcU8a_L{GIZbY%Pz#>pe6#^}gK1ydGiF#eR)Gjp;Gt+E+g;bR)c>^vjr(tmsM89goz
> z1A&B6?YN%{OOg{CnHyC3+<)`5__2GC1PwVg^uSGQF#eR)Gjp;GO~6}!wtV)haNl!i
> zV3weV8pz9g_D>aT=wIklyeHtL&G749Jsmxug;UC;VbMR~u&zFKv>o>O!QR-KmFgwy
> z50A^=RP+dFcJSHQk?!c?`F}z%LBKyYQbqk3bpDiyd|?vZ^C0TiDxb<tl1w`#F_kZ@
> zj|Ta;4hk`H!92Ly7#DV+#&F9_ZH8^mDu2j1dRS2>UXm?dxrK1*3vmvJgznO%+@MU9
> z&FbjovR)z~e@Aib@JDa0jX`Rx;muN>Oq+mTOc!e{6IVo}y#OjS&VNwbdcc{Hozr7r
> zK=WtO9rs}jIa>?olPm@&{Ada=x%f*h`1(<QBQE@pu!7r`tUXO;d_=|Ip@}+^Y_!rV
> zxw&QFHhIOe`ahl%;E{JMQbFlmJc$@rD}kiHR_%KIk*T>JWoBLv1#B$^hO@mwpbE8P
> oJi~Ha`5lx^HFfBzk{A9#Sl5<&4hy7?2*p>)dXg|iIn@xY?G8vRL;wH)
> 
> delta 1688
> zcmV;J250%G4WbQ@9)CA7F*0H`F*r5~Np5p=VQyn(Iv{3aVKy^mG-EAfIAUZiG-EX~
> zEn+h@W-VnnF)%n{F)}n_H#iDbLu_wjYdRo%F*+bHeF_TIx(m9^1|tB@X~^8mr2qf`
> z001bpFZ}>e)i411LZl8!&{$0YfIW;-52I0K5Pk_XKqcbbL4TXX{Q$^sCLS<!$HYcN
> zrlck%?V2P)zDy=66lajMGo8{c399v6>?$}HOBd`M$VQ%~lv2_F$pGX4H4)bLYoM_F
> zUb8aS<L%2!got#Q`vt)ecSh7jfUT39@}PG?aMT^oM?Zr1Rl@Y0k+2ssd)L)8md?BL
> z+KaGG6N(L)L4V!#uJXFRg?VSAEfnbpR|;V3Bw_oy@)m8a&eFd-zY)o}GpYb=o#YTD
> zI8eBl(f>l6^=P9+oQt(}l9N^)X1@PrGh*u`lPJ=cyP)f;QClZ@Z*){1>T_(q9F%FL
> zljE6A_=ID!QT@tvJ?z<_R2T!6{hf~>w#B#>I;WKup?@S7vOr0qa)qY~fI5W&d5bCs
> zwodX{59U|iVo}%e2l2wA|FG-3S+-Z+Vpq~s1KEB)q8QjZ$yOh)Y(F2nlAc<OU_Gv0
> zU5(N#uyvASe{KZpan+S{Mgb9*U|+G!GoqmXbna?Y1_6>+XGCKT^!s&9C<OzyPEy|o
> z!_WgU?0;MM;<E4Cc^PzdcFp#)zUyogzjGQA{o~K`u86`E0Q&smFm8KwHu^%~?tRPa
> z+Tp6ky5OqpdJJ**fuC1LI<xfGjy%31?`pI(z}88M*EQo^3~AuSAs9ed#D1)}Q6CjP
> zDSA+}I6-%Kc6L-IR3%g-VzrtYF;gOJourKwNq>bvsZc4D&MD^v;)FwULt`^UgPJ+b
> ze6V$rLo+p;DaLt_=@66Q5JM9)6SJYkZ;OHkm<%u&U@p8^c;Q}TYAscuu^}n66jP!=
> zsSAv)lXRIdV?$GZ{K<N(5&7SpHd`nX5ho+T)=BOQUM#vUuYI7N{k)zL;Q)E*oEG%u
> z8Gq>Y8LHpTJMf2hM)XJN`+oQ9XLUR?IV$3W`b4mGlCgfV9(V7GXb%KiC&^;v-MbrY
> z@xazej<N1WcU)cpDIE{T2IZwrsMMQQAZ|R94G4^5`n=R<!Z?4EbMXoT78>JrfK>QF
> zeeBpCwTFjl2TBJ`4h~0*4}G~|>m;iSMSm)*4XVT3u^v~qN@ZNCMp9Ce6-vX_NtWjG
> z{jaRY+YDPL37}3Y<5DvAiz6>-g%~)4;_oABVLTXa8qT0MiZxng-06kRuJ4wV_SiZ}
> z60B8AgmIAP#R}Y^Mvn)A$>=BX1<=1*uPCV7;wJI7a1oLHe57^$BderS#+_W)I)6zX
> z957}yT<Ki1pN};)L814AQW-bXB0#K2fkUX_SY!|h%m4s@04M+v1_vp}iUtyZusDjt
> zFpPm1hGPJXLIX!YASeh4G(fdrG$SlvT|;7_7Qm4EO(KKHATo#yVAEsm)_AK7Gb$3>
> zC6gNKVl4^83lbp=-T(O3#iSJ3$A7HF$X~Uh;uz#)u2r8lgl6Wrk2@AyOZRCSK~7uM
> z8$U_)nFHdHKZX;08u1@rJKYX)NY>k3oEKG_B~q0v=#;`(!Pa9YNfM{b`_(mY`A-$}
> z1<~_Xf*rb*hH+Bl7BE_-QC}nl4hTGi<FFVh0*M&LHgHaNmCP9qI(koV9)BdwfIA?N
> zZ;bSFGLlGSl5>kVQF_`iE$e&_$E6`_1|3L&1_U0$aafELfe;}4pUpXYH+svoBAh#P
> z@dgrmFJqZ*`1%)yE8f3x#FG1UA^0m+L$`uR>aa-u*&eP2ar7N_#ALTfgH?K#gg?kF
> ze^a3!qGf?*Vh-wjTpt$XLw|CdekL=j`lI~c`SthmqPlZpGpu>+RNndo^%05{*RnB2
> zFO7q%8RNhP&=_jtJYg6`dMQfI(FYbO;yfkH?`|Oo{6Yv#k>qY&%Nvw|f>|B8+~_3|
> zhVLkk4}WCVS{_LK8r}@$lW7Cs#dNOLFma;t)C)|ZaU8W>2Amn$Ie$I|1{9wb-EsNF
> zklWfhpY&pIktb6C&Bb?W!Of5Q9&w?5gxQ?FWDwKz$4GP=9`2|!!?sp>C6rqRrY5g=
> zslUN<5+2!(#TS%Lizk_JmHd$KZ`E$DKmVEmQD)}$P@7gzU^wj80=lYQ(=+Yk+TKB%
> itF48e>+~O}byP9oun~gm$Q^t|sV4~omjl%ht?dd`FccL4

1. Please, do not send binary files.

> diff --git a/test/sql/upgrade.result b/test/sql/upgrade.result
> index dcdb689..882bdd0 100644
> --- a/test/sql/upgrade.result
> +++ b/test/sql/upgrade.result
> @@ -79,10 +82,33 @@ box.space._space.index['name']:get('T_OUT')
>   - [514, 1, 'T_OUT', 'memtx', 1, {'sql': 'CREATE TABLE t_out(x INTEGER PRIMARY KEY)'},
>     [{'type': 'integer', 'nullable_action': 'abort', 'name': 'X', 'is_nullable': false}]]
>   ...
> -box.space._trigger:get('T1T')
> +t1t = box.space._trigger:get('T1T')
> +---
> +...
> +t2t = box.space._trigger:get('T2T')
> +---
> +...
> +t1t.name
> +---
> +- T1T
> +...
> +t1t.opts
>   ---
> -- ['T1T', {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
> -      END;'}]
> +- {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(1);
> +    END;'}
> +...
> +t2t.name
> +---
> +- T2T
> +...
> +t2t.opts
> +---
> +- {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t BEGIN INSERT INTO t_out VALUES(2);
> +    END;'}
> +...
> +assert(t1t.id == t2t.id)
> +---
> +- true

2. Please, add a test that t1t == box.space.T.id. Your current test would pass
even if t1t.id == t2t.id == -100.

>   ...
>   box.sql.execute("INSERT INTO T VALUES(1);")
>   ---

[tarantool-patches] Re: [PATCH v3 08/10] sql: new _trigger space format with space_id

[Kirill Shcherbatov]

> 1. Please, do not send binary files.
ok.

> 2. Please, add a test that t1t == box.space.T.id. Your current test would pass
> even if t1t.id == t2t.id == -100.
+assert(t1t.space_id == box.space.T.id)

[tarantool-patches] [PATCH v3 09/10] sql: move Triggers to server

[Kirill Shcherbatov]

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                      | 128 +++++++++++++++
 src/box/errcode.h                     |   2 +-
 src/box/lua/schema.lua                |   4 +
 src/box/space.c                       |   5 +
 src/box/space.h                       |   2 +
 src/box/sql.c                         |  39 -----
 src/box/sql.h                         |  50 ++++++
 src/box/sql/build.c                   |   8 +-
 src/box/sql/fkey.c                    |   2 -
 src/box/sql/insert.c                  |   6 +-
 src/box/sql/prepare.c                 |   1 +
 src/box/sql/sqliteInt.h               |   5 -
 src/box/sql/tokenize.c                |  20 ++-
 src/box/sql/trigger.c                 | 282 +++++++++++++++++-----------------
 src/box/sql/vdbe.c                    |  77 ++--------
 src/box/sql/vdbe.h                    |   1 -
 src/box/sql/vdbeaux.c                 |   9 --
 test/box/misc.result                  |   1 +
 test/engine/iterator.result           |   2 +-
 test/sql-tap/identifier_case.test.lua |   4 +-
 test/sql-tap/trigger1.test.lua        |  14 +-
 test/sql/triggers.result              | 255 ++++++++++++++++++++++++++++++
 test/sql/triggers.test.lua            |  92 +++++++++++
 23 files changed, 731 insertions(+), 278 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..4fc46fc 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -551,6 +551,10 @@ 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. */
+	struct Trigger *old_value = new_space->sql_triggers;
+	new_space->sql_triggers = old_space->sql_triggers;
+	old_space->sql_triggers = old_value;
 }
 
 /**
@@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
 	latch_lock(&schema_lock);
 }
 
+static void
+on_replace_trigger_rollback(struct trigger *trigger, void *event)
+{
+	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
+	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+	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
+on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
+{
+	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+	/* DELETE, REPLACE trigger. */
+	sql_trigger_delete(sql_get(), old_trigger);
+}
+
 /**
  * A trigger invoked on replace in a space containing
  * SQL triggers.
@@ -3100,6 +3147,87 @@ 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(on_replace_trigger_rollback, NULL);
+	struct trigger *on_commit =
+		txn_alter_trigger_new(on_replace_trigger_commit, NULL);
+
+	if (old_tuple != NULL && new_tuple == NULL) {
+		/* DROP trigger. */
+		uint32_t trigger_name_len;
+		const char *trigger_name_src =
+			tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
+					   &trigger_name_len);
+		char *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 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 {
+		/* INSERT, REPLACE trigger. */
+		uint32_t trigger_name_len;
+		const char *trigger_name_src =
+			tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
+					   &trigger_name_len);
+
+		const char *space_opts =
+			tuple_field_with_type_xc(new_tuple,
+						 BOX_TRIGGER_FIELD_OPTS,
+						 MP_MAP);
+		struct space_opts opts;
+		struct region *region = &fiber()->gc;
+		space_opts_decode(&opts, space_opts, region);
+		struct Trigger *new_trigger =
+			sql_trigger_compile(sql_get(), opts.sql);
+		if (new_trigger == NULL)
+			diag_raise();
+
+		const char *trigger_name = sql_trigger_name(new_trigger);
+		if (strlen(trigger_name) != trigger_name_len ||
+			memcmp(trigger_name_src, trigger_name,
+			       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");
+		}
+		uint32_t space_id =
+			tuple_field_u32_xc(new_tuple,
+					   BOX_TRIGGER_FIELD_SPACE_ID);
+		if (space_id != sql_trigger_space_id(new_trigger)) {
+			sql_trigger_delete(sql_get(), new_trigger);
+			tnt_raise(ClientError, ER_SQL,
+				  "tuple space_id does not match the value "
+				  "resolved on AST building from SQL");
+		}
+
+		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();
+		}
+
+		on_commit->data = old_trigger;
+		on_rollback->data = new_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/lua/schema.lua b/src/box/lua/schema.lua
index 8bd0269..d856eaf 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -502,6 +502,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]
@@ -510,6 +511,9 @@ box.schema.space.drop = function(space_id, space_name, opts)
         -- Delete automatically generated sequence.
         box.schema.sequence.drop(sequence_tuple[2])
     end
+    for _, t in _trigger.index.space_id:pairs({space_id}) do
+        _trigger:delete({t.name})
+    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..bf32577 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 part of deletion process.
+	 */
+	assert(space->sql_triggers == NULL);
 	space->vtab->destroy(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 564e5bf..9e7b4fd 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1228,9 +1228,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,
@@ -1299,42 +1296,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) == 3);
-
-		field = tuple_field(tuple, 0);
-		assert (field != NULL);
-		ptr = mp_decode_str(&field, &len);
-		name = strndup(ptr, len);
-
-		field = tuple_field(tuple, BOX_TRIGGER_FIELD_OPTS);
-		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);
 }
 
 /*********************************************************************
diff --git a/src/box/sql.h b/src/box/sql.h
index 35aacc3..51e42ab 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,45 @@ 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.
+ */
+const char *
+sql_trigger_name(struct Trigger *trigger);
+
+/**
+ * Get space_id of the space that trigger has been built for.
+ * @param trigger AST object.
+ * @return space identifier.
+ */
+uint32_t
+sql_trigger_space_id(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 606bb69..67cdf11 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2290,16 +2290,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/prepare.c b/src/box/sql/prepare.c
index e43fbcb..1fe6deb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
 	}
 	parser->disableLookaside = 0;
 	sqlite3DbFree(db, parser->zErrMsg);
+	sql_trigger_delete(db, parser->pNewTrigger);
 	region_destroy(&parser->region);
 }
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 19f5b86..332986f 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. */
@@ -3034,14 +3033,11 @@ struct Trigger {
 	char *zName;		/* The name of the trigger                        */
 	/** The ID of space the trigger refers to. */
 	uint32_t space_id;
-	char *table;		/* The table or view to which the trigger applies */
 	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 */
 };
@@ -4029,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 d22ce8c..e06cc0a 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -534,7 +534,6 @@ sqlite3RunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
 
 	if (pParse->pWithToFree)
 		sqlite3WithDelete(db, pParse->pWithToFree);
-	sql_trigger_delete(db, pParse->pNewTrigger);
 	sqlite3DbFree(db, pParse->pVList);
 	while (pParse->pZombieTab) {
 		Table *p = pParse->pZombieTab;
@@ -572,3 +571,22 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
 	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 &&
+	    parser.rc != SQL_TARANTOOL_ERROR) {
+		diag_set(ClientError, ER_SQL, sql_error);
+	} else {
+		trigger = parser.pNewTrigger;
+		parser.pNewTrigger = NULL;
+	}
+	sql_parser_destroy(&parser);
+	return trigger;
+}
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index b83c096..9ca4262 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,104 +82,134 @@ 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 != NULL);
 	assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE);
 	assert(op > 0 && op < 0xff);
 
-	if (!pTableName || db->mallocFailed) {
+	if (pTableName == NULL || 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;
+			pParse->nErr++;
 		} 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;
+		pParse->nErr++;
+		goto trigger_cleanup;
+	}
+	if (space_id == BOX_ID_NIL) {
+		diag_set(ClientError, ER_NO_SUCH_SPACE, table_name);
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
+		goto trigger_cleanup;
+	}
+
+	/* Do not create a trigger on a system table. */
+	if (sqlite3StrNICmp(table_name, "sqlite_", 7) == 0) {
+		diag_set(ClientError, ER_SQL,
+			 "cannot create trigger on system table");
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
 		goto trigger_cleanup;
 	}
 
-	/* INSTEAD of triggers are only for views and views only support INSTEAD
-	 * of triggers.
+	/*
+	 * INSTEAD of triggers are only for views and
+	 * views only support INSTEAD of triggers.
 	 */
-	if (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;
+		pParse->nErr++;
 		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;
+		pParse->nErr++;
 		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));
-	if (pTrigger == 0)
+	/* Build the Trigger object. */
+	pTrigger = (Trigger *)sqlite3DbMallocZero(db, sizeof(Trigger));
+	if (pTrigger == NULL)
 		goto trigger_cleanup;
+	pTrigger->space_id = space_id;
 	pTrigger->zName = zName;
-	pTrigger->space_id = pTab->def->id;
-	zName = 0;
-	pTrigger->table = sqlite3DbStrDup(db, pTableName->a[0].zName);
-	pTrigger->pSchema = db->pSchema;
-	pTrigger->pTabSchema = pTab->pSchema;
+	zName = NULL;
+
 	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);
-	assert(pParse->pNewTrigger == 0);
+	if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
+	    (pColumns != NULL && pTrigger->pColumns == NULL))
+		goto trigger_cleanup;
+	assert(pParse->pNewTrigger == NULL);
 	pParse->pNewTrigger = pTrigger;
 
  trigger_cleanup:
@@ -210,7 +241,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 +258,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;
@@ -289,37 +321,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:
@@ -330,7 +336,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);
 }
 
@@ -479,7 +485,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);
@@ -534,16 +539,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
@@ -566,34 +561,58 @@ 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;
+}
+
+const char *
+sql_trigger_name(struct Trigger *trigger)
+{
+	return trigger->zName;
+}
+
+uint32_t
+sql_trigger_space_id(struct Trigger *trigger)
+{
+	return trigger->space_id;
+}
+
+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;
 }
 
 /*
@@ -632,22 +651,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);
+	for (struct Trigger *p = trigger_list; p != NULL; p = p->pNext) {
+		if (p->op == op && checkColumnOverlap(p->pColumns,
+						      pChanges) != 0)
 			mask |= p->tr_tm;
-		}
 	}
-	if (pMask) {
+	if (pMask != NULL)
 		*pMask = mask;
-	}
-	return (mask ? pList : 0);
+	return (mask != 0) ? trigger_list : NULL;
 }
 
 /*
@@ -837,7 +852,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 == NULL || pTab->def->id == pTrigger->space_id);
 	assert(pTop->pVdbe);
 
 	/* Allocate the TriggerPrg and SubProgram objects. To ensure that they
@@ -954,7 +969,7 @@ getRowTrigger(Parse * pParse,	/* Current parse context */
 	Parse *pRoot = sqlite3ParseToplevel(pParse);
 	TriggerPrg *pPrg;
 
-	assert(pTrigger->zName == 0 || pTab == tableOfTrigger(pTrigger));
+	assert(pTrigger->zName == NULL || 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
@@ -1079,15 +1094,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..b5f1f1c 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,22 @@ 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; ) {
+		/* Store pointer as trigger will be destructed. */
+		struct Trigger *next_trigger = trigger->pNext;
+		rc = tarantoolSqlite3RenameTrigger(trigger->zName,
 						   zOldTableName, zNewTableName);
-		if (rc) goto abort_due_to_error;
+		if (rc != SQLITE_OK) {
+			sqlite3ResetAllSchemasOfConnection(db);
+			goto abort_due_to_error;
+		}
+		trigger = next_trigger;
 	}
+
 	sqlite3DbFree(db, (void*)zOldTableName);
 	sqlite3DbFree(db, (void*)zSqlStmt);
 	break;
@@ -4857,18 +4818,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 930d0a8..3b273a8 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -382,6 +382,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/engine/iterator.result b/test/engine/iterator.result
index 4984076..a36761d 100644
--- a/test/engine/iterator.result
+++ b/test/engine/iterator.result
@@ -4211,7 +4211,7 @@ s:replace{35}
 ...
 state, value = gen(param,state)
 ---
-- error: 'builtin/box/schema.lua:1045: usage: next(param, state)'
+- error: 'builtin/box/schema.lua:1049: usage: next(param, state)'
 ...
 value
 ---
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..eb7de6d
--- /dev/null
+++ b/test/sql/triggers.result
@@ -0,0 +1,255 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+-- get invariant part of the tuple
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
+---
+...
+--
+-- gh-3273: Move Triggers to server
+--
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}
+...
+space_id = box.space._space.index["name"]:get('T1').id
+---
+...
+-- checks for LUA tuples
+tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- error: Duplicate key exists in unique index 'primary' in space '_trigger'
+...
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple trigger name does not match extracted from SQL'
+...
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple trigger name does not match extracted from SQL'
+...
+tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- error: 'SQL error: tuple space_id does not match the value resolved on AST building
+    from SQL'
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}
+...
+box.sql.execute("DROP TABLE T1;")
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- []
+...
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+---
+...
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- - - T1T
+    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
+        END; '}
+...
+space_id = box.space._space.index["name"]:get('T1').id
+---
+...
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+---
+...
+box.sql.execute("SELECT * FROM t2;")
+---
+- - [1]
+...
+box.sql.execute("DELETE FROM t2;")
+---
+...
+-- test triggers
+tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- - - T2T
+    - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
+        END;'}
+...
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+---
+...
+immutable_part({box.space._trigger:insert(tuple)})
+---
+- - - T3T
+    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+        END;'}
+...
+immutable_part(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;")
+---
+...
+immutable_part({box.space._trigger:delete("T3T")})
+---
+- - - T3T
+    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
+        END;'}
+...
+immutable_part(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; ]])
+---
+...
+immutable_part(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.sql.execute("DROP TABLE t1;")
+---
+...
+box.sql.execute("DROP TABLE t2;")
+---
+...
+immutable_part(box.space._trigger:select())
+---
+- []
+...
+-- 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;")
+---
+...
+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..23d6f8e
--- /dev/null
+++ b/test/sql/triggers.test.lua
@@ -0,0 +1,92 @@
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+-- get invariant part of the tuple
+ function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
+
+--
+-- gh-3273: Move Triggers to server
+--
+
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+immutable_part(box.space._trigger:select())
+
+space_id = box.space._space.index["name"]:get('T1').id
+
+-- checks for LUA tuples
+tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+
+tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+immutable_part(box.space._trigger:select())
+
+box.sql.execute("DROP TABLE T1;")
+immutable_part(box.space._trigger:select())
+
+box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
+box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
+immutable_part(box.space._trigger:select())
+
+space_id = box.space._space.index["name"]:get('T1').id
+
+-- test, didn't trigger t1t degrade
+box.sql.execute("INSERT INTO t1 VALUES(1);")
+box.sql.execute("SELECT * FROM t2;")
+box.sql.execute("DELETE FROM t2;")
+
+
+-- test triggers
+tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
+immutable_part({box.space._trigger:insert(tuple)})
+immutable_part(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;")
+immutable_part({box.space._trigger:delete("T3T")})
+immutable_part(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; ]])
+immutable_part(box.space._trigger:select())
+box.sql.execute("INSERT INTO t1 VALUES(4);")
+box.sql.execute("SELECT * FROM t2;")
+
+-- clean up
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
+immutable_part(box.space._trigger:select())
+
+
+-- 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)
+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);")
+box.sql.execute("SELECT * from t1");
+box.sql.execute("SELECT * from t2");
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")
+
+test_run:cmd("clear filter")
-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server

[Vladislav Shpilevoy]

Thanks for the fixes! See 11 comments below.

Besides, I have pushed other review fixes as a separate commit. Please,
look and squash.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> Introduced sql_triggers field in space structure.
> Changed parser logic to do not insert built triggers, just only
> to do parsing. All triggers insertions and deletions are operated
> via on_replace_dd_trigger now.
> 
> Resolves #3273.
> ---
>   src/box/alter.cc                      | 128 +++++++++++++++
>   src/box/errcode.h                     |   2 +-
>   src/box/lua/schema.lua                |   4 +
>   src/box/space.c                       |   5 +
>   src/box/space.h                       |   2 +
>   src/box/sql.c                         |  39 -----
>   src/box/sql.h                         |  50 ++++++
>   src/box/sql/build.c                   |   8 +-
>   src/box/sql/fkey.c                    |   2 -
>   src/box/sql/insert.c                  |   6 +-
>   src/box/sql/prepare.c                 |   1 +
>   src/box/sql/sqliteInt.h               |   5 -
>   src/box/sql/tokenize.c                |  20 ++-
>   src/box/sql/trigger.c                 | 282 +++++++++++++++++-----------------
>   src/box/sql/vdbe.c                    |  77 ++--------
>   src/box/sql/vdbe.h                    |   1 -
>   src/box/sql/vdbeaux.c                 |   9 --
>   test/box/misc.result                  |   1 +
>   test/engine/iterator.result           |   2 +-
>   test/sql-tap/identifier_case.test.lua |   4 +-
>   test/sql-tap/trigger1.test.lua        |  14 +-
>   test/sql/triggers.result              | 255 ++++++++++++++++++++++++++++++
>   test/sql/triggers.test.lua            |  92 +++++++++++
>   23 files changed, 731 insertions(+), 278 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..4fc46fc 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3091,6 +3095,49 @@ lock_before_dd(struct trigger *trigger, void *event)
>   	latch_lock(&schema_lock);
>   }
>   
> +static void
> +on_replace_trigger_rollback(struct trigger *trigger, void *event)
> +{
> +	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +	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);

1. When you rollback insertion, that means you have no old_trigger.
So sql_trigger_name(old_trigger) crashes. Please, add a test
and fix the problem.

> +		(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
> +on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
> +{
> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
> +	/* DELETE, REPLACE trigger. */

2. Doesn't commit of insertion call this function?

> +	sql_trigger_delete(sql_get(), old_trigger);
> +}

3. Please, add comments to both on_replace_trigger functions.

> @@ -3100,6 +3147,87 @@ 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(on_replace_trigger_rollback, NULL);
> +	struct trigger *on_commit =
> +		txn_alter_trigger_new(on_replace_trigger_commit, NULL);
> +
> +	if (old_tuple != NULL && new_tuple == NULL) {
> +		/* DROP trigger. */
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
> +					   &trigger_name_len);
> +		char *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 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 {
> +		/* INSERT, REPLACE trigger. */
> +		uint32_t trigger_name_len;
> +		const char *trigger_name_src =
> +			tuple_field_str_xc(new_tuple, BOX_TRIGGER_FIELD_NAME,
> +					   &trigger_name_len);
> +
> +		const char *space_opts =
> +			tuple_field_with_type_xc(new_tuple,
> +						 BOX_TRIGGER_FIELD_OPTS,
> +						 MP_MAP);
> +		struct space_opts opts;
> +		struct region *region = &fiber()->gc;
> +		space_opts_decode(&opts, space_opts, region);
> +		struct Trigger *new_trigger =
> +			sql_trigger_compile(sql_get(), opts.sql);
> +		if (new_trigger == NULL)
> +			diag_raise();
> +
> +		const char *trigger_name = sql_trigger_name(new_trigger);
> +		if (strlen(trigger_name) != trigger_name_len ||
> +			memcmp(trigger_name_src, trigger_name,
> +			       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");
> +		}
> +		uint32_t space_id =
> +			tuple_field_u32_xc(new_tuple,
> +					   BOX_TRIGGER_FIELD_SPACE_ID);

4. If this function raises, new_trigger will leak.

> +		if (space_id != sql_trigger_space_id(new_trigger)) {
> +			sql_trigger_delete(sql_get(), new_trigger);
> +			tnt_raise(ClientError, ER_SQL,
> +				  "tuple space_id does not match the value "
> +				  "resolved on AST building from SQL");
> +		}
> +
> +		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();
> +		}
> +
> +		on_commit->data = old_trigger;
> +		on_rollback->data = new_trigger;
> +	}
> +
> +	txn_on_rollback(txn, on_rollback);
> +	txn_on_commit(txn, on_commit);
>   }
>   
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e43fbcb..1fe6deb 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -454,5 +454,6 @@ sql_parser_destroy(Parse *parser)
>   	}
>   	parser->disableLookaside = 0;
>   	sqlite3DbFree(db, parser->zErrMsg);
> +	sql_trigger_delete(db, parser->pNewTrigger);

Good. Lets use the same way for parsed_expr as I mentioned in one of
the previous letters.

>   	region_destroy(&parser->region);
>   }
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index d22ce8c..e06cc0a 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -572,3 +571,22 @@ sql_expr_compile(sqlite3 *db, const char *expr, int expr_len)
>   	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 &&
> +	    parser.rc != SQL_TARANTOOL_ERROR) {
> +		diag_set(ClientError, ER_SQL, sql_error);

5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you
ignore it and takes pNewTrigger. You should not.

> +	} else {
> +		trigger = parser.pNewTrigger;
> +		parser.pNewTrigger = NULL;
> +	}
> +	sql_parser_destroy(&parser);
> +	return trigger;
> +}
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3fe5875..b5f1f1c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -4799,19 +4757,22 @@ case OP_RenameTable: {
>   		goto abort_due_to_error;
>   	}
>   
> -	pTab = sqlite3HashFind(&db->pSchema->tblHash, zNewTableName);
> -	pTab->pTrigger = pTrig;
> +	space = space_by_id(space_id);

6. You said that space * becomes invalid, but anyway its triggers are
transferred with no changes, so you can save them into a local
variable before space rename, and remove this space_by_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; ) {
> +		/* Store pointer as trigger will be destructed. */
> +		struct Trigger *next_trigger = trigger->pNext;
> +		rc = tarantoolSqlite3RenameTrigger(trigger->zName,
>   						   zOldTableName, zNewTableName);
> -		if (rc) goto abort_due_to_error;
> +		if (rc != SQLITE_OK) {
> +			sqlite3ResetAllSchemasOfConnection(db);
> +			goto abort_due_to_error;
> +		}
> +		trigger = next_trigger;
>   	}
> +
>   	sqlite3DbFree(db, (void*)zOldTableName);
>   	sqlite3DbFree(db, (void*)zSqlStmt);
>   	break;
> diff --git a/test/sql/triggers.result b/test/sql/triggers.result
> new file mode 100644
> index 0000000..eb7de6d
> --- /dev/null
> +++ b/test/sql/triggers.result
> @@ -0,0 +1,255 @@
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")

7. Why do you need this filter, if no one of errors
emerged from a lua file?

> +---
> +- true
> +...
> +-- get invariant part of the tuple
> + function immutable_part(data) local r = {} for i, l in pairs(data) do table.insert(r, {l.name, l.opts}) end return r end
> +---
> +...
> +--
> +-- gh-3273: Move Triggers to server
> +--
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute("CREATE TABLE t2(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +space_id = box.space._space.index["name"]:get('T1').id
> +---
> +...
> +-- checks for LUA tuples
> +tuple = {"T1T", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})

8. Same as point 19 on the previous review.

> +---
> +- error: Duplicate key exists in unique index 'primary' in space '_trigger'
> +...
> +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple trigger name does not match extracted from SQL'

9. Please, replace 'tuple trigger' with just 'trigger' in all new
error messages.

> +...
> +tuple = {"T1t", space_id, {sql = "CREATE TRIGGER t12t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple trigger name does not match extracted from SQL'
> +...
> +tuple = {"T2T", 443, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- error: 'SQL error: tuple space_id does not match the value resolved on AST building
> +    from SQL'
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +box.sql.execute("DROP TABLE T1;")
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- []
> +...
> +box.sql.execute("CREATE TABLE t1(x INTEGER PRIMARY KEY);")
> +---
> +...
> +box.sql.execute([[CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1); END; ]])
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- - - T1T
> +    - {'sql': 'CREATE TRIGGER t1t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(1);
> +        END; '}
> +...
> +space_id = box.space._space.index["name"]:get('T1').id
> +---
> +...
> +-- test, didn't trigger t1t degrade
> +box.sql.execute("INSERT INTO t1 VALUES(1);")
> +---
> +...
> +box.sql.execute("SELECT * FROM t2;")
> +---
> +- - [1]
> +...
> +box.sql.execute("DELETE FROM t2;")
> +---
> +...
> +-- test triggers
> +tuple = {"T2T", space_id, {sql = "CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- - - T2T
> +    - {'sql': 'CREATE TRIGGER t2t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(2);
> +        END;'}

10. Instead of 'immutabling' result of insert you can just do

     _ = box.space._trigger:insert(tuple)

Anyway you are going to print that tuple from the next select(). Please
do it in other places of insertions and deletions too.

> +...
> +tuple = {"T3T", space_id, {sql = "CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3); END;"}}
> +---
> +...
> +immutable_part({box.space._trigger:insert(tuple)})
> +---
> +- - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END;'}
> +...
> +immutable_part(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;")
> +---
> +...
> +immutable_part({box.space._trigger:delete("T3T")})
> +---
> +- - - T3T
> +    - {'sql': 'CREATE TRIGGER t3t AFTER INSERT ON t1 BEGIN INSERT INTO t2 VALUES(3);
> +        END;'}
> +...
> +immutable_part(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; ]])
> +---
> +...
> +immutable_part(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.sql.execute("DROP TABLE t1;")
> +---
> +...
> +box.sql.execute("DROP TABLE t2;")
> +---
> +...
> +immutable_part(box.space._trigger:select())
> +---
> +- []
> +...
> +-- 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)

11. Error injections are disabled in Release build. Please, move this test
case into a separate file. We have test/sql/errinj.test.lua.

> +---
> +- 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;")
> +---
> +...
> +test_run:cmd("clear filter")
> +---
> +- true
> +...

[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server

[Kirill Shcherbatov]

> 1. When you rollback insertion, that means you have no old_trigger.
> So sql_trigger_name(old_trigger) crashes. Please, add a test
> and fix the problem.
No, there is no problem. Maybe "old_trigger" is a little confusing name, but on_rollback->data = new_trigger; for insert and it is not NULL.
New ErrInjection test validates this case.

--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -44,3 +44,18 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
 
 box.sql.execute('DROP TABLE test')
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
+
+-- 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.error.injection.set("ERRINJ_WAL_IO", true)
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.sql.execute("CREATE INDEX t1a ON t1(a);")
+box.error.injection.set("ERRINJ_WAL_IO", false)
+box.sql.execute("CREATE TRIGGER t1t INSERT ON t1 BEGIN INSERT INTO t2 VALUES (1, 1); END;")
+box.sql.execute("INSERT INTO t1 VALUES (3, 3);")
+box.sql.execute("SELECT * from t1");
+box.sql.execute("SELECT * from t2");
+box.sql.execute("DROP TABLE t1;")
+box.sql.execute("DROP TABLE t2;")


> 
>> +		(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
>> +on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
>> +{
>> +	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
>> +	/* DELETE, REPLACE trigger. */
> 
> 2. Doesn't commit of insertion call this function?
Yes, it is. But old_trigger is NULL in this case and this comment is a legacy, there was a if-clause.
-       /* DELETE, REPLACE trigger. */

> 
>> +	sql_trigger_delete(sql_get(), old_trigger);
>> +}
> 
> 3. Please, add comments to both on_replace_trigger functions.
Similar to on_rollback_dd_sequence comments:
 
+/**
+ * Trigger invoked on rollback in the _trigger space.
+ */
 static void
 on_replace_trigger_rollback(struct trigger *trigger, void *event)

+/**
+ * Trigger invoked on commit in the _trigger space.
+ */
 static void
 on_replace_trigger_commit(struct trigger *trigger, void * /* event */)

> 4. If this function raises, new_trigger will leak.
Ok, I believe that now it is a time to use guard that was rejected earlier. It is common for alter.cc.

@@ -3197,11 +3202,14 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                if (new_trigger == NULL)
                        diag_raise();
 
+               auto new_trigger_guard = make_scoped_guard([=] {
+                   sql_trigger_delete(sql_get(), new_trigger);
+               });
+
                const char *trigger_name = sql_trigger_name(new_trigger);
                if (strlen(trigger_name) != trigger_name_len ||
                        memcmp(trigger_name_src, trigger_name,
                               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");
@@ -3210,7 +3218,6 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                        tuple_field_u32_xc(new_tuple,
                                           BOX_TRIGGER_FIELD_SPACE_ID);
                if (space_id != sql_trigger_space_id(new_trigger)) {
-                       sql_trigger_delete(sql_get(), new_trigger);
                        tnt_raise(ClientError, ER_SQL,
                                  "tuple space_id does not match the value "
                                  "resolved on AST building from SQL");
@@ -3219,12 +3226,12 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
                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();
                }
 
                on_commit->data = old_trigger;
                on_rollback->data = new_trigger;
+               new_trigger_guard.is_active = false;
        }
 
        txn_on_rollback(txn, on_rollback);


> Good. Lets use the same way for parsed_expr as I mentioned in one of
> the previous letters.
Done in previous commit.

> 5. Same as in expr_compile. When rc == SQL_TARANTOOL_ERROR, you
> ignore it and takes pNewTrigger. You should not.
-       if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK &&
-           parser.rc != SQL_TARANTOOL_ERROR) {
+       if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK) {
+           if (parser.rc != SQL_TARANTOOL_ERROR)
                diag_set(ClientError, ER_SQL, sql_error);

> 6. You said that space * becomes invalid, but anyway its triggers are
> transferred with no changes, so you can save them into a local
> variable before space rename, and remove this space_by_id().
@@ -4713,6 +4713,8 @@ case OP_RenameTable: {
        space_id = SQLITE_PAGENO_TO_SPACEID(pOp->p1);
        space = space_by_id(space_id);
        assert(space);
+       /* Rename space op doesn't change triggers. */
+       struct Trigger *triggers = space->sql_triggers;
        zOldTableName = space_name(space);
        assert(zOldTableName);
        pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
@@ -4757,16 +4759,13 @@ case OP_RenameTable: {
                goto abort_due_to_error;
        }
 
-       space = space_by_id(space_id);
-       assert(space != NULL);

-       for (struct Trigger *trigger = space->sql_triggers; trigger != NULL; ) {
+       for (struct Trigger *trigger = triggers; trigger != NULL; ) {
                /* Store pointer as trigger will be destructed. */
                struct Trigger *next_trigger = trigger->pNext;
                rc = tarantoolSqlite3RenameTrigger(trigger->zName,

> 7. Why do you need this filter, if no one of errors
> emerged from a lua file?
Just copied from template.

> 8. Same as point 19 on the previous review.
Ok, let drop it. It is not really important test.

> 9. Please, replace 'tuple trigger' with just 'trigger' in all new
> error messages.
Ok.

> 10. Instead of 'immutabling' result of insert you can just do
> 
>      _ = box.space._trigger:insert(tuple)
> 
> Anyway you are going to print that tuple from the next select(). Please
> do it in other places of insertions and deletions too.
Ok.
> 11. Error injections are disabled in Release build. Please, move this test
> case into a separate file. We have test/sql/errinj.test.lua.
Moved to test/sql/errinj.test.lua

[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See 3 comments below.

On 15/06/2018 19:21, Kirill Shcherbatov wrote:
>> 1. When you rollback insertion, that means you have no old_trigger.
>> So sql_trigger_name(old_trigger) crashes. Please, add a test
>> and fix the problem.
> No, there is no problem. Maybe "old_trigger" is a little confusing name, but on_rollback->data = new_trigger; for insert and it is not NULL.
> New ErrInjection test validates this case.
> 
> --- a/test/sql/errinj.test.lua
> +++ b/test/sql/errinj.test.lua
> @@ -44,3 +44,18 @@ errinj.set("ERRINJ_IPROTO_TX_DELAY", false)
>   
>   box.sql.execute('DROP TABLE test')
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> +
> +

1. Please, start a sentence with capital letter and finish
with dot.

2. Put here link to the issue of the form:
--
-- gh-####: description.
--

3. I have pushed some fixes in a separate commit. Please,
look and squash. After the patch will be LGTM.

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

[tarantool-patches] Re: [PATCH v3 09/10] sql: move Triggers to server

[Kirill Shcherbatov]

Let's push it to the next review step)

> 1. Please, start a sentence with capital letter and finish
> with dot.
> 2. Put here link to the issue of the form:
--- test crash on error.injection
+----
+---- gh-3273: Move SQL TRIGGERs into server.
+----

> 3. I have pushed some fixes in a separate commit. Please,
> look and squash. After the patch will be LGTM.
Ok, Tnx!

[tarantool-patches] Re: [PATCH v3 00/10] 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

To move Triggers to server, we introduced special field sql_triggers 
in space struct. All triggers AST objects are stored in global Hash
db->pSchema->trigHash. Space refer to trigger AST by pointer, but not hold it.
All trigger actions operated with on_replace_dd_trigger that compiles
AST, makes all related checks(e.g. that other _trigger tuple fields
match extracred values), and does insertion in Hash and List in space.

To support LUA deletions we have changed _trigger space format to contain
space_id as secondary key: on space delete in LUA code we obtain relative 
_trigger tuples and pop up them from their space. This cause triggering
on_replace_dd_trigger that removes them from the sql_triggers list in space
and Cache, so on final space_delete no SQL trigger should be present in DB.

Kirill Shcherbatov (10):
  box: move db->pShchema init to sql_init
  sql: fix leak on CREATE TABLE and resolve self ref
  sql: fix sql len in tarantoolSqlite3RenameTrigger
  box: port schema_find_id to C
  sql: refactor sql_expr_compile to return AST
  sql: move sqlite3DeleteTrigger to sql.h
  box: sort error codes in misc.test
  sql: new  _trigger space format with space_id
  sql: move Triggers to server
  sql: VDBE tests for trigger existence

 src/box/alter.cc                                   | 134 ++++++++-
 src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
 src/box/errcode.h                                  |   2 +-
 src/box/lua/schema.lua                             |   4 +
 src/box/lua/upgrade.lua                            |   4 +
 src/box/schema.cc                                  |  54 +++-
 src/box/schema.h                                   |  23 +-
 src/box/schema_def.h                               |   6 +
 src/box/space.c                                    |   5 +
 src/box/space.h                                    |   2 +
 src/box/sql.c                                      |  85 ++----
 src/box/sql.h                                      |  66 ++++-
 src/box/sql/build.c                                |  59 +++-
 src/box/sql/callback.c                             |   7 +-
 src/box/sql/fkey.c                                 |   2 -
 src/box/sql/insert.c                               |   6 +-
 src/box/sql/main.c                                 |  10 +-
 src/box/sql/prepare.c                              |   2 +
 src/box/sql/sqliteInt.h                            |  30 +-
 src/box/sql/status.c                               |   6 +-
 src/box/sql/tokenize.c                             |  50 +++-
 src/box/sql/trigger.c                              | 320 ++++++++++----------
 src/box/sql/vdbe.c                                 |  87 ++----
 src/box/sql/vdbe.h                                 |   3 +-
 src/box/sql/vdbeapi.c                              |   5 +-
 src/box/sql/vdbeaux.c                              |  13 +-
 src/box/user.cc                                    |   4 +-
 src/diag.h                                         |   3 +
 test/app-tap/tarantoolctl.test.lua                 |   2 +-
 test/box-py/bootstrap.result                       |   5 +-
 test/box/access_misc.result                        |   4 +-
 test/box/access_sysview.result                     |   4 +-
 test/box/alter.result                              |   1 +
 test/box/misc.result                               | 326 +++++++++++----------
 test/box/misc.test.lua                             |   4 +-
 test/engine/iterator.result                        |   2 +-
 test/sql-tap/identifier_case.test.lua              |   4 +-
 test/sql-tap/trigger1.test.lua                     |  14 +-
 test/sql/gh2141-delete-trigger-drop-table.result   |   4 +-
 test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +-
 test/sql/persistency.result                        |   8 +-
 test/sql/persistency.test.lua                      |   8 +-
 test/sql/triggers.result                           | 255 ++++++++++++++++
 test/sql/triggers.test.lua                         |  92 ++++++
 test/sql/upgrade.result                            |  37 ++-
 test/sql/upgrade.test.lua                          |   9 +-
 46 files changed, 1205 insertions(+), 570 deletions(-)
 create mode 100644 test/sql/triggers.result
 create mode 100644 test/sql/triggers.test.lua

-- 
2.7.4

[tarantool-patches] Re: [PATCH v3 00/10] sql: remove Triggers to server

[Vladislav Shpilevoy]

Now the patchset LGTM.

Nikita, please, review.

On 14/06/2018 20:32, Kirill Shcherbatov wrote:
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-3273-no-sql-triggers
> Issue: https://github.com/tarantool/tarantool/issues/3273
> 
> Kirill Shcherbatov (10):
>    box: move db->pShchema init to sql_init
>    sql: fix leak on CREATE TABLE and resolve self ref
>    sql: fix sql len in tarantoolSqlite3RenameTrigger
>    box: port schema_find_id to C
>    sql: refactor sql_expr_compile to return AST
>    sql: move sqlite3DeleteTrigger to sql.h
>    box: sort error codes in misc.test
>    sql: new  _trigger space format with space_id
>    sql: move Triggers to server
>    sql: VDBE tests for trigger existence
> 
>   src/box/alter.cc                                   | 134 ++++++++-
>   src/box/bootstrap.snap                             | Bin 1698 -> 1704 bytes
>   src/box/errcode.h                                  |   2 +-
>   src/box/lua/schema.lua                             |   4 +
>   src/box/lua/upgrade.lua                            |   4 +
>   src/box/schema.cc                                  |  54 +++-
>   src/box/schema.h                                   |  23 +-
>   src/box/schema_def.h                               |   6 +
>   src/box/space.c                                    |   5 +
>   src/box/space.h                                    |   2 +
>   src/box/sql.c                                      |  85 ++----
>   src/box/sql.h                                      |  66 ++++-
>   src/box/sql/build.c                                |  59 +++-
>   src/box/sql/callback.c                             |   7 +-
>   src/box/sql/fkey.c                                 |   2 -
>   src/box/sql/insert.c                               |   6 +-
>   src/box/sql/main.c                                 |  10 +-
>   src/box/sql/prepare.c                              |   2 +
>   src/box/sql/sqliteInt.h                            |  30 +-
>   src/box/sql/status.c                               |   6 +-
>   src/box/sql/tokenize.c                             |  50 +++-
>   src/box/sql/trigger.c                              | 320 ++++++++++----------
>   src/box/sql/vdbe.c                                 |  87 ++----
>   src/box/sql/vdbe.h                                 |   3 +-
>   src/box/sql/vdbeapi.c                              |   5 +-
>   src/box/sql/vdbeaux.c                              |  13 +-
>   src/box/user.cc                                    |   4 +-
>   src/diag.h                                         |   3 +
>   test/app-tap/tarantoolctl.test.lua                 |   2 +-
>   test/box-py/bootstrap.result                       |   5 +-
>   test/box/access_misc.result                        |   4 +-
>   test/box/access_sysview.result                     |   4 +-
>   test/box/alter.result                              |   1 +
>   test/box/misc.result                               | 326 +++++++++++----------
>   test/box/misc.test.lua                             |   4 +-
>   test/engine/iterator.result                        |   2 +-
>   test/sql-tap/identifier_case.test.lua              |   4 +-
>   test/sql-tap/trigger1.test.lua                     |  14 +-
>   test/sql/gh2141-delete-trigger-drop-table.result   |   4 +-
>   test/sql/gh2141-delete-trigger-drop-table.test.lua |   4 +-
>   test/sql/persistency.result                        |   8 +-
>   test/sql/persistency.test.lua                      |   8 +-
>   test/sql/triggers.result                           | 255 ++++++++++++++++
>   test/sql/triggers.test.lua                         |  92 ++++++
>   test/sql/upgrade.result                            |  37 ++-
>   test/sql/upgrade.test.lua                          |   9 +-
>   46 files changed, 1205 insertions(+), 570 deletions(-)
>   create mode 100644 test/sql/triggers.result
>   create mode 100644 test/sql/triggers.test.lua
> 

[tarantool-patches] Re: [PATCH v3 00/10] sql: remove Triggers to server

[n.pettik]

LGTM as well. But patchset has exposed with two commits:
first one simply consists of refactoring of functions in sql/trigger.c,
renaming struct Trigger to struct sql_trigger and removing SQLITE_OMIT_TRIGGER
define guard. Second one removes global hash for triggers.
IDK what our review policy says: should you look at them or not?

> Now the patchset LGTM.
> 
> Nikita, please, review.