[tarantool-patches] Re: [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE
n.pettik
korablev at tarantool.org
Wed Jul 3 02:27:09 MSK 2019
>>> + */
>>> +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 * * *
More information about the Tarantool-patches
mailing list