[tarantool-patches] Re: [PATCH v1 1/2] sql: add OP_Error opcode in VDBE

[n.pettik]

> 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?

> +
> 	sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0,
> 		      ck_constraint_reg + 5);
> 	save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
> @@ -1131,14 +1138,19 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
> 	 * Lets check that constraint with this name hasn't
> 	 * been created before.
> 	 */
> -	const char *error_msg =
> +	const char *err =
> 		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
> -	if (vdbe_emit_halt_with_presence_test(parse_context,
> -					      BOX_FK_CONSTRAINT_ID, 0,
> -					      constr_tuple_reg, 2,
> -					      ER_CONSTRAINT_EXISTS, error_msg,
> -					      false, OP_NoConflict) != 0)
> -		return;
> +	int cursor = parse_context->nTab++;
> +	vdbe_emit_open_cursor(parse_context, cursor, 0,
> +			      space_by_id(BOX_FK_CONSTRAINT_ID));
> +	sqlVdbeChangeP5(vdbe, OPFLAG_SYSTEMSP);
> +	sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 3,
> +			 constr_tuple_reg, 2);
> +	sqlVdbeAddOp4(vdbe, OP_Error, ER_CONSTRAINT_EXISTS, vdbe->nOp + 1, 0,
> +		      err, P4_STATIC);
> +	sqlVdbeAddOp1(vdbe, OP_Halt, -1);
> +	sqlVdbeAddOp1(vdbe, OP_Close, cursor);
> +
> 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
> 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
> 			  fk_constraint_match_strs[fk->match], P4_STATIC);
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c8887f9..abf881a 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -885,6 +885,18 @@ case OP_Goto: {             /* jump */
> 	goto jump_to_p2;
> }
> 
> +/* Opcode:  Error P1 P2 * P4 *

I’d call it OP_DiagError or 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 

> + *
> + * The P1 parameter is error code of the erroe. The P4 parameter

-> is an error code to be set.

> + * is a description of the error.

-> is a text description.

> + */
> +case OP_Error: {             /* jump */
> +	box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);

Why not simple diag_set() ?

> +	goto jump_to_p2;
> +}
> +