From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE Date: Wed, 3 Jul 2019 02:27:09 +0300 [thread overview] Message-ID: <EB10687D-EB7D-4BDE-80D5-0DF74B4568CB@tarantool.org> (raw) In-Reply-To: <55b8f0d0ac93ed6df10a538e8485d9309c12f2b7.1561720728.git.imeevma@gmail.com> >>> + */ >>> +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. Do not understand what you mean. Could you provide some examples? > 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. What is destructor? You didn’t mention that now OP_Halt doesn’t set errors at all from now. Please, I am asking you not for the first time, provide decent commit messages. > 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); Are these manipulations with labels related to patch? ‘label’ or ‘addr' are more appropriate names IMHO. > sqlVdbeAddOp1(v, OP_Close, cursor); > return 0; > } > > 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); Why did you rename error_msg to err? It’s 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); > } > > @@ -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”); 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; > } > > +/* 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; > +} > + 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; } -/* 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 == 0) - break; - goto jump_to_p2; + if (pOp->p2 != 0) + goto jump_to_p2; + break; } /* Opcode: Gosub P1 P2 * * *
next prev parent reply other threads:[~2019-07-02 23:27 UTC|newest] Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-28 12:07 [tarantool-patches] [PATCH v2 0/3] sql: clean-up in case constraint creation failed imeevma 2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE imeevma 2019-07-02 23:27 ` n.pettik [this message] 2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 2/3] sql: clean-up in case constraint creation failed imeevma 2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 3/3] sql: Fix destructors for constraints imeevma 2019-07-02 23:27 ` [tarantool-patches] " n.pettik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=EB10687D-EB7D-4BDE-80D5-0DF74B4568CB@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox