[tarantool-patches] [PATCH v4 8/8] sql: remove global sql_trigger hash

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

As new _trigger format contain space_id, we can do not
store AST pointers in HASH. Requested AST could be found
by name in appropriate space.

Part of #3273.
---
 src/box/alter.cc               |  15 +++--
 src/box/errcode.h              |   2 +-
 src/box/sql.h                  |   3 +-
 src/box/sql/build.c            |  16 +++--
 src/box/sql/callback.c         |   9 ---
 src/box/sql/parse.y            |   2 +-
 src/box/sql/sqliteInt.h        |  31 ++++++----
 src/box/sql/status.c           |   8 ---
 src/box/sql/trigger.c          | 132 ++++++++++++++++++-----------------------
 test/sql-tap/trigger1.test.lua |   4 +-
 test/sql/persistency.result    |   2 +-
 11 files changed, 106 insertions(+), 118 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 449b4b1..9051d7d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3264,6 +3264,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event)
 		/* Rollback DELETE trigger. */
 		if (sql_trigger_replace(sql_get(),
 					sql_trigger_name(old_trigger),
+					sql_trigger_space_id(old_trigger),
 					old_trigger, &new_trigger) != 0)
 			panic("Out of memory on insertion into trigger hash");
 		assert(new_trigger == NULL);
@@ -3271,6 +3272,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event)
 		/* Rollback INSERT trigger. */
 		int rc = sql_trigger_replace(sql_get(),
 					     sql_trigger_name(old_trigger),
+					     sql_trigger_space_id(old_trigger),
 					     NULL, &new_trigger);
 		(void)rc;
 		assert(rc == 0);
@@ -3280,6 +3282,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event)
 		/* Rollback REPLACE trigger. */
 		if (sql_trigger_replace(sql_get(),
 					sql_trigger_name(old_trigger),
+					sql_trigger_space_id(old_trigger),
 					old_trigger, &new_trigger) != 0)
 			panic("Out of memory on insertion into trigger hash");
 		assert(old_trigger != new_trigger);
@@ -3322,6 +3325,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		const char *trigger_name_src =
 			tuple_field_str_xc(old_tuple, BOX_TRIGGER_FIELD_NAME,
 					   &trigger_name_len);
+		uint32_t space_id =
+			tuple_field_u32_xc(old_tuple,
+					   BOX_TRIGGER_FIELD_SPACE_ID);
 		char *trigger_name =
 			(char *)region_alloc_xc(&fiber()->gc,
 						trigger_name_len + 1);
@@ -3329,8 +3335,8 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		trigger_name[trigger_name_len] = 0;
 
 		struct sql_trigger *old_trigger;
-		int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
-					     &old_trigger);
+		int rc = sql_trigger_replace(sql_get(), trigger_name, space_id,
+					     NULL, &old_trigger);
 		(void)rc;
 		assert(rc == 0);
 		assert(old_trigger != NULL);
@@ -3378,8 +3384,9 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		}
 
 		struct sql_trigger *old_trigger;
-		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
-					&old_trigger) != 0)
+		if (sql_trigger_replace(sql_get(), trigger_name,
+					sql_trigger_space_id(new_trigger),
+					new_trigger, &old_trigger) != 0)
 			diag_raise();
 
 		on_commit->data = old_trigger;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index eb05936..6375764 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -86,7 +86,7 @@ struct errcode_record {
 	/* 31 */_(ER_KEY_PART_COUNT,		"Invalid key part count (expected [0..%u], got %u)") \
 	/* 32 */_(ER_PROC_LUA,			"%s") \
 	/* 33 */_(ER_NO_SUCH_PROC,		"Procedure '%.*s' is not defined") \
-	/* 34 */_(ER_NO_SUCH_TRIGGER,		"Trigger is not found") \
+	/* 34 */_(ER_NO_SUCH_TRIGGER,		"Trigger '%s' doesn't exist") \
 	/* 35 */_(ER_NO_SUCH_INDEX,		"No index #%u is defined in space '%s'") \
 	/* 36 */_(ER_NO_SUCH_SPACE,		"Space '%s' does not exist") \
 	/* 37 */_(ER_NO_SUCH_FIELD,		"Field %d was not found in the tuple") \
diff --git a/src/box/sql.h b/src/box/sql.h
index f483921..3ee974e 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -126,6 +126,7 @@ 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 space_id of the space to insert trigger.
  * @param trigger AST object to insert.
  * @param[out] old_trigger Old object if exists.
  *
@@ -133,7 +134,7 @@ space_trigger_list(uint32_t space_id);
  * @retval -1 on error.
  */
 int
-sql_trigger_replace(struct sqlite3 *db, const char *name,
+sql_trigger_replace(struct sqlite3 *db, const char *name, uint32_t space_id,
 		    struct sql_trigger *trigger,
 		    struct sql_trigger **old_trigger);
 
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 093fea5..59d9656 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2121,7 +2121,7 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	parse_context->nested++;
 	struct sql_trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
-		vdbe_code_drop_trigger_ptr(parse_context, trigger);
+		vdbe_code_drop_trigger_ptr(parse_context, trigger->zName);
 		trigger = trigger->next;
 	}
 	parse_context->nested--;
@@ -3973,10 +3973,14 @@ sqlite3WithDelete(sqlite3 * db, With * pWith)
 #endif				/* !defined(SQLITE_OMIT_CTE) */
 
 int
-vdbe_emit_halt_if_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)
-{
+vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
+				  int index_id,
+				  const char *name_src,
+				  int tarantool_error_code,
+				  const char *error_src, bool no_error,
+				  int cond_opcode)
+{
+	assert(cond_opcode == OP_NoConflict || cond_opcode == OP_Found);
 	struct Vdbe *v = sqlite3GetVdbe(parser);
 	assert(v != NULL);
 
@@ -3996,7 +4000,7 @@ vdbe_emit_halt_if_exists(struct Parse *parser, int space_id, int index_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);
+	sqlite3VdbeAddOp4Int(v, cond_opcode, cursor, label + 3, name_reg, 1);
 	if (no_error) {
 		sqlite3VdbeAddOp0(v, OP_Halt);
 	} else {
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index c3c38cb..01e8dd8 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -283,18 +283,10 @@ sqlite3SchemaClear(sqlite3 * db)
 	assert(db->pSchema != NULL);
 
 	Hash temp1;
-	Hash temp2;
 	HashElem *pElem;
 	Schema *pSchema = db->pSchema;
 
 	temp1 = pSchema->tblHash;
-	temp2 = pSchema->trigHash;
-	sqlite3HashInit(&pSchema->trigHash);
-	for (pElem = sqliteHashFirst(&temp2); pElem != NULL;
-	     pElem = sqliteHashNext(pElem))
-		sql_trigger_delete(NULL,
-				   (struct sql_trigger *)sqliteHashData(pElem));
-	sqlite3HashClear(&temp2);
 	sqlite3HashInit(&pSchema->tblHash);
 	for (pElem = sqliteHashFirst(&temp1); pElem;
 	     pElem = sqliteHashNext(pElem)) {
@@ -317,7 +309,6 @@ sqlite3SchemaCreate(sqlite3 * db)
 		sqlite3OomFault(db);
 	} else {
 		sqlite3HashInit(&p->tblHash);
-		sqlite3HashInit(&p->trigHash);
 		sqlite3HashInit(&p->fkeyHash);
 	}
 	return p;
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 91bdc6e..2ae0535 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1421,7 +1421,7 @@ raisetype(A) ::= FAIL.      {A = ON_CONFLICT_ACTION_FAIL;}
 
 ////////////////////////  DROP TRIGGER statement //////////////////////////////
 cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
-  sqlite3DropTrigger(pParse,X,NOERR);
+  vdbe_code_drop_trigger(pParse,X,NOERR);
 }
 
 ////////////////////////// REINDEX collation //////////////////////////////////
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index d6ae5c2..5f7a0f1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1523,7 +1523,6 @@ typedef int VList;
 struct Schema {
 	int schema_cookie;      /* Database schema version number for this file */
 	Hash tblHash;		/* All tables indexed by name */
-	Hash trigHash;		/* All triggers indexed by name */
 	Hash fkeyHash;		/* All foreign keys by referenced table name */
 };
 
@@ -4103,16 +4102,24 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
  */
 void sql_trigger_finish(Parse *, TriggerStep *, Token *);
 
-void sqlite3DropTrigger(Parse *, SrcList *, int);
+/**
+ * This function is called from parser to generate drop trigger
+ * VDBE code.
+ *
+ * @param parser Parser context.
+ * @param name The name of trigger to drop.
+ * @param no_err Suppress errors if the trigger already exists.
+ */
+void vdbe_code_drop_trigger(Parse *, SrcList *, int);
 
 /**
  * Drop a trigger given a pointer to that trigger.
  *
- * @param parser Parse context.
- * @param trigger Trigger to drop.
+ * @param parser Parser context.
+ * @param name The name of trigger to drop.
  */
 void
-vdbe_code_drop_trigger_ptr(struct Parse *parser, struct sql_trigger *trigger);
+vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name);
 
 /**
  * Return a list of all triggers on table pTab if there exists at
@@ -4784,8 +4791,8 @@ 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 object with specified name is already present (or doesn't
+ * present is raise_if_not_exists flag is set) in specified space.
  * The function allocates error and name resources for VDBE itself.
  *
  * @param parser Parsing context.
@@ -4795,13 +4802,17 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
  * @param tarantool_error_code to set on halt.
  * @param error_src Error message to display on VDBE halt.
  * @param no_error Do not raise error flag.
+ * @param cond_opcode Condition to raise - OP_NoConflict or OP_Found.
  *
  * @retval -1 on memory allocation error.
  * @retval 0 on success.
  */
 int
-vdbe_emit_halt_if_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);
+vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
+				  int index_id,
+				  const char *name_src,
+				  int tarantool_error_code,
+				  const char *error_src, bool no_error,
+				  int cond_opcode);
 
 #endif				/* SQLITEINT_H */
diff --git a/src/box/sql/status.c b/src/box/sql/status.c
index 161136c..5bb1f8f 100644
--- a/src/box/sql/status.c
+++ b/src/box/sql/status.c
@@ -248,18 +248,10 @@ sqlite3_db_status(sqlite3 * db,	/* The database connection whose status is desir
 				nByte +=
 				    ROUND8(sizeof(HashElem)) *
 				    (pSchema->tblHash.count +
-				     pSchema->trigHash.count +
 				     pSchema->fkeyHash.count);
 				nByte += sqlite3_msize(pSchema->tblHash.ht);
-				nByte += sqlite3_msize(pSchema->trigHash.ht);
 				nByte += sqlite3_msize(pSchema->fkeyHash.ht);
 
-				for (p = sqliteHashFirst(&pSchema->trigHash); p;
-				     p = sqliteHashNext(p)) {
-					sql_trigger_delete(db,
-							   (struct sql_trigger *)
-							   sqliteHashData(p));
-				}
 				for (p = sqliteHashFirst(&pSchema->tblHash); p;
 				     p = sqliteHashNext(p)) {
 					sqlite3DeleteTable(db,
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 4c28580..ca01e6e 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -125,11 +125,11 @@ sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
 		const char *error_msg =
 			tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS),
 				   trigger_name);
-		if (vdbe_emit_halt_if_exists(parse, BOX_TRIGGER_ID,
-					     0, trigger_name,
-					     ER_TRIGGER_EXISTS,
-					     error_msg,
-					     (no_err != 0)) != 0)
+		if (vdbe_emit_halt_with_presence_test(parse, BOX_TRIGGER_ID, 0,
+						      trigger_name,
+						      ER_TRIGGER_EXISTS,
+						      error_msg, (no_err != 0),
+						      OP_NoConflict) != 0)
 			goto trigger_cleanup;
 	}
 
@@ -441,21 +441,36 @@ sql_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
 	sqlite3DbFree(db, trigger);
 }
 
-/*
- * This function is called to drop a trigger from the database
- * schema.
- *
- * This may be called directly from the parser and therefore
- * identifies the trigger by name.  The sql_drop_trigger_ptr()
- * routine does the same job as this routine except it takes a
- * pointer to the trigger instead of the trigger name.
- */
 void
-sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
+vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name)
 {
-	struct sql_trigger *trigger = NULL;
-	const char *zName;
-	sqlite3 *db = pParse->db;
+	sqlite3 *db = parser->db;
+	assert(db->pSchema != NULL);
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	if (v == NULL)
+		return;
+	/*
+	 * Generate code to delete entry from _trigger and
+	 * internal SQL structures.
+	 */
+	int trig_name_reg = ++parser->nMem;
+	int record_to_delete = ++parser->nMem;
+	sqlite3VdbeAddOp4(v, OP_String8, 0, trig_name_reg, 0,
+			  sqlite3DbStrDup(db, trigger_name), P4_DYNAMIC);
+	sqlite3VdbeAddOp3(v, OP_MakeRecord, trig_name_reg, 1,
+			  record_to_delete);
+	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRIGGER_ID,
+			  record_to_delete);
+	if (parser->nested == 0)
+		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+	sqlite3ChangeCookie(parser);
+}
+
+void
+vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
+{
+
+	sqlite3 *db = parser->db;
 
 	if (db->mallocFailed)
 		goto drop_trigger_cleanup;
@@ -467,68 +482,40 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
 	 * here to account DROP TRIGGER IF EXISTS case if the trigger
 	 * actually does not exist.
 	 */
-	if (!pParse->nested) {
-		Vdbe *v = sqlite3GetVdbe(pParse);
+	if (!parser->nested) {
+		Vdbe *v = sqlite3GetVdbe(parser);
 		if (v != NULL)
 			sqlite3VdbeCountChanges(v);
 	}
 
-	assert(pName->nSrc == 1);
-	zName = pName->a[0].zName;
-	trigger = sqlite3HashFind(&(db->pSchema->trigHash), zName);
-	if (trigger == NULL) {
-		if (!noErr) {
-			sqlite3ErrorMsg(pParse, "no such trigger: %S", pName,
-					0);
-		}
-		pParse->checkSchema = 1;
+	assert(name->nSrc == 1);
+	const char *trigger_name = name->a[0].zName;
+	const char *error_msg =
+		tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_TRIGGER),
+			   trigger_name);
+	if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
+					      trigger_name, ER_NO_SUCH_TRIGGER,
+					      error_msg, (no_err != 0),
+					      OP_Found) != 0)
 		goto drop_trigger_cleanup;
-	}
-	vdbe_code_drop_trigger_ptr(pParse, trigger);
 
- drop_trigger_cleanup:
-	sqlite3SrcListDelete(db, pName);
-}
+	vdbe_code_drop_trigger_ptr(parser, trigger_name);
 
-void
-vdbe_code_drop_trigger_ptr(struct Parse *parser, struct sql_trigger *trigger)
-{
-	struct Vdbe *v = sqlite3GetVdbe(parser);
-	if (v == NULL)
-		return;
-	/*
-	 * Generate code to delete entry from _trigger and
-	 * internal SQL structures.
-	 */
-	int trig_name_reg = ++parser->nMem;
-	int record_to_delete = ++parser->nMem;
-	sqlite3VdbeAddOp4(v, OP_String8, 0, trig_name_reg, 0,
-			  trigger->zName, P4_STATIC);
-	sqlite3VdbeAddOp3(v, OP_MakeRecord, trig_name_reg, 1,
-			  record_to_delete);
-	sqlite3VdbeAddOp2(v, OP_SDelete, BOX_TRIGGER_ID,
-			  record_to_delete);
-	if (!parser->nested)
-		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-
-	sqlite3ChangeCookie(parser);
+ drop_trigger_cleanup:
+	sqlite3SrcListDelete(db, name);
 }
 
 int
-sql_trigger_replace(struct sqlite3 *db, const char *name,
+sql_trigger_replace(struct sqlite3 *db, const char *name, uint32_t space_id,
 		    struct sql_trigger *trigger,
 		    struct sql_trigger **old_trigger)
 {
 	assert(db->pSchema != NULL);
 	assert(trigger == NULL || strcmp(name, trigger->zName) == 0);
 
-	struct Hash *hash = &db->pSchema->trigHash;
-
-	struct sql_trigger *src_trigger =
-		trigger != NULL ? trigger : sqlite3HashFind(hash, name);
-	assert(src_trigger != NULL);
-	struct space *space = space_cache_find(src_trigger->space_id);
+	struct space *space = space_cache_find(space_id);
 	assert(space != NULL);
+	*old_trigger = NULL;
 
 	if (trigger != NULL) {
 		/* Do not create a trigger on a system space. */
@@ -562,24 +549,19 @@ sql_trigger_replace(struct sqlite3 *db, const char *name,
 			trigger->tr_tm = TRIGGER_AFTER;
 	}
 
-
-	*old_trigger = sqlite3HashInsert(hash, name, trigger);
-	if (*old_trigger == trigger) {
-		diag_set(OutOfMemory, sizeof(struct HashElem),
-			 "sqlite3HashInsert", "ret");
-		return -1;
+	struct sql_trigger **ptr = &space->sql_triggers;
+	while (*ptr != NULL && strcmp((*ptr)->zName, name) != 0)
+		ptr = &((*ptr)->next);
+	if (*ptr != NULL) {
+		*old_trigger = *ptr;
+		*ptr = (*ptr)->next;
 	}
 
-	if (*old_trigger != NULL) {
-		struct sql_trigger **pp;
-		for (pp = &space->sql_triggers; *pp != *old_trigger;
-		     pp = &((*pp)->next));
-		*pp = (*pp)->next;
-	}
 	if (trigger != NULL) {
 		trigger->next = space->sql_triggers;
 		space->sql_triggers = trigger;
 	}
+
 	return 0;
 }
 
diff --git a/test/sql-tap/trigger1.test.lua b/test/sql-tap/trigger1.test.lua
index c45fdab..0707c8f 100755
--- a/test/sql-tap/trigger1.test.lua
+++ b/test/sql-tap/trigger1.test.lua
@@ -159,7 +159,7 @@ test:do_catchsql_test(
         DROP TRIGGER biggles;
     ]], {
         -- <trigger1-1.6.2>
-        1, "no such trigger: BIGGLES"
+        1, "Trigger 'BIGGLES' doesn't exist"
         -- </trigger1-1.6.2>
     })
 
@@ -170,7 +170,7 @@ test:do_catchsql_test(
         DROP TRIGGER tr1;
     ]], {
         -- <trigger1-1.7>
-        1, "no such trigger: TR1"
+        1, "Trigger 'TR1' doesn't exist"
         -- </trigger1-1.7>
     })
 
diff --git a/test/sql/persistency.result b/test/sql/persistency.result
index d85d7cc..8f89039 100644
--- a/test/sql/persistency.result
+++ b/test/sql/persistency.result
@@ -190,7 +190,7 @@ box.sql.execute("DROP TRIGGER tfoobar")
 -- Should error
 box.sql.execute("DROP TRIGGER tfoobar")
 ---
-- error: 'no such trigger: TFOOBAR'
+- error: Trigger 'TFOOBAR' doesn't exist
 ...
 -- Should be empty
 box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"")
-- 
2.7.4