Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH v4 6/8] sql: refactor AST trigger object name
       [not found] <cover.1530029141.git.kshcherbatov@tarantool.org>
@ 2018-06-26 16:13 ` Kirill Shcherbatov
  2018-06-27 15:57   ` [tarantool-patches] " n.pettik
  2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 8/8] sql: remove global sql_trigger hash Kirill Shcherbatov
  1 sibling, 1 reply; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-26 16:13 UTC (permalink / raw)
  To: tarantool-patches; +Cc: n.pettik, Kirill Shcherbatov

Part of #3273.
---
 src/box/alter.cc        |  14 +-
 src/box/space.h         |   2 +-
 src/box/sql.h           |  17 +-
 src/box/sql/build.c     |   6 +-
 src/box/sql/callback.c  |   3 +-
 src/box/sql/delete.c    |  24 +-
 src/box/sql/expr.c      |   2 -
 src/box/sql/fkey.c      | 162 +++++------
 src/box/sql/insert.c    |  37 ++-
 src/box/sql/parse.y     |   9 +-
 src/box/sql/pragma.c    |   2 -
 src/box/sql/pragma.h    |   6 +-
 src/box/sql/resolve.c   |   2 -
 src/box/sql/select.c    |   2 -
 src/box/sql/sqliteInt.h | 253 ++++++++++++++---
 src/box/sql/status.c    |   2 +-
 src/box/sql/tokenize.c  |   4 +-
 src/box/sql/treeview.c  |   2 -
 src/box/sql/trigger.c   | 709 +++++++++++++++++++++---------------------------
 src/box/sql/update.c    |  32 +--
 src/box/sql/vdbe.c      |  10 +-
 src/box/sql/vdbe.h      |   2 -
 22 files changed, 678 insertions(+), 624 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 1c0e889..449b4b1 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -554,7 +554,7 @@ space_swap_triggers(struct space *new_space, struct space *old_space)
 	rlist_swap(&new_space->on_replace, &old_space->on_replace);
 	rlist_swap(&new_space->on_stmt_begin, &old_space->on_stmt_begin);
 	/** Swap SQL Triggers pointer. */
-	struct Trigger *new_value = new_space->sql_triggers;
+	struct sql_trigger *new_value = new_space->sql_triggers;
 	new_space->sql_triggers = old_space->sql_triggers;
 	old_space->sql_triggers = new_value;
 }
@@ -3257,8 +3257,8 @@ 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;
+	struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
+	struct sql_trigger *new_trigger;
 
 	if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
 		/* Rollback DELETE trigger. */
@@ -3294,7 +3294,7 @@ on_replace_trigger_rollback(struct trigger *trigger, void *event)
 static void
 on_replace_trigger_commit(struct trigger *trigger, void * /* event */)
 {
-	struct Trigger *old_trigger = (struct Trigger *)trigger->data;
+	struct sql_trigger *old_trigger = (struct sql_trigger *)trigger->data;
 	sql_trigger_delete(sql_get(), old_trigger);
 }
 
@@ -3328,7 +3328,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		memcpy(trigger_name, trigger_name_src, trigger_name_len);
 		trigger_name[trigger_name_len] = 0;
 
-		struct Trigger *old_trigger;
+		struct sql_trigger *old_trigger;
 		int rc = sql_trigger_replace(sql_get(), trigger_name, NULL,
 					     &old_trigger);
 		(void)rc;
@@ -3351,7 +3351,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 		struct space_opts opts;
 		struct region *region = &fiber()->gc;
 		space_opts_decode(&opts, space_opts, region);
-		struct Trigger *new_trigger =
+		struct sql_trigger *new_trigger =
 			sql_trigger_compile(sql_get(), opts.sql);
 		if (new_trigger == NULL)
 			diag_raise();
@@ -3377,7 +3377,7 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 				  "resolved on AST building from SQL");
 		}
 
-		struct Trigger *old_trigger;
+		struct sql_trigger *old_trigger;
 		if (sql_trigger_replace(sql_get(), trigger_name, new_trigger,
 					&old_trigger) != 0)
 			diag_raise();
diff --git a/src/box/space.h b/src/box/space.h
index 64aa8c7..7da2ee5 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -147,7 +147,7 @@ struct space {
 	/** Triggers fired before space statement */
 	struct rlist on_stmt_begin;
 	/** SQL Trigger list. */
-	struct Trigger *sql_triggers;
+	struct sql_trigger *sql_triggers;
 	/**
 	 * The number of *enabled* indexes in the space.
 	 *
diff --git a/src/box/sql.h b/src/box/sql.h
index 2572a15..f483921 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -66,7 +66,7 @@ struct Expr;
 struct Parse;
 struct Select;
 struct Table;
-struct Trigger;
+struct sql_trigger;
 
 /**
  * Perform parsing of provided expression. This is done by
@@ -100,9 +100,9 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt);
  * @param sql request to parse.
  *
  * @retval NULL on error
- * @retval not NULL Trigger AST pointer on success.
+ * @retval not NULL sql_trigger AST pointer on success.
  */
-struct Trigger *
+struct sql_trigger *
 sql_trigger_compile(struct sqlite3 *db, const char *sql);
 
 /**
@@ -111,7 +111,7 @@ sql_trigger_compile(struct sqlite3 *db, const char *sql);
  * @param trigger AST object.
  */
 void
-sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
+sql_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger);
 
 /**
  * Get server triggers list by space_id.
@@ -119,7 +119,7 @@ sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger);
  *
  * @retval trigger AST list.
  */
-struct Trigger *
+struct sql_trigger *
 space_trigger_list(uint32_t space_id);
 
 /**
@@ -134,7 +134,8 @@ space_trigger_list(uint32_t space_id);
  */
 int
 sql_trigger_replace(struct sqlite3 *db, const char *name,
-		    struct Trigger *trigger, struct Trigger **old_trigger);
+		    struct sql_trigger *trigger,
+		    struct sql_trigger **old_trigger);
 
 /**
  * Get trigger name by trigger AST object.
@@ -142,7 +143,7 @@ sql_trigger_replace(struct sqlite3 *db, const char *name,
  * @return trigger name string.
  */
 const char *
-sql_trigger_name(struct Trigger *trigger);
+sql_trigger_name(struct sql_trigger *trigger);
 
 /**
  * Get space_id of the space that trigger has been built for.
@@ -150,7 +151,7 @@ sql_trigger_name(struct Trigger *trigger);
  * @return space identifier.
  */
 uint32_t
-sql_trigger_space_id(struct Trigger *trigger);
+sql_trigger_space_id(struct sql_trigger *trigger);
 
 /**
  * Store duplicate of a parsed expression into @a parser.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 2b4e6c7..b88b8fe 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2119,10 +2119,10 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space,
 	 * accounted in DELETE from _space below.
 	 */
 	parse_context->nested++;
-	struct Trigger *trigger = space->sql_triggers;
+	struct sql_trigger *trigger = space->sql_triggers;
 	while (trigger != NULL) {
-		sqlite3DropTriggerPtr(parse_context, trigger);
-		trigger = trigger->pNext;
+		vdbe_code_drop_trigger_ptr(parse_context, trigger);
+		trigger = trigger->next;
 	}
 	parse_context->nested--;
 	/*
diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
index bd8db99..c3c38cb 100644
--- a/src/box/sql/callback.c
+++ b/src/box/sql/callback.c
@@ -292,7 +292,8 @@ sqlite3SchemaClear(sqlite3 * db)
 	sqlite3HashInit(&pSchema->trigHash);
 	for (pElem = sqliteHashFirst(&temp2); pElem != NULL;
 	     pElem = sqliteHashNext(pElem))
-		sql_trigger_delete(NULL, (Trigger *) sqliteHashData(pElem));
+		sql_trigger_delete(NULL,
+				   (struct sql_trigger *)sqliteHashData(pElem));
 	sqlite3HashClear(&temp2);
 	sqlite3HashInit(&pSchema->tblHash);
 	for (pElem = sqliteHashFirst(&temp1); pElem;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 8b13f60..818bbbd 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -93,7 +93,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 	/* Figure out if we have any triggers and if the table
 	 * being deleted from is a view.
 	 */
-	struct Trigger *trigger_list = NULL;
+	struct sql_trigger *trigger_list = NULL;
 	/* True if there are triggers or FKs or subqueries in the
 	 * WHERE clause.
 	 */
@@ -124,8 +124,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
 		space = space_by_id(space_id);
 		assert(space != NULL);
-		trigger_list =sqlite3TriggersExist(table, TK_DELETE,
-						   NULL, NULL);
+		trigger_list = sql_triggers_exist(table, TK_DELETE, NULL, NULL);
 		is_complex = trigger_list != NULL ||
 			     sqlite3FkRequired(table, NULL);
 	}
@@ -424,8 +423,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 
 void
 sql_generate_row_delete(struct Parse *parse, struct Table *table,
-			struct Trigger *trigger_list, int cursor, int reg_pk,
-			short npk, bool need_update_count,
+			struct sql_trigger *trigger_list, int cursor,
+			int reg_pk, short npk, bool need_update_count,
 			enum on_conflict_action onconf, u8 mode,
 			int idx_noseek)
 {
@@ -457,9 +456,10 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 		/* Mask of OLD.* columns in use */
 		/* TODO: Could use temporary registers here. */
 		uint32_t mask =
-		    sqlite3TriggerColmask(parse, trigger_list, 0, 0,
-					  TRIGGER_BEFORE | TRIGGER_AFTER, table,
-					  onconf);
+			sql_trigger_colmask(parse, trigger_list, 0, 0,
+					    TRIGGER_BEFORE | TRIGGER_AFTER,
+					    table,
+					    onconf);
 		mask |= sqlite3FkOldmask(parse, table);
 		first_old_reg = parse->nMem + 1;
 		parse->nMem += (1 + (int)table->def->field_count);
@@ -483,7 +483,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 
 		/* Invoke BEFORE DELETE trigger programs. */
 		int addr_start = sqlite3VdbeCurrentAddr(v);
-		sqlite3CodeRowTrigger(parse, trigger_list, TK_DELETE, NULL,
+		vdbe_code_row_trigger(parse, trigger_list, TK_DELETE, NULL,
 				      TRIGGER_BEFORE, table, first_old_reg,
 				      onconf, label);
 
@@ -537,9 +537,9 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 		sqlite3FkActions(parse, table, 0, first_old_reg, 0);
 
 		/* Invoke AFTER DELETE trigger programs. */
-		sqlite3CodeRowTrigger(parse, trigger_list,
-				      TK_DELETE, 0, TRIGGER_AFTER, table,
-				      first_old_reg, onconf, label);
+		vdbe_code_row_trigger(parse, trigger_list, TK_DELETE, 0,
+				      TRIGGER_AFTER, table, first_old_reg,
+				      onconf, label);
 	}
 
 	/* Jump here if the row had already been deleted before
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 59e7cb4..70e134f 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4289,7 +4289,6 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			sqlite3VdbeResolveLabel(v, endLabel);
 			break;
 		}
-#ifndef SQLITE_OMIT_TRIGGER
 	case TK_RAISE:{
 			assert(pExpr->affinity == ON_CONFLICT_ACTION_ROLLBACK
 			       || pExpr->affinity == ON_CONFLICT_ACTION_ABORT
@@ -4319,7 +4318,6 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 
 			break;
 		}
-#endif
 	}
 	sqlite3ReleaseTempReg(pParse, regFree1);
 	sqlite3ReleaseTempReg(pParse, regFree2);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index ce63ff0..121831b 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -40,7 +40,6 @@
 #include "tarantoolInt.h"
 
 #ifndef SQLITE_OMIT_FOREIGN_KEY
-#ifndef SQLITE_OMIT_TRIGGER
 
 /*
  * Deferred and Immediate FKs
@@ -730,25 +729,29 @@ sqlite3FkReferences(Table * pTab)
 					pTab->def->name);
 }
 
-/*
+/**
  * The second argument is a Trigger structure allocated by the
- * fkActionTrigger() routine. This function deletes the Trigger structure
- * and all of its sub-components.
+ * fkActionTrigger() routine. This function deletes the sql_trigger
+ * structure and all of its sub-components.
  *
- * The Trigger structure or any of its sub-components may be allocated from
- * the lookaside buffer belonging to database handle dbMem.
+ * The Trigger structure or any of its sub-components may be
+ * allocated from the lookaside buffer belonging to database
+ * handle dbMem.
+ *
+ * @param db Database connection.
+ * @param trigger AST object.
  */
 static void
-fkTriggerDelete(sqlite3 * dbMem, Trigger * p)
+sql_fk_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
 {
-	if (p) {
-		TriggerStep *pStep = p->step_list;
-		sql_expr_delete(dbMem, pStep->pWhere, false);
-		sql_expr_list_delete(dbMem, pStep->pExprList);
-		sql_select_delete(dbMem, pStep->pSelect);
-		sql_expr_delete(dbMem, p->pWhen, false);
-		sqlite3DbFree(dbMem, p);
-	}
+	if (trigger == NULL)
+		return;
+	struct TriggerStep *trigger_step = trigger->step_list;
+	sql_expr_delete(db, trigger_step->pWhere, false);
+	sql_expr_list_delete(db, trigger_step->pExprList);
+	sql_select_delete(db, trigger_step->pSelect);
+	sql_expr_delete(db, trigger->pWhen, false);
+	sqlite3DbFree(db, trigger);
 }
 
 /**
@@ -858,15 +861,13 @@ static int
 isSetNullAction(Parse * pParse, FKey * pFKey)
 {
 	Parse *pTop = sqlite3ParseToplevel(pParse);
-	if (pTop->pTriggerPrg) {
-		Trigger *p = pTop->pTriggerPrg->pTrigger;
-		if ((p == pFKey->apTrigger[0]
-		     && pFKey->aAction[0] == OE_SetNull)
-		    || (p == pFKey->apTrigger[1]
-			&& pFKey->aAction[1] == OE_SetNull)
-		    ) {
+	if (pTop->pTriggerPrg != NULL) {
+		struct sql_trigger *trigger = pTop->pTriggerPrg->trigger;
+		if ((trigger == pFKey->apTrigger[0] &&
+		     pFKey->aAction[0] == OE_SetNull) ||
+		    (trigger == pFKey->apTrigger[1]
+			&& pFKey->aAction[1] == OE_SetNull))
 			return 1;
-		}
 	}
 	return 0;
 }
@@ -1175,21 +1176,24 @@ sqlite3FkRequired(Table * pTab,	/* Table being modified */
 	return 0;
 }
 
-/*
- * This function is called when an UPDATE or DELETE operation is being
- * compiled on table pTab, which is the parent table of foreign-key pFKey.
- * If the current operation is an UPDATE, then the pChanges parameter is
- * passed a pointer to the list of columns being modified. If it is a
- * DELETE, pChanges is passed a NULL pointer.
- *
- * It returns a pointer to a Trigger structure containing a trigger
- * equivalent to the ON UPDATE or ON DELETE action specified by pFKey.
- * If the action is "NO ACTION" or "RESTRICT", then a NULL pointer is
- * returned (these actions require no special handling by the triggers
- * sub-system, code for them is created by fkScanChildren()).
- *
- * For example, if pFKey is the foreign key and pTab is table "p" in
- * the following schema:
+/**
+ * This function is called when an UPDATE or DELETE operation is
+ * being compiled on table pTab, which is the parent table of
+ * foreign-key pFKey.
+ * If the current operation is an UPDATE, then the pChanges
+ * parameter is passed a pointer to the list of columns being
+ * modified. If it is a DELETE, pChanges is passed a NULL pointer.
+ *
+ * It returns a pointer to a sql_trigger structure containing a
+ * trigger equivalent to the ON UPDATE or ON DELETE action
+ * specified by pFKey.
+ * If the action is "NO ACTION" or "RESTRICT", then a NULL pointer
+ * is returned (these actions require no special handling by the
+ * triggers sub-system, code for them is created by
+ * fkScanChildren()).
+ *
+ * For example, if pFKey is the foreign key and pTab is table "p"
+ * in the following schema:
  *
  *   CREATE TABLE p(pk PRIMARY KEY);
  *   CREATE TABLE c(ck REFERENCES p ON DELETE CASCADE);
@@ -1200,20 +1204,25 @@ sqlite3FkRequired(Table * pTab,	/* Table being modified */
  *     DELETE FROM c WHERE ck = old.pk;
  *   END;
  *
- * The returned pointer is cached as part of the foreign key object. It
- * is eventually freed along with the rest of the foreign key object by
- * sqlite3FkDelete().
+ * The returned pointer is cached as part of the foreign key
+ * object. It is eventually freed along with the rest of the
+ * foreign key object by sqlite3FkDelete().
+ *
+ * @param pParse Parse context.
+ * @param pTab Table being updated or deleted from.
+ * @param pFKey Foreign key to get action for.
+ * @param pChanges Change-list for UPDATE, NULL for DELETE.
+ *
+ * @retval not NULL on success.
+ * @retval NULL on failure.
  */
-static Trigger *
-fkActionTrigger(Parse * pParse,	/* Parse context */
-		Table * pTab,	/* Table being updated or deleted from */
-		FKey * pFKey,	/* Foreign key to get action for */
-		ExprList * pChanges	/* Change-list for UPDATE, NULL for DELETE */
-    )
+static struct sql_trigger *
+fkActionTrigger(Parse * pParse, Table * pTab, FKey * pFKey, ExprList * pChanges)
 {
 	sqlite3 *db = pParse->db;	/* Database handle */
 	int action;		/* One of OE_None, OE_Cascade etc. */
-	Trigger *pTrigger;	/* Trigger definition to return */
+	/* Trigger definition to return. */
+	struct sql_trigger *trigger;
 	int iAction = (pChanges != 0);	/* 1 for UPDATE, 0 for DELETE */
 	struct session *user_session = current_session();
 
@@ -1222,9 +1231,9 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 	    && (user_session->sql_flags & SQLITE_DeferFKs)) {
 		return 0;
 	}
-	pTrigger = pFKey->apTrigger[iAction];
+	trigger = pFKey->apTrigger[iAction];
 
-	if (action != ON_CONFLICT_ACTION_NONE && !pTrigger) {
+	if (action != ON_CONFLICT_ACTION_NONE && trigger == NULL) {
 		char const *zFrom;	/* Name of child table */
 		int nFrom;	/* Length in bytes of zFrom */
 		Index *pIdx = 0;	/* Parent key index for this FK */
@@ -1379,13 +1388,13 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 		/* Disable lookaside memory allocation */
 		db->lookaside.bDisable++;
 
-		pTrigger = (Trigger *) sqlite3DbMallocZero(db, sizeof(Trigger) +	/* struct Trigger */
-							   sizeof(TriggerStep) +	/* Single step in trigger program */
-							   nFrom + 1	/* Space for pStep->zTarget */
-		    );
-		if (pTrigger) {
-			pStep = pTrigger->step_list =
-			    (TriggerStep *) & pTrigger[1];
+		size_t trigger_size = sizeof(struct sql_trigger) +
+				      sizeof(TriggerStep) + nFrom + 1;
+		trigger =
+			(struct sql_trigger *)sqlite3DbMallocZero(db,
+								  trigger_size);
+		if (trigger != NULL) {
+			pStep = trigger->step_list = (TriggerStep *)&trigger[1];
 			pStep->zTarget = (char *)&pStep[1];
 			memcpy((char *)pStep->zTarget, zFrom, nFrom);
 
@@ -1397,7 +1406,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 			    sqlite3SelectDup(db, pSelect, EXPRDUP_REDUCE);
 			if (pWhen) {
 				pWhen = sqlite3PExpr(pParse, TK_NOT, pWhen, 0);
-				pTrigger->pWhen =
+				trigger->pWhen =
 				    sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE);
 			}
 		}
@@ -1410,7 +1419,7 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 		sql_expr_list_delete(db, pList);
 		sql_select_delete(db, pSelect);
 		if (db->mallocFailed == 1) {
-			fkTriggerDelete(db, pTrigger);
+			sql_fk_trigger_delete(db, trigger);
 			return 0;
 		}
 		assert(pStep != 0);
@@ -1428,12 +1437,12 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
 		default:
 			pStep->op = TK_UPDATE;
 		}
-		pStep->pTrig = pTrigger;
-		pFKey->apTrigger[iAction] = pTrigger;
-		pTrigger->op = (pChanges ? TK_UPDATE : TK_DELETE);
+		pStep->trigger = trigger;
+		pFKey->apTrigger[iAction] = trigger;
+		trigger->op = pChanges ? TK_UPDATE : TK_DELETE;
 	}
 
-	return pTrigger;
+	return trigger;
 }
 
 /*
@@ -1460,23 +1469,20 @@ sqlite3FkActions(Parse * pParse,	/* Parse context */
 		     pFKey = pFKey->pNextTo) {
 			if (aChange == 0
 			    || fkParentIsModified(pTab, pFKey, aChange)) {
-				Trigger *pAct =
-				    fkActionTrigger(pParse, pTab, pFKey,
-						    pChanges);
-				if (pAct) {
-					sqlite3CodeRowTriggerDirect(pParse,
-								    pAct, pTab,
-								    regOld,
-								    ON_CONFLICT_ACTION_ABORT,
-								    0);
-				}
+				struct sql_trigger *pAct =
+					fkActionTrigger(pParse, pTab, pFKey,
+							pChanges);
+				if (pAct == NULL)
+					continue;
+				vdbe_code_row_trigger_direct(pParse, pAct, pTab,
+							     regOld,
+							     ON_CONFLICT_ACTION_ABORT,
+							     0);
 			}
 		}
 	}
 }
 
-#endif				/* ifndef SQLITE_OMIT_TRIGGER */
-
 /*
  * Free all memory associated with foreign key definitions attached to
  * table pTab. Remove the deleted foreign keys from the Schema.fkeyHash
@@ -1511,10 +1517,8 @@ sqlite3FkDelete(sqlite3 * db, Table * pTab)
 		assert(pFKey->isDeferred == 0 || pFKey->isDeferred == 1);
 
 		/* Delete any triggers created to implement actions for this FK. */
-#ifndef SQLITE_OMIT_TRIGGER
-		fkTriggerDelete(db, pFKey->apTrigger[0]);
-		fkTriggerDelete(db, pFKey->apTrigger[1]);
-#endif
+		sql_fk_trigger_delete(db, pFKey->apTrigger[0]);
+		sql_fk_trigger_delete(db, pFKey->apTrigger[1]);
 
 		pNext = pFKey->pNextFrom;
 		sqlite3DbFree(db, pFKey);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index db8165a..dac6965 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -357,7 +357,8 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	int regData;		/* register holding first column to insert */
 	int *aRegIdx = 0;	/* One register allocated to each index */
 	uint32_t space_id = 0;
-	Trigger *pTrigger;	/* List of triggers on pTab, if required */
+	/* List of triggers on pTab, if required. */
+	struct sql_trigger *trigger;
 	int tmask;		/* Mask of trigger times */
 
 	db = pParse->db;
@@ -393,9 +394,10 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	/* Figure out if we have any triggers and if the table being
 	 * inserted into is a view
 	 */
-	pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask);
+	trigger = sql_triggers_exist(pTab, TK_INSERT, NULL, &tmask);
 	bool is_view = pTab->def->opts.is_view;
-	assert((pTrigger && tmask) || (pTrigger == 0 && tmask == 0));
+	assert((trigger != NULL && tmask != 0) ||
+	       (trigger == NULL && tmask == 0));
 
 	/* If pTab is really a view, make sure it has been initialized.
 	 * ViewGetColumnNames() is a no-op if pTab is not a view.
@@ -419,7 +421,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		goto insert_cleanup;
 	if (pParse->nested == 0)
 		sqlite3VdbeCountChanges(v);
-	sql_set_multi_write(pParse, pSelect || pTrigger);
+	sql_set_multi_write(pParse, pSelect != NULL || trigger != NULL);
 
 #ifndef SQLITE_OMIT_XFER_OPT
 	/* If the statement is of the form
@@ -432,7 +434,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	 * This is the 2nd template.
 	 */
 	if (pColumn == 0 && xferOptimization(pParse, pTab, pSelect, on_error)) {
-		assert(!pTrigger);
+		assert(trigger == NULL);
 		assert(pList == 0);
 		goto insert_end;
 	}
@@ -536,9 +538,8 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		 * of the tables being read by the SELECT statement.  Also use a
 		 * temp table in the case of row triggers.
 		 */
-		if (pTrigger || readsTable(pParse, pTab)) {
+		if (trigger != NULL || readsTable(pParse, pTab))
 			useTempTable = 1;
-		}
 
 		if (useTempTable) {
 			/* Invoke the coroutine to extract information from the SELECT
@@ -731,7 +732,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		}
 
 		/* Fire BEFORE or INSTEAD OF triggers */
-		sqlite3CodeRowTrigger(pParse, pTrigger, TK_INSERT, 0,
+		vdbe_code_row_trigger(pParse, trigger, TK_INSERT, 0,
 				      TRIGGER_BEFORE, pTab,
 				      regCols - def->field_count - 1, on_error,
 				      endOfLoop);
@@ -880,9 +881,9 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 		sqlite3VdbeAddOp2(v, OP_AddImm, regRowCount, 1);
 	}
 
-	if (pTrigger) {
+	if (trigger != NULL) {
 		/* Code AFTER triggers */
-		sqlite3CodeRowTrigger(pParse, pTrigger, TK_INSERT, 0,
+		vdbe_code_row_trigger(pParse, trigger, TK_INSERT, 0,
 				      TRIGGER_AFTER, pTab,
 				      regData - 2 - def->field_count, on_error,
 				      endOfLoop);
@@ -1360,9 +1361,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 		bool no_delete_triggers =
 			(0 == (user_session->sql_flags &
 			       SQLITE_RecTriggers) ||
-			 0 == sqlite3TriggersExist(pTab,
-						   TK_DELETE,
-						   0, 0));
+			 sql_triggers_exist(pTab, TK_DELETE, NULL, NULL) ==
+			 NULL);
 		bool no_foreign_keys =
 			(0 == (user_session->sql_flags &
 			       SQLITE_ForeignKeys) ||
@@ -1473,15 +1473,14 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			sqlite3VdbeGoto(v, ignoreDest);
 			break;
 		default: {
-			Trigger *pTrigger = NULL;
+			struct sql_trigger *trigger = NULL;
 			assert(on_error == ON_CONFLICT_ACTION_REPLACE);
 			sql_set_multi_write(pParse, true);
-			if (user_session->
-			    sql_flags & SQLITE_RecTriggers) {
-				pTrigger = sqlite3TriggersExist(pTab, TK_DELETE,
-								NULL, NULL);
+			if (user_session->sql_flags & SQLITE_RecTriggers) {
+				trigger = sql_triggers_exist(pTab, TK_DELETE,
+							      NULL, NULL);
 			}
-			sql_generate_row_delete(pParse, pTab, pTrigger,
+			sql_generate_row_delete(pParse, pTab, trigger,
 						iDataCur, regR, nPkField, false,
 						ON_CONFLICT_ACTION_REPLACE,
 						pIdx == pPk ? ONEPASS_SINGLE :
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ccd9d02..91bdc6e 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1300,20 +1300,18 @@ plus_num(A) ::= number(A).
 minus_num(A) ::= MINUS number(X).     {A = X;}
 //////////////////////////// The CREATE TRIGGER command /////////////////////
 
-%ifndef SQLITE_OMIT_TRIGGER
-
 cmd ::= createkw trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). {
   Token all;
   all.z = A.z;
   all.n = (int)(Z.z - A.z) + Z.n;
   pParse->initiateTTrans = false;
-  sqlite3FinishTrigger(pParse, S, &all);
+  sql_trigger_finish(pParse, S, &all);
 }
 
 trigger_decl(A) ::= TRIGGER ifnotexists(NOERR) nm(B)
                     trigger_time(C) trigger_event(D)
                     ON fullname(E) foreach_clause when_clause(G). {
-  sqlite3BeginTrigger(pParse, &B, C, D.a, D.b, E, G, NOERR);
+  sql_trigger_begin(pParse, &B, C, D.a, D.b, E, G, NOERR);
   A = B; /*A-overwrites-T*/
 }
 
@@ -1414,7 +1412,6 @@ expr(A) ::= RAISE(X) LP raisetype(T) COMMA STRING(Z) RP(Y).  {
     A.pExpr->affinity = (char)T;
   }
 }
-%endif  !SQLITE_OMIT_TRIGGER
 
 %type raisetype {int}
 raisetype(A) ::= ROLLBACK.  {A = ON_CONFLICT_ACTION_ROLLBACK;}
@@ -1423,11 +1420,9 @@ raisetype(A) ::= FAIL.      {A = ON_CONFLICT_ACTION_FAIL;}
 
 
 ////////////////////////  DROP TRIGGER statement //////////////////////////////
-%ifndef SQLITE_OMIT_TRIGGER
 cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
   sqlite3DropTrigger(pParse,X,NOERR);
 }
-%endif  !SQLITE_OMIT_TRIGGER
 
 ////////////////////////// REINDEX collation //////////////////////////////////
 /* gh-2174: Commended until REINDEX is implemented in scope of gh-3195 */
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 5fb29c7..be6a01c 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -583,7 +583,6 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 #endif				/* !defined(SQLITE_OMIT_FOREIGN_KEY) */
 
 #ifndef SQLITE_OMIT_FOREIGN_KEY
-#ifndef SQLITE_OMIT_TRIGGER
 	case PragTyp_FOREIGN_KEY_CHECK:{
 			FKey *pFK;	/* A foreign key constraint */
 			Table *pTab;	/* Child table contain "REFERENCES"
@@ -755,7 +754,6 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 			}
 			break;
 		}
-#endif				/* !defined(SQLITE_OMIT_TRIGGER) */
 #endif				/* !defined(SQLITE_OMIT_FOREIGN_KEY) */
 
 #ifndef NDEBUG
diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h
index f966018..06b7eea 100644
--- a/src/box/sql/pragma.h
+++ b/src/box/sql/pragma.h
@@ -126,7 +126,7 @@ static const PragmaName aPragmaName[] = {
 	 /* iArg:      */ SQLITE_CountRows},
 #endif
 #if !defined(SQLITE_OMIT_FLAG_PRAGMAS)
-#if !defined(SQLITE_OMIT_FOREIGN_KEY) && !defined(SQLITE_OMIT_TRIGGER)
+#if !defined(SQLITE_OMIT_FOREIGN_KEY)
 	{ /* zName:     */ "defer_foreign_keys",
 	 /* ePragTyp:  */ PragTyp_FLAG,
 	 /* ePragFlg:  */ PragFlg_Result0 | PragFlg_NoColumns1,
@@ -134,7 +134,7 @@ static const PragmaName aPragmaName[] = {
 	 /* iArg:      */ SQLITE_DeferFKs},
 #endif
 #endif
-#if !defined(SQLITE_OMIT_FOREIGN_KEY) && !defined(SQLITE_OMIT_TRIGGER)
+#if !defined(SQLITE_OMIT_FOREIGN_KEY)
 	{ /* zName:     */ "foreign_key_check",
 	 /* ePragTyp:  */ PragTyp_FOREIGN_KEY_CHECK,
 	 /* ePragFlg:  */ PragFlg_NeedSchema,
@@ -150,7 +150,7 @@ static const PragmaName aPragmaName[] = {
 	 /* iArg:      */ 0},
 #endif
 #if !defined(SQLITE_OMIT_FLAG_PRAGMAS)
-#if !defined(SQLITE_OMIT_FOREIGN_KEY) && !defined(SQLITE_OMIT_TRIGGER)
+#if !defined(SQLITE_OMIT_FOREIGN_KEY)
 	{ /* zName:     */ "foreign_keys",
 	 /* ePragTyp:  */ PragTyp_FLAG,
 	 /* ePragFlg:  */ PragFlg_Result0 | PragFlg_NoColumns1,
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 23e1618..10c717f 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -309,7 +309,6 @@ lookupName(Parse * pParse,	/* The parsing context */
 			}
 		}
 		/* if( pSrcList ) */
-#ifndef SQLITE_OMIT_TRIGGER
 		/* If we have not already resolved the name, then maybe
 		 * it is a new.* or old.* trigger argument reference
 		 */
@@ -369,7 +368,6 @@ lookupName(Parse * pParse,	/* The parsing context */
 				}
 			}
 		}
-#endif				/* !defined(SQLITE_OMIT_TRIGGER) */
 
 		/*
 		 * If the input is of the form Z (not Y.Z or X.Y.Z) then the name Z
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 368bcd6..4e61ec1 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1270,7 +1270,6 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 		}
 #endif				/* SQLITE_OMIT_CTE */
 
-#if !defined(SQLITE_OMIT_TRIGGER)
 		/* Discard the results.  This is used for SELECT statements inside
 		 * the body of a TRIGGER.  The purpose of such selects is to call
 		 * user-defined functions that have side effects.  We do not care
@@ -1280,7 +1279,6 @@ selectInnerLoop(Parse * pParse,		/* The parser context */
 			assert(eDest == SRT_Discard);
 			break;
 		}
-#endif
 	}
 
 	/* Jump to the end of the loop if the LIMIT is reached.  Except, if
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index d5e3263..aa7d48e 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1493,7 +1493,6 @@ typedef struct StrAccum StrAccum;
 typedef struct Table Table;
 typedef struct Token Token;
 typedef struct TreeView TreeView;
-typedef struct Trigger Trigger;
 typedef struct TriggerPrg TriggerPrg;
 typedef struct TriggerStep TriggerStep;
 typedef struct UnpackedRecord UnpackedRecord;
@@ -1994,7 +1993,8 @@ struct FKey {
 	/* EV: R-30323-21917 */
 	u8 isDeferred;		/* True if constraint checking is deferred till COMMIT */
 	u8 aAction[2];		/* ON DELETE and ON UPDATE actions, respectively */
-	Trigger *apTrigger[2];	/* Triggers for aAction[] actions */
+	/** Triggers for aAction[] actions. */
+	struct sql_trigger *apTrigger[2];
 	struct sColMap {	/* Mapping of columns in pFrom to columns in zTo */
 		int iFrom;	/* Index of column in pFrom */
 		char *zCol;	/* Name of column in zTo.  If NULL use PRIMARY KEY */
@@ -2842,7 +2842,8 @@ struct SelectDest {
  * a mask of new.* columns used by the program.
  */
 struct TriggerPrg {
-	Trigger *pTrigger;	/* Trigger this program was coded from */
+	/** Trigger this program was coded from. */
+	struct sql_trigger *trigger;
 	TriggerPrg *pNext;	/* Next entry in Parse.pTriggerPrg list */
 	SubProgram *pProgram;	/* Program implementing pTrigger/orconf */
 	int orconf;		/* Default ON CONFLICT policy */
@@ -2965,7 +2966,7 @@ struct Parse {
 	union {
 		struct Expr *expr;
 		struct Select *select;
-		struct Trigger *trigger;
+		struct sql_trigger *trigger;
 	} parsed_ast;
 };
 
@@ -3018,21 +3019,23 @@ struct Parse {
 					 */
 
 /*
- * Each trigger present in the database schema is stored as an instance of
- * struct Trigger.
+ * Each trigger present in the database schema is stored as an
+ * instance of struct sql_trigger.
  *
  * Pointers to instances of struct Trigger are stored in two ways.
- * 1. In the "trigHash" hash table (part of the sqlite3* that represents the
- *    database). This allows Trigger structures to be retrieved by name.
- * 2. All triggers associated with a single table form a linked list, using the
- *    pNext member of struct Trigger. A pointer to the first element of the
- *    linked list is stored as the "pTrigger" member of the associated
- *    struct Table.
- *
- * The "step_list" member points to the first element of a linked list
- * containing the SQL statements specified as the trigger program.
- */
-struct Trigger {
+ * 1. In the "trigHash" hash table (part of the sqlite3* that
+ *    represents the database). This allows Trigger structures to
+ *    be retrieved by name.
+ * 2. All triggers associated with a single table form a linked
+ *    list, using the next member of struct sql_trigger. A pointer
+ *    to the first element of the linked list is stored as the
+ *    "pTrigger" member of the associated struct Table.
+ *
+ * The "step_list" member points to the first element of a linked
+ * list containing the SQL statements specified as the trigger
+ * program.
+ */
+struct sql_trigger {
 	char *zName;		/* The name of the trigger                        */
 	/** The ID of space the trigger refers to. */
 	uint32_t space_id;
@@ -3042,7 +3045,8 @@ struct Trigger {
 	IdList *pColumns;	/* If this is an UPDATE OF <column-list> trigger,
 				   the <column-list> is stored here */
 	TriggerStep *step_list;	/* Link list of trigger program steps             */
-	Trigger *pNext;		/* Next trigger associated with the table */
+	/** Next trigger associated with the table. */
+	struct sql_trigger *next;
 };
 
 /*
@@ -3096,7 +3100,8 @@ struct Trigger {
 struct TriggerStep {
 	u8 op;			/* One of TK_DELETE, TK_UPDATE, TK_INSERT, TK_SELECT */
 	u8 orconf;		/* ON_CONFLICT_ACTION_ROLLBACK etc. */
-	Trigger *pTrig;		/* The trigger that this step is a part of */
+	/** The trigger that this step is a part of */
+	struct sql_trigger *trigger;
 	Select *pSelect;	/* SELECT statement or RHS of INSERT INTO SELECT ... */
 	char *zTarget;		/* Target table for DELETE, UPDATE, INSERT */
 	Expr *pWhere;		/* The WHERE clause for DELETE or UPDATE steps */
@@ -3933,8 +3938,8 @@ int sqlite3ExprNeedsNoAffinityChange(const Expr *, char);
  */
 void
 sql_generate_row_delete(struct Parse *parse, struct Table *table,
-			struct Trigger *trigger_list, int cursor, int reg_pk,
-			short npk, bool need_update_count,
+			struct sql_trigger *trigger_list, int cursor,
+			int reg_pk, short npk, bool need_update_count,
 			enum on_conflict_action onconf, u8 mode,
 			int idx_noseek);
 
@@ -4064,17 +4069,143 @@ void
 sql_materialize_view(struct Parse *parse, const char *name, struct Expr *where,
 		     int cursor);
 
-#ifndef SQLITE_OMIT_TRIGGER
-void sqlite3BeginTrigger(Parse *, Token *, int, int, IdList *, SrcList *,
-			 Expr *, int);
-void sqlite3FinishTrigger(Parse *, TriggerStep *, Token *);
+/**
+ * This is called by the parser when it sees a CREATE TRIGGER
+ * statement up to the point of the BEGIN before the trigger
+ * actions.  A sql_trigger structure is generated based on the
+ * information available and stored in parse->parsed_ast.trigger.
+ * After the trigger actions have been parsed, the
+ * sql_trigger_finish() function is called to complete the trigger
+ * construction process.
+ *
+ * @param parse The parse context of the CREATE TRIGGER statement.
+ * @param name The name of the trigger.
+ * @param tr_tm One of TK_BEFORE, TK_AFTER, TK_INSTEAD.
+ * @param op One of TK_INSERT, TK_UPDATE, TK_DELETE.
+ * @param columns column list if this is an UPDATE OF trigger.
+ * @param table The name of the table/view the trigger applies to.
+ * @param when  WHEN clause.
+ * @param no_err Suppress errors if the trigger already exists.
+ */
+void
+sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
+		  int op, struct IdList *columns, struct SrcList *table,
+		  struct Expr *when, int no_err);
+
+/**
+ * This routine is called after all of the trigger actions have
+ * been parsed in order to complete the process of building the
+ * trigger.
+ *
+ * @param parse Parser context.
+ * @param step_list The triggered program.
+ * @param token Token that describes the complete CREATE TRIGGER.
+ */
+void sql_trigger_finish(Parse *, TriggerStep *, Token *);
+
 void sqlite3DropTrigger(Parse *, SrcList *, int);
-void sqlite3DropTriggerPtr(Parse *, Trigger *);
-Trigger *sqlite3TriggersExist(Table *, int, ExprList *, int *pMask);
-void sqlite3CodeRowTrigger(Parse *, Trigger *, int, ExprList *, int, Table *,
-			   int, int, int);
-void sqlite3CodeRowTriggerDirect(Parse *, Trigger *, Table *, int, int, int);
-void sqliteViewTriggers(Parse *, Table *, Expr *, int, ExprList *);
+
+/**
+ * Drop a trigger given a pointer to that trigger.
+ *
+ * @param parser Parse context.
+ * @param trigger Trigger to drop.
+ */
+void
+vdbe_code_drop_trigger_ptr(struct Parse *parser, struct sql_trigger *trigger);
+
+/**
+ * Return a list of all triggers on table pTab if there exists at
+ * least one trigger that must be fired when an operation of type
+ * 'op' is performed on the table, and, if that operation is an
+ * UPDATE, if at least one of the columns in changes_list is being
+ * modified.
+ *
+ * @param table The table the contains the triggers.
+ * @param op operation one of TK_DELETE, TK_INSERT, TK_UPDATE.
+ * @param changes_list Columns that change in an UPDATE statement.
+ * @param[out] pMask Mask of TRIGGER_BEFORE|TRIGGER_AFTER
+ */
+struct sql_trigger *
+sql_triggers_exist(struct Table *table, int op, struct ExprList *changes_list,
+		   int *mask_ptr);
+
+/**
+ * This is called to code the required FOR EACH ROW triggers for
+ * an operation on table. The operation to code triggers for
+ * (INSERT, UPDATE or DELETE) is given by the op parameter. The
+ * tr_tm parameter determines whether the BEFORE or AFTER triggers
+ * are coded. If the operation is an UPDATE, then parameter
+ * changes_list is passed the list of columns being modified.
+ *
+ * If there are no triggers that fire at the specified time for
+ * the specified operation on table, this function is a no-op.
+ *
+ * The reg argument is the address of the first in an array of
+ * registers that contain the values substituted for the new.*
+ * and old.* references in the trigger program. If N is the number
+ * of columns in table table, then registers are populated as
+ * follows:
+ *
+ *   Register       Contains
+ *   ------------------------------------------------------
+ *   reg+0          OLD.PK
+ *   reg+1          OLD.* value of left-most column of pTab
+ *   ...            ...
+ *   reg+N          OLD.* value of right-most column of pTab
+ *   reg+N+1        NEW.PK
+ *   reg+N+2        OLD.* value of left-most column of pTab
+ *   ...            ...
+ *   reg+N+N+1      NEW.* value of right-most column of pTab
+ *
+ * For ON DELETE triggers, the registers containing the NEW.*
+ * values will never be accessed by the trigger program, so they
+ * are not allocated or populated by the caller (there is no data
+ * to populate them with anyway). Similarly, for ON INSERT
+ * triggers the values stored in the OLD.* registers are never
+ * accessed, and so are not allocated by the caller. So, for an
+ * ON INSERT trigger, the value passed to this function as
+ * parameter reg is not a readable register, although registers
+ * (reg+N) through (reg+N+N+1) are.
+ *
+ * Parameter orconf is the default conflict resolution algorithm
+ * for the trigger program to use (REPLACE, IGNORE etc.).
+ * Parameter ignoreJump is the instruction that control should
+ * jump to if a trigger program raises an IGNORE exception.
+ *
+ * @param parser Parse context.
+ * @param trigger List of triggers on table.
+ * @param op operation, one of TK_UPDATE, TK_INSERT, TK_DELETE.
+ * @param changes_list Changes list for any UPDATE OF triggers.
+ * @param tr_tm One of TRIGGER_BEFORE, TRIGGER_AFTER.
+ * @param table The table to code triggers from.
+ * @param reg The first in an array of registers.
+ * @param orconf ON CONFLICT policy.
+ * @param ignore_jump Instruction to jump to for RAISE(IGNORE).
+ */
+void
+vdbe_code_row_trigger(struct Parse *parser, struct sql_trigger *trigger,
+		      int op, struct ExprList *changes_list, int tr_tm,
+		      struct Table *table, int reg, int orconf, int ignore_jump);
+
+/**
+ * Generate code for the trigger program associated with trigger
+ * p on table table. The reg, orconf and ignoreJump parameters
+ * passed to this function are the same as those described in the
+ * header function for sql_code_row_trigger().
+ *
+ * @param parser Parse context.
+ * @param trigger Trigger to code.
+ * @param table The table to code triggers from.
+ * @param reg Reg array containing OLD.* and NEW.* values.
+ * @param orconf ON CONFLICT policy.
+ * @param ignore_jump Instruction to jump to for RAISE(IGNORE).
+ */
+void
+vdbe_code_row_trigger_direct(struct Parse *parser, struct sql_trigger *trigger,
+			     struct Table *table, int reg, int orconf,
+			     int ignore_jump);
+
 void sqlite3DeleteTriggerStep(sqlite3 *, TriggerStep *);
 TriggerStep *sqlite3TriggerSelectStep(sqlite3 *, Select *);
 TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
@@ -4082,21 +4213,53 @@ TriggerStep *sqlite3TriggerInsertStep(sqlite3 *, Token *, IdList *,
 TriggerStep *sqlite3TriggerUpdateStep(sqlite3 *, Token *, ExprList *, Expr *,
 				      u8);
 TriggerStep *sqlite3TriggerDeleteStep(sqlite3 *, Token *, Expr *);
-u32 sqlite3TriggerColmask(Parse *, Trigger *, ExprList *, int, int, Table *,
-			  int);
+
+/**
+ * Triggers may access values stored in the old.* or new.*
+ * pseudo-table.
+ * This function returns a 32-bit bitmask indicating which columns
+ * of the old.* or new.* tables actually are used by triggers.
+ * This information may be used by the caller, for example, to
+ * avoid having to load the entire old.* record into memory when
+ * executing an UPDATE or DELETE command.
+ *
+ * Bit 0 of the returned mask is set if the left-most column of
+ * the table may be accessed using an [old|new].<col> reference.
+ * Bit 1 is set if the second leftmost column value is required,
+ * and so on. If there are more than 32 columns in the table, and
+ * at least one of the columns with an index greater than 32 may
+ * be accessed, 0xffffffff is returned.
+ *
+ * It is not possible to determine if the old.PK or new.PK column
+ * is accessed by triggers. The caller must always assume that it
+ * is.
+ *
+ * Parameter isNew must be either 1 or 0. If it is 0, then the
+ * mask returned applies to the old.* table. If 1, the new.* table.
+ *
+ * Parameter tr_tm must be a mask with one or both of the
+ * TRIGGER_BEFORE and TRIGGER_AFTER bits set. Values accessed by
+ * BEFORE triggers are only included in the returned mask if the
+ * TRIGGER_BEFORE bit is set in the tr_tm parameter. Similarly,
+ * values accessed by AFTER triggers are only included in the
+ * returned mask if the TRIGGER_AFTER bit is set in tr_tm.
+ *
+ * @param parser  Parse context.
+ * @param trigger List of triggers on table.
+ * @param changes_list Changes list for any UPDATE OF triggers.
+ * @param new  1 for new.* ref mask, 0 for old.* ref mask.
+ * @param tr_tm Mask of TRIGGER_BEFORE|TRIGGER_AFTER.
+ * @param table The table to code triggers from.
+ * @param orconf Default ON CONFLICT policy for trigger steps.
+ *
+ * @retval mask value.
+ */
+u32
+sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
+		    ExprList *changes_list, int new, int tr_tm,
+		    Table *table, int orconf);
 #define sqlite3ParseToplevel(p) ((p)->pToplevel ? (p)->pToplevel : (p))
 #define sqlite3IsToplevel(p) ((p)->pToplevel==0)
-#else
-#define sqlite3TriggersExist(C,D,E,F) 0
-#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)
-#define sqlite3CodeRowTriggerDirect(A,B,C,D,E,F)
-#define sqlite3ParseToplevel(p) p
-#define sqlite3IsToplevel(p) 1
-#define sqlite3TriggerColmask(A,B,C,D,E,F,G) 0
-#endif
 
 int sqlite3JoinType(Parse *, Token *, Token *, Token *);
 void sqlite3CreateForeignKey(Parse *, ExprList *, Token *, ExprList *, int);
@@ -4465,7 +4628,7 @@ void sqlite3WithPush(Parse *, With *, u8);
  * this case foreign keys are parsed, but no other functionality is
  * provided (enforcement of FK constraints requires the triggers sub-system).
  */
-#if !defined(SQLITE_OMIT_FOREIGN_KEY) && !defined(SQLITE_OMIT_TRIGGER)
+#if !defined(SQLITE_OMIT_FOREIGN_KEY)
 void sqlite3FkCheck(Parse *, Table *, int, int, int *);
 void sqlite3FkDropTable(Parse *, SrcList *, Table *);
 void sqlite3FkActions(Parse *, Table *, ExprList *, int, int *);
diff --git a/src/box/sql/status.c b/src/box/sql/status.c
index dda91c5..161136c 100644
--- a/src/box/sql/status.c
+++ b/src/box/sql/status.c
@@ -257,7 +257,7 @@ sqlite3_db_status(sqlite3 * db,	/* The database connection whose status is desir
 				for (p = sqliteHashFirst(&pSchema->trigHash); p;
 				     p = sqliteHashNext(p)) {
 					sql_trigger_delete(db,
-							   (Trigger *)
+							   (struct sql_trigger *)
 							   sqliteHashData(p));
 				}
 				for (p = sqliteHashFirst(&pSchema->tblHash); p;
diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
index edcc45b..7c3dabe 100644
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -597,14 +597,14 @@ sql_view_compile(struct sqlite3 *db, const char *view_stmt)
 	return select;
 }
 
-struct Trigger *
+struct sql_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;
+	struct sql_trigger *trigger = NULL;
 	if (sqlite3RunParser(&parser, sql, &sql_error) != SQLITE_OK ||
 	    parser.parsed_ast_type != AST_TYPE_TRIGGER) {
 	    if (parser.rc != SQL_TARANTOOL_ERROR)
diff --git a/src/box/sql/treeview.c b/src/box/sql/treeview.c
index 84d839e..4261e73 100644
--- a/src/box/sql/treeview.c
+++ b/src/box/sql/treeview.c
@@ -565,7 +565,6 @@ sqlite3TreeViewExpr(TreeView * pView, const Expr * pExpr, u8 moreToFollow)
 			sqlite3TreeViewExprList(pView, pExpr->x.pList, 0, 0);
 			break;
 		}
-#ifndef SQLITE_OMIT_TRIGGER
 	case TK_RAISE:{
 			const char *zType = "unk";
 			switch (pExpr->affinity) {
@@ -586,7 +585,6 @@ sqlite3TreeViewExpr(TreeView * pView, const Expr * pExpr, u8 moreToFollow)
 					    pExpr->u.zToken);
 			break;
 		}
-#endif
 	case TK_MATCH:{
 			sqlite3TreeViewLine(pView, "MATCH {%d:%d}%s",
 					    pExpr->iTable, pExpr->iColumn,
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 3ec77c7..b23827d 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -42,7 +42,6 @@
 /* See comment in sqliteInt.h */
 int sqlSubProgramsRemaining;
 
-#ifndef SQLITE_OMIT_TRIGGER
 /*
  * Delete a linked list of TriggerStep structures.
  */
@@ -62,46 +61,36 @@ sqlite3DeleteTriggerStep(sqlite3 * db, TriggerStep * pTriggerStep)
 	}
 }
 
-/*
- * This is called by the parser when it sees a CREATE TRIGGER statement
- * up to the point of the BEGIN before the trigger actions.  A Trigger
- * structure is generated based on the information available and stored
- * in pParse->pNewTrigger.  After the trigger actions have been parsed, the
- * sqlite3FinishTrigger() function is called to complete the trigger
- * construction process.
- */
 void
-sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER statement */
-		    Token * pName,	/* The name of the trigger */
-		    int tr_tm,	/* One of TK_BEFORE, TK_AFTER, TK_INSTEAD */
-		    int op,	/* One of TK_INSERT, TK_UPDATE, TK_DELETE */
-		    IdList * pColumns,	/* column list if this is an UPDATE OF trigger */
-		    SrcList * pTableName,	/* The name of the table/view the trigger applies to */
-		    Expr * pWhen,	/* WHEN clause */
-		    int noErr	/* Suppress errors if the trigger already exists */
-    )
+sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
+		  int op, struct IdList *columns, struct SrcList *table,
+		  struct Expr *when, int no_err)
 {
-	Trigger *pTrigger = 0;	/* The new trigger */
-	char *zName = 0;	/* Name of the trigger */
-	sqlite3 *db = pParse->db;	/* The database connection */
-	DbFixer sFix;		/* State vector for the DB fixer */
+	/* The new trigger. */
+	struct sql_trigger *trigger = NULL;
+	/* The database connection. */
+	struct sqlite3 *db = parse->db;
+	/* State vector for the DB fixer. */
+	struct DbFixer fixdb;
+	/* The name of the Trigger. */
+	char *trigger_name = NULL;
 
 	/*
 	 * 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 (!parse->nested) {
+		struct Vdbe *v = sqlite3GetVdbe(parse);
 		if (v != NULL)
 			sqlite3VdbeCountChanges(v);
 	}
 	/* pName->z might be NULL, but not pName itself. */
-	assert(pName != NULL);
+	assert(name != NULL);
 	assert(op == TK_INSERT || op == TK_UPDATE || op == TK_DELETE);
 	assert(op > 0 && op < 0xff);
 
-	if (pTableName == NULL || db->mallocFailed)
+	if (table == NULL || db->mallocFailed)
 		goto trigger_cleanup;
 
 	/*
@@ -110,31 +99,31 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	 */
 	if (db->mallocFailed)
 		goto trigger_cleanup;
-	assert(pTableName->nSrc == 1);
-	sqlite3FixInit(&sFix, pParse, "trigger", pName);
-	if (sqlite3FixSrcList(&sFix, pTableName) != 0)
+	assert(table->nSrc == 1);
+	sqlite3FixInit(&fixdb, parse, "trigger", name);
+	if (sqlite3FixSrcList(&fixdb, table) != 0)
 		goto trigger_cleanup;
 
-	zName = sqlite3NameFromToken(db, pName);
-	if (zName == NULL)
+	trigger_name = sqlite3NameFromToken(db, name);
+	if (trigger_name == NULL)
 		goto trigger_cleanup;
 
-	if (sqlite3CheckIdentifierName(pParse, zName) != SQLITE_OK)
+	if (sqlite3CheckIdentifierName(parse, trigger_name) != 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++;
+	if (!parse->parse_only &&
+	    sqlite3HashFind(&db->pSchema->trigHash, trigger_name) != NULL) {
+		if (!no_err) {
+			diag_set(ClientError, ER_TRIGGER_EXISTS, trigger_name);
+			parse->rc = SQL_TARANTOOL_ERROR;
+			parse->nErr++;
 		} else {
 			assert(!db->init.busy);
 		}
 		goto trigger_cleanup;
 	}
 
-	const char *table_name = pTableName->a[0].zName;
+	const char *table_name = table->a[0].zName;
 	uint32_t space_id;
 	if (schema_find_id(BOX_SPACE_ID, 2, table_name, strlen(table_name),
 			   &space_id) != 0)
@@ -145,159 +134,161 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	}
 
 	/* Build the Trigger object. */
-	pTrigger = (Trigger *)sqlite3DbMallocZero(db, sizeof(Trigger));
-	if (pTrigger == NULL)
+	trigger = (struct sql_trigger *)sqlite3DbMallocZero(db,
+							    sizeof(struct
+								   sql_trigger));
+	if (trigger == NULL)
 		goto trigger_cleanup;
-	pTrigger->space_id = space_id;
-	pTrigger->zName = zName;
-	zName = NULL;
-
-	pTrigger->op = (u8) op;
-	pTrigger->tr_tm = tr_tm;
-	pTrigger->pWhen = sqlite3ExprDup(db, pWhen, EXPRDUP_REDUCE);
-	pTrigger->pColumns = sqlite3IdListDup(db, pColumns);
-	if ((pWhen != NULL && pTrigger->pWhen == NULL) ||
-	    (pColumns != NULL && pTrigger->pColumns == NULL))
+	trigger->space_id = space_id;
+	trigger->zName = trigger_name;
+	trigger_name = NULL;
+
+	trigger->op = (u8) op;
+	trigger->tr_tm = tr_tm;
+	trigger->pWhen = sqlite3ExprDup(db, when, EXPRDUP_REDUCE);
+	trigger->pColumns = sqlite3IdListDup(db, columns);
+	if ((when != NULL && trigger->pWhen == NULL) ||
+	    (columns != NULL && trigger->pColumns == NULL))
 		goto trigger_cleanup;
-	assert(pParse->parsed_ast.trigger == NULL);
-	pParse->parsed_ast.trigger = pTrigger;
-	pParse->parsed_ast_type = AST_TYPE_TRIGGER;
+	assert(parse->parsed_ast.trigger == NULL);
+	parse->parsed_ast.trigger = trigger;
+	parse->parsed_ast_type = AST_TYPE_TRIGGER;
 
  trigger_cleanup:
-	sqlite3DbFree(db, zName);
-	sqlite3SrcListDelete(db, pTableName);
-	sqlite3IdListDelete(db, pColumns);
-	sql_expr_delete(db, pWhen, false);
-	if (pParse->parsed_ast.trigger == NULL)
-		sql_trigger_delete(db, pTrigger);
+	sqlite3DbFree(db, trigger_name);
+	sqlite3SrcListDelete(db, table);
+	sqlite3IdListDelete(db, columns);
+	sql_expr_delete(db, when, false);
+	if (parse->parsed_ast.trigger == NULL)
+		sql_trigger_delete(db, trigger);
 	else
-		assert(pParse->parsed_ast.trigger == pTrigger);
+		assert(parse->parsed_ast.trigger == trigger);
 
 	return;
 
 set_tarantool_error_and_cleanup:
-	pParse->rc = SQL_TARANTOOL_ERROR;
-	pParse->nErr++;
+	parse->rc = SQL_TARANTOOL_ERROR;
+	parse->nErr++;
 	goto trigger_cleanup;
 }
 
-/*
- * This routine is called after all of the trigger actions have been parsed
- * in order to complete the process of building the trigger.
- */
 void
-sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
-		     TriggerStep * pStepList,	/* The triggered program */
-		     Token * pAll	/* Token that describes the complete CREATE TRIGGER */
-    )
+sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
+		   struct Token *token)
 {
 	/* Trigger being finished. */
-	Trigger *pTrig = pParse->parsed_ast.trigger;
-	char *zName;		/* Name of trigger */
-	char *zSql = 0;		/* SQL text */
-	char *zOpts = 0;	/* MsgPack containing SQL options */
-	sqlite3 *db = pParse->db;	/* The database */
-	DbFixer sFix;		/* Fixer object */
-	Token nameToken;	/* Trigger name for error reporting */
-
-	pParse->parsed_ast.trigger = NULL;
-	if (NEVER(pParse->nErr) || !pTrig)
+	struct sql_trigger *trigger = parse->parsed_ast.trigger;
+	/* Name of trigger. */
+	char *trigger_name;
+	/* SQL text. */
+	char *sql_str = NULL;
+	/* MsgPack containing SQL options. */
+	char *opts_buff = NULL;
+	/* The database. */
+	struct sqlite3 *db = parse->db;
+
+	parse->parsed_ast.trigger = NULL;
+	if (NEVER(parse->nErr) || trigger == NULL)
 		goto triggerfinish_cleanup;
-	zName = pTrig->zName;
-	pTrig->step_list = pStepList;
-	while (pStepList) {
-		pStepList->pTrig = pTrig;
-		pStepList = pStepList->pNext;
+	trigger_name = trigger->zName;
+	trigger->step_list = step_list;
+	while (step_list != NULL) {
+		step_list->trigger = trigger;
+		step_list = step_list->pNext;
 	}
-	sqlite3TokenInit(&nameToken, pTrig->zName);
-	sqlite3FixInit(&sFix, pParse, "trigger", &nameToken);
-	if (sqlite3FixTriggerStep(&sFix, pTrig->step_list)
-	    || sqlite3FixExpr(&sFix, pTrig->pWhen)
-	    ) {
+
+	/* Trigger name for error reporting. */
+	struct Token trigger_name_token;
+	/* Fixer object. */
+	struct DbFixer fixdb;
+	sqlite3TokenInit(&trigger_name_token, trigger->zName);
+	sqlite3FixInit(&fixdb, parse, "trigger", &trigger_name_token);
+	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list)
+	    || sqlite3FixExpr(&fixdb, trigger->pWhen))
 		goto triggerfinish_cleanup;
-	}
 
 	/*
 	 * Generate byte code to insert a new trigger into
 	 * Tarantool for non-parsing mode or export trigger.
 	 */
-	if (!pParse->parse_only) {
-		Vdbe *v;
-		int zOptsSz;
-		Table *pSysTrigger;
-		int iFirstCol;
-		int iCursor = pParse->nTab++;
-		int iRecord;
-
-		/* Make an entry in the _trigger space.  */
-		v = sqlite3GetVdbe(pParse);
+	if (!parse->parse_only) {
+		/* Make an entry in the _trigger space. */
+		struct Vdbe *v = sqlite3GetVdbe(parse);
 		if (v == 0)
 			goto triggerfinish_cleanup;
 
-		pSysTrigger = sqlite3HashFind(&pParse->db->pSchema->tblHash,
-					      TARANTOOL_SYS_TRIGGER_NAME);
-		if (NEVER(!pSysTrigger))
+		struct Table *sys_trigger =
+			sqlite3HashFind(&parse->db->pSchema->tblHash,
+					TARANTOOL_SYS_TRIGGER_NAME);
+		if (NEVER(sys_trigger == NULL))
 			goto triggerfinish_cleanup;
 
-		zSql = sqlite3MPrintf(db, "CREATE TRIGGER %s", pAll->z);
+		sql_str = sqlite3MPrintf(db, "CREATE TRIGGER %s", token->z);
 		if (db->mallocFailed)
 			goto triggerfinish_cleanup;
 
-		sqlite3OpenTable(pParse, iCursor, pSysTrigger, OP_OpenWrite);
+		int cursor = parse->nTab++;
+		sqlite3OpenTable(parse, cursor, sys_trigger, OP_OpenWrite);
 
-		/* makerecord(cursor(iRecord), [reg(iFirstCol), reg(iFirstCol+1)])  */
-		iFirstCol = pParse->nMem + 1;
-		pParse->nMem += 3;
-		iRecord = ++pParse->nMem;
+		/*
+		 * makerecord(cursor(iRecord),
+		 * [reg(first_col), reg(first_col+1)]).
+		 */
+		int first_col = parse->nMem + 1;
+		parse->nMem += 3;
+		int record = ++parse->nMem;
 
-		zOpts = sqlite3DbMallocRaw(pParse->db,
+		opts_buff =
+			sqlite3DbMallocRaw(parse->db,
 					   tarantoolSqlite3MakeTableOpts(0,
-									 zSql,
+									 sql_str,
 									 NULL) +
 					   1);
 		if (db->mallocFailed)
 			goto triggerfinish_cleanup;
 
-		zOptsSz = tarantoolSqlite3MakeTableOpts(0, zSql, zOpts);
+		int opts_buff_sz =
+			tarantoolSqlite3MakeTableOpts(0, sql_str, opts_buff);
 
-		zName = sqlite3DbStrDup(pParse->db, zName);
+		trigger_name = sqlite3DbStrDup(parse->db, trigger_name);
 		if (db->mallocFailed)
 			goto triggerfinish_cleanup;
 
 		sqlite3VdbeAddOp4(v,
-				  OP_String8, 0, iFirstCol, 0,
-				  zName, P4_DYNAMIC);
-		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, 3, iRecord);
-		sqlite3VdbeAddOp2(v, OP_IdxInsert, iCursor, iRecord);
-		/* Do not account nested operations: the count of such
-		 * operations depends on Tarantool data dictionary internals,
-		 * such as data layout in system spaces.
+				  OP_String8, 0, first_col, 0,
+				  trigger_name, P4_DYNAMIC);
+		sqlite3VdbeAddOp2(v, OP_Integer, trigger->space_id,
+				  first_col + 1);
+		sqlite3VdbeAddOp4(v, OP_Blob, opts_buff_sz, first_col + 2,
+				  MSGPACK_SUBTYPE, opts_buff, P4_DYNAMIC);
+		sqlite3VdbeAddOp3(v, OP_MakeRecord, first_col, 3, record);
+		sqlite3VdbeAddOp2(v, OP_IdxInsert, cursor, record);
+		/*
+		 * 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)
+		if (!parse->nested)
 			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-		sqlite3VdbeAddOp1(v, OP_Close, iCursor);
+		sqlite3VdbeAddOp1(v, OP_Close, cursor);
 
-		sql_set_multi_write(pParse, false);
-		sqlite3ChangeCookie(pParse);
+		sql_set_multi_write(parse, false);
+		sqlite3ChangeCookie(parse);
 	} else {
-		pParse->parsed_ast.trigger = pTrig;
-		pParse->parsed_ast_type = AST_TYPE_TRIGGER;
-		pTrig = NULL;
+		parse->parsed_ast.trigger = trigger;
+		parse->parsed_ast_type = AST_TYPE_TRIGGER;
+		trigger = NULL;
 	}
 
  triggerfinish_cleanup:
 	if (db->mallocFailed) {
-		sqlite3DbFree(db, zSql);
-		sqlite3DbFree(db, zOpts);
-		/* No need to free zName sinceif we reach this point
-		   alloc for it either wasn't called at all or failed.  */
+		sqlite3DbFree(db, sql_str);
+		sqlite3DbFree(db, opts_buff);
 	}
-	sql_trigger_delete(db, pTrig);
-	assert(pParse->parsed_ast.trigger == NULL || pParse->parse_only);
-	sqlite3DeleteTriggerStep(db, pStepList);
+	sql_trigger_delete(db, trigger);
+	assert(parse->parsed_ast.trigger == NULL || parse->parse_only);
+	sqlite3DeleteTriggerStep(db, step_list);
 }
 
 /*
@@ -439,7 +430,7 @@ sqlite3TriggerDeleteStep(sqlite3 * db,	/* Database connection */
 }
 
 void
-sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
+sql_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
 {
 	if (trigger == NULL)
 		return;
@@ -451,17 +442,18 @@ sql_trigger_delete(struct sqlite3 *db, struct Trigger *trigger)
 }
 
 /*
- * This function is called to drop a trigger from the database schema.
+ * 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 sqlite3DropTriggerPtr() routine does the
- * same job as this routine except it takes a pointer to the trigger
- * instead of the trigger name.
+ * 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)
 {
-	Trigger *pTrigger = 0;
+	struct sql_trigger *trigger = NULL;
 	const char *zName;
 	sqlite3 *db = pParse->db;
 
@@ -483,8 +475,8 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
 
 	assert(pName->nSrc == 1);
 	zName = pName->a[0].zName;
-	pTrigger = sqlite3HashFind(&(db->pSchema->trigHash), zName);
-	if (!pTrigger) {
+	trigger = sqlite3HashFind(&(db->pSchema->trigHash), zName);
+	if (trigger == NULL) {
 		if (!noErr) {
 			sqlite3ErrorMsg(pParse, "no such trigger: %S", pName,
 					0);
@@ -492,48 +484,47 @@ sqlite3DropTrigger(Parse * pParse, SrcList * pName, int noErr)
 		pParse->checkSchema = 1;
 		goto drop_trigger_cleanup;
 	}
-	sqlite3DropTriggerPtr(pParse, pTrigger);
+	vdbe_code_drop_trigger_ptr(pParse, trigger);
 
  drop_trigger_cleanup:
 	sqlite3SrcListDelete(db, pName);
 }
 
-/*
- * Drop a trigger given a pointer to that trigger.
- */
 void
-sqlite3DropTriggerPtr(Parse * pParse, Trigger * pTrigger)
+vdbe_code_drop_trigger_ptr(struct Parse *parser, struct sql_trigger *trigger)
 {
-	Vdbe *v;
-	/* Generate code to delete entry from _trigger and
+	struct Vdbe *v = sqlite3GetVdbe(parser);
+	if (v == NULL)
+		return;
+	/*
+	 * Generate code to delete entry from _trigger and
 	 * internal SQL structures.
 	 */
-	if ((v = sqlite3GetVdbe(pParse)) != 0) {
-		int trig_name_reg = ++pParse->nMem;
-		int record_to_delete = ++pParse->nMem;
-		sqlite3VdbeAddOp4(v, OP_String8, 0, trig_name_reg, 0,
-				  pTrigger->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 (!pParse->nested)
-			sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-
-		sqlite3ChangeCookie(pParse);
-	}
+	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);
 }
 
 int
 sql_trigger_replace(struct sqlite3 *db, const char *name,
-		    struct Trigger *trigger, struct Trigger **old_trigger)
+		    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 Trigger *src_trigger =
+	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);
@@ -580,31 +571,31 @@ sql_trigger_replace(struct sqlite3 *db, const char *name,
 	}
 
 	if (*old_trigger != NULL) {
-		struct Trigger **pp;
+		struct sql_trigger **pp;
 		for (pp = &space->sql_triggers; *pp != *old_trigger;
-		     pp = &((*pp)->pNext));
-		*pp = (*pp)->pNext;
+		     pp = &((*pp)->next));
+		*pp = (*pp)->next;
 	}
 	if (trigger != NULL) {
-		trigger->pNext = space->sql_triggers;
+		trigger->next = space->sql_triggers;
 		space->sql_triggers = trigger;
 	}
 	return 0;
 }
 
 const char *
-sql_trigger_name(struct Trigger *trigger)
+sql_trigger_name(struct sql_trigger *trigger)
 {
 	return trigger->zName;
 }
 
 uint32_t
-sql_trigger_space_id(struct Trigger *trigger)
+sql_trigger_space_id(struct sql_trigger *trigger)
 {
 	return trigger->space_id;
 }
 
-struct Trigger *
+struct sql_trigger *
 space_trigger_list(uint32_t space_id)
 {
 	struct space *space = space_cache_find(space_id);
@@ -635,31 +626,22 @@ checkColumnOverlap(IdList * pIdList, ExprList * pEList)
 	return 0;
 }
 
-/*
- * Return a list of all triggers on table pTab if there exists at least
- * one trigger that must be fired when an operation of type 'op' is
- * performed on the table, and, if that operation is an UPDATE, if at
- * least one of the columns in pChanges is being modified.
- */
-Trigger *
-sqlite3TriggersExist(Table * pTab,	/* The table the contains the triggers */
-		     int op,	/* one of TK_DELETE, TK_INSERT, TK_UPDATE */
-		     ExprList * pChanges,	/* Columns that change in an UPDATE statement */
-		     int *pMask	/* OUT: Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
-    )
+struct sql_trigger *
+sql_triggers_exist(struct Table *table, int op, struct ExprList *changes_list,
+		   int *mask_ptr)
 {
 	int mask = 0;
-	struct Trigger *trigger_list = NULL;
+	struct sql_trigger *trigger_list = NULL;
 	struct session *user_session = current_session();
 	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) {
+		trigger_list = space_trigger_list(table->def->id);
+	for (struct sql_trigger *p = trigger_list; p != NULL; p = p->next) {
 		if (p->op == op && checkColumnOverlap(p->pColumns,
-						      pChanges) != 0)
+						      changes_list) != 0)
 			mask |= p->tr_tm;
 	}
-	if (pMask != NULL)
-		*pMask = mask;
+	if (mask_ptr != NULL)
+		*mask_ptr = mask;
 	return mask != 0 ? trigger_list : NULL;
 }
 
@@ -829,28 +811,33 @@ transferParseError(Parse * pTo, Parse * pFrom)
 	pFrom->zErrMsg = NULL;
 }
 
-/*
+/**
  * Create and populate a new TriggerPrg object with a sub-program
  * implementing trigger pTrigger with ON CONFLICT policy orconf.
+ *
+ * @param parser Current parse context.
+ * @param trigger sql_trigger to code.
+ * @param table trigger is attached to.
+ * @param orconf ON CONFLICT policy to code trigger program with.
+ *
+ * @retval not NULL on success.
+ * @retval NULL on error.
  */
 static TriggerPrg *
-codeRowTrigger(Parse * pParse,	/* Current parse context */
-	       Trigger * pTrigger,	/* Trigger to code */
-	       Table * pTab,	/* The table pTrigger is attached to */
-	       int orconf	/* ON CONFLICT policy to code trigger program with */
-    )
+sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
+			struct Table *table, int orconf)
 {
-	Parse *pTop = sqlite3ParseToplevel(pParse);
-	sqlite3 *db = pParse->db;	/* Database handle */
+	Parse *pTop = sqlite3ParseToplevel(parser);
+	/* Database handle. */
+	sqlite3 *db = parser->db;
 	TriggerPrg *pPrg;	/* Value to return */
 	Expr *pWhen = 0;	/* Duplicate of trigger WHEN expression */
-	Vdbe *v;		/* Temporary VM */
 	NameContext sNC;	/* Name context for sub-vdbe */
 	SubProgram *pProgram = 0;	/* Sub-vdbe for trigger program */
 	Parse *pSubParse;	/* Parse context for sub-vdbe */
 	int iEndTrigger = 0;	/* Label to jump to if WHEN is false */
 
-	assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
+	assert(trigger->zName == NULL || table->def->id == trigger->space_id);
 	assert(pTop->pVdbe);
 
 	/* Allocate the TriggerPrg and SubProgram objects. To ensure that they
@@ -866,13 +853,14 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 	if (!pProgram)
 		return 0;
 	sqlite3VdbeLinkSubProgram(pTop->pVdbe, pProgram);
-	pPrg->pTrigger = pTrigger;
+	pPrg->trigger = trigger;
 	pPrg->orconf = orconf;
 	pPrg->aColmask[0] = 0xffffffff;
 	pPrg->aColmask[1] = 0xffffffff;
 
-	/* Allocate and populate a new Parse context to use for coding the
-	 * trigger sub-program.
+	/*
+	 * Allocate and populate a new Parse context to use for
+	 * coding the trigger sub-program.
 	 */
 	pSubParse = sqlite3StackAllocZero(db, sizeof(Parse));
 	if (!pSubParse)
@@ -880,34 +868,37 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 	sql_parser_create(pSubParse, db);
 	memset(&sNC, 0, sizeof(sNC));
 	sNC.pParse = pSubParse;
-	pSubParse->pTriggerTab = pTab;
+	pSubParse->pTriggerTab = table;
 	pSubParse->pToplevel = pTop;
-	pSubParse->eTriggerOp = pTrigger->op;
-	pSubParse->nQueryLoop = pParse->nQueryLoop;
+	pSubParse->eTriggerOp = trigger->op;
+	pSubParse->nQueryLoop = parser->nQueryLoop;
 
-	v = sqlite3GetVdbe(pSubParse);
-	if (v) {
+	/* Temporary VM. */
+	struct Vdbe *v = sqlite3GetVdbe(pSubParse);
+	if (v != NULL) {
 		VdbeComment((v, "Start: %s.%s (%s %s%s%s ON %s)",
-			     pTrigger->zName, onErrorText(orconf),
-			     (pTrigger->tr_tm ==
+			     trigger->zName, onErrorText(orconf),
+			     (trigger->tr_tm ==
 			      TRIGGER_BEFORE ? "BEFORE" : "AFTER"),
-			     (pTrigger->op == TK_UPDATE ? "UPDATE" : ""),
-			     (pTrigger->op == TK_INSERT ? "INSERT" : ""),
-			     (pTrigger->op == TK_DELETE ? "DELETE" : ""),
-			     pTab->def->name));
+			     (trigger->op == TK_UPDATE ? "UPDATE" : ""),
+			     (trigger->op == TK_INSERT ? "INSERT" : ""),
+			     (trigger->op == TK_DELETE ? "DELETE" : ""),
+			      table->def->name));
 #ifndef SQLITE_OMIT_TRACE
 		sqlite3VdbeChangeP4(v, -1,
 				    sqlite3MPrintf(db, "-- TRIGGER %s",
-						   pTrigger->zName),
+						   trigger->zName),
 				    P4_DYNAMIC);
 #endif
 
-		/* If one was specified, code the WHEN clause. If it evaluates to false
-		 * (or NULL) the sub-vdbe is immediately halted by jumping to the
-		 * OP_Halt inserted at the end of the program.
+		/*
+		 * If one was specified, code the WHEN clause. If
+		 * it evaluates to false (or NULL) the sub-vdbe is
+		 * immediately halted by jumping to the OP_Halt
+		 * inserted at the end of the program.
 		 */
-		if (pTrigger->pWhen) {
-			pWhen = sqlite3ExprDup(db, pTrigger->pWhen, 0);
+		if (trigger->pWhen != NULL) {
+			pWhen = sqlite3ExprDup(db, trigger->pWhen, 0);
 			if (SQLITE_OK == sqlite3ResolveExprNames(&sNC, pWhen)
 			    && db->mallocFailed == 0) {
 				iEndTrigger = sqlite3VdbeMakeLabel(v);
@@ -919,17 +910,16 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 		}
 
 		/* Code the trigger program into the sub-vdbe. */
-		codeTriggerProgram(pSubParse, pTrigger->step_list, orconf);
+		codeTriggerProgram(pSubParse, trigger->step_list, orconf);
 
 		/* Insert an OP_Halt at the end of the sub-program. */
-		if (iEndTrigger) {
+		if (iEndTrigger)
 			sqlite3VdbeResolveLabel(v, iEndTrigger);
-		}
 		sqlite3VdbeAddOp0(v, OP_Halt);
-		VdbeComment((v, "End: %s.%s", pTrigger->zName,
+		VdbeComment((v, "End: %s.%s", trigger->zName,
 			     onErrorText(orconf)));
 
-		transferParseError(pParse, pSubParse);
+		transferParseError(parser, pSubParse);
 		if (db->mallocFailed == 0) {
 			pProgram->aOp =
 			    sqlite3VdbeTakeOpArray(v, &pProgram->nOp,
@@ -937,7 +927,7 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 		}
 		pProgram->nMem = pSubParse->nMem;
 		pProgram->nCsr = pSubParse->nTab;
-		pProgram->token = (void *)pTrigger;
+		pProgram->token = (void *)trigger;
 		pPrg->aColmask[0] = pSubParse->oldmask;
 		pPrg->aColmask[1] = pSubParse->newmask;
 		sqlite3VdbeDelete(v);
@@ -951,211 +941,130 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 	return pPrg;
 }
 
-/*
- * Return a pointer to a TriggerPrg object containing the sub-program for
- * trigger pTrigger with default ON CONFLICT algorithm orconf. If no such
- * TriggerPrg object exists, a new object is allocated and populated before
- * being returned.
+/**
+ * Return a pointer to a TriggerPrg object containing the
+ * sub-program for trigger with default ON CONFLICT algorithm
+ * orconf. If no such TriggerPrg object exists, a new object is
+ * allocated and populated before being returned.
+ *
+ * @param parser Current parse context.
+ * @param trigger Trigger to code.
+ * @param table table trigger is attached to.
+ * @param orconf ON CONFLICT algorithm.
+ *
+ * @retval not NULL on success.
+ * @retval NULL on error.
  */
 static TriggerPrg *
-getRowTrigger(Parse * pParse,	/* Current parse context */
-	      Trigger * pTrigger,	/* Trigger to code */
-	      Table * pTab,	/* The table trigger pTrigger is attached to */
-	      int orconf	/* ON CONFLICT algorithm. */
-    )
+sql_row_trigger(struct Parse *parser, struct sql_trigger *trigger,
+		struct Table *table, int orconf)
 {
-	Parse *pRoot = sqlite3ParseToplevel(pParse);
+	Parse *pRoot = sqlite3ParseToplevel(parser);
 	TriggerPrg *pPrg;
 
-	assert(pTrigger->zName == NULL || pTab->def->id == pTrigger->space_id);
+	assert(trigger->zName == NULL || table->def->id == trigger->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
-	 * a matching TriggerPrg.pTrigger field will be present somewhere
-	 * in the Parse.pTriggerPrg list. Search for such an entry.
+	/*
+	 * 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 a matching TriggerPrg.pTrigger
+	 * field will be present somewhere in the
+	 * Parse.pTriggerPrg list. Search for such an entry.
 	 */
 	for (pPrg = pRoot->pTriggerPrg;
-	     pPrg && (pPrg->pTrigger != pTrigger || pPrg->orconf != orconf);
+	     pPrg && (pPrg->trigger != trigger || pPrg->orconf != orconf);
 	     pPrg = pPrg->pNext) ;
 
-	/* If an existing TriggerPrg could not be located, create a new one. */
-	if (!pPrg) {
-		pPrg = codeRowTrigger(pParse, pTrigger, pTab, orconf);
-	}
+	/*
+	 * If an existing TriggerPrg could not be located, create
+	 * a new one.
+	 */
+	if (pPrg == NULL)
+		pPrg = sql_row_trigger_program(parser, trigger, table, orconf);
 
 	return pPrg;
 }
 
-/*
- * Generate code for the trigger program associated with trigger p on
- * table pTab. The reg, orconf and ignoreJump parameters passed to this
- * function are the same as those described in the header function for
- * sqlite3CodeRowTrigger()
- */
 void
-sqlite3CodeRowTriggerDirect(Parse * pParse,	/* Parse context */
-			    Trigger * p,	/* Trigger to code */
-			    Table * pTab,	/* The table to code triggers from */
-			    int reg,	/* Reg array containing OLD.* and NEW.* values */
-			    int orconf,	/* ON CONFLICT policy */
-			    int ignoreJump	/* Instruction to jump to for RAISE(IGNORE) */
-    )
+vdbe_code_row_trigger_direct(struct Parse *parser, struct sql_trigger *trigger,
+			     struct Table *table, int reg, int orconf,
+			     int ignore_jump)
 {
-	Vdbe *v = sqlite3GetVdbe(pParse);	/* Main VM */
-	TriggerPrg *pPrg;
-	struct session *user_session = current_session();
+	/* Main VM. */
+	struct Vdbe *v = sqlite3GetVdbe(parser);
 
-	pPrg = getRowTrigger(pParse, p, pTab, orconf);
-	assert(pPrg || pParse->nErr || pParse->db->mallocFailed);
+	TriggerPrg *pPrg = sql_row_trigger(parser, trigger, table, orconf);
+	assert(pPrg != NULL || parser->nErr != 0 ||
+	       parser->db->mallocFailed != 0);
 
-	/* Code the OP_Program opcode in the parent VDBE. P4 of the OP_Program
-	 * is a pointer to the sub-vdbe containing the trigger program.
+	/*
+	 * Code the OP_Program opcode in the parent VDBE. P4 of
+	 * the OP_Program is a pointer to the sub-vdbe containing
+	 * the trigger program.
 	 */
-	if (pPrg) {
-		int bRecursive = (p->zName &&
-				  0 ==
-				  (user_session->
-				   sql_flags & SQLITE_RecTriggers));
-
-		sqlite3VdbeAddOp4(v, OP_Program, reg, ignoreJump,
-				  ++pParse->nMem, (const char *)pPrg->pProgram,
-				  P4_SUBPROGRAM);
-		VdbeComment((v, "Call: %s.%s", (p->zName ? p->zName : "fkey"),
-			     onErrorText(orconf)));
+	if (pPrg == NULL)
+		return;
 
-		/* Set the P5 operand of the OP_Program instruction to non-zero if
-		 * recursive invocation of this trigger program is disallowed. Recursive
-		 * invocation is disallowed if (a) the sub-program is really a trigger,
-		 * not a foreign key action, and (b) the flag to enable recursive triggers
-		 * is clear.
-		 */
-		sqlite3VdbeChangeP5(v, (u8) bRecursive);
-	}
+	struct session *user_session = current_session();
+	bool recursive = (trigger->zName && !(user_session->sql_flags &
+					      SQLITE_RecTriggers));
+
+	sqlite3VdbeAddOp4(v, OP_Program, reg, ignore_jump,
+			  ++parser->nMem, (const char *)pPrg->pProgram,
+			  P4_SUBPROGRAM);
+	VdbeComment((v, "Call: %s.%s", (trigger->zName ? trigger->zName :
+					"fkey"),
+		     onErrorText(orconf)));
+
+	/*
+	 * Set the P5 operand of the OP_Program
+	 * instruction to non-zero if recursive invocation
+	 * of this trigger program is disallowed.
+	 * Recursive invocation is disallowed if (a) the
+	 * sub-program is really a trigger, not a foreign
+	 * key action, and (b) the flag to enable
+	 * recursive triggers is clear.
+	 */
+	sqlite3VdbeChangeP5(v, (u8)recursive);
 }
 
-/*
- * This is called to code the required FOR EACH ROW triggers for an operation
- * on table pTab. The operation to code triggers for (INSERT, UPDATE or DELETE)
- * is given by the op parameter. The tr_tm parameter determines whether the
- * BEFORE or AFTER triggers are coded. If the operation is an UPDATE, then
- * parameter pChanges is passed the list of columns being modified.
- *
- * If there are no triggers that fire at the specified time for the specified
- * operation on pTab, this function is a no-op.
- *
- * The reg argument is the address of the first in an array of registers
- * that contain the values substituted for the new.* and old.* references
- * in the trigger program. If N is the number of columns in table pTab
- * (a copy of pTab->nCol), then registers are populated as follows:
- *
- *   Register       Contains
- *   ------------------------------------------------------
- *   reg+0          OLD.PK
- *   reg+1          OLD.* value of left-most column of pTab
- *   ...            ...
- *   reg+N          OLD.* value of right-most column of pTab
- *   reg+N+1        NEW.PK
- *   reg+N+2        OLD.* value of left-most column of pTab
- *   ...            ...
- *   reg+N+N+1      NEW.* value of right-most column of pTab
- *
- * For ON DELETE triggers, the registers containing the NEW.* values will
- * never be accessed by the trigger program, so they are not allocated or
- * populated by the caller (there is no data to populate them with anyway).
- * Similarly, for ON INSERT triggers the values stored in the OLD.* registers
- * are never accessed, and so are not allocated by the caller. So, for an
- * ON INSERT trigger, the value passed to this function as parameter reg
- * is not a readable register, although registers (reg+N) through
- * (reg+N+N+1) are.
- *
- * Parameter orconf is the default conflict resolution algorithm for the
- * trigger program to use (REPLACE, IGNORE etc.). Parameter ignoreJump
- * is the instruction that control should jump to if a trigger program
- * raises an IGNORE exception.
- */
 void
-sqlite3CodeRowTrigger(Parse * pParse,	/* Parse context */
-		      Trigger * pTrigger,	/* List of triggers on table pTab */
-		      int op,	/* One of TK_UPDATE, TK_INSERT, TK_DELETE */
-		      ExprList * pChanges,	/* Changes list for any UPDATE OF triggers */
-		      int tr_tm,	/* One of TRIGGER_BEFORE, TRIGGER_AFTER */
-		      Table * pTab,	/* The table to code triggers from */
-		      int reg,	/* The first in an array of registers (see above) */
-		      int orconf,	/* ON CONFLICT policy */
-		      int ignoreJump	/* Instruction to jump to for RAISE(IGNORE) */
-    )
+vdbe_code_row_trigger(struct Parse *parser, struct sql_trigger *trigger,
+		      int op, struct ExprList *changes_list, int tr_tm,
+		      struct Table *table, int reg, int orconf, int ignore_jump)
 {
-	Trigger *p;		/* Used to iterate through pTrigger list */
-
 	assert(op == TK_UPDATE || op == TK_INSERT || op == TK_DELETE);
 	assert(tr_tm == TRIGGER_BEFORE || tr_tm == TRIGGER_AFTER);
-	assert((op == TK_UPDATE) == (pChanges != 0));
-
-	for (p = pTrigger; p; p = p->pNext) {
-		/* Determine whether we should code this trigger */
-		if (p->op == op
-		    && p->tr_tm == tr_tm
-		    && checkColumnOverlap(p->pColumns, pChanges)
-		    ) {
-			sqlite3CodeRowTriggerDirect(pParse, p, pTab, reg,
-						    orconf, ignoreJump);
+	assert((op == TK_UPDATE) == (changes_list != NULL));
+
+	for (struct sql_trigger *p = trigger; p != NULL; p = p->next) {
+		/* Determine whether we should code trigger. */
+		if (p->op == op && p->tr_tm == tr_tm &&
+		    checkColumnOverlap(p->pColumns, changes_list)) {
+			vdbe_code_row_trigger_direct(parser, p, table, reg,
+						     orconf, ignore_jump);
 		}
 	}
 }
 
-/*
- * Triggers may access values stored in the old.* or new.* pseudo-table.
- * This function returns a 32-bit bitmask indicating which columns of the
- * old.* or new.* tables actually are used by triggers. This information
- * may be used by the caller, for example, to avoid having to load the entire
- * old.* record into memory when executing an UPDATE or DELETE command.
- *
- * Bit 0 of the returned mask is set if the left-most column of the
- * table may be accessed using an [old|new].<col> reference. Bit 1 is set if
- * the second leftmost column value is required, and so on. If there
- * are more than 32 columns in the table, and at least one of the columns
- * with an index greater than 32 may be accessed, 0xffffffff is returned.
- *
- * It is not possible to determine if the old.PK or new.PK column is
- * accessed by triggers. The caller must always assume that it is.
- *
- * Parameter isNew must be either 1 or 0. If it is 0, then the mask returned
- * applies to the old.* table. If 1, the new.* table.
- *
- * Parameter tr_tm must be a mask with one or both of the TRIGGER_BEFORE
- * and TRIGGER_AFTER bits set. Values accessed by BEFORE triggers are only
- * included in the returned mask if the TRIGGER_BEFORE bit is set in the
- * tr_tm parameter. Similarly, values accessed by AFTER triggers are only
- * included in the returned mask if the TRIGGER_AFTER bit is set in tr_tm.
- */
 u32
-sqlite3TriggerColmask(Parse * pParse,	/* Parse context */
-		      Trigger * pTrigger,	/* List of triggers on table pTab */
-		      ExprList * pChanges,	/* Changes list for any UPDATE OF triggers */
-		      int isNew,	/* 1 for new.* ref mask, 0 for old.* ref mask */
-		      int tr_tm,	/* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
-		      Table * pTab,	/* The table to code triggers from */
-		      int orconf	/* Default ON CONFLICT policy for trigger steps */
-    )
+sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
+		    ExprList *changes_list, int new, int tr_tm,
+		    Table *table, int orconf)
 {
-	const int op = pChanges ? TK_UPDATE : TK_DELETE;
+	const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE;
 	u32 mask = 0;
-	Trigger *p;
 
-	assert(isNew == 1 || isNew == 0);
-	for (p = pTrigger; p; p = p->pNext) {
+	assert(new == 1 || new == 0);
+	for (struct sql_trigger *p = trigger; p != NULL; p = p->next) {
 		if (p->op == op && (tr_tm & p->tr_tm)
-		    && checkColumnOverlap(p->pColumns, pChanges)
-		    ) {
-			TriggerPrg *pPrg;
-			pPrg = getRowTrigger(pParse, p, pTab, orconf);
-			if (pPrg) {
-				mask |= pPrg->aColmask[isNew];
-			}
+		    && checkColumnOverlap(p->pColumns, changes_list)) {
+			TriggerPrg *prg =
+				sql_row_trigger(parser, p, table, orconf);
+			if (prg != NULL)
+				mask |= prg->aColmask[new];
 		}
 	}
 
 	return mask;
 }
-
-#endif				/* !defined(SQLITE_OMIT_TRIGGER) */
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 10385eb..212adbc 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -106,7 +106,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	struct session *user_session = current_session();
 
 	bool is_view;		/* True when updating a view (INSTEAD OF trigger) */
-	Trigger *pTrigger;	/* List of triggers on pTab, if required */
+	/* List of triggers on pTab, if required. */
+	struct sql_trigger *trigger;
 	int tmask;		/* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
 	int newmask;		/* Mask of NEW.* columns accessed by BEFORE triggers */
 	int iEph = 0;		/* Ephemeral table holding all primary key values */
@@ -136,9 +137,9 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	/* Figure out if we have any triggers and if the table being
 	 * updated is a view.
 	 */
-	pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask);
+	trigger = sql_triggers_exist(pTab, TK_UPDATE, pChanges, &tmask);
 	is_view = pTab->def->opts.is_view;
-	assert(pTrigger || tmask == 0);
+	assert(trigger != NULL || tmask == 0);
 
 	if (is_view &&
 	    sql_view_assign_cursors(pParse,pTab->def->opts.sql) != 0) {
@@ -269,11 +270,9 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	/* Allocate required registers. */
 	regOldPk = regNewPk = ++pParse->nMem;
 
-	if (chngPk || pTrigger || hasFK) {
+	if (chngPk != 0 || trigger != NULL || hasFK != 0) {
 		regOld = pParse->nMem + 1;
 		pParse->nMem += def->field_count;
-	}
-	if (chngPk || pTrigger || hasFK) {
 		regNewPk = ++pParse->nMem;
 	}
 	regNew = pParse->nMem + 1;
@@ -424,18 +423,18 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	 * then regNewPk is the same register as regOldPk, which is
 	 * already populated.
 	 */
-	assert(chngPk || pTrigger || hasFK || regOldPk == regNewPk);
+	assert(chngPk != 0 || trigger != NULL || hasFK != 0 ||
+	       regOldPk == regNewPk);
 
 
 	/* Compute the old pre-UPDATE content of the row being changed, if that
 	 * information is needed
 	 */
-	if (chngPk || hasFK || pTrigger) {
+	if (chngPk != 0 || hasFK != 0 || trigger != NULL) {
 		u32 oldmask = (hasFK ? sqlite3FkOldmask(pParse, pTab) : 0);
-		oldmask |= sqlite3TriggerColmask(pParse,
-						 pTrigger, pChanges, 0,
-						 TRIGGER_BEFORE | TRIGGER_AFTER,
-						 pTab, on_error);
+		oldmask |= sql_trigger_colmask(pParse, trigger, pChanges, 0,
+					       TRIGGER_BEFORE | TRIGGER_AFTER,
+					       pTab, on_error);
 		for (i = 0; i < (int)def->field_count; i++) {
 			if (oldmask == 0xffffffff
 			    || (i < 32 && (oldmask & MASKBIT32(i)) != 0)
@@ -464,9 +463,8 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	 * may have modified them). So not loading those that are not going to
 	 * be used eliminates some redundant opcodes.
 	 */
-	newmask =
-	    sqlite3TriggerColmask(pParse, pTrigger, pChanges, 1, TRIGGER_BEFORE,
-				  pTab, on_error);
+	newmask = sql_trigger_colmask(pParse, trigger, pChanges, 1,
+				      TRIGGER_BEFORE, pTab, on_error);
 	for (i = 0; i < (int)def->field_count; i++) {
 		if (i == pTab->iPKey) {
 			sqlite3VdbeAddOp2(v, OP_Null, 0, regNew + i);
@@ -498,7 +496,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	 */
 	if (tmask & TRIGGER_BEFORE) {
 		sqlite3TableAffinity(v, pTab, regNew);
-		sqlite3CodeRowTrigger(pParse, pTrigger, TK_UPDATE, pChanges,
+		vdbe_code_row_trigger(pParse, trigger, TK_UPDATE, pChanges,
 				      TRIGGER_BEFORE, pTab, regOldPk,
 				      on_error, labelContinue);
 
@@ -611,7 +609,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 		sqlite3VdbeAddOp2(v, OP_AddImm, regRowCount, 1);
 	}
 
-	sqlite3CodeRowTrigger(pParse, pTrigger, TK_UPDATE, pChanges,
+	vdbe_code_row_trigger(pParse, trigger, TK_UPDATE, pChanges,
 			      TRIGGER_AFTER, pTab, regOldPk, on_error,
 			      labelContinue);
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index cac1ad3..b0ab916 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4733,7 +4733,7 @@ case OP_RenameTable: {
 	space = space_by_id(space_id);
 	assert(space);
 	/* Rename space op doesn't change triggers. */
-	struct Trigger *triggers = space->sql_triggers;
+	struct sql_trigger *triggers = space->sql_triggers;
 	zOldTableName = space_name(space);
 	assert(zOldTableName);
 	pTab = sqlite3HashFind(&db->pSchema->tblHash, zOldTableName);
@@ -4784,9 +4784,9 @@ case OP_RenameTable: {
 	 * due to lack of transactional DDL, but just do the best
 	 * effort.
 	 */
-	for (struct Trigger *trigger = triggers; trigger != NULL; ) {
+	for (struct sql_trigger *trigger = triggers; trigger != NULL; ) {
 		/* Store pointer as trigger will be destructed. */
-		struct Trigger *next_trigger = trigger->pNext;
+		struct sql_trigger *next_trigger = trigger->next;
 		rc = tarantoolSqlite3RenameTrigger(trigger->zName,
 						   zOldTableName, zNewTableName);
 		if (rc != SQLITE_OK) {
@@ -4849,8 +4849,6 @@ case OP_DropIndex: {
 	break;
 }
 
-#ifndef SQLITE_OMIT_TRIGGER
-
 /* Opcode: Program P1 P2 P3 P4 P5
  *
  * Execute the trigger program passed as P4 (type P4_SUBPROGRAM).
@@ -5006,8 +5004,6 @@ case OP_Param: {           /* out2 */
 	break;
 }
 
-#endif /* #ifndef SQLITE_OMIT_TRIGGER */
-
 #ifndef SQLITE_OMIT_FOREIGN_KEY
 /* Opcode: FkCounter P1 P2 * * *
  * Synopsis: fkctr[P1]+=P2
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 45a89d9..03ae44e 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -302,9 +302,7 @@ UnpackedRecord *sqlite3VdbeAllocUnpackedRecord(struct sqlite3 *,
 					       struct key_def *);
 int sql_vdbe_mem_alloc_region(Mem *, uint32_t);
 
-#ifndef SQLITE_OMIT_TRIGGER
 void sqlite3VdbeLinkSubProgram(Vdbe *, SubProgram *);
-#endif
 
 /* Use SQLITE_ENABLE_COMMENTS to enable generation of extra comments on
  * each VDBE opcode.
-- 
2.7.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] [PATCH v4 8/8] sql: remove global sql_trigger hash
       [not found] <cover.1530029141.git.kshcherbatov@tarantool.org>
  2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 6/8] sql: refactor AST trigger object name Kirill Shcherbatov
@ 2018-06-26 16:13 ` Kirill Shcherbatov
  2018-06-27 17:28   ` [tarantool-patches] " n.pettik
  1 sibling, 1 reply; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-26 16:13 UTC (permalink / raw)
  To: tarantool-patches; +Cc: n.pettik, Kirill Shcherbatov

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name
  2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 6/8] sql: refactor AST trigger object name Kirill Shcherbatov
@ 2018-06-27 15:57   ` n.pettik
  2018-06-27 16:35     ` Kirill Shcherbatov
  0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2018-06-27 15:57 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Kirill Shcherbatov

I see that in this patch you provide changes which are related not
only to renaming, but to refactoring of some functions, to removing
#ifdef SQLITE_OMIT_TRIGGER macros etc.

Lets mention these facts in commit message.

Moreover, if you placed this patch before ’sql: move Triggers to server ‘,
you would avoid additional diffs in alter.cc and trigger.c

> @@ -457,9 +456,10 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
> 		/* Mask of OLD.* columns in use */
> 		/* TODO: Could use temporary registers here. */
> 		uint32_t mask =
> -		    sqlite3TriggerColmask(parse, trigger_list, 0, 0,
> -					  TRIGGER_BEFORE | TRIGGER_AFTER, table,
> -					  onconf);
> +			sql_trigger_colmask(parse, trigger_list, 0, 0,
> +					    TRIGGER_BEFORE | TRIGGER_AFTER,
> +					    table,
> +					    onconf);

You can fit ’table’ and ‘onconf’ args in one line.

> @@ -3018,21 +3019,23 @@ struct Parse {
> 					 */
> 
> /*
> - * Each trigger present in the database schema is stored as an instance of
> - * struct Trigger.
> + * Each trigger present in the database schema is stored as an
> + * instance of struct sql_trigger.
>  *
>  * Pointers to instances of struct Trigger are stored in two ways.
> - * 1. In the "trigHash" hash table (part of the sqlite3* that represents the
> - *    database). This allows Trigger structures to be retrieved by name.
> - * 2. All triggers associated with a single table form a linked list, using the
> - *    pNext member of struct Trigger. A pointer to the first element of the
> - *    linked list is stored as the "pTrigger" member of the associated
> - *    struct Table.
> - *
> - * The "step_list" member points to the first element of a linked list
> - * containing the SQL statements specified as the trigger program.
> - */
> -struct Trigger {
> + * 1. In the "trigHash" hash table (part of the sqlite3* that
> + *    represents the database). This allows Trigger structures to
> + *    be retrieved by name.
> + * 2. All triggers associated with a single table form a linked
> + *    list, using the next member of struct sql_trigger. A pointer
> + *    to the first element of the linked list is stored as the
> + *    "pTrigger" member of the associated struct Table.
> + *
> + * The "step_list" member points to the first element of a linked
> + * list containing the SQL statements specified as the trigger
> + * program.
> + */
> +struct sql_trigger {
> 	char *zName;		/* The name of the trigger                        */
> 	/** The ID of space the trigger refers to. */
> 	uint32_t space_id;
> @@ -3042,7 +3045,8 @@ struct Trigger {
> 	IdList *pColumns;	/* If this is an UPDATE OF <column-list> trigger,
> 				   the <column-list> is stored here */
> 	TriggerStep *step_list;	/* Link list of trigger program steps             */
> -	Trigger *pNext;		/* Next trigger associated with the table */
> +	/** Next trigger associated with the table. */
> +	struct sql_trigger *next;
> };

I would also fix comments inside struct definition.

> @@ -4064,17 +4069,143 @@
> +void
> +sql_trigger_begin(struct Parse *parse, struct Token *name, int tr_tm,
> +		  int op, struct IdList *columns, struct SrcList *table,
> +		  struct Expr *when, int no_err);
> +
> +/**
> + * This routine is called after all of the trigger actions have
> + * been parsed in order to complete the process of building the
> + * trigger.
> + *
> + * @param parse Parser context.
> + * @param step_list The triggered program.
> + * @param token Token that describes the complete CREATE TRIGGER.
> + */
> +void sql_trigger_finish(Parse *, TriggerStep *, Token *);

Place type of return value at separate line. Also add names of args to prototype.

> -/*
> - * This routine is called after all of the trigger actions have been parsed
> - * in order to complete the process of building the trigger.
> - */
> void
> -sqlite3FinishTrigger(Parse * pParse,	/* Parser context */
> -		     TriggerStep * pStepList,	/* The triggered program */
> -		     Token * pAll	/* Token that describes the complete CREATE TRIGGER */
> -    )
> +sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
> +		   struct Token *token)
> {
> 	/* Trigger being finished. */
> -	Trigger *pTrig = pParse->parsed_ast.trigger;
> -	char *zName;		/* Name of trigger */
> -	char *zSql = 0;		/* SQL text */
> -	char *zOpts = 0;	/* MsgPack containing SQL options */
> -	sqlite3 *db = pParse->db;	/* The database */
> -	DbFixer sFix;		/* Fixer object */
> -	Token nameToken;	/* Trigger name for error reporting */
> -
> -	pParse->parsed_ast.trigger = NULL;
> -	if (NEVER(pParse->nErr) || !pTrig)
> +	struct sql_trigger *trigger = parse->parsed_ast.trigger;
> +	/* Name of trigger. */
> +	char *trigger_name;

You don’t need to declare all variables beforehand: its obsolete style
used in SQLite.

> +	/* SQL text. */
> +	char *sql_str = NULL;
> +	/* MsgPack containing SQL options. */
> +	char *opts_buff = NULL;
> +	/* The database. */
> +	struct sqlite3 *db = parse->db;
> +
> +	parse->parsed_ast.trigger = NULL;
> +	if (NEVER(parse->nErr) || trigger == NULL)
> 		goto triggerfinish_cleanup;
> -	zName = pTrig->zName;
> -	pTrig->step_list = pStepList;
> -	while (pStepList) {
> -		pStepList->pTrig = pTrig;
> -		pStepList = pStepList->pNext;
> +	trigger_name = trigger->zName;
> +	trigger->step_list = step_list;
> +	while (step_list != NULL) {
> +		step_list->trigger = trigger;
> +		step_list = step_list->pNext;
> 	}
> -	sqlite3TokenInit(&nameToken, pTrig->zName);
> -	sqlite3FixInit(&sFix, pParse, "trigger", &nameToken);
> -	if (sqlite3FixTriggerStep(&sFix, pTrig->step_list)
> -	    || sqlite3FixExpr(&sFix, pTrig->pWhen)
> -	    ) {
> +
> +	/* Trigger name for error reporting. */
> +	struct Token trigger_name_token;
> +	/* Fixer object. */
> +	struct DbFixer fixdb;
> +	sqlite3TokenInit(&trigger_name_token, trigger->zName);
> +	sqlite3FixInit(&fixdb, parse, "trigger", &trigger_name_token);
> +	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list)
> +	    || sqlite3FixExpr(&fixdb, trigger->pWhen))

Put ‘or’ operator at previous line:

	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
	    sqlite3FixExpr(&fixdb, trigger->pWhen))

> -/*
> +/**
>  * Create and populate a new TriggerPrg object with a sub-program
>  * implementing trigger pTrigger with ON CONFLICT policy orconf.
> + *
> + * @param parser Current parse context.
> + * @param trigger sql_trigger to code.
> + * @param table trigger is attached to.
> + * @param orconf ON CONFLICT policy to code trigger program with.
> + *
> + * @retval not NULL on success.
> + * @retval NULL on error.
>  */
> static TriggerPrg *
> -codeRowTrigger(Parse * pParse,	/* Current parse context */
> -	       Trigger * pTrigger,	/* Trigger to code */
> -	       Table * pTab,	/* The table pTrigger is attached to */
> -	       int orconf	/* ON CONFLICT policy to code trigger program with */
> -    )
> +sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
> +			struct Table *table, int orconf)

Lets call it vdbe_code_row_trigger() or smth like that.

> @@ -880,34 +868,37 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
> 	sql_parser_create(pSubParse, db);
> 	memset(&sNC, 0, sizeof(sNC));
> 	sNC.pParse = pSubParse;
> -	pSubParse->pTriggerTab = pTab;
> +	pSubParse->pTriggerTab = table;
> 	pSubParse->pToplevel = pTop;
> -	pSubParse->eTriggerOp = pTrigger->op;
> -	pSubParse->nQueryLoop = pParse->nQueryLoop;
> +	pSubParse->eTriggerOp = trigger->op;
> +	pSubParse->nQueryLoop = parser->nQueryLoop;
> 
> -	v = sqlite3GetVdbe(pSubParse);
> -	if (v) {
> +	/* Temporary VM. */

Why VM is temporary?

> +vdbe_code_row_trigger_direct(struct Parse *parser, struct sql_trigger *trigger,
> +			     struct Table *table, int reg, int orconf,
> +			     int ignore_jump)
> {
> -	Vdbe *v = sqlite3GetVdbe(pParse);	/* Main VM */
> -	TriggerPrg *pPrg;
> -	struct session *user_session = current_session();
> +	/* Main VM. */
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> 
> -	pPrg = getRowTrigger(pParse, p, pTab, orconf);
> -	assert(pPrg || pParse->nErr || pParse->db->mallocFailed);
> +	TriggerPrg *pPrg = sql_row_trigger(parser, trigger, table, orconf);
> +	assert(pPrg != NULL || parser->nErr != 0 ||
> +	       parser->db->mallocFailed != 0);
> 
> -	/* Code the OP_Program opcode in the parent VDBE. P4 of the OP_Program
> -	 * is a pointer to the sub-vdbe containing the trigger program.
> +	/*
> +	 * Code the OP_Program opcode in the parent VDBE. P4 of
> +	 * the OP_Program is a pointer to the sub-vdbe containing
> +	 * the trigger program.
> 	 */
> -	if (pPrg) {
> -		int bRecursive = (p->zName &&
> -				  0 ==
> -				  (user_session->
> -				   sql_flags & SQLITE_RecTriggers));
> -
> -		sqlite3VdbeAddOp4(v, OP_Program, reg, ignoreJump,
> -				  ++pParse->nMem, (const char *)pPrg->pProgram,
> -				  P4_SUBPROGRAM);
> -		VdbeComment((v, "Call: %s.%s", (p->zName ? p->zName : "fkey"),
> -			     onErrorText(orconf)));
> +	if (pPrg == NULL)
> +		return;
> 
> -		/* Set the P5 operand of the OP_Program instruction to non-zero if
> -		 * recursive invocation of this trigger program is disallowed. Recursive
> -		 * invocation is disallowed if (a) the sub-program is really a trigger,
> -		 * not a foreign key action, and (b) the flag to enable recursive triggers
> -		 * is clear.
> -		 */
> -		sqlite3VdbeChangeP5(v, (u8) bRecursive);
> -	}
> +	struct session *user_session = current_session();
> +	bool recursive = (trigger->zName && !(user_session->sql_flags &
> +					      SQLITE_RecTriggers));

We call bool variables with ‘is_’ or ‘if_’  prefix, as a rule.

> u32
> -sqlite3TriggerColmask(Parse * pParse,	/* Parse context */
> -		      Trigger * pTrigger,	/* List of triggers on table pTab */
> -		      ExprList * pChanges,	/* Changes list for any UPDATE OF triggers */
> -		      int isNew,	/* 1 for new.* ref mask, 0 for old.* ref mask */
> -		      int tr_tm,	/* Mask of TRIGGER_BEFORE|TRIGGER_AFTER */
> -		      Table * pTab,	/* The table to code triggers from */
> -		      int orconf	/* Default ON CONFLICT policy for trigger steps */
> -    )
> +sql_trigger_colmask(Parse *parser, struct sql_trigger *trigger,
> +		    ExprList *changes_list, int new, int tr_tm,
> +		    Table *table, int orconf)
> {
> -	const int op = pChanges ? TK_UPDATE : TK_DELETE;
> +	const int op = changes_list != NULL ? TK_UPDATE : TK_DELETE;
> 	u32 mask = 0;
> -	Trigger *p;
> 
> -	assert(isNew == 1 || isNew == 0);
> -	for (p = pTrigger; p; p = p->pNext) {
> +	assert(new == 1 || new == 0);

We call bool variables with ‘is_’ or ‘if_’ prefixes, as a rule.

> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name
  2018-06-27 15:57   ` [tarantool-patches] " n.pettik
@ 2018-06-27 16:35     ` Kirill Shcherbatov
  2018-06-27 17:41       ` n.pettik
  0 siblings, 1 reply; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-27 16:35 UTC (permalink / raw)
  To: tarantool-patches; +Cc: n.pettik

Thank you for review!

On 27.06.2018 18:57, n.pettik wrote:
> I see that in this patch you provide changes which are related not
> only to renaming, but to refactoring of some functions, to removing
> #ifdef SQLITE_OMIT_TRIGGER macros etc> Lets mention these facts in commit message.
Changed Trigger structure name to sql trigger to avoid
confusing similarity with Tarantool trigger structure.
Refactored related code to match tarantool codestyle.

> Moreover, if you placed this patch before ’sql: move Triggers to server ‘,
> you would avoid additional diffs in alter.cc and trigger.c
I believe that it is not critical.
First of all, I've changed triggers logic a lot, so some functions was dropped. The would be 
another useless diff on refactoring.
Then, It is not trivial to rebase this changes to be before 'move trigger comment' as 
I've reworked sqlite3{Begin, End}Trigger a lot and I don't like rebase constructive patch changes.
This, as usual, cause new bugs and typos. Let's do not mix important and not really changes.
This is just only refactoring patch and it doesn't really matter, where is it located.
 
> You can fit ’table’ and ‘onconf’ args in one line.
-                                           table,
-                                           onconf);
+                                           table, onconf);

> I would also fix comments inside struct definition.
 struct sql_trigger {
-       char *zName;            /* The name of the trigger                        */
+       /** The name of the trigger. */
+       char *zName;
        /** The ID of space the trigger refers to. */
        uint32_t space_id;
-       u8 op;                  /* One of TK_DELETE, TK_UPDATE, TK_INSERT         */
-       u8 tr_tm;               /* One of TRIGGER_BEFORE, TRIGGER_AFTER */
-       Expr *pWhen;            /* The WHEN clause of the expression (may be NULL) */
-       IdList *pColumns;       /* If this is an UPDATE OF <column-list> trigger,
-                                  the <column-list> is stored here */
-       TriggerStep *step_list; /* Link list of trigger program steps             */
+       /** One of TK_DELETE, TK_UPDATE, TK_INSERT. */
+       u8 op;
+       /** One of TRIGGER_BEFORE, TRIGGER_AFTER. */
+       u8 tr_tm;
+       /** The WHEN clause of the expression (may be NULL). */
+       Expr *pWhen;
+       /**
+        * If this is an UPDATE OF <column-list> trigger,
+        * the <column-list> is stored here
+        */
+       IdList *pColumns;
+       /** Link list of trigger program steps. */
+       TriggerStep *step_list;
        /** Next trigger associated with the table. */
        struct sql_trigger *next;
 };

> Place type of return value at separate line. Also add names of args to prototype.
-void sql_trigger_finish(Parse *, TriggerStep *, Token *);
+void
+sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
+                  struct Token *token);

> You don’t need to declare all variables beforehand: its obsolete style
> used in SQLite.
It is important here, as there is goto clause before first usage. 
This variable should be initialized with NULL. 

> Put ‘or’ operator at previous line:
-       if (sqlite3FixTriggerStep(&fixdb, trigger->step_list)
-           || sqlite3FixExpr(&fixdb, trigger->pWhen))
+       if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
+           sqlite3FixExpr(&fixdb, trigger->pWhen))

> 
> 	if (sqlite3FixTriggerStep(&fixdb, trigger->step_list) ||
> 	    sqlite3FixExpr(&fixdb, trigger->pWhen))
> 
>> -/*
>> +/**
>>  * Create and populate a new TriggerPrg object with a sub-program
>>  * implementing trigger pTrigger with ON CONFLICT policy orconf.
>> + *
>> + * @param parser Current parse context.
>> + * @param trigger sql_trigger to code.
>> + * @param table trigger is attached to.
>> + * @param orconf ON CONFLICT policy to code trigger program with.
>> + *
>> + * @retval not NULL on success.
>> + * @retval NULL on error.
>>  */
>> static TriggerPrg *
>> -codeRowTrigger(Parse * pParse,	/* Current parse context */
>> -	       Trigger * pTrigger,	/* Trigger to code */
>> -	       Table * pTab,	/* The table pTrigger is attached to */
>> -	       int orconf	/* ON CONFLICT policy to code trigger program with */
>> -    )
>> +sql_row_trigger_program(struct Parse *parser, struct sql_trigger *trigger,
>> +			struct Table *table, int orconf)
> 
> Lets call it vdbe_code_row_trigger() or smth like that.
I already have another one function vdbe_code_row_trigger(and also vdbe_code_row_trigger_direct)
that generates vdbe code.
As I see, sql_row_trigger and sql_row_trigger_program aimed to return TriggerPrg * pointer and MAY cause
generating VDBE code. Let's keep it as is.

> Why VM is temporary?
moped is not mine
this comment almost was there, I've just moved it; believe, this VM lives on stack af this function

> We call bool variables with ‘is_’ or ‘if_’  prefix, as a rule.
-       bool recursive = (trigger->zName && !(user_session->sql_flags &
-                                             SQLITE_RecTriggers));
+       bool is_recursive = (trigger->zName && !(user_session->sql_flags &
+                                                SQLITE_RecTriggers));

> We call bool variables with ‘is_’ or ‘if_’ prefixes, as a rule.
It is not boolean value.. It used as array index. For me, boolean array index is a little more
confusing.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 8/8] sql: remove global sql_trigger hash Kirill Shcherbatov
@ 2018-06-27 17:28   ` n.pettik
  2018-06-27 18:04     ` Kirill Shcherbatov
  0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2018-06-27 17:28 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Kirill Shcherbatov


> On 26 Jun 2018, at 19:13, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> As new _trigger format contain space_id,

Typo: ‘contains’.

> we can do not

https://www.urbandictionary.com/define.php?term=Can%20Don%27t

It is better to say: ‘we can avoid storing AST pointers in HASH’.

> store AST pointers in HASH. Requested AST could be found
> by name in appropriate space.

Personally I would squash this commit with ’sql: move Triggers to server’,
but it is up to you.

> @@ -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,

You can fit index_id and name_src in one line.

> @@ -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);

void
vdbe_code_drop_trigger...

And add arg names to prototype.

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

I don’t see such flag.

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

Lets drop ‘_ptr’ suffix.

> {
> -	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)

Lets call it with ’sql_’ prefix - this function is invoked from parser
(see for instance sql_drop_table(), sql_drop_index() etc).

> @@ -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;
> 	}
> +

Redundant diff.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name
  2018-06-27 16:35     ` Kirill Shcherbatov
@ 2018-06-27 17:41       ` n.pettik
  2018-06-27 18:04         ` Kirill Shcherbatov
  0 siblings, 1 reply; 12+ messages in thread
From: n.pettik @ 2018-06-27 17:41 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Kirill Shcherbatov


> -void sql_trigger_finish(Parse *, TriggerStep *, Token *);
> +void
> +sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
> +                  struct Token *token);
> 
>> You don’t need to declare all variables beforehand: its obsolete style
>> used in SQLite.
> It is important here, as there is goto clause before first usage. 
> This variable should be initialized with NULL. 

I mean not all of these variables are initialised. For example, you can move

/* Name of trigger. */
char *trigger_name;

closer to its usage. Surely, it is not vital, but anyway. 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-27 17:28   ` [tarantool-patches] " n.pettik
@ 2018-06-27 18:04     ` Kirill Shcherbatov
  2018-06-28 19:17       ` Vladislav Shpilevoy
  0 siblings, 1 reply; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-27 18:04 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Nikita Pettik

> Typo: ‘contains’.
> It is better to say: ‘we can avoid storing AST pointers in HASH’.
Ok, tnx:
    sql: remove global sql_trigger hash
    
    As new _trigger format contains space_id, we can avoid
    storing AST pointers in HASH. Requested AST could be found
    by name in appropriate space.
    
    Part of #3273.

> Personally I would squash this commit with ’sql: move Triggers to server’,
> but it is up to you.
I'd like to keep it as is.

> You can fit index_id and name_src in one line.
-                                 int index_id,
-                                 const char *name_src,
+                                 int index_id,const char *name_src,

> And add arg names to prototype.
-void vdbe_code_drop_trigger(Parse *, SrcList *, int);
+void
+vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err);

> I don’t see such flag.
- * present is raise_if_not_exists flag is set) in specified space.
+ * present - configure with cond_opcodeq) in specified space.
> Lets drop ‘_ptr’ suffix.
> Lets call it with ’sql_’ prefix - this function is invoked from parser
> (see for instance sql_drop_table(), sql_drop_index() etc).
-vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
+sql_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)

-vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name)
+vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name)

> Redundant diff.
dropped.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 6/8] sql: refactor AST trigger object name
  2018-06-27 17:41       ` n.pettik
@ 2018-06-27 18:04         ` Kirill Shcherbatov
  0 siblings, 0 replies; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-27 18:04 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Nikita Pettik

> I mean not all of these variables are initialised. For example, you can move
-       /* Name of trigger. */
-       char *trigger_name;
        /* SQL text. */
        char *sql_str = NULL;
        /* MsgPack containing SQL options. */
@@ -190,7 +188,7 @@ sql_trigger_finish(struct Parse *parse, struct TriggerStep *step_list,
        parse->parsed_ast.trigger = NULL;
        if (NEVER(parse->nErr) || trigger == NULL)
                goto triggerfinish_cleanup;
-       trigger_name = trigger->zName;
+       char *trigger_name = trigger->zName;

Ok, as you wish.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-27 18:04     ` Kirill Shcherbatov
@ 2018-06-28 19:17       ` Vladislav Shpilevoy
  2018-06-29 13:28         ` Kirill Shcherbatov
  0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-28 19:17 UTC (permalink / raw)
  To: tarantool-patches, Kirill Shcherbatov; +Cc: Nikita Pettik

Hello. Thanks for the patch! I have pushed my review fixes
as a separate commit for this patch and for
"sql: refactor trigger API functions".

On 27/06/2018 21:04, Kirill Shcherbatov wrote:
>> Typo: ‘contains’.
>> It is better to say: ‘we can avoid storing AST pointers in HASH’.
> Ok, tnx:
>      sql: remove global sql_trigger hash
>      
>      As new _trigger format contains space_id, we can avoid
>      storing AST pointers in HASH. Requested AST could be found
>      by name in appropriate space.
>      
>      Part of #3273.
> 
>> Personally I would squash this commit with ’sql: move Triggers to server’,
>> but it is up to you.
> I'd like to keep it as is.
> 
>> You can fit index_id and name_src in one line.
> -                                 int index_id,
> -                                 const char *name_src,
> +                                 int index_id,const char *name_src,
> 
>> And add arg names to prototype.
> -void vdbe_code_drop_trigger(Parse *, SrcList *, int);
> +void
> +vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err);
> 
>> I don’t see such flag.
> - * present is raise_if_not_exists flag is set) in specified space.
> + * present - configure with cond_opcodeq) in specified space.
>> Lets drop ‘_ptr’ suffix.
>> Lets call it with ’sql_’ prefix - this function is invoked from parser
>> (see for instance sql_drop_table(), sql_drop_index() etc).
> -vdbe_code_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
> +sql_drop_trigger(struct Parse *parser, struct SrcList *name, int no_err)
> 
> -vdbe_code_drop_trigger_ptr(struct Parse *parser, const char *trigger_name)
> +vdbe_code_drop_trigger(struct Parse *parser, const char *trigger_name)
> 
>> Redundant diff.
> dropped.
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-28 19:17       ` Vladislav Shpilevoy
@ 2018-06-29 13:28         ` Kirill Shcherbatov
  2018-06-29 13:31           ` Vladislav Shpilevoy
  0 siblings, 1 reply; 12+ messages in thread
From: Kirill Shcherbatov @ 2018-06-29 13:28 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Vladislav Shpilevoy

> Hello. Thanks for the patch! I have pushed my review fixes
> as a separate commit for this patch and for
> "sql: refactor trigger API functions".
Hello! Thank you for fixes!
I've spitted them and merged into appropriate commits.

Do we ready for merge it?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-29 13:28         ` Kirill Shcherbatov
@ 2018-06-29 13:31           ` Vladislav Shpilevoy
  2018-06-29 13:54             ` Kirill Yukhin
  0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-29 13:31 UTC (permalink / raw)
  To: Kirill Shcherbatov, tarantool-patches, Kirill Yukhin, Nikita Pettik



On 29/06/2018 16:28, Kirill Shcherbatov wrote:
>> Hello. Thanks for the patch! I have pushed my review fixes
>> as a separate commit for this patch and for
>> "sql: refactor trigger API functions".
> Hello! Thank you for fixes!
> I've spitted them and merged into appropriate commits.

Then LGTM. Now I think it is time to merge the whole
triggers patchset.

> 
> Do we ready for merge it?
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [tarantool-patches] Re: [PATCH v4 8/8] sql: remove global sql_trigger hash
  2018-06-29 13:31           ` Vladislav Shpilevoy
@ 2018-06-29 13:54             ` Kirill Yukhin
  0 siblings, 0 replies; 12+ messages in thread
From: Kirill Yukhin @ 2018-06-29 13:54 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: Kirill Shcherbatov, tarantool-patches, Nikita Pettik

On 29 июн 16:31, Vladislav Shpilevoy wrote:
> 
> 
> On 29/06/2018 16:28, Kirill Shcherbatov wrote:
> > > Hello. Thanks for the patch! I have pushed my review fixes
> > > as a separate commit for this patch and for
> > > "sql: refactor trigger API functions".
> > Hello! Thank you for fixes!
> > I've spitted them and merged into appropriate commits.
> 
> Then LGTM. Now I think it is time to merge the whole
> triggers patchset.
I've checked the patchset in 2.0 branch.


--
Regards, Kirill Yukhin

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-06-29 13:54 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1530029141.git.kshcherbatov@tarantool.org>
2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 6/8] sql: refactor AST trigger object name Kirill Shcherbatov
2018-06-27 15:57   ` [tarantool-patches] " n.pettik
2018-06-27 16:35     ` Kirill Shcherbatov
2018-06-27 17:41       ` n.pettik
2018-06-27 18:04         ` Kirill Shcherbatov
2018-06-26 16:13 ` [tarantool-patches] [PATCH v4 8/8] sql: remove global sql_trigger hash Kirill Shcherbatov
2018-06-27 17:28   ` [tarantool-patches] " n.pettik
2018-06-27 18:04     ` Kirill Shcherbatov
2018-06-28 19:17       ` Vladislav Shpilevoy
2018-06-29 13:28         ` Kirill Shcherbatov
2018-06-29 13:31           ` Vladislav Shpilevoy
2018-06-29 13:54             ` Kirill Yukhin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox