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