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 E8A1322FE5 for ; Tue, 2 Jul 2019 19:27:13 -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 X6GUfmL54hu1 for ; Tue, 2 Jul 2019 19:27:13 -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 5005122F2E for ; Tue, 2 Jul 2019 19:27:13 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE From: "n.pettik" In-Reply-To: <55b8f0d0ac93ed6df10a538e8485d9309c12f2b7.1561720728.git.imeevma@gmail.com> Date: Wed, 3 Jul 2019 02:27:09 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <55b8f0d0ac93ed6df10a538e8485d9309c12f2b7.1561720728.git.imeevma@gmail.com> 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: tarantool-patches@freelists.org Cc: Imeev Mergen >>> + */ >>> +case OP_Error: { /* jump */ >>> + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z); >>=20 >> Why not simple diag_set() ? >>=20 > 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. Do not understand what you mean. Could you provide some examples? > New patch: >=20 > =46rom 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 >=20 > 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. What is destructor? You didn=E2=80=99t mention that now OP_Halt doesn=E2=80=99t set errors at all from now. Please, I am asking you not for the first time, provide decent commit messages. > Needed for #4183 >=20 > 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 =3D parser->nTab++; > vdbe_emit_open_cursor(parser, cursor, index_id, = space_by_id(space_id)); > sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); > - int label =3D sqlVdbeCurrentAddr(v); > - sqlVdbeAddOp4Int(v, cond_opcode, cursor, label + 3, key_reg, > - key_len); > + int jmp =3D 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); Are these manipulations with labels related to patch? =E2=80=98label=E2=80=99 or =E2=80=98addr' are more appropriate names = IMHO. > sqlVdbeAddOp1(v, OP_Close, cursor); > return 0; > } >=20 > 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 =3D sqlVdbeMakeLabel(v); > sqlExprIfTrue(parser, expr, check_is_passed, SQL_JUMPIFNULL); > const char *fmt =3D tnt_errcode_desc(ER_CK_CONSTRAINT_FAILED); > - const char *error_msg =3D 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 =3D tt_sprintf(fmt, ck_constraint_name, = expr_str); Why did you rename error_msg to err? It=E2=80=99s redundant diff. > + 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); > } >=20 > @@ -5405,10 +5408,11 @@ vdbe_code_raise_on_multiple_rows(struct Parse = *parser, int limit_reg, int end_ma > int r1 =3D sqlGetTempReg(parser); > sqlVdbeAddOp2(v, OP_Integer, 0, r1); > sqlVdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg); > - const char *error =3D = 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 =3D tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), > + "Expression subquery returned more = than " > + "1 row=E2=80=9D); Why do you enlarge diff providing meaningless renaming - error -> err? > 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; > } >=20 > +/* 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 =3D=3D 0) > + break; > + goto jump_to_p2; > +} > + diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index ffb0df4b3..f0d21dc88 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -885,19 +885,22 @@ case OP_Goto: { /* jump */ goto jump_to_p2; } =20 -/* Opcode: SetDiag P1 P2 * P4 * +/* 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. + * 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. * - * The P1 parameter is an error code to be set. The P4 parameter - * is a text description. + * 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 =3D=3D 0) - break; - goto jump_to_p2; + if (pOp->p2 !=3D 0) + goto jump_to_p2; + break; } =20 /* Opcode: Gosub P1 P2 * * *