From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4E0C12DDC3 for ; Fri, 26 Apr 2019 03:37:10 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DykEgwHVKDbE for ; Fri, 26 Apr 2019 03:37:10 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D3CFF2DDC1 for ; Fri, 26 Apr 2019 03:37:09 -0400 (EDT) Date: Fri, 26 Apr 2019 10:37:06 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v1 3/3] sql: make SQL_TARANTOOL_ERROR the only errcode of OP_Halt Message-ID: <20190426073705.GB23413@tarantool.org> References: <2b1c4f5e030ffa41069a3839f6ba39826ea739e1.1555072183.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" Cc: tarantool-patches@freelists.org 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);