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

[Mergen Imeev <imeevma@tarantool.org>]

On Mon, Apr 15, 2019 at 05:06:32PM +0300, n.pettik wrote:
> 
> 
> > On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
> > 
> > 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.
> 
> Could you be more specific when pointing out the reason of removal?
> What was the feature you are removing and why it can be removed?
> Argument like ‘it is used only in debug mode’ doesn’t seem to be
> convincing enough.
> 
Fixed.

> > Part of #4074
> 
> Code involved in this patch doesn’t throw any errors,
> so why it is a part of diag replacement?
> 
> I guess this code clean-up can be OK, but we must
> be sure that this functionality can’t be applied to our
> SQL implementation.
> 
Removed.


New patch:

From 136d14a7e7b939f77e79a9c932e5bdb3d34e30d7 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Apr 2019 13:42:51 +0300
Subject: [PATCH] sql: remove mayAbort field from struct Parse

Currently, the mayAbort field is working with SQL error
SQL_CONSTRAINT. Since we want to replace SQL_CONSTRAINT with a
Tarantool error, we need to change the way the mayAbort field
works or delete it. Since this field is used only for make an
assertion in debug mode, it is better to simply remove it.

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 17d00d4..1151425 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);
@@ -2980,29 +2978,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.
@@ -3018,9 +2993,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 ba7eea5..6ac42d7 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4380,8 +4380,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 219e7b2..ae68ff3 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2575,7 +2575,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 */
@@ -3902,7 +3901,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 25d4cd7..140bb97 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -500,133 +500,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.