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

imeevma at tarantool.org imeevma at tarantool.org
Tue Jun 25 18:31:54 MSK 2019


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





More information about the Tarantool-patches mailing list