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

[tarantool-patches] [PATCH v1 0/2] 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

Mergen Imeev (2):
  sql: add OP_Error opcode in VDBE
  sql: clean-up in case constraint creation failed

 src/box/sql/build.c      | 94 +++++++++++++++++++++++++++++-------------------
 src/box/sql/sqlInt.h     |  3 ++
 src/box/sql/vdbe.c       | 12 +++++++
 test/sql/checks.result   |  3 +-
 test/sql/checks.test.lua |  2 +-
 test/sql/clear.result    | 27 ++++++++++++++
 test/sql/clear.test.lua  | 11 ++++++
 7 files changed, 113 insertions(+), 39 deletions(-)

-- 
2.7.4

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

[imeevma]

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
---
 src/box/sql/build.c | 38 +++++++++++++++++++++++++-------------
 src/box/sql/vdbe.c  | 12 ++++++++++++
 2 files changed, 37 insertions(+), 13 deletions(-)

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);
+
 	sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0,
 		      ck_constraint_reg + 5);
 	save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2,
@@ -1131,14 +1138,19 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	 * Lets check that constraint with this name hasn't
 	 * been created before.
 	 */
-	const char *error_msg =
+	const char *err =
 		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
-	if (vdbe_emit_halt_with_presence_test(parse_context,
-					      BOX_FK_CONSTRAINT_ID, 0,
-					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
-		return;
+	int cursor = parse_context->nTab++;
+	vdbe_emit_open_cursor(parse_context, cursor, 0,
+			      space_by_id(BOX_FK_CONSTRAINT_ID));
+	sqlVdbeChangeP5(vdbe, OPFLAG_SYSTEMSP);
+	sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 3,
+			 constr_tuple_reg, 2);
+	sqlVdbeAddOp4(vdbe, OP_Error, ER_CONSTRAINT_EXISTS, vdbe->nOp + 1, 0,
+		      err, P4_STATIC);
+	sqlVdbeAddOp1(vdbe, OP_Halt, -1);
+	sqlVdbeAddOp1(vdbe, OP_Close, cursor);
+
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
 			  fk_constraint_match_strs[fk->match], P4_STATIC);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..abf881a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,18 @@ case OP_Goto: {             /* jump */
 	goto jump_to_p2;
 }
 
+/* Opcode:  Error P1 P2 * P4 *
+ *
+ * Setting an error and an unconditional jump to address P2.
+ *
+ * The P1 parameter is error code of the erroe. The P4 parameter
+ * is a description of the error.
+ */
+case OP_Error: {             /* jump */
+	box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
+	goto jump_to_p2;
+}
+
 /* Opcode:  Gosub P1 P2 * * *
  *
  * Write the current address onto register P1
-- 
2.7.4

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

[imeevma]

This patch makes VDBE run destructors before halting in case
constraint creation failed.

Closes #4183
---
 src/box/sql/build.c      | 64 +++++++++++++++++++++++++++---------------------
 src/box/sql/sqlInt.h     |  3 +++
 test/sql/checks.result   |  3 ++-
 test/sql/checks.test.lua |  2 +-
 test/sql/clear.result    | 27 ++++++++++++++++++++
 test/sql/clear.test.lua  | 11 +++++++++
 6 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index c75e1d8..d05ca0b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -71,8 +71,10 @@ 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 number of the operation. */
+	int op_number;
+	/** Flag to show that operation is SInsert. */
+	bool is_sinsert;
 };
 
 /**
@@ -82,11 +84,11 @@ struct saved_record
  * @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_number Number of the operation.
  */
 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_number, bool is_sinsert)
 {
 	struct saved_record *record =
 		region_alloc(&parser->region, sizeof(*record));
@@ -99,7 +101,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_number = op_number;
+	record->is_sinsert = is_sinsert;
 	rlist_add_entry(&parser->record_list, record, link);
 }
 
@@ -119,27 +122,32 @@ sql_finish_coding(struct Parse *parse_context)
 	 * 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.
 	 */
 	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 P2 of operation. */
+		sqlVdbeChangeP2(v, record->op_number, 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_sinsert) {
+				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 P2 of operation. */
+			sqlVdbeChangeP2(v, record->op_number, v->nOp);
 		}
 		sqlVdbeAddOp1(v, OP_Halt, -1);
 		VdbeComment((v,
@@ -891,7 +899,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 +958,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;
@@ -1074,17 +1082,17 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	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,
+	sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 2,
 			 ck_constraint_reg, 2);
 	sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, err,
 		      P4_STATIC);
-	sqlVdbeAddOp1(v, OP_Halt, -1);
+	save_record(parser, 0, 0, 0, v->nOp - 1, false);
 	sqlVdbeAddOp1(v, OP_Close, cursor);
 
 	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);
 }
@@ -1144,11 +1152,11 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	vdbe_emit_open_cursor(parse_context, cursor, 0,
 			      space_by_id(BOX_FK_CONSTRAINT_ID));
 	sqlVdbeChangeP5(vdbe, OPFLAG_SYSTEMSP);
-	sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 3,
+	sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 2,
 			 constr_tuple_reg, 2);
 	sqlVdbeAddOp4(vdbe, OP_Error, ER_CONSTRAINT_EXISTS, vdbe->nOp + 1, 0,
 		      err, P4_STATIC);
-	sqlVdbeAddOp1(vdbe, OP_Halt, -1);
+	save_record(parse_context, 0, 0, 0, vdbe->nOp - 1, false);
 	sqlVdbeAddOp1(vdbe, OP_Close, cursor);
 
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
@@ -1199,7 +1207,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:
@@ -1322,7 +1330,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,
@@ -1330,7 +1338,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;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 73dc6e4..445b58a 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4521,6 +4521,9 @@ 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.
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..532a2a0 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -166,5 +166,32 @@ box.execute("DROP TABLE zoobar")
 ---
 - row_count: 1
 ...
+--
+-- 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));")
+---
+- 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()
+---
+- []
+...
 -- Debug
 -- require("console").start()
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));")
+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()
+
 -- Debug
 -- require("console").start()
-- 
2.7.4

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

[n.pettik]

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 292168f..c75e1d8 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1066,14 +1066,21 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
> 		      sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC);
> 	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5,
> 		      ck_constraint_reg + 5);
> -	const char *error_msg =
> +	/* Check that there is no CHECK with such name. */
> +	const char *err =
> 		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
> 					    ck_def->name);
> -	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
> -					      ck_constraint_reg, 2,
> -					      ER_CONSTRAINT_EXISTS, error_msg,
> -					      false, OP_NoConflict) != 0)
> -		return;
> +	int cursor = parser->nTab++;
> +	vdbe_emit_open_cursor(parser, cursor, 0,
> +			      space_by_id(BOX_CK_CONSTRAINT_ID));
> +	sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +	sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 3,
> +			 ck_constraint_reg, 2);
> +	sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, err,
> +		      P4_STATIC);
> +	sqlVdbeAddOp1(v, OP_Halt, -1);
> +	sqlVdbeAddOp1(v, OP_Close, cursor);

I don’t like code duplication: this snippet looks almost the same as
vdbe_emit_halt_with_presence_test(). Wouldn’t it be better if OP_Error
was integrated to that function? Also, why not move error setting
to OP_Error from other OP_Halt usages?

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

I’d call it OP_DiagError or OP_SetDiag.

> + *
> + * Setting an error and an unconditional jump to address P2.

Nit: -> Set an error and process unconditional jump …
Or -> Set an error and unconditionally jump to 

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

-> is an error code to be set.

> + * is a description of the error.

-> is a text description.

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

Why not simple diag_set() ?

> +	goto jump_to_p2;
> +}
> +

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

[n.pettik]

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

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.


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