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

[tarantool-patches] [PATCH v2 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_Error, 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 v2:
1) Opcode OP_Error was renamed to OP_SetDiag
2) Now OP_SetDiag is used for diag_set() for all VDBE errors. This
functionality has been removed from OP_Halt.
3) Fixed error with broken destructors for constraints.

Mergen Imeev (3):
  sql: add OP_SetDiag opcode in VDBE
  sql: clean-up in case constraint creation failed
  sql: Fix destructors for constraints

 src/box/sql/build.c         | 115 +++++++++++++++++++++++++++-----------------
 src/box/sql/expr.c          |   6 ++-
 src/box/sql/fk_constraint.c |   3 +-
 src/box/sql/insert.c        |  13 ++---
 src/box/sql/select.c        |  20 +++++---
 src/box/sql/sqlInt.h        |   2 +-
 src/box/sql/trigger.c       |   6 ++-
 src/box/sql/vdbe.c          |  53 ++++++++++----------
 test/sql/checks.result      |   3 +-
 test/sql/checks.test.lua    |   2 +-
 test/sql/clear.result       |  47 ++++++++++++++++++
 test/sql/clear.test.lua     |  20 ++++++++
 12 files changed, 200 insertions(+), 90 deletions(-)

-- 
2.7.4

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

[imeevma]

Hi! Thank you for review! My answers and new patch below.


On 6/26/19 7:11 PM, n.pettik wrote:
>
>> 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?
>
Fixed.

>>
>> +/* Opcode:  Error P1 P2 * P4 *
>
> I’d call it OP_DiagError or OP_SetDiag.
>
Fixed. Changed to 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 
>
Fixed with new description.

>> + *
>> + * The P1 parameter is error code of the erroe. The P4 parameter
>
> -> is an error code to be set.
>
Fixed.

>> + * is a description of the error.
>
> -> is a text description.
>
Fixed.

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


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.

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);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
 	return 0;
 }
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 898d16c..d549c6b 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4347,8 +4347,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..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);
+	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);
 }
@@ -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 a257e72..faa7c34 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);
@@ -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");
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, 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..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;
+}
+
 /* Opcode:  Gosub P1 P2 * * *
  *
  * Write the current address onto register P1
@@ -989,34 +1004,27 @@ case OP_Yield: {            /* in1, jump */
 	break;
 }
 
-/* Opcode:  Halt P1 P2 P3 P4 *
+/* 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 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.
- *
- * If P4 is not null then it is an error message string.
+ * 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.
  *
- * 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.
+ * 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.
  */
 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 +1053,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 v2 2/3] sql: clean-up in case constraint creation failed

[imeevma]

Thank you for review! My answers and new patch below.

On 6/26/19 7:11 PM, n.pettik wrote:
>
>
>> On 25 Jun 2019, at 18:31, imeevma@tarantool.org wrote:
>>
>> This patch makes VDBE run destructors before halting in case
>> constraint creation failed.
>>
>> Closes #4183
>
> tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(if > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT ck1 CHECK(id < 0));")
> ---
> - error: 'Supplied key type of part 0 does not match index part type: expected string'
> ...
>
> tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(if > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT ck1 CHECK(id < 0));")
> ---
> - error: Space 'T1' already exists
> ...
>
> Fix this bug. Also apply this diff containing explanations to the patch:
>
Fixed in next patch.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index d05ca0bc3..0b907b167 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -74,21 +74,29 @@ struct saved_record
>         /** The number of the operation. */
>         int op_number;
>         /** Flag to show that operation is SInsert. */
> -       bool 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_Error. In the first case, record inserted to the system space
> + * is supposed to be deleted; in the latter - jump target specified
> + * in OP_Error 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 op_number Number of the operation.
> + * @param op_addr Address of opcode (OP_Error or OP_SInter). Used
> + *                to fix jump target (see sql_finish_coding()).
> + * @param is_op_insert 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 op_number, bool is_sinsert)
> +           int reg_key_count, int op_number, bool is_insert_op)
>  {
>         struct saved_record *record =
>                 region_alloc(&parser->region, sizeof(*record));
> @@ -102,7 +110,7 @@ save_record(struct Parse *parser, uint32_t space_id, int reg_key,
>         record->reg_key = reg_key;
>         record->reg_key_count = reg_key_count;
>         record->op_number = op_number;
> -       record->is_sinsert = is_sinsert;
> +       record->is_insert = is_insert_op;
>         rlist_add_entry(&parser->record_list, record, link);
>  }
>  
> @@ -121,11 +129,12 @@ 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.
> -        *
> -        * Here it was assumed that after OP_Error there is at
> -        * least one OP_SInsert. That means that the last opcode
> -        * in the current list is OP_SInsert.
> +        * 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_Error 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 =
> @@ -136,7 +145,7 @@ sql_finish_coding(struct Parse *parse_context)
>                 MAYBE_UNUSED const char *comment =
>                         "Delete entry from %s if CREATE TABLE fails";
>                 rlist_foreach_entry(record, &parse_context->record_list, link) {
> -                       if (record->is_sinsert) {
> +                       if (record->is_insert) {
>                                 int rec_reg = ++parse_context->nMem;
>                                 sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key,
>                                               record->reg_key_count, rec_reg);
> @@ -146,7 +155,7 @@ sql_finish_coding(struct Parse *parse_context)
>                                         space_by_id(record->space_id);
>                                 VdbeComment((v, comment, space_name(space)));
>                         }
> -                       /* Set P2 of operation. */
> +                       /* Set jump target for OP_Error and OP_SInsert. */
>                         sqlVdbeChangeP2(v, record->op_number, v->nOp);
>                 }
>                 sqlVdbeAddOp1(v, OP_Halt, -1);
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 445b58a62..73dc6e4d7 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4521,9 +4521,6 @@ extern int sqlSubProgramsRemaining;
>   * The function allocates error and name resources for VDBE
>   * itself.
>   *
> - * This function should not be used after the OP_SInsert, since it
> - * can lead to garbage in the case of an error.
> - *
>   * @param parser Parsing context.
>   * @param space_id Space to lookup identifier.
>   * @param index_id Index identifier of key.
>
>
Thank you. Fixed.

>> diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua
>> index c67a447..1495134 100644
>> --- a/test/sql/clear.test.lua
>> +++ b/test/sql/clear.test.lua
>> @@ -27,5 +27,16 @@ box.execute("SELECT * from zoobar")
>> box.execute("DROP INDEX zoobar2 ON zoobar")
>> box.execute("DROP TABLE zoobar")
>>
>> +--
>> +-- gh-4183: Garbage in case of failed constraint creation.
>> +--
>> +box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(if > 0), CONSTRAINT ck1 CHECK(id < 0));”)
>
> if > 0 -> id > 0
> Please, use correct condition in check constraint.
>
Sorry, fixed.


New patch:

From 34cdfac3f2acbfd77cd394481616041c863c1300 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 run destructors before halting in case
constraint creation failed.

Part of #4183

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index aab60dd..15b8162 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);
@@ -3283,6 +3306,8 @@ 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);
+		if (is_clean_needed)
+			save_record(parser, 0, 0, 0, v->nOp - 1, false);
 		sqlVdbeAddOp1(v, OP_Halt, -1);
 	}
 	sqlVdbeJumpHere(v, jmp);
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..4e10b0e 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -168,3 +168,30 @@ box.execute("DROP TABLE zoobar")
 ...
 -- Debug
 -- require("console").start()
+--
+-- gh-4183: Garbage in case of failed constraint creation.
+--
+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..4e212e3 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -29,3 +29,14 @@ box.execute("DROP TABLE zoobar")
 
 -- Debug
 -- require("console").start()
+
+--
+-- gh-4183: Garbage in case of failed constraint creation.
+--
+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 v2 3/3] sql: Fix destructors for constraints

[imeevma]

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.

Closes #4183
---
 src/box/sql/build.c     | 16 ++++++++++------
 test/sql/clear.result   | 20 ++++++++++++++++++++
 test/sql/clear.test.lua |  9 +++++++++
 3 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 15b8162..aa2d55d 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 4e10b0e..cb98765 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -195,3 +195,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 4e212e3..a2ec209 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -40,3 +40,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()
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 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?

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

[tarantool-patches] Re: [PATCH v2 3/3] sql: Fix destructors for constraints

[n.pettik]

> 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