[tarantool-patches] Re: [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE

["n.pettik" <korablev@tarantool.org>]

>>> + */
>>> +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 * * *