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

Mergen Imeev imeevma at tarantool.org
Fri Apr 26 10:37:06 MSK 2019


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





More information about the Tarantool-patches mailing list