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 673A524889 for ; Mon, 15 Jul 2019 11:16:24 -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 vbxTVBBQNGYJ for ; Mon, 15 Jul 2019 11:16:24 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 F2A3524880 for ; Mon, 15 Jul 2019 11:16:23 -0400 (EDT) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE Date: Mon, 15 Jul 2019 18:16:21 +0300 Message-Id: <2c78dc1cbb024a18c2e10580d393a918f2a2210c.1563203545.git.imeevma@gmail.com> In-Reply-To: References: 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: v.shpilevoy@tarantool.org Cc: korablev@tarantool.org, tarantool-patches@freelists.org To separate the error setting and execution halting, a new opcode OP_SetDiag was created. The only functionality of the opcode is the execution of diag_set(). It is important to note that OP_SetDiag does not set is_aborted to true, so we can continue working with other opcodes, if necessary. This function allows us to perform cleanup in some special cases, for example, when creating a constraint failed because of the creation of two or more constraints with the same name in the same CREATE TABLE statement. Since now diag_set() is executed in OP_SetDiag, this functionality has been removed from OP_Halt. Needed for #4183 --- src/box/sql/build.c | 9 +++++---- src/box/sql/expr.c | 6 ++++-- src/box/sql/fk_constraint.c | 3 ++- src/box/sql/insert.c | 11 ++++++----- src/box/sql/select.c | 14 +++++++++----- src/box/sql/vdbe.c | 31 ++++++++++++++++++++----------- 6 files changed, 46 insertions(+), 28 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 396de63..e56b9da 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3297,15 +3297,16 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, int cursor = parser->nTab++; vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id)); sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); - int label = sqlVdbeCurrentAddr(v); - sqlVdbeAddOp4Int(v, cond_opcode, cursor, label + 3, key_reg, - key_len); + int addr = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, + key_len); if (no_error) { sqlVdbeAddOp0(v, OP_Halt); } else { - sqlVdbeAddOp4(v, OP_Halt, -1, 0, tarantool_error_code, error, + sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error, P4_DYNAMIC); + sqlVdbeAddOp1(v, OP_Halt, -1); } + sqlVdbeJumpHere(v, addr); sqlVdbeAddOp1(v, OP_Close, cursor); return 0; } diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 7673832..916b0a0 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -4339,8 +4339,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), pExpr->u.zToken); - sqlVdbeAddOp4(v, OP_Halt, -1, pExpr->on_conflict_action, - ER_SQL_EXECUTE, err, 0); + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, + P4_STATIC); + sqlVdbeAddOp2(v, OP_Halt, -1, + pExpr->on_conflict_action); } break; } diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c index 06c863a..4717677 100644 --- a/src/box/sql/fk_constraint.c +++ b/src/box/sql/fk_constraint.c @@ -287,8 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent, assert(incr_count == 1); const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), "FOREIGN KEY constraint failed"); - sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err, + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, P4_STATIC); + sqlVdbeAddOp1(v, OP_Halt, -1); } 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 b353148..4386140 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -810,9 +810,9 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, sqlExprIfTrue(parser, expr, check_is_passed, SQL_JUMPIFNULL); const char *fmt = tnt_errcode_desc(ER_CK_CONSTRAINT_FAILED); const char *error_msg = tt_sprintf(fmt, ck_constraint_name, expr_str); - sqlVdbeAddOp4(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT, - 0, sqlDbStrDup(parser->db, error_msg), P4_DYNAMIC); - sqlVdbeChangeP5(v, ER_CK_CONSTRAINT_FAILED); + sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0, + sqlDbStrDup(parser->db, error_msg), P4_DYNAMIC); + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); VdbeNoopComment((v, "END: ck constraint %s test", ck_constraint_name)); sqlVdbeResolveLabel(v, check_is_passed); } @@ -863,8 +863,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space, "failed: %s.%s", def->name, def->fields[i].name)); addr = sqlVdbeAddOp1(v, OP_NotNull, new_tuple_reg + i); - sqlVdbeAddOp4(v, OP_Halt, -1, on_conflict_nullable, - ER_SQL_EXECUTE, err, P4_STATIC); + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, + P4_STATIC); + sqlVdbeAddOp2(v, OP_Halt, -1, on_conflict_nullable); sqlVdbeJumpHere(v, addr); break; case ON_CONFLICT_ACTION_IGNORE: diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 7c8da25..bd144c1 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -2092,8 +2092,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) "Only positive integers are "\ "allowed in the LIMIT clause"); sqlVdbeResolveLabel(v, halt_label); - sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err, + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, P4_STATIC); + sqlVdbeAddOp1(v, OP_Halt, -1); sqlVdbeResolveLabel(v, positive_limit_label); VdbeCoverage(v); @@ -2124,8 +2125,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) err = tt_sprintf(err, "Expression subquery "\ "could be limited only "\ "with 1"); - sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, - err, P4_STATIC); + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, + 0, err, P4_STATIC); + sqlVdbeAddOp1(v, OP_Halt, -1); sqlVdbeResolveLabel(v, no_err); sqlReleaseTempReg(pParse, r1); @@ -2150,8 +2152,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak) "Only positive integers are allowed "\ "in the OFFSET clause"); sqlVdbeResolveLabel(v, offset_error_label); - sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err, + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, P4_STATIC); + sqlVdbeAddOp1(v, OP_Halt, -1); sqlVdbeResolveLabel(v, positive_offset_label); sqlReleaseTempReg(pParse, r1); @@ -5406,7 +5409,8 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma const char *error = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), "Expression subquery returned more "\ "than 1 row"); - sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, error, P4_STATIC); + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, error, P4_STATIC); + sqlVdbeAddOp1(v, OP_Halt, -1); sqlReleaseTempReg(parser, r1); } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 9f4ee7a..6a4a303 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -885,6 +885,24 @@ case OP_Goto: { /* jump */ goto jump_to_p2; } +/* Opcode: SetDiag P1 P2 * P4 * + * + * Set diag error. After that jump to address P2 if it is not 0. + * Otherwise, go to the next instruction. Note that is_aborted + * flag is not set in this case, which allows to continue VDBE + * execution. For instance, to provide auxiliary query-specific + * clean-up. + * + * P1 parameter is an error code to be set. The P4 parameter is a + * text description of the error. + */ +case OP_SetDiag: { /* jump */ + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z); + if (pOp->p2 != 0) + goto jump_to_p2; + break; +} + /* Opcode: Gosub P1 P2 * * * * * Write the current address onto register P1 @@ -989,7 +1007,7 @@ case OP_Yield: { /* in1, jump */ break; } -/* Opcode: Halt P1 P2 P3 P4 * +/* Opcode: Halt P1 P2 * * * * * Exit immediately. All open cursors, etc are closed * automatically. @@ -1005,11 +1023,6 @@ case OP_Yield: { /* in1, jump */ * have occurred during this execution of the VDBE, but do not * rollback the transaction. * - * If P4 is not null then it is an error message string. - * - * If P1 is -1 then P3 is a ClientError code and P4 is error - * message to set. - * * There is an implied "Halt 0 0 0" instruction inserted at the * very end of every program. So a jump past the last instruction * of the program is the same as executing Halt. @@ -1017,6 +1030,7 @@ case OP_Yield: { /* in1, jump */ case OP_Halt: { VdbeFrame *pFrame; int pcx; + assert(pOp->p1 == 0 || ! diag_is_empty(diag_get())); pcx = (int)(pOp - aOp); if (pOp->p1 == 0 && p->pFrame != NULL) { @@ -1045,11 +1059,6 @@ case OP_Halt: { p->is_aborted = true; p->errorAction = (u8)pOp->p2; p->pc = pcx; - if (p->is_aborted) { - if (pOp->p4.z != NULL) - box_error_set(__FILE__, __LINE__, pOp->p3, pOp->p4.z); - assert(! diag_is_empty(diag_get())); - } sqlVdbeHalt(p); rc = p->is_aborted ? -1 : SQL_DONE; goto vdbe_return; -- 2.7.4