[tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed

[tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed

[imeevma]

This patch makes VDBE run destructors before halting in case
constraint creation failed. This is done using new opcode
OP_SetDiag, which allows to set an error and do a jump without
halting VDBE.

https://github.com/tarantool/tarantool/issues/4183
https://github.com/tarantool/tarantool/tree/imeevma/gh-4183-clean-up-when-constraint-creation-failed

Changes in v3:
1) Removed unnecessary changes.
2) Fixed commit-messages.
3) Now OP_Halt won't be added after OP_SetDiag in case clean-up
needed.

Mergen Imeev (3):
  sql: add OP_SetDiag opcode in VDBE
  sql: clean-up in case constraint creation failed
  sql: use common registers instead of temp. for constraints data

 src/box/sql/build.c         | 117 ++++++++++++++++++++++++++++----------------
 src/box/sql/expr.c          |   6 ++-
 src/box/sql/fk_constraint.c |   3 +-
 src/box/sql/insert.c        |  11 +++--
 src/box/sql/select.c        |  14 ++++--
 src/box/sql/sqlInt.h        |   2 +-
 src/box/sql/trigger.c       |   6 ++-
 src/box/sql/vdbe.c          |  31 +++++++-----
 test/sql/checks.result      |   3 +-
 test/sql/checks.test.lua    |   2 +-
 test/sql/clear.result       |  50 +++++++++++++++++++
 test/sql/clear.test.lua     |  23 +++++++++
 12 files changed, 196 insertions(+), 72 deletions(-)

-- 
2.7.4

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

[imeevma]

Hi! Thank you for review. New patch and diff between versions of
the patch below.

On 7/3/19 2:27 AM, n.pettik wrote:
>
>>>> + */
>>>> +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?
>
Sorry, I can't. But I have beed said so by Vlad:

On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote:
> There was a concrete reason, why it was possible - because different
> error codes have different arguments of various types, and the only way
> to set an error at parsing stage is to allow to set arbitrary error
> message to any error code. Without '...', va_arg etc. Besides, we could
> use it to set correct line number and function name in future. Now you
> use diag_set, which restricts us.
>
> So why do you need that patch? We will need to revert it when an error
> appears requiring more than one argument, or an argument of not
> const char * type. That will definitely happen.
>

It can be ween here:
https://www.freelists.org/post/tarantool-patches/PATCH-v1-49-sql-rework-diag-set-in-OP-Halt,1

>> 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.
>
Fixed.

>> 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.
>
I think that these manipulations should be done, since in this
patch we should jump to different lengths depending on the value
of no_error. This will be even more severe after the next patch.

>>  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.
>
Fixed.

>> +  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?
>
Fixed.

>> 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 * * *
>
Fixed.


Diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index aab60dd..5eb11c2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3277,7 +3277,8 @@ 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 jmp = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, key_len);
+	int addr = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg,
+				    key_len);
 	if (no_error) {
 		sqlVdbeAddOp0(v, OP_Halt);
 	} else {
@@ -3285,7 +3286,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 			      P4_DYNAMIC);
 		sqlVdbeAddOp1(v, OP_Halt, -1);
 	}
-	sqlVdbeJumpHere(v, jmp);
+	sqlVdbeJumpHere(v, addr);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
 	return 0;
 }
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b065de3..4386140 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -809,9 +809,9 @@ 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 *err = tt_sprintf(fmt, ck_constraint_name, expr_str);
+	const char *error_msg = tt_sprintf(fmt, ck_constraint_name, expr_str);
 	sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
-		      sqlDbStrDup(parser->db, err), P4_DYNAMIC);
+		      sqlDbStrDup(parser->db, error_msg), 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);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 301a4c4..bd144c1 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5406,10 +5406,10 @@ 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 *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
-				     "Expression subquery returned more than "
-				     "1 row");
-	sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, P4_STATIC);
+	const char *error = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
+				       "Expression subquery returned more "\
+				       "than 1 row");
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, error, P4_STATIC);
 	sqlVdbeAddOp1(v, OP_Halt, -1);
 	sqlReleaseTempReg(parser, r1);
 }
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ffb0df4..be78c9b 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 * * *
@@ -1006,20 +1009,23 @@ case OP_Yield: {            /* in1, jump */
 
 /* Opcode:  Halt P1 P2 * * *
  *
- * Exit immediately. All open cursors, etc are closed
+ * Exit immediately.  All open cursors, etc are closed
  * automatically.
  *
- * P1 is the result code. For a normal halt, this should be 0. For
- * errors, it is -1. If P1 != 0 then P2 will determine whether or
- * not to rollback the current transaction. Do not rollback if
- * P2 is ON_CONFLICT_ACTION_FAIL. Do the rollback if P2 is
- * ON_CONFLICT_ACTION_ROLLBACK. If P2 is ON_CONFLICT_ACTION_ABORT,
- * then back out all changes that have occurred during this
- * execution of the VDBE, but do not rollback the transaction.
- *
- * There is an implied "Halt 0 0" instruction inserted at the very
- * end of every program. So a jump past the last instruction of
- * the program is the same as executing Halt.
+ * P1 is the result code returned by sql_exec(),
+ * sql_reset(), or sql_finalize().  For a normal halt,
+ * this should be 0.
+ * For errors, it can be some other value.  If P1!=0 then P2 will
+ * determine whether or not to rollback the current transaction.
+ * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
+ * if P2==ON_CONFLICT_ACTION_ROLLBACK.  If
+ * P2==ON_CONFLICT_ACTION_ABORT, then back out all changes that
+ * have occurred during this execution of the VDBE, but do not
+ * rollback the transaction.
+ *
+ * There is an implied "Halt 0 0 0" instruction inserted at the
+ * very end of every program.  So a jump past the last instruction
+ * of the program is the same as executing Halt.
  */
 case OP_Halt: {
 	VdbeFrame *pFrame;


New patch:

From 0a0f232c4c4c7068df8514230c149bc953a080a5 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

To separate the error setting and execution halting, a new opcode
OP_SetDiag was created. The only functionality of the opcode is
the execution of diag_set(). It is important to note that
OP_SetDiag does not set is_aborted to true, so we can continue
working with other opcodes, if necessary. This function allows us
to perform cleanup in some special cases, for example, when
creating a constraint failed because of the creation of two or
more constraints with the same name in the same CREATE TABLE
statement.

Since now diag_set() is executed in OP_SetDiag, this functionality
has been removed from OP_Halt.

Needed for #4183

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 292168f..5eb11c2 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3277,15 +3277,16 @@ 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 addr = 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, addr);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
 	return 0;
 }
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index d7104d8..d7d0375 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4339,8 +4339,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			const char *err =
 				tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
 					   pExpr->u.zToken);
-			sqlVdbeAddOp4(v, OP_Halt, -1, pExpr->on_conflict_action,
-				      ER_SQL_EXECUTE, err, 0);
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+				      P4_STATIC);
+			sqlVdbeAddOp2(v, OP_Halt, -1,
+				      pExpr->on_conflict_action);
 		}
 		break;
 	}
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 06c863a..4717677 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -287,8 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 		assert(incr_count == 1);
 		const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
 					     "FOREIGN KEY constraint failed");
-		sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+		sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 			      P4_STATIC);
+		sqlVdbeAddOp1(v, OP_Halt, -1);
 	} else {
 		sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
 				  incr_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b353148..4386140 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -810,9 +810,9 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
 	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);
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
+		      sqlDbStrDup(parser->db, error_msg), 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);
 }
@@ -863,8 +863,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 						    "failed: %s.%s", def->name,
 						    def->fields[i].name));
 			addr = sqlVdbeAddOp1(v, OP_NotNull, new_tuple_reg + i);
-			sqlVdbeAddOp4(v, OP_Halt, -1, on_conflict_nullable,
-				      ER_SQL_EXECUTE, err, P4_STATIC);
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+				      P4_STATIC);
+			sqlVdbeAddOp2(v, OP_Halt, -1, on_conflict_nullable);
 			sqlVdbeJumpHere(v, addr);
 			break;
 		case ON_CONFLICT_ACTION_IGNORE:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 7c8da25..bd144c1 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2092,8 +2092,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					     "Only positive integers are "\
 					     "allowed in the LIMIT clause");
 		sqlVdbeResolveLabel(v, halt_label);
-		sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+		sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 			      P4_STATIC);
+		sqlVdbeAddOp1(v, OP_Halt, -1);
 
 		sqlVdbeResolveLabel(v, positive_limit_label);
 		VdbeCoverage(v);
@@ -2124,8 +2125,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				err = tt_sprintf(err, "Expression subquery "\
 						 "could be limited only "\
 						 "with 1");
-				sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE,
-					      err, P4_STATIC);
+				sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0,
+					      0, err, P4_STATIC);
+				sqlVdbeAddOp1(v, OP_Halt, -1);
 				sqlVdbeResolveLabel(v, no_err);
 				sqlReleaseTempReg(pParse, r1);
 
@@ -2150,8 +2152,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					 "Only positive integers are allowed "\
 					 "in the OFFSET clause");
 			sqlVdbeResolveLabel(v, offset_error_label);
-			sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 				      P4_STATIC);
+			sqlVdbeAddOp1(v, OP_Halt, -1);
 
 			sqlVdbeResolveLabel(v, positive_offset_label);
             		sqlReleaseTempReg(pParse, r1);
@@ -5406,7 +5409,8 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
 	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);
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, error, P4_STATIC);
+	sqlVdbeAddOp1(v, OP_Halt, -1);
 	sqlReleaseTempReg(parser, r1);
 }
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..be78c9b 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,24 @@ case OP_Goto: {             /* jump */
 	goto jump_to_p2;
 }
 
+/* Opcode: SetDiag P1 P2 * P4 *
+ *
+ * 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.
+ *
+ * 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)
+		goto jump_to_p2;
+	break;
+}
+
 /* Opcode:  Gosub P1 P2 * * *
  *
  * Write the current address onto register P1
@@ -989,7 +1007,7 @@ case OP_Yield: {            /* in1, jump */
 	break;
 }
 
-/* Opcode:  Halt P1 P2 P3 P4 *
+/* Opcode:  Halt P1 P2 * * *
  *
  * Exit immediately.  All open cursors, etc are closed
  * automatically.
@@ -1005,11 +1023,6 @@ case OP_Yield: {            /* in1, jump */
  * have occurred during this execution of the VDBE, but do not
  * rollback the transaction.
  *
- * If P4 is not null then it is an error message string.
- *
- * If P1 is -1 then P3 is a ClientError code and  P4 is error
- * message to set.
- *
  * There is an implied "Halt 0 0 0" instruction inserted at the
  * very end of every program.  So a jump past the last instruction
  * of the program is the same as executing Halt.
@@ -1017,6 +1030,7 @@ case OP_Yield: {            /* in1, jump */
 case OP_Halt: {
 	VdbeFrame *pFrame;
 	int pcx;
+	assert(pOp->p1 == 0 || ! diag_is_empty(diag_get()));
 
 	pcx = (int)(pOp - aOp);
 	if (pOp->p1 == 0 && p->pFrame != NULL) {
@@ -1045,11 +1059,6 @@ case OP_Halt: {
 		p->is_aborted = true;
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
-	if (p->is_aborted) {
-		if (pOp->p4.z != NULL)
-			box_error_set(__FILE__, __LINE__, pOp->p3, pOp->p4.z);
-		assert(! diag_is_empty(diag_get()));
-	}
 	sqlVdbeHalt(p);
 	rc = p->is_aborted ? -1 : SQL_DONE;
 	goto vdbe_return;

[tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed

[imeevma]

Diff between patches and the new patch below. In addition to the
changes shown in diff, a new commit-message was written.

Diff between versions:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 12b6e5e..0dd2906 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3309,7 +3309,8 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 			      P4_DYNAMIC);
 		if (is_clean_needed)
 			save_record(parser, 0, 0, 0, v->nOp - 1, false);
-		sqlVdbeAddOp1(v, OP_Halt, -1);
+		else
+			sqlVdbeAddOp1(v, OP_Halt, -1);
 	}
 	sqlVdbeJumpHere(v, addr);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/test/sql/clear.result b/test/sql/clear.result
index 4e10b0e..2034d82 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -169,7 +169,10 @@ box.execute("DROP TABLE zoobar")
 -- Debug
 -- require("console").start()
 --
--- gh-4183: Garbage in case of failed constraint creation.
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
 --
 box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
 ---
diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua
index 4e212e3..7ed18a4 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -31,7 +31,10 @@ box.execute("DROP TABLE zoobar")
 -- require("console").start()
 
 --
--- gh-4183: Garbage in case of failed constraint creation.
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
 --
 box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
 box.space.t1


New version:

From 58bd81c8bac35962942d6e18948ed2d6b267366b Mon Sep 17 00:00:00 2001
Date: Thu, 27 Jun 2019 16:59:42 +0300
Subject: [PATCH] sql: clean-up in case constraint creation failed

This patch makes VDBE to perform a clean-up if the creation of a
constraint fails because of the creation of two or more
constraints of the same type with the same name and in the same
CREATE TABLE statement.

For example:
CREATE TABLE t1(
	id INT PRIMARY KEY,
	CONSTRAINT ck1 CHECK(id > 1),
	CONSTRAINT ck1 CHECK(id < 10)
);

Part of #4183

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5eb11c2..0dd2906 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -71,22 +71,33 @@ struct saved_record
 	int reg_key;
 	/** Number of registers the key consists of. */
 	int reg_key_count;
-	/** The number of the OP_SInsert operation. */
-	int insertion_opcode;
+	/** The address of the opcode. */
+	int op_addr;
+	/** Flag to show that operation is SInsert. */
+	bool is_insert;
 };
 
 /**
- * Save inserted in system space record in list.
+ * Save inserted in system space record in list. This procedure is
+ * called after generation of either OP_SInsert or OP_NoColflict +
+ * OP_SetDiag. In the first case, record inserted to the system
+ * space is supposed to be deleted on error; in the latter - jump
+ * target specified in OP_SetDiag should be adjusted to the start
+ * of clean-up routines (current entry isn't inserted to the space
+ * yet, so there's no need to delete it).
  *
  * @param parser SQL Parser object.
  * @param space_id Id of table in which record is inserted.
  * @param reg_key Register that contains first field of the key.
  * @param reg_key_count Exact number of fields of the key.
- * @param insertion_opcode Number of OP_SInsert opcode.
+ * @param op_addr Address of opcode (OP_SetDiag or OP_SInsert).
+ *                Used to fix jump target (see
+ *                sql_finish_coding()).
+ * @param is_insert_op Whether opcode is OP_SInsert or not.
  */
 static inline void
 save_record(struct Parse *parser, uint32_t space_id, int reg_key,
-	    int reg_key_count, int insertion_opcode)
+	    int reg_key_count, int op_addr, bool is_insert_op)
 {
 	struct saved_record *record =
 		region_alloc(&parser->region, sizeof(*record));
@@ -99,7 +110,8 @@ save_record(struct Parse *parser, uint32_t space_id, int reg_key,
 	record->space_id = space_id;
 	record->reg_key = reg_key;
 	record->reg_key_count = reg_key_count;
-	record->insertion_opcode = insertion_opcode;
+	record->op_addr = op_addr;
+	record->is_insert = is_insert_op;
 	rlist_add_entry(&parser->record_list, record, link);
 }
 
@@ -118,28 +130,39 @@ sql_finish_coding(struct Parse *parse_context)
 	 * it won't be created. This code works the same way for
 	 * other "CREATE ..." statements but it won't delete
 	 * anything as these statements create no more than one
-	 * record.
+	 * record. Hence for processed insertions we should remove
+	 * entries from corresponding system spaces alongside
+	 * with fixing jump address for OP_SInsert opcode in
+	 * case it fails during VDBE runtime; for OP_SetDiag only
+	 * adjust jump target to the start of clean-up program
+	 * for already inserted entries.
 	 */
 	if (!rlist_empty(&parse_context->record_list)) {
 		struct saved_record *record =
 			rlist_shift_entry(&parse_context->record_list,
 					  struct saved_record, link);
-		/* Set P2 of SInsert. */
-		sqlVdbeChangeP2(v, record->insertion_opcode, v->nOp);
+		/*
+		 * Set jump target for OP_SetDiag and OP_SInsert.
+		 */
+		sqlVdbeChangeP2(v, record->op_addr, v->nOp);
 		MAYBE_UNUSED const char *comment =
 			"Delete entry from %s if CREATE TABLE fails";
 		rlist_foreach_entry(record, &parse_context->record_list, link) {
-			int record_reg = ++parse_context->nMem;
-			sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key,
-					  record->reg_key_count, record_reg);
-			sqlVdbeAddOp2(v, OP_SDelete, record->space_id,
-					  record_reg);
-			MAYBE_UNUSED struct space *space =
-				space_by_id(record->space_id);
-			VdbeComment((v, comment, space_name(space)));
-			/* Set P2 of SInsert. */
-			sqlVdbeChangeP2(v, record->insertion_opcode,
-					    v->nOp);
+			if (record->is_insert) {
+				int rec_reg = ++parse_context->nMem;
+				sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key,
+					      record->reg_key_count, rec_reg);
+				sqlVdbeAddOp2(v, OP_SDelete, record->space_id,
+					      rec_reg);
+				MAYBE_UNUSED struct space *space =
+					space_by_id(record->space_id);
+				VdbeComment((v, comment, space_name(space)));
+			}
+			/*
+			 * Set jump target for OP_SetDiag and
+			 * OP_SInsert.
+			 */
+			sqlVdbeChangeP2(v, record->op_addr, v->nOp);
 		}
 		sqlVdbeAddOp1(v, OP_Halt, -1);
 		VdbeComment((v,
@@ -891,7 +914,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
 			  SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC);
 	sqlVdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg);
 	sqlVdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg);
-	save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
+	save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1, true);
 	return;
 error:
 	parse->is_aborted = true;
@@ -950,7 +973,7 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg,
 	sqlVdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, tuple_reg);
 	sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, tuple_reg);
 	sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
-	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1);
+	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1, true);
 	return;
 error:
 	pParse->is_aborted = true;
@@ -1072,12 +1095,12 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	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)
+					      false, OP_NoConflict, true) != 0)
 		return;
 	sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0,
 		      ck_constraint_reg + 5);
 	save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
-		    v->nOp - 1);
+		    v->nOp - 1, true);
 	VdbeComment((v, "Create CK constraint %s", ck_def->name));
 	sqlReleaseTempRange(parser, ck_constraint_reg, 5);
 }
@@ -1137,7 +1160,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      constr_tuple_reg, 2,
 					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
+					      false, OP_NoConflict, true) != 0)
 		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1187,7 +1210,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 		sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);
 	}
 	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
-		    vdbe->nOp - 1);
+		    vdbe->nOp - 1, true);
 	sqlReleaseTempRange(parse_context, constr_tuple_reg, 10);
 	return;
 error:
@@ -1284,7 +1307,7 @@ sqlEndTable(struct Parse *pParse)
 	if (vdbe_emit_halt_with_presence_test(pParse, BOX_SPACE_ID, 2,
 					      name_reg, 1, ER_SPACE_EXISTS,
 					      error_msg, (no_err != 0),
-					      OP_NoConflict) != 0)
+					      OP_NoConflict, false) != 0)
 		return;
 
 	int reg_space_id = getNewSpaceId(pParse);
@@ -1310,7 +1333,7 @@ sqlEndTable(struct Parse *pParse)
 		sqlVdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
 				  reg_seq_record);
 		save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
-			    v->nOp - 1);
+			    v->nOp - 1, true);
 		/* Do an insertion into _space_sequence. */
 		int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
 							reg_space_id, reg_seq_id,
@@ -1318,7 +1341,7 @@ sqlEndTable(struct Parse *pParse)
 		sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0,
 				  reg_space_seq_record);
 		save_record(pParse, BOX_SPACE_SEQUENCE_ID,
-			    reg_space_seq_record + 1, 1, v->nOp - 1);
+			    reg_space_seq_record + 1, 1, v->nOp - 1, true);
 	}
 	/* Code creation of FK constraints, if any. */
 	struct fk_constraint_parse *fk_parse;
@@ -1445,7 +1468,7 @@ sql_create_view(struct Parse *parse_context)
 	if (vdbe_emit_halt_with_presence_test(parse_context, BOX_SPACE_ID, 2,
 					      name_reg, 1, ER_SPACE_EXISTS,
 					      error_msg, (no_err != 0),
-					      OP_NoConflict) != 0)
+					      OP_NoConflict, false) != 0)
 		goto create_view_fail;
 
 	vdbe_emit_space_create(parse_context, getNewSpaceId(parse_context),
@@ -1564,7 +1587,7 @@ vdbe_emit_fk_constraint_drop(struct Parse *parse_context, char *constraint_name,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      key_reg, 2, ER_NO_SUCH_CONSTRAINT,
 					      error_msg, false,
-					      OP_Found) != 0) {
+					      OP_Found, false) != 0) {
 		sqlDbFree(parse_context->db, constraint_name);
 		return;
 	}
@@ -1597,7 +1620,7 @@ vdbe_emit_ck_constraint_drop(struct Parse *parser, const char *ck_name,
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
 					      key_reg, 2, ER_NO_SUCH_CONSTRAINT,
 					      error_msg, false,
-					      OP_Found) != 0)
+					      OP_Found, false) != 0)
 		return;
 	sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2);
 	sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2);
@@ -3263,7 +3286,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 				  int index_id, int key_reg, uint32_t key_len,
 				  int tarantool_error_code,
 				  const char *error_src, bool no_error,
-				  int cond_opcode)
+				  int cond_opcode, bool is_clean_needed)
 {
 	assert(cond_opcode == OP_NoConflict || cond_opcode == OP_Found);
 	struct Vdbe *v = sqlGetVdbe(parser);
@@ -3284,7 +3307,10 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 	} else {
 		sqlVdbeAddOp4(v, OP_SetDiag, tarantool_error_code, 0, 0, error,
 			      P4_DYNAMIC);
-		sqlVdbeAddOp1(v, OP_Halt, -1);
+		if (is_clean_needed)
+			save_record(parser, 0, 0, 0, v->nOp - 1, false);
+		else
+			sqlVdbeAddOp1(v, OP_Halt, -1);
 	}
 	sqlVdbeJumpHere(v, addr);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..1d9408a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4540,7 +4540,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
 				  int index_id, int key_reg, uint32_t key_len,
 				  int tarantool_error_code,
 				  const char *error_src, bool no_error,
-				  int cond_opcode);
+				  int cond_opcode, bool is_clean_needed);
 
 /**
  * Generate VDBE code to delete records from system _sql_stat1 or
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index d746ef8..5627239 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -112,7 +112,8 @@ sql_trigger_begin(struct Parse *parse)
 						      name_reg, 1,
 						      ER_TRIGGER_EXISTS,
 						      error_msg, (no_err != 0),
-						      OP_NoConflict) != 0)
+						      OP_NoConflict,
+						      false) != 0)
 			goto trigger_cleanup;
 	}
 
@@ -420,7 +421,8 @@ sql_drop_trigger(struct Parse *parser)
 	sqlVdbeAddOp4(v, OP_String8, 0, name_reg, 0, name_copy, P4_DYNAMIC);
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_TRIGGER_ID, 0,
 					      name_reg, 1, ER_NO_SUCH_TRIGGER,
-					      error_msg, no_err, OP_Found) != 0)
+					      error_msg, no_err, OP_Found,
+					      false) != 0)
 		goto drop_trigger_cleanup;
 
 	vdbe_code_drop_trigger(parser, trigger_name, true);
diff --git a/test/sql/checks.result b/test/sql/checks.result
index bdcf507..5279ae0 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -251,8 +251,9 @@ box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), C
 ---
 - error: Constraint CK1 already exists
 ...
-box.space.T2:drop()
+box.space.T2
 ---
+- null
 ...
 box.space._ck_constraint:select()
 ---
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 50cb13c..cacba59 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -86,7 +86,7 @@ box.execute("INSERT INTO \"test\" VALUES(11, 11);")
 box.execute("INSERT INTO \"test\" VALUES(12, 11);")
 s:drop()
 box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))")
-box.space.T2:drop()
+box.space.T2
 box.space._ck_constraint:select()
 
 --
diff --git a/test/sql/clear.result b/test/sql/clear.result
index 0a3baa3..2034d82 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -168,3 +168,33 @@ box.execute("DROP TABLE zoobar")
 ...
 -- Debug
 -- require("console").start()
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
+---
+- error: Constraint CK1 already exists
+...
+box.space.t1
+---
+- null
+...
+box.space._ck_constraint:select()
+---
+- []
+...
+box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);")
+---
+- error: Constraint FK1 already exists
+...
+box.space.t2
+---
+- null
+...
+box.space._fk_constraint:select()
+---
+- []
+...
diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua
index c67a447..7ed18a4 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -29,3 +29,17 @@ box.execute("DROP TABLE zoobar")
 
 -- Debug
 -- require("console").start()
+
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
+box.space.t1
+box.space._ck_constraint:select()
+
+box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);")
+box.space.t2
+box.space._fk_constraint:select()

[tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data

[imeevma]

Thank you for review! New patch below.

On 7/3/19 2:27 AM, n.pettik wrote:
>
>
>> On 28 Jun 2019, at 15:07, imeevma@tarantool.org wrote:
>>
>> Prior to this patch, the data needed to create the constraints in
>> VDBE was stored in the temporary registers of the parser. Since
>> this data is necessary for creating destructors, it was
>> transferred to standard registers.
>
>     sql: use common registers instead of temp. for constraints data
>     
>     Prior to this patch, data needed to form tuple to be inserted to
>     _fk_constraint and _ck_constraint system spaces (to create corresponding
>     constraints) was stored in the range of temporary register. After
>     insertion, temporary registers are released. On the other hand, this
>     data is required for providing clean-up in case of creation fail (i.e.
>     removing already created constraints within one CREATE TABLE statement).
>     Hence, instead of using temporary registers let's use ordinary ones.
>     
>     Closes #4183
Fixed.

From 75fdfbc986b1aae8a476017f3c417c25c357dd65 Mon Sep 17 00:00:00 2001
Date: Thu, 27 Jun 2019 17:57:28 +0300
Subject: [PATCH] sql: use common registers instead of temp. for constraints
 data

Prior to this patch, data needed to form tuple to be inserted to
_fk_constraint and _ck_constraint system spaces (to create
corresponding constraints) was stored in the range of temporary
register. After insertion, temporary registers are released. On
the other hand, this data is required for providing clean-up in
case of creation fail (i.e. removing already created constraints
within one CREATE TABLE statement). Hence, instead of using
temporary registers let's use ordinary ones.

Closes #4183

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0dd2906..1c4b4f7 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1078,7 +1078,12 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	struct sql *db = parser->db;
 	struct Vdbe *v = sqlGetVdbe(parser);
 	assert(v != NULL);
-	int ck_constraint_reg = sqlGetTempRange(parser, 6);
+	/*
+	 * Occupy registers for 5 fields: each member in
+	 * _ck_constraint space plus one for final msgpack tuple.
+	 */
+	int ck_constraint_reg = parser->nMem + 1;
+	parser->nMem += 6;
 	sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg);
 	sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0,
 		      sqlDbStrDup(db, ck_def->name), P4_DYNAMIC);
@@ -1102,7 +1107,6 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
 		    v->nOp - 1, true);
 	VdbeComment((v, "Create CK constraint %s", ck_def->name));
-	sqlReleaseTempRange(parser, ck_constraint_reg, 5);
 }
 
 /**
@@ -1121,10 +1125,11 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	struct Vdbe *vdbe = sqlGetVdbe(parse_context);
 	assert(vdbe != NULL);
 	/*
-	 * Occupy registers for 8 fields: each member in
-	 * _constraint space plus one for final msgpack tuple.
+	 * Occupy registers for 9 fields: each member in
+	 * _fk_constraint space plus one for final msgpack tuple.
 	 */
-	int constr_tuple_reg = sqlGetTempRange(parse_context, 10);
+	int constr_tuple_reg = parse_context->nMem + 1;
+	parse_context->nMem += 10;
 	char *name_copy = sqlDbStrDup(parse_context->db, fk->name);
 	if (name_copy == NULL)
 		return;
@@ -1211,7 +1216,6 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	}
 	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
 		    vdbe->nOp - 1, true);
-	sqlReleaseTempRange(parse_context, constr_tuple_reg, 10);
 	return;
 error:
 	parse_context->is_aborted = true;
diff --git a/test/sql/clear.result b/test/sql/clear.result
index 2034d82..2fb1761 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -198,3 +198,23 @@ box.space._fk_constraint:select()
 ---
 - []
 ...
+--
+-- Make sure that keys for tuples inserted into system spaces were
+-- not stored in temporary cells.
+--
+box.execute("CREATE TABLE t3(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT ck1 CHECK(id < 0));")
+---
+- error: Constraint CK1 already exists
+...
+box.space.t1
+---
+- null
+...
+box.space._ck_constraint:select()
+---
+- []
+...
+box.space._fk_constraint:select()
+---
+- []
+...
diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua
index 7ed18a4..4c58767 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -43,3 +43,12 @@ box.space._ck_constraint:select()
 box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);")
 box.space.t2
 box.space._fk_constraint:select()
+
+--
+-- Make sure that keys for tuples inserted into system spaces were
+-- not stored in temporary cells.
+--
+box.execute("CREATE TABLE t3(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT ck1 CHECK(id < 0));")
+box.space.t1
+box.space._ck_constraint:select()
+box.space._fk_constraint:select()

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

[n.pettik]

>>>>> + */
>>>>> +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?
>> 
> Sorry, I can't. But I have beed said so by Vlad:
> 
> On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote:
>> There was a concrete reason, why it was possible - because different
>> error codes have different arguments of various types, and the only way
>> to set an error at parsing stage is to allow to set arbitrary error
>> message to any error code. Without '...', va_arg etc. Besides, we could
>> use it to set correct line number and function name in future. Now you
>> use diag_set, which restricts us.
>> 
>> So why do you need that patch? We will need to revert it when an error
>> appears requiring more than one argument, or an argument of not
>> const char * type. That will definitely happen.
>> 
> 
> It can be ween here:
> https://www.freelists.org/post/tarantool-patches/PATCH-v1-49-sql-rework-diag-set-in-OP-Halt,1

Ok, now it’s clear: it allows us to construct string containing
error message during parsing stage, so that we avoid argument
substitution in diag_set(). It is vital since "error codes have
different arguments of various types”.

>>> 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.
>> 
> I think that these manipulations should be done, since in this
> patch we should jump to different lengths depending on the value
> of no_error. This will be even more severe after the next patch.

Ok, now I see.

This patch LGTM.

[tarantool-patches] Re: [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data

[n.pettik]

Thanks, now whole patch-set seems to be OK.

[tarantool-patches] Re: [PATCH v3 0/3] sql: clean-up in case constraint creation failed

[Kirill Yukhin]

Hello,

On 04 Jul 13:49, imeevma@tarantool.org wrote:
> This patch makes VDBE run destructors before halting in case
> constraint creation failed. This is done using new opcode
> OP_SetDiag, which allows to set an error and do a jump without
> halting VDBE.
> 
> https://github.com/tarantool/tarantool/issues/4183
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4183-clean-up-when-constraint-creation-failed

I've checked your patchset into master.

--
Regards, Kirill Yukhin

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

[imeevma]

To separate the error setting and execution halting, a new opcode
OP_SetDiag was created. The only functionality of the opcode is
the execution of diag_set(). It is important to note that
OP_SetDiag does not set is_aborted to true, so we can continue
working with other opcodes, if necessary. This function allows us
to perform cleanup in some special cases, for example, when
creating a constraint failed because of the creation of two or
more constraints with the same name in the same CREATE TABLE
statement.

Since now diag_set() is executed in OP_SetDiag, this functionality
has been removed from OP_Halt.

Needed for #4183
---
 src/box/sql/build.c         |  9 +++++----
 src/box/sql/expr.c          |  6 ++++--
 src/box/sql/fk_constraint.c |  3 ++-
 src/box/sql/insert.c        | 11 ++++++-----
 src/box/sql/select.c        | 14 +++++++++-----
 src/box/sql/vdbe.c          | 31 ++++++++++++++++++++-----------
 6 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 396de63..e56b9da 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3297,15 +3297,16 @@ 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 addr = 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, addr);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
 	return 0;
 }
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 7673832..916b0a0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4339,8 +4339,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			const char *err =
 				tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
 					   pExpr->u.zToken);
-			sqlVdbeAddOp4(v, OP_Halt, -1, pExpr->on_conflict_action,
-				      ER_SQL_EXECUTE, err, 0);
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+				      P4_STATIC);
+			sqlVdbeAddOp2(v, OP_Halt, -1,
+				      pExpr->on_conflict_action);
 		}
 		break;
 	}
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 06c863a..4717677 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -287,8 +287,9 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
 		assert(incr_count == 1);
 		const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
 					     "FOREIGN KEY constraint failed");
-		sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+		sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 			      P4_STATIC);
+		sqlVdbeAddOp1(v, OP_Halt, -1);
 	} else {
 		sqlVdbeAddOp2(v, OP_FkCounter, fk_def->is_deferred,
 				  incr_count);
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b353148..4386140 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -810,9 +810,9 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
 	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);
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
+		      sqlDbStrDup(parser->db, error_msg), 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);
 }
@@ -863,8 +863,9 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space,
 						    "failed: %s.%s", def->name,
 						    def->fields[i].name));
 			addr = sqlVdbeAddOp1(v, OP_NotNull, new_tuple_reg + i);
-			sqlVdbeAddOp4(v, OP_Halt, -1, on_conflict_nullable,
-				      ER_SQL_EXECUTE, err, P4_STATIC);
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
+				      P4_STATIC);
+			sqlVdbeAddOp2(v, OP_Halt, -1, on_conflict_nullable);
 			sqlVdbeJumpHere(v, addr);
 			break;
 		case ON_CONFLICT_ACTION_IGNORE:
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 7c8da25..bd144c1 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -2092,8 +2092,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					     "Only positive integers are "\
 					     "allowed in the LIMIT clause");
 		sqlVdbeResolveLabel(v, halt_label);
-		sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+		sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 			      P4_STATIC);
+		sqlVdbeAddOp1(v, OP_Halt, -1);
 
 		sqlVdbeResolveLabel(v, positive_limit_label);
 		VdbeCoverage(v);
@@ -2124,8 +2125,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 				err = tt_sprintf(err, "Expression subquery "\
 						 "could be limited only "\
 						 "with 1");
-				sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE,
-					      err, P4_STATIC);
+				sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0,
+					      0, err, P4_STATIC);
+				sqlVdbeAddOp1(v, OP_Halt, -1);
 				sqlVdbeResolveLabel(v, no_err);
 				sqlReleaseTempReg(pParse, r1);
 
@@ -2150,8 +2152,9 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
 					 "Only positive integers are allowed "\
 					 "in the OFFSET clause");
 			sqlVdbeResolveLabel(v, offset_error_label);
-			sqlVdbeAddOp4(v, OP_Halt, -1, 0, ER_SQL_EXECUTE, err,
+			sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err,
 				      P4_STATIC);
+			sqlVdbeAddOp1(v, OP_Halt, -1);
 
 			sqlVdbeResolveLabel(v, positive_offset_label);
             		sqlReleaseTempReg(pParse, r1);
@@ -5406,7 +5409,8 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
 	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);
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, error, P4_STATIC);
+	sqlVdbeAddOp1(v, OP_Halt, -1);
 	sqlReleaseTempReg(parser, r1);
 }
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9f4ee7a..6a4a303 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,24 @@ case OP_Goto: {             /* jump */
 	goto jump_to_p2;
 }
 
+/* Opcode: SetDiag P1 P2 * P4 *
+ *
+ * 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.
+ *
+ * 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)
+		goto jump_to_p2;
+	break;
+}
+
 /* Opcode:  Gosub P1 P2 * * *
  *
  * Write the current address onto register P1
@@ -989,7 +1007,7 @@ case OP_Yield: {            /* in1, jump */
 	break;
 }
 
-/* Opcode:  Halt P1 P2 P3 P4 *
+/* Opcode:  Halt P1 P2 * * *
  *
  * Exit immediately.  All open cursors, etc are closed
  * automatically.
@@ -1005,11 +1023,6 @@ case OP_Yield: {            /* in1, jump */
  * have occurred during this execution of the VDBE, but do not
  * rollback the transaction.
  *
- * If P4 is not null then it is an error message string.
- *
- * If P1 is -1 then P3 is a ClientError code and  P4 is error
- * message to set.
- *
  * There is an implied "Halt 0 0 0" instruction inserted at the
  * very end of every program.  So a jump past the last instruction
  * of the program is the same as executing Halt.
@@ -1017,6 +1030,7 @@ case OP_Yield: {            /* in1, jump */
 case OP_Halt: {
 	VdbeFrame *pFrame;
 	int pcx;
+	assert(pOp->p1 == 0 || ! diag_is_empty(diag_get()));
 
 	pcx = (int)(pOp - aOp);
 	if (pOp->p1 == 0 && p->pFrame != NULL) {
@@ -1045,11 +1059,6 @@ case OP_Halt: {
 		p->is_aborted = true;
 	p->errorAction = (u8)pOp->p2;
 	p->pc = pcx;
-	if (p->is_aborted) {
-		if (pOp->p4.z != NULL)
-			box_error_set(__FILE__, __LINE__, pOp->p3, pOp->p4.z);
-		assert(! diag_is_empty(diag_get()));
-	}
 	sqlVdbeHalt(p);
 	rc = p->is_aborted ? -1 : SQL_DONE;
 	goto vdbe_return;
-- 
2.7.4