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 26D7032AFB for ; Fri, 28 Jun 2019 08:07:54 -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 5-3dkN679orb for ; Fri, 28 Jun 2019 08:07:54 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 AC25E32698 for ; Fri, 28 Jun 2019 08:07:53 -0400 (EDT) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE Date: Fri, 28 Jun 2019 15:07:51 +0300 Message-Id: <55b8f0d0ac93ed6df10a538e8485d9309c12f2b7.1561720728.git.imeevma@gmail.com> MIME-Version: 1.0 In-Reply-To: References: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit 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: korablev@tarantool.org Cc: tarantool-patches@freelists.org Hi! Thank you for review! My answers and new patch below. On 6/26/19 7:11 PM, n.pettik wrote: > >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 292168f..c75e1d8 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -1066,14 +1066,21 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, >> sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC); >> sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5, >> ck_constraint_reg + 5); >> - const char *error_msg = >> + /* Check that there is no CHECK with such name. */ >> + const char *err = >> tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), >> ck_def->name); >> - if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0, >> - ck_constraint_reg, 2, >> - ER_CONSTRAINT_EXISTS, error_msg, >> - false, OP_NoConflict) != 0) >> - return; >> + int cursor = parser->nTab++; >> + vdbe_emit_open_cursor(parser, cursor, 0, >> + space_by_id(BOX_CK_CONSTRAINT_ID)); >> + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); >> + sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 3, >> + ck_constraint_reg, 2); >> + sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, err, >> + P4_STATIC); >> + sqlVdbeAddOp1(v, OP_Halt, -1); >> + sqlVdbeAddOp1(v, OP_Close, cursor); > > I don’t like code duplication: this snippet looks almost the same as > vdbe_emit_halt_with_presence_test(). Wouldn’t it be better if OP_Error > was integrated to that function? Also, why not move error setting > to OP_Error from other OP_Halt usages? > Fixed. >> >> +/* Opcode: Error P1 P2 * P4 * > > I’d call it OP_DiagError or OP_SetDiag. > Fixed. Changed to OP_SetDiag. >> + * >> + * Setting an error and an unconditional jump to address P2. > > Nit: -> Set an error and process unconditional jump … > Or -> Set an error and unconditionally jump to > Fixed with new description. >> + * >> + * The P1 parameter is error code of the erroe. The P4 parameter > > -> is an error code to be set. > Fixed. >> + * is a description of the error. > > -> is a text description. > Fixed. >> + */ >> +case OP_Error: { /* jump */ >> + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z); > > Why not simple diag_set() ? > I did it like this because using diag_set() would be a bit troublesome if we decided to use an argument of a different type or more than one argument. New patch: >From 55b8f0d0ac93ed6df10a538e8485d9309c12f2b7 Mon Sep 17 00:00:00 2001 Date: Thu, 27 Jun 2019 16:28:30 +0300 Subject: [PATCH] sql: add OP_SetDiag opcode in VDBE This opcode is required to set an error using diag_set() without halting VDBE. This will allow us to run destructors between error setting and VDBE halting. Needed for #4183 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 292168f..aab60dd 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3277,15 +3277,15 @@ 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 jmp = 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, jmp); sqlVdbeAddOp1(v, OP_Close, cursor); return 0; } diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index 898d16c..d549c6b 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -4347,8 +4347,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..b065de3 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -809,10 +809,10 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, int check_is_passed = sqlVdbeMakeLabel(v); 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); + const char *err = tt_sprintf(fmt, ck_constraint_name, expr_str); + sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0, + sqlDbStrDup(parser->db, err), 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 a257e72..faa7c34 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); @@ -5405,10 +5408,11 @@ 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 = 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); + const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), + "Expression subquery returned more than " + "1 row"); + sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, 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 c8887f9..ffb0df4 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -885,6 +885,21 @@ case OP_Goto: { /* jump */ goto jump_to_p2; } +/* Opcode: SetDiag P1 P2 * P4 * + * + * Set an error. After that jump to address P2 if P2 is not 0, + * else go to the next instruction. + * + * The P1 parameter is an error code to be set. The P4 parameter + * is a text description. + */ +case OP_SetDiag: { /* jump */ + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z); + if (pOp->p2 == 0) + break; + goto jump_to_p2; +} + /* Opcode: Gosub P1 P2 * * * * * Write the current address onto register P1 @@ -989,34 +1004,27 @@ case OP_Yield: { /* in1, jump */ break; } -/* Opcode: Halt P1 P2 P3 P4 * +/* Opcode: Halt P1 P2 * * * * - * Exit immediately. All open cursors, etc are closed + * Exit immediately. All open cursors, etc are closed * automatically. * - * P1 is the result code returned by sql_exec(), - * sql_reset(), or sql_finalize(). For a normal halt, - * this should be 0. - * For errors, it can be some other value. If P1!=0 then P2 will - * determine whether or not to rollback the current transaction. - * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback - * if P2==ON_CONFLICT_ACTION_ROLLBACK. If - * P2==ON_CONFLICT_ACTION_ABORT, then back out all changes that - * 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. + * P1 is the result code. For a normal halt, this should be 0. For + * errors, it is -1. If P1 != 0 then P2 will determine whether or + * not to rollback the current transaction. Do not rollback if + * P2 is ON_CONFLICT_ACTION_FAIL. Do the rollback if P2 is + * ON_CONFLICT_ACTION_ROLLBACK. If P2 is ON_CONFLICT_ACTION_ABORT, + * then back out all changes that have occurred during this + * execution of the VDBE, but do not rollback the transaction. * - * 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. + * There is an implied "Halt 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. */ 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 +1053,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;