[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