[tarantool-patches] [PATCH v1 0/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[tarantool-patches] [PATCH v1 0/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[imeevma]

Currently there are two ways to set an error in SQL - through
OP_Halt and through abort_due_to_error. After this patch, all
errors set via OP_Halt will be set using diag_set().

https://github.com/tarantool/tarantool/issues/4074
https://github.com/tarantool/tarantool/tree/imeevma/gh-4074-diag_set-in-op_halt

Mergen Imeev (3):
  sql: remove mayAbort field from struct Parse
  sql: rework diag_set() in OP_Halt
  sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

 src/box/sql/build.c             |  57 +-----------------
 src/box/sql/expr.c              |   9 ++-
 src/box/sql/fk_constraint.c     |  42 +------------
 src/box/sql/insert.c            |  31 +++++-----
 src/box/sql/select.c            |  17 +++---
 src/box/sql/sqlInt.h            |   3 -
 src/box/sql/trigger.c           |  12 ++--
 src/box/sql/vdbe.c              |  24 ++------
 src/box/sql/vdbe.h              |   3 -
 src/box/sql/vdbeaux.c           | 127 ----------------------------------------
 test/sql-tap/e_select1.test.lua |   4 +-
 test/sql-tap/limit.test.lua     |  26 ++++----
 test/sql-tap/select4.test.lua   |   4 +-
 test/sql/iproto.result          |  18 ++++--
 14 files changed, 68 insertions(+), 309 deletions(-)

-- 
2.7.4

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

[imeevma]

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

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

[n.pettik]

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

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

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

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

Hi! Thank you for review!

On 4/15/19 5:06 PM, 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.
I'll add that this allows us to remove SQL errcode SQL_CONSTRAINT.
>
>> 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.
>
In fact, this code works with the error code SQL_CONSTRAINT, and
if we want to remove this error code, we must think of a way to
replace it with some other similar checks. I think this will make
the code less understandable.


[-- Attachment #2: Type: text/html, Size: 2071 bytes --]

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

[Mergen Imeev]

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.

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

[n.pettik]

LGTM.

[tarantool-patches] [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[imeevma]

Before this patch it was possible to have an error code with wrong
error description. This patch fixes it.

Part of #4074
---
 src/box/sql/build.c             |  9 ++-------
 src/box/sql/select.c            | 17 ++++++++---------
 src/box/sql/trigger.c           | 12 ++++--------
 src/box/sql/vdbe.c              |  3 +--
 test/sql-tap/e_select1.test.lua |  4 ++--
 test/sql-tap/limit.test.lua     | 26 +++++++++++++-------------
 test/sql-tap/select4.test.lua   |  4 ++--
 test/sql/iproto.result          | 18 ++++++++++++------
 8 files changed, 44 insertions(+), 49 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index febb2e8..17cd0a0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1013,12 +1013,10 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	 * Lets check that constraint with this name hasn't
 	 * been created before.
 	 */
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
+					      ER_CONSTRAINT_EXISTS, name_copy,
 					      false, OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, 0, constr_tuple_reg + 3);
@@ -1400,13 +1398,10 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
 			  P4_DYNAMIC);
 	sqlVdbeAddOp2(vdbe, OP_Integer, child_id,  key_reg + 1);
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
-			   constraint_name);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      key_reg, 2, ER_NO_SUCH_CONSTRAINT,
-					      error_msg, false,
+					      constraint_name, false,
 					      OP_Found) != 0) {
 		sqlDbFree(parse_context->db, constraint_name);
 		return;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b1ec8c7..c0c526d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2113,6 +2113,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				  0, 0,
 				  wrong_limit_error,
 				  P4_STATIC);
+		sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 
 		sqlVdbeResolveLabel(v, positive_limit_label);
 		sqlReleaseTempReg(pParse, r1);
@@ -2140,13 +2141,12 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				sqlVdbeAddOp2(v, OP_Integer, 1, r1);
 				int no_err = sqlVdbeMakeLabel(v);
 				sqlVdbeAddOp3(v, OP_Eq, iLimit, no_err, r1);
-				const char *error =
-					"SQL error: Expression subquery could "
-					"be limited only with 1";
+				const char *error = "Expression subquery could "
+						    "be limited only with 1";
 				sqlVdbeAddOp4(v, OP_Halt,
 						  SQL_TARANTOOL_ERROR,
 						  0, 0, error, P4_STATIC);
-				sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
+				sqlVdbeChangeP5(v, ER_SQL);
 				sqlVdbeResolveLabel(v, no_err);
 				sqlReleaseTempReg(pParse, r1);
 
@@ -2176,6 +2176,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					  0, 0,
 					  wrong_offset_error,
 					  P4_STATIC);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 
 			sqlVdbeResolveLabel(v, positive_offset_label);
             		sqlReleaseTempReg(pParse, r1);
@@ -5443,11 +5444,9 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
 	int r1 = sqlGetTempReg(parser);
 	sqlVdbeAddOp2(v, OP_Integer, 0, r1);
 	sqlVdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg);
-	const char *error =
-		"SQL error: Expression subquery returned more than 1 row";
-	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, error,
-			  P4_STATIC);
-	sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
+	const char *error = "Expression subquery returned more than 1 row";
+	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, error, P4_STATIC);
+	sqlVdbeChangeP5(v, ER_SQL);
 	sqlReleaseTempReg(parser, r1);
 }
 
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 14e4198..9dd9d9f 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -100,9 +100,6 @@ sql_trigger_begin(struct Parse *parse)
 		struct Vdbe *v = sqlGetVdbe(parse);
 		if (v != NULL)
 			sqlVdbeCountChanges(v);
-		const char *error_msg =
-			tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS),
-				   trigger_name);
 		char *name_copy = sqlDbStrDup(db, trigger_name);
 		if (name_copy == NULL)
 			goto trigger_cleanup;
@@ -113,7 +110,8 @@ sql_trigger_begin(struct Parse *parse)
 		if (vdbe_emit_halt_with_presence_test(parse, BOX_TRIGGER_ID, 0,
 						      name_reg, 1,
 						      ER_TRIGGER_EXISTS,
-						      error_msg, (no_err != 0),
+						      trigger_name,
+						      (no_err != 0),
 						      OP_NoConflict) != 0)
 			goto trigger_cleanup;
 	}
@@ -412,9 +410,6 @@ sql_drop_trigger(struct Parse *parser)
 
 	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);
 	char *name_copy = sqlDbStrDup(db, trigger_name);
 	if (name_copy == NULL)
 		goto drop_trigger_cleanup;
@@ -422,7 +417,8 @@ sql_drop_trigger(struct Parse *parser)
 	sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC);
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
 					      name_reg, 1, ER_NO_SUCH_TRIGGER,
-					      error_msg, no_err, OP_Found) != 0)
+					      trigger_name,
+					      no_err, OP_Found) != 0)
 		goto drop_trigger_cleanup;
 
 	vdbe_code_drop_trigger(parser, trigger_name, true);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ed7bf88..3d20aa9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1073,8 +1073,7 @@ case OP_Halt: {
 			if (pOp->p4.z == NULL) {
 				assert(! diag_is_empty(diag_get()));
 			} else {
-				box_error_set(__FILE__, __LINE__, pOp->p5,
-					      pOp->p4.z);
+				diag_set(ClientError, pOp->p5, pOp->p4.z);
 			}
 		} else if (pOp->p5 != 0) {
 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 8e9a2bb..6f54b7f 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -2172,7 +2172,7 @@ for _, val in ipairs({
         "e_select-9.2."..tn,
         select,
         {
-            1, "Only positive integers are allowed in the LIMIT clause"})
+            1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"})
 end
 
 -- EVIDENCE-OF: R-03014-26414 If the LIMIT expression evaluates to a
@@ -2226,7 +2226,7 @@ for _, val in ipairs({
     test:do_catchsql_test(
         "e_select-9.7."..tn,
         select, {
-            1, "Only positive integers are allowed in the OFFSET clause"
+            1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         })
 
 end
diff --git a/test/sql-tap/limit.test.lua b/test/sql-tap/limit.test.lua
index 632c634..7ebf7b3 100755
--- a/test/sql-tap/limit.test.lua
+++ b/test/sql-tap/limit.test.lua
@@ -84,7 +84,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT 5 OFFSET -2
     ]], {
         -- <limit-1.2.13>
-        1 ,"Only positive integers are allowed in the OFFSET clause"
+        1 ,"Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-1.2.13>
     })
 
@@ -94,7 +94,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT 2, -5
     ]], {
         -- <limit-1.2.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-1.2.4>
     })
 
@@ -115,7 +115,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT -2, 5
     ]], {
         -- <limit-1.2.6>
-        1, "Only positive integers are allowed in the OFFSET clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-1.2.6>
     })
 
@@ -135,7 +135,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT -2, -5
     ]], {
         -- <limit-1.2.8>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-1.2.8>
     })
 
@@ -384,7 +384,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1 OFFSET -1;
     ]], {
         -- <limit-6.2>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.2>
     })
 
@@ -394,7 +394,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT 2 OFFSET -123;
     ]], {
         -- <limit-6.3>
-        1, "Only positive integers are allowed in the OFFSET clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-6.3>
     })
 
@@ -414,7 +414,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -432 OFFSET 2;
     ]], {
         -- <limit-6.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.4>
     })
 
@@ -434,7 +434,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1
     ]], {
         -- <limit-6.5>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.5>
     })
 
@@ -454,7 +454,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1 OFFSET 1
     ]], {
         -- <limit-6.6>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.6>
     })
 
@@ -734,7 +734,7 @@ test:do_test(
         return test:catchsql("SELECT x FROM t1 WHERE x<10 LIMIT "..limit)
     end, {
         -- <limit-10.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-10.4>
     })
 
@@ -745,7 +745,7 @@ test:do_test(
         return test:catchsql("SELECT x FROM t1 WHERE x<10 LIMIT "..limit)
     end, {
         -- <limit-10.5>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-10.5>
     })
 
@@ -1320,7 +1320,7 @@ test:do_catchsql_test(
         SELECT 123 LIMIT -1 OFFSET 0
     ]], {
         -- <limit-14.6.1>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-14.6.1>
     })
 
@@ -1340,7 +1340,7 @@ test:do_catchsql_test(
         SELECT 123 LIMIT -1 OFFSET 1
     ]], {
         -- <limit-14.7.1>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-14.7.1>
     })
 
diff --git a/test/sql-tap/select4.test.lua b/test/sql-tap/select4.test.lua
index 3aafedb..56dbaf5 100755
--- a/test/sql-tap/select4.test.lua
+++ b/test/sql-tap/select4.test.lua
@@ -990,7 +990,7 @@ test:do_catchsql_test(
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1
     ]], {
         -- <select4-10.4.1>
-    1,"Only positive integers are allowed in the LIMIT clause"
+    1,"Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </select4-10.4.1>
     })
 test:do_execsql_test(
@@ -1009,7 +1009,7 @@ test:do_catchsql_test(
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1 OFFSET 2
     ]], {
         -- <select4-10.5.1>
-        1,"Only positive integers are allowed in the LIMIT clause"
+        1,"Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </select4-10.5.1>
     })
 test:do_execsql_test(
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index e734872..aa36fab 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -164,15 +164,18 @@ cn:execute('select * from test limit ?', {2})
 ...
 cn:execute('select * from test limit ?', {-2})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit ?', {2.7})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit ?', {'Hello'})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {2})
 ---
@@ -188,15 +191,18 @@ cn:execute('select * from test limit 1 offset ?', {2})
 ...
 cn:execute('select * from test limit 1 offset ?', {-2})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {2.7})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {'Hello'})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 -- gh-2608 SQL iproto DDL
 cn:execute('create table test2(id int primary key, a int, b int, c int)')
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[n.pettik]

> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
> 
> Before this patch it was possible to have an error code with wrong
> error description. This patch fixes it.

Could you please supply this statement with an example(s)?

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[Imeev Mergen]

On 4/15/19 6:21 PM, n.pettik wrote:
>
>> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
>>
>> Before this patch it was possible to have an error code with wrong
>> error description. This patch fixes it.
> Could you please supply this statement with an example(s)?

Here it is:

tarantool> box.execute("CREATE TABLE t(a INT PRIMARY KEY, b INT);")
---
- row_count: 1
...

tarantool> box.execute("INSERT INTO t VALUES (1,2), (3,4);")
---
- row_count: 2
...

tarantool> box.execute("SELECT b FROM t WHERE a = (SELECT a FROM t);")
---
- error: 'SQL error: Expression subquery returned more than 1 row'
...

tarantool> box.error.last().code
---
- 159
...


 From the error message, we can say that this is an ER_SQL(160)
error. But this is wrong, as this is an ER_SQL_EXECUTE(159) error.

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[n.pettik]

> On 22 Apr 2019, at 11:24, Imeev Mergen <imeevma@tarantool.org> wrote:
> 
> 
> On 4/15/19 6:21 PM, n.pettik wrote:
>> 
>>> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
>>> 
>>> Before this patch it was possible to have an error code with wrong
>>> error description. This patch fixes it.
>> Could you please supply this statement with an example(s)?
> 
> Here it is:
> 
> tarantool> box.execute("CREATE TABLE t(a INT PRIMARY KEY, b INT);")
> ---
> - row_count: 1
> ...
> 
> tarantool> box.execute("INSERT INTO t VALUES (1,2), (3,4);")
> ---
> - row_count: 2
> ...
> 
> tarantool> box.execute("SELECT b FROM t WHERE a = (SELECT a FROM t);")
> ---
> - error: 'SQL error: Expression subquery returned more than 1 row'
> ...
> 
> tarantool> box.error.last().code
> ---
> - 159
> ...
> 
> 
> From the error message, we can say that this is an ER_SQL(160)
> error. But this is wrong, as this is an ER_SQL_EXECUTE(159) error.

I mean add this explanation to the commit message. Also, what is the
difference between ER_SQL and ER_SQL_EXECUTE? Their descriptions
look almost identical.

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[Mergen Imeev]

On Mon, Apr 15, 2019 at 06:21:55PM +0300, n.pettik wrote:
> 
> 
> > On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
> > 
> > Before this patch it was possible to have an error code with wrong
> > error description. This patch fixes it.
> 
> Could you please supply this statement with an example(s)?
> 
I changed the commit message. In fact, I found an error that had
no error code at all:
...

tarantool> box.execute('select 1 limit true')
---
- error: Only positive integers are allowed in the LIMIT clause
...

tarantool> box.error.last().code
---
- 0
...


I still think it would be better to use P3 instead of P5.


New patch:

From 4c1ad4e67773232168f13b0d501f4476673b57c6 Mon Sep 17 00:00:00 2001
Date: Fri, 12 Apr 2019 14:16:18 +0300
Subject: [PATCH] sql: rework diag_set() in OP_Halt

Prior to this patch, the way to set Tarantool error in OP_Halt was
too universal. It was possible to set a description of the error
that does not match its errcode. This change will also make it
easier to work with an error in OP_Halt, since you no longer need
to create a complete error message.

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 1151425..28dcbc3 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1004,12 +1004,10 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	 * Lets check that constraint with this name hasn't
 	 * been created before.
 	 */
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
+					      ER_CONSTRAINT_EXISTS, name_copy,
 					      false, OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
@@ -1392,13 +1390,10 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name,
 			  P4_DYNAMIC);
 	sqlVdbeAddOp2(vdbe, OP_Integer, child_id,  key_reg + 1);
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
-			   constraint_name);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      key_reg, 2, ER_NO_SUCH_CONSTRAINT,
-					      error_msg, false,
+					      constraint_name, false,
 					      OP_Found) != 0) {
 		sqlDbFree(parse_context->db, constraint_name);
 		return;
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d3472a9..3f0b540 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2116,6 +2116,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				  0, 0,
 				  wrong_limit_error,
 				  P4_STATIC);
+		sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 
 		sqlVdbeResolveLabel(v, positive_limit_label);
 		VdbeCoverage(v);
@@ -2142,9 +2143,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				sqlVdbeAddOp2(v, OP_Integer, 1, r1);
 				int no_err = sqlVdbeMakeLabel(v);
 				sqlVdbeAddOp3(v, OP_Eq, iLimit, no_err, r1);
-				const char *error =
-					"SQL error: Expression subquery could "
-					"be limited only with 1";
+				const char *error = "Expression subquery could "
+						    "be limited only with 1";
 				sqlVdbeAddOp4(v, OP_Halt,
 						  SQL_TARANTOOL_ERROR,
 						  0, 0, error, P4_STATIC);
@@ -2178,6 +2178,7 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					  0, 0,
 					  wrong_offset_error,
 					  P4_STATIC);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 
 			sqlVdbeResolveLabel(v, positive_offset_label);
             		sqlReleaseTempReg(pParse, r1);
@@ -5446,10 +5447,8 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
 	int r1 = sqlGetTempReg(parser);
 	sqlVdbeAddOp2(v, OP_Integer, 0, r1);
 	sqlVdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg);
-	const char *error =
-		"SQL error: Expression subquery returned more than 1 row";
-	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, error,
-			  P4_STATIC);
+	const char *error = "Expression subquery returned more than 1 row";
+	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, error, P4_STATIC);
 	sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 	sqlReleaseTempReg(parser, r1);
 }
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 4fdbb60..3005362 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -100,9 +100,6 @@ sql_trigger_begin(struct Parse *parse)
 		struct Vdbe *v = sqlGetVdbe(parse);
 		if (v != NULL)
 			sqlVdbeCountChanges(v);
-		const char *error_msg =
-			tt_sprintf(tnt_errcode_desc(ER_TRIGGER_EXISTS),
-				   trigger_name);
 		char *name_copy = sqlDbStrDup(db, trigger_name);
 		if (name_copy == NULL)
 			goto trigger_cleanup;
@@ -113,7 +110,8 @@ sql_trigger_begin(struct Parse *parse)
 		if (vdbe_emit_halt_with_presence_test(parse, BOX_TRIGGER_ID, 0,
 						      name_reg, 1,
 						      ER_TRIGGER_EXISTS,
-						      error_msg, (no_err != 0),
+						      trigger_name,
+						      (no_err != 0),
 						      OP_NoConflict) != 0)
 			goto trigger_cleanup;
 	}
@@ -412,9 +410,6 @@ sql_drop_trigger(struct Parse *parser)
 
 	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);
 	char *name_copy = sqlDbStrDup(db, trigger_name);
 	if (name_copy == NULL)
 		goto drop_trigger_cleanup;
@@ -422,7 +417,8 @@ sql_drop_trigger(struct Parse *parser)
 	sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC);
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
 					      name_reg, 1, ER_NO_SUCH_TRIGGER,
-					      error_msg, no_err, OP_Found) != 0)
+					      trigger_name,
+					      no_err, OP_Found) != 0)
 		goto drop_trigger_cleanup;
 
 	vdbe_code_drop_trigger(parser, trigger_name, true);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 5222a4e..9f0d760 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1032,12 +1032,9 @@ case OP_Halt: {
 	p->pc = pcx;
 	if (p->rc) {
 		if (p->rc == SQL_TARANTOOL_ERROR) {
-			if (pOp->p4.z == NULL) {
-				assert(! diag_is_empty(diag_get()));
-			} else {
-				box_error_set(__FILE__, __LINE__, pOp->p5,
-					      pOp->p4.z);
-			}
+			if (pOp->p4.z != NULL)
+				diag_set(ClientError, pOp->p5, pOp->p4.z);
+			assert(! diag_is_empty(diag_get()));
 		} else if (pOp->p5 != 0) {
 			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
 							       "FOREIGN KEY" };
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index c4724e6..0c626c9 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -2170,7 +2170,7 @@ for _, val in ipairs({
         "e_select-9.2."..tn,
         select,
         {
-            1, "Only positive integers are allowed in the LIMIT clause"})
+            1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"})
 end
 
 -- EVIDENCE-OF: R-03014-26414 If the LIMIT expression evaluates to a
@@ -2224,7 +2224,7 @@ for _, val in ipairs({
     test:do_catchsql_test(
         "e_select-9.7."..tn,
         select, {
-            1, "Only positive integers are allowed in the OFFSET clause"
+            1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         })
 
 end
diff --git a/test/sql-tap/limit.test.lua b/test/sql-tap/limit.test.lua
index 9b728d8..40b787b 100755
--- a/test/sql-tap/limit.test.lua
+++ b/test/sql-tap/limit.test.lua
@@ -84,7 +84,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT 5 OFFSET -2
     ]], {
         -- <limit-1.2.13>
-        1 ,"Only positive integers are allowed in the OFFSET clause"
+        1 ,"Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-1.2.13>
     })
 
@@ -94,7 +94,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT 2, -5
     ]], {
         -- <limit-1.2.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-1.2.4>
     })
 
@@ -115,7 +115,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT -2, 5
     ]], {
         -- <limit-1.2.6>
-        1, "Only positive integers are allowed in the OFFSET clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-1.2.6>
     })
 
@@ -135,7 +135,7 @@ test:do_catchsql_test(
         SELECT x FROM t1 ORDER BY x+1 LIMIT -2, -5
     ]], {
         -- <limit-1.2.8>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-1.2.8>
     })
 
@@ -384,7 +384,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1 OFFSET -1;
     ]], {
         -- <limit-6.2>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.2>
     })
 
@@ -394,7 +394,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT 2 OFFSET -123;
     ]], {
         -- <limit-6.3>
-        1, "Only positive integers are allowed in the OFFSET clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the OFFSET clause"
         -- </limit-6.3>
     })
 
@@ -414,7 +414,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -432 OFFSET 2;
     ]], {
         -- <limit-6.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.4>
     })
 
@@ -434,7 +434,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1
     ]], {
         -- <limit-6.5>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.5>
     })
 
@@ -454,7 +454,7 @@ test:do_catchsql_test(
         SELECT * FROM t6 LIMIT -1 OFFSET 1
     ]], {
         -- <limit-6.6>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-6.6>
     })
 
@@ -734,7 +734,7 @@ test:do_test(
         return test:catchsql("SELECT x FROM t1 WHERE x<10 LIMIT "..limit)
     end, {
         -- <limit-10.4>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-10.4>
     })
 
@@ -745,7 +745,7 @@ test:do_test(
         return test:catchsql("SELECT x FROM t1 WHERE x<10 LIMIT "..limit)
     end, {
         -- <limit-10.5>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-10.5>
     })
 
@@ -1320,7 +1320,7 @@ test:do_catchsql_test(
         SELECT 123 LIMIT -1 OFFSET 0
     ]], {
         -- <limit-14.6.1>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-14.6.1>
     })
 
@@ -1340,7 +1340,7 @@ test:do_catchsql_test(
         SELECT 123 LIMIT -1 OFFSET 1
     ]], {
         -- <limit-14.7.1>
-        1, "Only positive integers are allowed in the LIMIT clause"
+        1, "Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </limit-14.7.1>
     })
 
diff --git a/test/sql-tap/select4.test.lua b/test/sql-tap/select4.test.lua
index b78091b..1c0804b 100755
--- a/test/sql-tap/select4.test.lua
+++ b/test/sql-tap/select4.test.lua
@@ -990,7 +990,7 @@ test:do_catchsql_test(
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1
     ]], {
         -- <select4-10.4.1>
-    1,"Only positive integers are allowed in the LIMIT clause"
+    1,"Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </select4-10.4.1>
     })
 test:do_execsql_test(
@@ -1009,7 +1009,7 @@ test:do_catchsql_test(
         SELECT DISTINCT log FROM t1 ORDER BY log LIMIT -1 OFFSET 2
     ]], {
         -- <select4-10.5.1>
-        1,"Only positive integers are allowed in the LIMIT clause"
+        1,"Failed to execute SQL statement: Only positive integers are allowed in the LIMIT clause"
         -- </select4-10.5.1>
     })
 test:do_execsql_test(
@@ -1402,7 +1402,7 @@ test:do_catchsql_test(
         SELECT (VALUES(1),(2),(3),(4))
     ]], {
         -- <select4-14.10>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </select4-14.10>
     })
 
@@ -1412,7 +1412,7 @@ test:do_catchsql_test(
         SELECT (SELECT 1 UNION ALL SELECT 2 UNION ALL SELECT 3 UNION ALL SELECT 4)
     ]], {
         -- <select4-14.11>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </select4-14.11>
     })
 
diff --git a/test/sql-tap/subselect.test.lua b/test/sql-tap/subselect.test.lua
index 5b71390..ebfdf43 100755
--- a/test/sql-tap/subselect.test.lua
+++ b/test/sql-tap/subselect.test.lua
@@ -350,7 +350,7 @@ test:do_catchsql_test(
         SELECT (SELECT a FROM t5);
     ]], {
     -- <subselect-5.1>
-    1, "SQL error: Expression subquery returned more than 1 row"
+    1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
     -- </subselect-5.1>
 })
 
@@ -360,7 +360,7 @@ test:do_catchsql_test(
         SELECT b FROM t5 WHERE a = (SELECT a FROM t5 WHERE b=6);
     ]], {
     -- <subselect-5.2>
-    1, "SQL error: Expression subquery returned more than 1 row"
+    1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
     -- </subselect-5.2>
 })
 
@@ -380,7 +380,7 @@ test:do_catchsql_test(
         SELECT b FROM t1 WHERE a = (SELECT a FROM t1 WHERE b=6 LIMIT (SELECT b FROM t1 WHERE a =1));
     ]], {
     -- <subselect-5.2>
-    1, "SQL error: Expression subquery could be limited only with 1"
+    1, "Failed to execute SQL statement: Expression subquery could be limited only with 1"
     -- </subselect-5.2>
 })
 
diff --git a/test/sql-tap/tkt1473.test.lua b/test/sql-tap/tkt1473.test.lua
index 3e93203..ada18d0 100755
--- a/test/sql-tap/tkt1473.test.lua
+++ b/test/sql-tap/tkt1473.test.lua
@@ -125,7 +125,7 @@ test:do_catchsql_test(
         SELECT (SELECT 1 FROM t1 WHERE a=1 UNION ALL SELECT 2 FROM t1 WHERE b=0)
     ]], {
         -- <tkt1473-2.2>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-2.2>
     })
 
@@ -145,7 +145,7 @@ test:do_catchsql_test(
         SELECT (SELECT 1 FROM t1 WHERE a=1 UNION ALL SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-2.4>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-2.4>
     })
 
@@ -155,7 +155,7 @@ test:do_catchsql_test(
         SELECT (SELECT 1 FROM t1 WHERE a=1 UNION SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-2.5>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-2.5>
     })
 
@@ -165,7 +165,7 @@ test:do_catchsql_test(
         SELECT (SELECT 1 FROM t1 WHERE a=0 UNION ALL SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-2.6>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-2.6>
     })
 
@@ -206,7 +206,7 @@ test:do_catchsql_test(
           (SELECT 1 FROM t1 WHERE a=1 UNION ALL SELECT 2 FROM t1 WHERE b=0)
     ]], {
         -- <tkt1473-3.2>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-3.2>
     })
 
@@ -228,7 +228,7 @@ test:do_catchsql_test(
           (SELECT 1 FROM t1 WHERE a=1 UNION ALL SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-3.4>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-3.4>
     })
 
@@ -239,7 +239,7 @@ test:do_catchsql_test(
           (SELECT 1 FROM t1 WHERE a=1 UNION SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-3.5>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-3.5>
     })
 
@@ -250,7 +250,7 @@ test:do_catchsql_test(
           (SELECT 1 FROM t1 WHERE a=0 UNION ALL SELECT 2 FROM t1 WHERE b=4)
     ]], {
         -- <tkt1473-3.6>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-3.6>
     })
 
@@ -359,7 +359,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-4.3>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-4.3>
     })
 
@@ -389,7 +389,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-4.4>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-4.4>
     })
 
@@ -419,7 +419,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-4.5>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-4.5>
     })
 
@@ -449,7 +449,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-4.6>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-4.6>
     })
 
@@ -509,7 +509,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-5.3>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-5.3>
     })
 
@@ -539,7 +539,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-5.4>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-5.4>
     })
 
@@ -569,7 +569,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-5.5>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-5.5>
     })
 
@@ -599,7 +599,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-5.6>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-5.6>
     })
 
@@ -659,7 +659,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-6.3>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-6.3>
     })
 
@@ -689,7 +689,7 @@ test:do_catchsql_test(
         )
     ]], {
         -- <tkt1473-6.4>
-        1, "SQL error: Expression subquery returned more than 1 row"
+        1, "Failed to execute SQL statement: Expression subquery returned more than 1 row"
         -- </tkt1473-6.4>
     })
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 73497b4..9639ba7 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -164,15 +164,18 @@ cn:execute('select * from test limit ?', {2})
 ...
 cn:execute('select * from test limit ?', {-2})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit ?', {2.7})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit ?', {'Hello'})
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {2})
 ---
@@ -188,15 +191,18 @@ cn:execute('select * from test limit 1 offset ?', {2})
 ...
 cn:execute('select * from test limit 1 offset ?', {-2})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {2.7})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 cn:execute('select * from test limit 1 offset ?', {'Hello'})
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 -- gh-2608 SQL iproto DDL
 cn:execute('create table test2(id int primary key, a int, b int, c int)')
diff --git a/test/sql/types.result b/test/sql/types.result
index 8f442dc..8ec02f1 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -351,11 +351,13 @@ box.execute("SELECT true IN (1, 'abc', false)")
 ...
 box.execute("SELECT 1 LIMIT true;")
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 box.execute("SELECT 1 LIMIT 1 OFFSET true;")
 ---
-- error: Only positive integers are allowed in the OFFSET clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    OFFSET clause'
 ...
 box.execute("SELECT 'abc' || true;")
 ---
@@ -519,7 +521,8 @@ box.execute("SELECT b FROM t GROUP BY b LIMIT 1;")
 ...
 box.execute("SELECT b FROM t LIMIT true;")
 ---
-- error: Only positive integers are allowed in the LIMIT clause
+- error: 'Failed to execute SQL statement: Only positive integers are allowed in the
+    LIMIT clause'
 ...
 -- Most of aggregates don't accept boolean arguments.
 --

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[n.pettik]

> On 26 Apr 2019, at 10:48, Mergen Imeev <imeevma@tarantool.org> wrote:
> 
> On Mon, Apr 15, 2019 at 06:21:55PM +0300, n.pettik wrote:
>> 
>> 
>>> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
>>> 
>>> Before this patch it was possible to have an error code with wrong
>>> error description. This patch fixes it.
>> 
>> Could you please supply this statement with an example(s)?
>> 
> I changed the commit message. In fact, I found an error that had
> no error code at all:

Could you fix that?

> ...
> 
> tarantool> box.execute('select 1 limit true')
> ---
> - error: Only positive integers are allowed in the LIMIT clause
> ...
> 
> tarantool> box.error.last().code
> ---
> - 0
> ...
> 
> 
> I still think it would be better to use P3 instead of P5.

Does it matter?

> 
> 
> New patch:
> 
> From 4c1ad4e67773232168f13b0d501f4476673b57c6 Mon Sep 17 00:00:00 2001
> Date: Fri, 12 Apr 2019 14:16:18 +0300
> Subject: [PATCH] sql: rework diag_set() in OP_Halt
> 
> Prior to this patch, the way to set Tarantool error in OP_Halt was
> too universal. It was possible to set a description of the error
> that does not match its errcode. This change will also make it
> easier to work with an error in OP_Halt, since you no longer need
> to create a complete error message.

Please, inline exactly the same example as you shown
in previous mail.

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[Imeev Mergen]

Hi! Thank you for review. Next time I will send this patch as part
of "diag_set() in vdbe" patch-set.

On 4/29/19 2:35 AM, n.pettik wrote:
>
>> On 26 Apr 2019, at 10:48, Mergen Imeev <imeevma@tarantool.org> wrote:
>>
>> On Mon, Apr 15, 2019 at 06:21:55PM +0300, n.pettik wrote:
>>>
>>>> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
>>>>
>>>> Before this patch it was possible to have an error code with wrong
>>>> error description. This patch fixes it.
>>> Could you please supply this statement with an example(s)?
>>>
>> I changed the commit message. In fact, I found an error that had
>> no error code at all:
> Could you fix that?
Already done in this commit.
>
>> ...
>>
>> tarantool> box.execute('select 1 limit true')
>> ---
>> - error: Only positive integers are allowed in the LIMIT clause
>> ...
>>
>> tarantool> box.error.last().code
>> ---
>> - 0
>> ...
>>
>>
>> I still think it would be better to use P3 instead of P5.
> Does it matter?
This will allow using only one statement instead of two. The error
described above was caused by the lack of the second statement.
After this patch, this case is less likely, because if there is no
second operator, an “unknown error” will be shown.

[tarantool-patches] Re: [PATCH v1 2/3] sql: rework diag_set() in OP_Halt

[Imeev Mergen]

On 4/29/19 2:35 AM, n.pettik wrote:
>
>> On 26 Apr 2019, at 10:48, Mergen Imeev <imeevma@tarantool.org> wrote:
>> New patch:
>>
>>  From 4c1ad4e67773232168f13b0d501f4476673b57c6 Mon Sep 17 00:00:00 2001
>> Date: Fri, 12 Apr 2019 14:16:18 +0300
>> Subject: [PATCH] sql: rework diag_set() in OP_Halt
>>
>> Prior to this patch, the way to set Tarantool error in OP_Halt was
>> too universal. It was possible to set a description of the error
>> that does not match its errcode. This change will also make it
>> easier to work with an error in OP_Halt, since you no longer need
>> to create a complete error message.
> Please, inline exactly the same example as you shown
> in previous mail.
I do not think that one example will be useful now, since we
replaced ER_SQL with ER_SQL_EXECUTE in previous patches. I am
going to include this one:
...

tarantool> box.execute('select 1 limit true')
---
- error: Only positive integers are allowed in the LIMIT clause
...

tarantool> box.error.last().code
---
- 0
...

[tarantool-patches] [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[imeevma]

After this patch, the only error code that the OP_Halt opcode
will work with is SQL_TARANTOOL_ERROR.

Part of #4074
---
 src/box/sql/build.c         | 20 --------------------
 src/box/sql/expr.c          |  7 ++++---
 src/box/sql/fk_constraint.c |  7 +++----
 src/box/sql/insert.c        | 29 ++++++++++++++---------------
 src/box/sql/sqlInt.h        |  1 -
 src/box/sql/vdbe.c          | 23 ++++-------------------
 6 files changed, 25 insertions(+), 62 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 17cd0a0..922013b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2978,26 +2978,6 @@ sql_set_multi_write(struct Parse *parse_context, bool is_set)
 	pToplevel->isMultiWrite |= is_set;
 }
 
-/*
- * 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.
- */
-void
-sqlHaltConstraint(Parse * pParse,	/* Parsing context */
-		      int errCode,	/* extended error code */
-		      int onError,	/* Constraint type */
-		      char *p4,	/* Error message */
-		      i8 p4type,	/* P4_STATIC or P4_TRANSIENT */
-		      u8 p5Errmsg	/* P5_ErrMsg type */
-    )
-{
-	Vdbe *v = sqlGetVdbe(pParse);
-	assert((errCode & 0xff) == SQL_CONSTRAINT);
-	sqlVdbeAddOp4(v, OP_Halt, errCode, onError, 0, p4, p4type);
-	sqlVdbeChangeP5(v, p5Errmsg);
-}
-
 #ifndef SQL_OMIT_CTE
 /*
  * This routine is invoked once per CTE by the parser while parsing a
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index eb08f7e..ba41837 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 					  ON_CONFLICT_ACTION_IGNORE, 0,
 					  pExpr->u.zToken, 0);
 		} else {
-			sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
-					      pExpr->on_conflict_action,
-					      pExpr->u.zToken, 0, 0);
+			sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+				      pExpr->on_conflict_action, 0,
+				      pExpr->u.zToken, 0);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 		}
 		break;
 	}
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 7d36edc..602f439 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -287,10 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 		 * transaction.
 		 */
 		assert(incr_count == 1);
-		sqlHaltConstraint(parse_context,
-				      SQL_CONSTRAINT_FOREIGNKEY,
-				      ON_CONFLICT_ACTION_ABORT, 0, P4_STATIC,
-				      P5_ConstraintFK);
+		sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, "FOREIGN "\
+			      "KEY constraint failed", P4_STATIC);
+		sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 	} else {
 		sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
 				  incr_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f725478..dcadd7c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -865,7 +865,6 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 			    enum on_conflict_action on_conflict,
 			    int ignore_label, int *upd_cols)
 {
-	struct sql *db = parse_context->db;
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
 	bool is_update = upd_cols != NULL;
@@ -895,20 +894,18 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 		if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE &&
 		    dflt == NULL)
 			on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
-		char *err_msg;
+		const char *err;
 		int addr;
 		switch (on_conflict_nullable) {
 		case ON_CONFLICT_ACTION_ABORT:
 		case ON_CONFLICT_ACTION_ROLLBACK:
 		case ON_CONFLICT_ACTION_FAIL:
-			err_msg = sqlMPrintf(db, "%s.%s", def->name,
-						 def->fields[i].name);
-			sqlVdbeAddOp3(v, OP_HaltIfNull,
-					  SQL_CONSTRAINT_NOTNULL,
-					  on_conflict_nullable,
-					  new_tuple_reg + i);
-			sqlVdbeAppendP4(v, err_msg, P4_DYNAMIC);
-			sqlVdbeChangeP5(v, P5_ConstraintNotNull);
+			err = tt_sprintf("NOT NULL constraint failed: %s.%s",
+					 def->name, def->fields[i].name);
+			sqlVdbeAddOp4(v, OP_HaltIfNull, SQL_TARANTOOL_ERROR,
+				      on_conflict_nullable, new_tuple_reg + i,
+				      err, P4_STATIC);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 			break;
 		case ON_CONFLICT_ACTION_IGNORE:
 			sqlVdbeAddOp2(v, OP_IsNull, new_tuple_reg + i,
@@ -951,11 +948,13 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 				char *name = checks->a[i].zName;
 				if (name == NULL)
 					name = def->name;
-				sqlHaltConstraint(parse_context,
-						      SQL_CONSTRAINT_CHECK,
-						      on_conflict_check, name,
-						      P4_TRANSIENT,
-						      P5_ConstraintCheck);
+				const char *err =
+					tt_sprintf("CHECK constraint failed: "\
+						   "%s", name);
+				sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+					      on_conflict_check, 0, err,
+					      P4_STATIC);
+				sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 			}
 			sqlVdbeResolveLabel(v, all_ok);
 		}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index bc8ba49..53e19ac 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3976,7 +3976,6 @@ vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 
 void
 sql_set_multi_write(Parse *, bool);
-void sqlHaltConstraint(Parse *, int, int, char *, i8, u8);
 
 Expr *sqlExprDup(sql *, Expr *, int);
 SrcList *sqlSrcListDup(sql *, SrcList *, int);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3d20aa9..a9c3f68 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1069,27 +1069,12 @@ case OP_Halt: {
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
 	if (p->rc) {
-		if (p->rc == SQL_TARANTOOL_ERROR) {
-			if (pOp->p4.z == NULL) {
-				assert(! diag_is_empty(diag_get()));
-			} else {
-				diag_set(ClientError, pOp->p5, pOp->p4.z);
-			}
-		} else if (pOp->p5 != 0) {
-			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
-							       "FOREIGN KEY" };
-			testcase( pOp->p5==1);
-			testcase( pOp->p5==2);
-			testcase( pOp->p5==3);
-			testcase( pOp->p5==4);
-			sqlVdbeError(p, "%s constraint failed", azType[pOp->p5-1]);
-			if (pOp->p4.z) {
-				p->zErrMsg = sqlMPrintf(db, "%z: %s", p->zErrMsg, pOp->p4.z);
-			}
+		assert(p->rc == SQL_TARANTOOL_ERROR);
+		if (pOp->p4.z == NULL) {
+			assert(! diag_is_empty(diag_get()));
 		} else {
-			sqlVdbeError(p, "%s", pOp->p4.z);
+			diag_set(ClientError, pOp->p5, pOp->p4.z);
 		}
-		sql_log(pOp->p1, "abort at %d in [%s]: %s", pcx, p->zSql, p->zErrMsg);
 	}
 	rc = sqlVdbeHalt(p);
 	assert(rc==SQL_BUSY || rc==SQL_OK || rc==SQL_ERROR);
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[n.pettik]

'make ... the only errcode of OP_Halt’ ->
make … be the only ...

> On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
> 
> After this patch, the only error code that the OP_Halt opcode
> will work with is SQL_TARANTOOL_ERROR.

So, why do we need it at all now? Let’s use simple flag
is_aborted like in parser.

> 
> Part of #4074
> ---
> src/box/sql/build.c         | 20 --------------------
> src/box/sql/expr.c          |  7 ++++---
> src/box/sql/fk_constraint.c |  7 +++----
> src/box/sql/insert.c        | 29 ++++++++++++++---------------
> src/box/sql/sqlInt.h        |  1 -
> src/box/sql/vdbe.c          | 23 ++++-------------------
> 6 files changed, 25 insertions(+), 62 deletions(-)
> 
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index eb08f7e..ba41837 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> 					  ON_CONFLICT_ACTION_IGNORE, 0,
> 					  pExpr->u.zToken, 0);
> 		} else {
> -			sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
> -					      pExpr->on_conflict_action,
> -					      pExpr->u.zToken, 0, 0);
> +			sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +				      pExpr->on_conflict_action, 0,
> +				      pExpr->u.zToken, 0);
> +			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);

Are there any other options for P5 argument,
except for ER_SQL_EXECUTE? If not so, let’s always
assume it is ER_SQL_EXECUTE and avoid passing
extra argument.

> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3d20aa9..a9c3f68 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1069,27 +1069,12 @@ case OP_Halt: {
> 	p->errorAction = (u8)pOp->p2;
> 	p->pc = pcx;
> 	if (p->rc) {
> -		if (p->rc == SQL_TARANTOOL_ERROR) {
> -			if (pOp->p4.z == NULL) {
> -				assert(! diag_is_empty(diag_get()));
> -			} else {
> -				diag_set(ClientError, pOp->p5, pOp->p4.z);
> -			}
> -		} else if (pOp->p5 != 0) {
> -			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> -							       "FOREIGN KEY" };
> -			testcase( pOp->p5==1);
> -			testcase( pOp->p5==2);
> -			testcase( pOp->p5==3);
> -			testcase( pOp->p5==4);
> -			sqlVdbeError(p, "%s constraint failed", azType[pOp->p5-1]);
> -			if (pOp->p4.z) {
> -				p->zErrMsg = sqlMPrintf(db, "%z: %s", p->zErrMsg, pOp->p4.z);
> -			}
> +		assert(p->rc == SQL_TARANTOOL_ERROR);
> +		if (pOp->p4.z == NULL) {
> +			assert(! diag_is_empty(diag_get()));
> 		} else {
> -			sqlVdbeError(p, "%s", pOp->p4.z);
> +			diag_set(ClientError, pOp->p5, pOp->p4.z);
> 		}

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a9c3f688e..673d6a73f 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1070,11 +1070,9 @@ case OP_Halt: {
        p->pc = pcx;
        if (p->rc) {
                assert(p->rc == SQL_TARANTOOL_ERROR);
-               if (pOp->p4.z == NULL) {
-                       assert(! diag_is_empty(diag_get()));
-               } else {
+               if (pOp->p4.z != NULL)
                        diag_set(ClientError, pOp->p5, pOp->p4.z);
-               }
+               assert(! diag_is_empty(diag_get()));
        }
        rc = sqlVdbeHalt(p);
        assert(rc==SQL_BUSY || rc==SQL_OK || rc==SQL_ERROR);

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[Imeev Mergen]

On 4/15/19 6:19 PM, n.pettik wrote:
> 'make ... the only errcode of OP_Halt’ ->
> make … be the only ...
>
>> On 12 Apr 2019, at 15:34,imeevma@tarantool.org  wrote:
>>
>> After this patch, the only error code that the OP_Halt opcode
>> will work with is SQL_TARANTOOL_ERROR.
> So, why do we need it at all now? Let’s use simple flag
> is_aborted like in parser.
I don't think we should do is_aborted in the current situation.
All errors that go through OP_Halt are errors that were detected
by VDBE means. It can be said that this is a separate type of
error. Another type of errors go through abort_due_to_error and do
not work with OP_Halt opcode.

Currently, error handling in OP_Halt is quite complex, so I try to
make it simpler and clearer. This comes with the diag_set()
implementation here.

But I think we will add the is_aborted field after reworking errors
of the other type.

>> Part of #4074
>> ---
>> src/box/sql/build.c         | 20 --------------------
>> src/box/sql/expr.c          |  7 ++++---
>> src/box/sql/fk_constraint.c |  7 +++----
>> src/box/sql/insert.c        | 29 ++++++++++++++---------------
>> src/box/sql/sqlInt.h        |  1 -
>> src/box/sql/vdbe.c          | 23 ++++-------------------
>> 6 files changed, 25 insertions(+), 62 deletions(-)
>>
>>
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index eb08f7e..ba41837 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>> 					  ON_CONFLICT_ACTION_IGNORE, 0,
>> 					  pExpr->u.zToken, 0);
>> 		} else {
>> -			sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
>> -					      pExpr->on_conflict_action,
>> -					      pExpr->u.zToken, 0, 0);
>> +			sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
>> +				      pExpr->on_conflict_action, 0,
>> +				      pExpr->u.zToken, 0);
>> +			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
> Are there any other options for P5 argument,
> except for ER_SQL_EXECUTE? If not so, let’s always
> assume it is ER_SQL_EXECUTE and avoid passing
> extra argument.
At the moment OP_Halt works with five errors: ER_TRIGGER_EXISTS,
ER_NO_SUCH_TRIGGER, ER_CONSTRAINT_EXISTS, ER_NO_SUCH_CONSTRAINT,
ER_SQL_EXECUTE. In my patch, I added ER_SQL, since some error
messages had the prefix of this error.

But I can move errcode from p5 to p3. Should I do this?

Also, should I use ER_SQL_EXECUTE instead of ER_SQL? I have to
edit tests in this case.

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[Imeev Mergen]

On 4/22/19 11:47 AM, Imeev Mergen wrote:
>
> On 4/15/19 6:19 PM, n.pettik wrote:
>> 'make ... the only errcode of OP_Halt’ ->
>> make … be the only ...
>>
>>> On 12 Apr 2019, at 15:34,imeevma@tarantool.org  wrote:
>>>
>>> After this patch, the only error code that the OP_Halt opcode
>>> will work with is SQL_TARANTOOL_ERROR.
>> So, why do we need it at all now? Let’s use simple flag
>> is_aborted like in parser.
> I don't think we should do is_aborted in the current situation.
> All errors that go through OP_Halt are errors that were detected
> by VDBE means. It can be said that this is a separate type of
> error. Another type of errors go through abort_due_to_error and do
> not work with OP_Halt opcode.
>
> Currently, error handling in OP_Halt is quite complex, so I try to
> make it simpler and clearer. This comes with the diag_set()
> implementation here.
>
> But I think we will add the is_aborted field after reworking errors
> of the other type.
>
Thinking a little, I realized that the idea to add is_aborted is
very good. I will try to do this and send a new patch later.
>>> Part of #4074
>>> ---
>>> src/box/sql/build.c         | 20 --------------------
>>> src/box/sql/expr.c          |  7 ++++---
>>> src/box/sql/fk_constraint.c |  7 +++----
>>> src/box/sql/insert.c        | 29 ++++++++++++++---------------
>>> src/box/sql/sqlInt.h        |  1 -
>>> src/box/sql/vdbe.c          | 23 ++++-------------------
>>> 6 files changed, 25 insertions(+), 62 deletions(-)
>>>
>>>
>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index eb08f7e..ba41837 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * 
>>> pExpr, int target)
>>>                       ON_CONFLICT_ACTION_IGNORE, 0,
>>>                       pExpr->u.zToken, 0);
>>>         } else {
>>> -            sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
>>> -                          pExpr->on_conflict_action,
>>> -                          pExpr->u.zToken, 0, 0);
>>> +            sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
>>> +                      pExpr->on_conflict_action, 0,
>>> +                      pExpr->u.zToken, 0);
>>> +            sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
>> Are there any other options for P5 argument,
>> except for ER_SQL_EXECUTE? If not so, let’s always
>> assume it is ER_SQL_EXECUTE and avoid passing
>> extra argument.
> At the moment OP_Halt works with five errors: ER_TRIGGER_EXISTS,
> ER_NO_SUCH_TRIGGER, ER_CONSTRAINT_EXISTS, ER_NO_SUCH_CONSTRAINT,
> ER_SQL_EXECUTE. In my patch, I added ER_SQL, since some error
> messages had the prefix of this error.
>
> But I can move errcode from p5 to p3. Should I do this?
>
> Also, should I use ER_SQL_EXECUTE instead of ER_SQL? I have to
> edit tests in this case.
>
>

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[Mergen Imeev]

On Mon, Apr 15, 2019 at 06:19:34PM +0300, n.pettik wrote:
> 'make ... the only errcode of OP_Halt’ ->
> make … be the only ...
> 
> > On 12 Apr 2019, at 15:34, imeevma@tarantool.org wrote:
> > 
> > After this patch, the only error code that the OP_Halt opcode
> > will work with is SQL_TARANTOOL_ERROR.
> 
> So, why do we need it at all now? Let’s use simple flag
> is_aborted like in parser.
> 
I could not do it now. I think we will do this when rc is one of
SQL_OK, SQL_ROW, SQL_DONE or SQL_TARANTOOL_ERROR. This will be in
the next patch-set.

> > 
> > Part of #4074
> > ---
> > src/box/sql/build.c         | 20 --------------------
> > src/box/sql/expr.c          |  7 ++++---
> > src/box/sql/fk_constraint.c |  7 +++----
> > src/box/sql/insert.c        | 29 ++++++++++++++---------------
> > src/box/sql/sqlInt.h        |  1 -
> > src/box/sql/vdbe.c          | 23 ++++-------------------
> > 6 files changed, 25 insertions(+), 62 deletions(-)
> > 
> > 
> > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> > index eb08f7e..ba41837 100644
> > --- a/src/box/sql/expr.c
> > +++ b/src/box/sql/expr.c
> > @@ -4409,9 +4409,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
> > 					  ON_CONFLICT_ACTION_IGNORE, 0,
> > 					  pExpr->u.zToken, 0);
> > 		} else {
> > -			sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
> > -					      pExpr->on_conflict_action,
> > -					      pExpr->u.zToken, 0, 0);
> > +			sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> > +				      pExpr->on_conflict_action, 0,
> > +				      pExpr->u.zToken, 0);
> > +			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
> 
> Are there any other options for P5 argument,
> except for ER_SQL_EXECUTE? If not so, let’s always
> assume it is ER_SQL_EXECUTE and avoid passing
> extra argument.
> 
ER_SQL_EXECUTE error is not the only one that can be here.
Currently P5 can be one of ER_SQL_EXECUTE, ER_CONSTRAINT_EXISTS,
ER_NO_SUCH_CONSTRAINT, ER_TRIGGER_EXISTS, ER_NO_SUCH_TRIGGER.

> > 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 3d20aa9..a9c3f68 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1069,27 +1069,12 @@ case OP_Halt: {
> > 	p->errorAction = (u8)pOp->p2;
> > 	p->pc = pcx;
> > 	if (p->rc) {
> > -		if (p->rc == SQL_TARANTOOL_ERROR) {
> > -			if (pOp->p4.z == NULL) {
> > -				assert(! diag_is_empty(diag_get()));
> > -			} else {
> > -				diag_set(ClientError, pOp->p5, pOp->p4.z);
> > -			}
> > -		} else if (pOp->p5 != 0) {
> > -			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> > -							       "FOREIGN KEY" };
> > -			testcase( pOp->p5==1);
> > -			testcase( pOp->p5==2);
> > -			testcase( pOp->p5==3);
> > -			testcase( pOp->p5==4);
> > -			sqlVdbeError(p, "%s constraint failed", azType[pOp->p5-1]);
> > -			if (pOp->p4.z) {
> > -				p->zErrMsg = sqlMPrintf(db, "%z: %s", p->zErrMsg, pOp->p4.z);
> > -			}
> > +		assert(p->rc == SQL_TARANTOOL_ERROR);
> > +		if (pOp->p4.z == NULL) {
> > +			assert(! diag_is_empty(diag_get()));
> > 		} else {
> > -			sqlVdbeError(p, "%s", pOp->p4.z);
> > +			diag_set(ClientError, pOp->p5, pOp->p4.z);
> > 		}
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a9c3f688e..673d6a73f 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1070,11 +1070,9 @@ case OP_Halt: {
>         p->pc = pcx;
>         if (p->rc) {
>                 assert(p->rc == SQL_TARANTOOL_ERROR);
> -               if (pOp->p4.z == NULL) {
> -                       assert(! diag_is_empty(diag_get()));
> -               } else {
> +               if (pOp->p4.z != NULL)
>                         diag_set(ClientError, pOp->p5, pOp->p4.z);
> -               }
> +               assert(! diag_is_empty(diag_get()));
>         }
>         rc = sqlVdbeHalt(p);
>         assert(rc==SQL_BUSY || rc==SQL_OK || rc==SQL_ERROR);
> 
>
Thanks, added. 


New patch:

From 4078b48ad8f61fb58b749928b1660d6a73c565ca Mon Sep 17 00:00:00 2001
Date: Fri, 12 Apr 2019 14:58:45 +0300
Subject: [PATCH] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

Currently, in OP_Halt, you can get a SQL error other than
SQL_TARANTOOL_ERROR, for example, the SQL_CONSTRAINT error. After
this patch, all errors going through OP_Halt will have SQL error
code SQL_TARANTOOL_ERROR and have diag set.

Part of #4074

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28dcbc3..32c101d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2972,26 +2972,6 @@ sql_set_multi_write(struct Parse *parse_context, bool is_set)
 	pToplevel->isMultiWrite |= is_set;
 }
 
-/*
- * 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.
- */
-void
-sqlHaltConstraint(Parse * pParse,	/* Parsing context */
-		      int errCode,	/* extended error code */
-		      int onError,	/* Constraint type */
-		      char *p4,	/* Error message */
-		      i8 p4type,	/* P4_STATIC or P4_TRANSIENT */
-		      u8 p5Errmsg	/* P5_ErrMsg type */
-    )
-{
-	Vdbe *v = sqlGetVdbe(pParse);
-	assert((errCode & 0xff) == SQL_CONSTRAINT);
-	sqlVdbeAddOp4(v, OP_Halt, errCode, onError, 0, p4, p4type);
-	sqlVdbeChangeP5(v, p5Errmsg);
-}
-
 #ifndef SQL_OMIT_CTE
 /*
  * This routine is invoked once per CTE by the parser while parsing a
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 6ac42d7..a4a2d71 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4386,9 +4386,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 					  ON_CONFLICT_ACTION_IGNORE, 0,
 					  pExpr->u.zToken, 0);
 		} else {
-			sqlHaltConstraint(pParse, SQL_CONSTRAINT_TRIGGER,
-					      pExpr->on_conflict_action,
-					      pExpr->u.zToken, 0, 0);
+			sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+				      pExpr->on_conflict_action, 0,
+				      pExpr->u.zToken, 0);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 		}
 		break;
 	}
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 7d36edc..602f439 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -287,10 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 		 * transaction.
 		 */
 		assert(incr_count == 1);
-		sqlHaltConstraint(parse_context,
-				      SQL_CONSTRAINT_FOREIGNKEY,
-				      ON_CONFLICT_ACTION_ABORT, 0, P4_STATIC,
-				      P5_ConstraintFK);
+		sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 0, "FOREIGN "\
+			      "KEY constraint failed", P4_STATIC);
+		sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 	} else {
 		sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
 				  incr_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f725478..dcadd7c 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -865,7 +865,6 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 			    enum on_conflict_action on_conflict,
 			    int ignore_label, int *upd_cols)
 {
-	struct sql *db = parse_context->db;
 	struct Vdbe *v = sqlGetVdbe(parse_context);
 	assert(v != NULL);
 	bool is_update = upd_cols != NULL;
@@ -895,20 +894,18 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 		if (on_conflict_nullable == ON_CONFLICT_ACTION_REPLACE &&
 		    dflt == NULL)
 			on_conflict_nullable = ON_CONFLICT_ACTION_ABORT;
-		char *err_msg;
+		const char *err;
 		int addr;
 		switch (on_conflict_nullable) {
 		case ON_CONFLICT_ACTION_ABORT:
 		case ON_CONFLICT_ACTION_ROLLBACK:
 		case ON_CONFLICT_ACTION_FAIL:
-			err_msg = sqlMPrintf(db, "%s.%s", def->name,
-						 def->fields[i].name);
-			sqlVdbeAddOp3(v, OP_HaltIfNull,
-					  SQL_CONSTRAINT_NOTNULL,
-					  on_conflict_nullable,
-					  new_tuple_reg + i);
-			sqlVdbeAppendP4(v, err_msg, P4_DYNAMIC);
-			sqlVdbeChangeP5(v, P5_ConstraintNotNull);
+			err = tt_sprintf("NOT NULL constraint failed: %s.%s",
+					 def->name, def->fields[i].name);
+			sqlVdbeAddOp4(v, OP_HaltIfNull, SQL_TARANTOOL_ERROR,
+				      on_conflict_nullable, new_tuple_reg + i,
+				      err, P4_STATIC);
+			sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 			break;
 		case ON_CONFLICT_ACTION_IGNORE:
 			sqlVdbeAddOp2(v, OP_IsNull, new_tuple_reg + i,
@@ -951,11 +948,13 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 				char *name = checks->a[i].zName;
 				if (name == NULL)
 					name = def->name;
-				sqlHaltConstraint(parse_context,
-						      SQL_CONSTRAINT_CHECK,
-						      on_conflict_check, name,
-						      P4_TRANSIENT,
-						      P5_ConstraintCheck);
+				const char *err =
+					tt_sprintf("CHECK constraint failed: "\
+						   "%s", name);
+				sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
+					      on_conflict_check, 0, err,
+					      P4_STATIC);
+				sqlVdbeChangeP5(v, ER_SQL_EXECUTE);
 			}
 			sqlVdbeResolveLabel(v, all_ok);
 		}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ae68ff3..311adc9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3901,7 +3901,6 @@ vdbe_emit_insertion_completion(struct Vdbe *v, struct space *space,
 
 void
 sql_set_multi_write(Parse *, bool);
-void sqlHaltConstraint(Parse *, int, int, char *, i8, u8);
 
 Expr *sqlExprDup(sql *, Expr *, int);
 SrcList *sqlSrcListDup(sql *, SrcList *, int);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9f0d760..85cec85 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1031,25 +1031,10 @@ case OP_Halt: {
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
 	if (p->rc) {
-		if (p->rc == SQL_TARANTOOL_ERROR) {
-			if (pOp->p4.z != NULL)
-				diag_set(ClientError, pOp->p5, pOp->p4.z);
-			assert(! diag_is_empty(diag_get()));
-		} else if (pOp->p5 != 0) {
-			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
-							       "FOREIGN KEY" };
-			testcase( pOp->p5==1);
-			testcase( pOp->p5==2);
-			testcase( pOp->p5==3);
-			testcase( pOp->p5==4);
-			sqlVdbeError(p, "%s constraint failed", azType[pOp->p5-1]);
-			if (pOp->p4.z) {
-				p->zErrMsg = sqlMPrintf(db, "%z: %s", p->zErrMsg, pOp->p4.z);
-			}
-		} else {
-			sqlVdbeError(p, "%s", pOp->p4.z);
-		}
-		sql_log(pOp->p1, "abort at %d in [%s]: %s", pcx, p->zSql, p->zErrMsg);
+		assert(p->rc == SQL_TARANTOOL_ERROR);
+		if (pOp->p4.z != NULL)
+			diag_set(ClientError, pOp->p5, pOp->p4.z);
+		assert(! diag_is_empty(diag_get()));
 	}
 	rc = sqlVdbeHalt(p);
 	assert(rc==SQL_BUSY || rc==SQL_OK || rc==SQL_ERROR);

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]



> On 26 Apr 2019, at 10:37, Mergen Imeev <imeevma@tarantool.org> wrote:
> 
> On Mon, Apr 15, 2019 at 06:19:34PM +0300, n.pettik wrote:
>> 'make ... the only errcode of OP_Halt’ ->
>> make … be the only ...
>> 
>>> On 12 Apr 2019, at 15:34, imeevma@tarantool.org <mailto:imeevma@tarantool.org> wrote:
>>> 
>>> After this patch, the only error code that the OP_Halt opcode
>>> will work with is SQL_TARANTOOL_ERROR.
>> 
>> So, why do we need it at all now? Let’s use simple flag
>> is_aborted like in parser.
>> 
> I could not do it now.
> I think we will do this when rc is one of
> SQL_OK, SQL_ROW, SQL_DONE or SQL_TARANTOOL_ERROR. This will be in
> the next patch-set.

That’s OK, but still don’t understand what prevents you from
doing it right now..BTW, I don’t see corresponding patch in the
patch-set you’ve already sent (next one).



[-- Attachment #2: Type: text/html, Size: 5498 bytes --]

[tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]


On 4/29/19 2:35 AM, n.pettik wrote:
>
>
>> On 26 Apr 2019, at 10:37, Mergen Imeev <imeevma@tarantool.org 
>> <mailto:imeevma@tarantool.org>> wrote:
>>
>> On Mon, Apr 15, 2019 at 06:19:34PM +0300, n.pettik wrote:
>>> 'make ... the only errcode of OP_Halt’ ->
>>> make … be the only ...
>>>
>>>> On 12 Apr 2019, at 15:34,imeevma@tarantool.org 
>>>> <mailto:imeevma@tarantool.org>wrote:
>>>>
>>>> After this patch, the only error code that the OP_Halt opcode
>>>> will work with is SQL_TARANTOOL_ERROR.
>>>
>>> So, why do we need it at all now? Let’s use simple flag
>>> is_aborted like in parser.
>>>
>> I could not do it now.
>> I think we will do this when rc is one of
>> SQL_OK, SQL_ROW, SQL_DONE or SQL_TARANTOOL_ERROR. This will be in
>> the next patch-set.
>
> That’s OK, but still don’t understand what prevents you from
> doing it right now..BTW, I don’t see corresponding patch in the
> patch-set you’ve already sent (next one).
The rc field is used to return some information from functions
that return an error code in case something went wrong. In this
regard, I think that we can completely remove this field instead
of replacing it with is_aborted. However, after I tried to do
this, I found that at the moment it had too many connections with
the code. I'm not quite sure that it will be safe to remove the rc
field right now. I would suggest removing some unused modules and
reworking the memory system into SQL before we remove the rc field
from the Vdbe structure.

[-- Attachment #2: Type: text/html, Size: 7397 bytes --]