[tarantool-patches] [PATCH v1 1/3] sql: remove mayAbort field from struct Parse

[imeevma@tarantool.org]

Currently, the mayAbort field is used only in one place in debug
mode and is not used in non-debug mode. This patch removes this
field.

Part of #4074
---
 src/box/sql/build.c         |  28 ----------
 src/box/sql/expr.c          |   2 -
 src/box/sql/fk_constraint.c |  35 ------------
 src/box/sql/insert.c        |   2 -
 src/box/sql/sqlInt.h        |   2 -
 src/box/sql/vdbe.h          |   3 --
 src/box/sql/vdbeaux.c       | 127 --------------------------------------------
 7 files changed, 199 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a06ba3e..febb2e8 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -152,8 +152,6 @@ sql_finish_coding(struct Parse *parse_context)
 	 * Begin by generating some termination code at the end
 	 * of the vdbe program
 	 */
-	assert(!parse_context->isMultiWrite ||
-	       sqlVdbeAssertMayAbort(v, parse_context->mayAbort));
 	int last_instruction = v->nOp;
 	if (parse_context->initiateTTrans)
 		sqlVdbeAddOp0(v, OP_TTransaction);
@@ -2986,29 +2984,6 @@ sql_set_multi_write(struct Parse *parse_context, bool is_set)
 }
 
 /*
- * The code generator calls this routine if is discovers that it is
- * possible to abort a statement prior to completion.  In order to
- * perform this abort without corrupting the database, we need to make
- * sure that the statement is protected by a statement transaction.
- *
- * Technically, we only need to set the mayAbort flag if the
- * isMultiWrite flag was previously set.  There is a time dependency
- * such that the abort must occur after the multiwrite.  This makes
- * some statements involving the REPLACE conflict resolution algorithm
- * go a little faster.  But taking advantage of this time dependency
- * makes it more difficult to prove that the code is correct (in
- * particular, it prevents us from writing an effective
- * implementation of sqlAssertMayAbort()) and so we have chosen
- * to take the safe route and skip the optimization.
- */
-void
-sqlMayAbort(Parse * pParse)
-{
-	Parse *pToplevel = sqlParseToplevel(pParse);
-	pToplevel->mayAbort = 1;
-}
-
-/*
  * Code an OP_Halt that causes the vdbe to return an SQL_CONSTRAINT
  * error. The onError parameter determines which (if any) of the statement
  * and/or current transaction is rolled back.
@@ -3024,9 +2999,6 @@ sqlHaltConstraint(Parse * pParse,	/* Parsing context */
 {
 	Vdbe *v = sqlGetVdbe(pParse);
 	assert((errCode & 0xff) == SQL_CONSTRAINT);
-	if (onError == ON_CONFLICT_ACTION_ABORT) {
-		sqlMayAbort(pParse);
-	}
 	sqlVdbeAddOp4(v, OP_Halt, errCode, onError, 0, p4, p4type);
 	sqlVdbeChangeP5(v, p5Errmsg);
 }
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 6ff41a5..eb08f7e 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4403,8 +4403,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			pParse->is_aborted = true;
 			return 0;
 		}
-		if (pExpr->on_conflict_action == ON_CONFLICT_ACTION_ABORT)
-			sqlMayAbort(pParse);
 		assert(!ExprHasProperty(pExpr, EP_IntValue));
 		if (pExpr->on_conflict_action == ON_CONFLICT_ACTION_IGNORE) {
 			sqlVdbeAddOp4(v, OP_Halt, SQL_OK,
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 8151c66..7d36edc 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -292,8 +292,6 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 				      ON_CONFLICT_ACTION_ABORT, 0, P4_STATIC,
 				      P5_ConstraintFK);
 	} else {
-		if (incr_count > 0 && !fk_def->is_deferred)
-			sqlMayAbort(parse_context);
 		sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
 				  incr_count);
 	}
@@ -634,41 +632,8 @@ fk_constraint_emit_check(struct Parse *parser, struct space *space, int reg_old,
 						    fk->def, reg_new, -1);
 		}
 		if (reg_old != 0) {
-			enum fk_constraint_action action = fk_def->on_update;
 			fk_constraint_scan_children(parser, src, space->def,
 						    fk->def, reg_old, 1);
-			/*
-			 * If this is a deferred FK constraint, or
-			 * a CASCADE or SET NULL action applies,
-			 * then any foreign key violations caused
-			 * by removing the parent key will be
-			 * rectified by the action trigger. So do
-			 * not set the "may-abort" flag in this
-			 * case.
-			 *
-			 * Note 1: If the FK is declared "ON
-			 * UPDATE CASCADE", then the may-abort
-			 * flag will eventually be set on this
-			 * statement anyway (when this function is
-			 * called as part of processing the UPDATE
-			 * within the action trigger).
-			 *
-			 * Note 2: At first glance it may seem
-			 * like sql could simply omit all
-			 * OP_FkCounter related scans when either
-			 * CASCADE or SET NULL applies. The
-			 * trouble starts if the CASCADE or SET
-			 * NULL action trigger causes other
-			 * triggers or action rules attached to
-			 * the child table to fire. In these cases
-			 * the fk constraint counters might be set
-			 * incorrectly if any OP_FkCounter related
-			 * scans are omitted.
-			 */
-			if (!fk_def->is_deferred &&
-			    action != FKEY_ACTION_CASCADE &&
-			    action != FKEY_ACTION_SET_NULL)
-				sqlMayAbort(parser);
 		}
 		sqlSrcListDelete(db, src);
 	}
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index c2aac55..f725478 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -899,8 +899,6 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 		int addr;
 		switch (on_conflict_nullable) {
 		case ON_CONFLICT_ACTION_ABORT:
-			sqlMayAbort(parse_context);
-			FALLTHROUGH;
 		case ON_CONFLICT_ACTION_ROLLBACK:
 		case ON_CONFLICT_ACTION_FAIL:
 			err_msg = sqlMPrintf(db, "%s.%s", def->name,
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b322602..bc8ba49 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2612,7 +2612,6 @@ struct Parse {
 	u8 colNamesSet;		/* TRUE after OP_ColumnName has been issued to pVdbe */
 	u8 nTempReg;		/* Number of temporary registers in aTempReg[] */
 	u8 isMultiWrite;	/* True if statement may modify/insert multiple rows */
-	u8 mayAbort;		/* True if statement may throw an ABORT exception */
 	u8 hasCompound;		/* Need to invoke convertCompoundSelectToSubquery() */
 	u8 okConstFactor;	/* OK to factor out constants */
 	u8 disableLookaside;	/* Number of times lookaside has been disabled */
@@ -3977,7 +3976,6 @@ vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 
 void
 sql_set_multi_write(Parse *, bool);
-void sqlMayAbort(Parse *);
 void sqlHaltConstraint(Parse *, int, int, char *, i8, u8);
 
 Expr *sqlExprDup(sql *, Expr *, int);
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index f9bb96f..6234dff 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -253,9 +253,6 @@ void sqlVdbeMakeReady(Vdbe *, Parse *);
 int sqlVdbeFinalize(Vdbe *);
 void sqlVdbeResolveLabel(Vdbe *, int);
 int sqlVdbeCurrentAddr(Vdbe *);
-#ifdef SQL_DEBUG
-int sqlVdbeAssertMayAbort(Vdbe *, int);
-#endif
 void sqlVdbeResetStepResult(Vdbe *);
 void sqlVdbeRewind(Vdbe *);
 int sqlVdbeReset(Vdbe *);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 0cc3c14..c8bc731 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -499,133 +499,6 @@ sqlVdbeRunOnlyOnce(Vdbe * p)
 	p->runOnlyOnce = 1;
 }
 
-#ifdef SQL_DEBUG		/* sqlAssertMayAbort() logic */
-
-/*
- * The following type and function are used to iterate through all opcodes
- * in a Vdbe main program and each of the sub-programs (triggers) it may
- * invoke directly or indirectly. It should be used as follows:
- *
- *   Op *pOp;
- *   VdbeOpIter sIter;
- *
- *   memset(&sIter, 0, sizeof(sIter));
- *   sIter.v = v;                            // v is of type Vdbe*
- *   while( (pOp = opIterNext(&sIter)) ){
- *     // Do something with pOp
- *   }
- *   sqlDbFree(v->db, sIter.apSub);
- *
- */
-typedef struct VdbeOpIter VdbeOpIter;
-struct VdbeOpIter {
-	Vdbe *v;		/* Vdbe to iterate through the opcodes of */
-	SubProgram **apSub;	/* Array of subprograms */
-	int nSub;		/* Number of entries in apSub */
-	int iAddr;		/* Address of next instruction to return */
-	int iSub;		/* 0 = main program, 1 = first sub-program etc. */
-};
-
-static Op *
-opIterNext(VdbeOpIter * p)
-{
-	Vdbe *v = p->v;
-	Op *pRet = 0;
-	Op *aOp;
-	int nOp;
-
-	if (p->iSub <= p->nSub) {
-
-		if (p->iSub == 0) {
-			aOp = v->aOp;
-			nOp = v->nOp;
-		} else {
-			aOp = p->apSub[p->iSub - 1]->aOp;
-			nOp = p->apSub[p->iSub - 1]->nOp;
-		}
-		assert(p->iAddr < nOp);
-
-		pRet = &aOp[p->iAddr];
-		p->iAddr++;
-		if (p->iAddr == nOp) {
-			p->iSub++;
-			p->iAddr = 0;
-		}
-
-		if (pRet->p4type == P4_SUBPROGRAM) {
-			int nByte = (p->nSub + 1) * sizeof(SubProgram *);
-			int j;
-			for (j = 0; j < p->nSub; j++) {
-				if (p->apSub[j] == pRet->p4.pProgram)
-					break;
-			}
-			if (j == p->nSub) {
-				p->apSub =
-				    sqlDbReallocOrFree(v->db, p->apSub,
-							   nByte);
-				if (!p->apSub) {
-					pRet = 0;
-				} else {
-					p->apSub[p->nSub++] = pRet->p4.pProgram;
-				}
-			}
-		}
-	}
-
-	return pRet;
-}
-
-/*
- * Check if the program stored in the VM associated with pParse may
- * throw an ABORT exception (causing the statement, but not entire transaction
- * to be rolled back). This condition is true if the main program or any
- * sub-programs contains any of the following:
- *
- *   *  OP_Halt with P1=SQL_CONSTRAINT and P2=ON_CONFLICT_ACTION_ABORT.
- *   *  OP_HaltIfNull with P1=SQL_CONSTRAINT and P2=ON_CONFLICT_ACTION_ABORT.
- *   *  OP_FkCounter with P2==0 (immediate foreign key constraint)
- *
- * Then check that the value of Parse.mayAbort is true if an
- * ABORT may be thrown, or false otherwise. Return true if it does
- * match, or false otherwise. This function is intended to be used as
- * part of an assert statement in the compiler. Similar to:
- *
- *   assert( sqlVdbeAssertMayAbort(pParse->pVdbe, pParse->mayAbort) );
- */
-int
-sqlVdbeAssertMayAbort(Vdbe * v, int mayAbort)
-{
-	int hasAbort = 0;
-	int hasFkCounter = 0;
-	Op *pOp;
-	VdbeOpIter sIter;
-	memset(&sIter, 0, sizeof(sIter));
-	sIter.v = v;
-
-	while ((pOp = opIterNext(&sIter)) != 0) {
-		int opcode = pOp->opcode;
-		if ((opcode == OP_Halt || opcode == OP_HaltIfNull) &&
-		    (pOp->p1 & 0xff) == SQL_CONSTRAINT &&
-	            pOp->p2 == ON_CONFLICT_ACTION_ABORT){
-			hasAbort = 1;
-			break;
-		}
-		if (opcode == OP_FkCounter && pOp->p1 == 0 && pOp->p2 == 1) {
-			hasFkCounter = 1;
-		}
-	}
-	sqlDbFree(v->db, sIter.apSub);
-
-	/* Return true if hasAbort==mayAbort. Or if a malloc failure occurred.
-	 * If malloc failed, then the while() loop above may not have iterated
-	 * through all opcodes and hasAbort may be set incorrectly. Return
-	 * true for this case to prevent the assert() in the callers frame
-	 * from failing.
-	 */
-	return (v->db->mallocFailed || hasAbort == mayAbort || hasFkCounter);
-}
-#endif				/* SQL_DEBUG - the sqlAssertMayAbort() function */
-
 /*
  * This routine is called after all opcodes have been inserted.  It loops
  * through all the opcodes and fixes up some details.
-- 
2.7.4