* [tarantool-patches] [PATCH v1 0/2] sql: clean-up in case constraint creation failed
@ 2019-06-25 15:31 imeevma
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE imeevma
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed imeevma
0 siblings, 2 replies; 5+ messages in thread
From: imeevma @ 2019-06-25 15:31 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE
2019-06-25 15:31 [tarantool-patches] [PATCH v1 0/2] sql: clean-up in case constraint creation failed imeevma
@ 2019-06-25 15:31 ` imeevma
2019-06-26 16:11 ` [tarantool-patches] " n.pettik
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed imeevma
1 sibling, 1 reply; 5+ messages in thread
From: imeevma @ 2019-06-25 15:31 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed
2019-06-25 15:31 [tarantool-patches] [PATCH v1 0/2] sql: clean-up in case constraint creation failed imeevma
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE imeevma
@ 2019-06-25 15:31 ` imeevma
2019-06-26 16:11 ` [tarantool-patches] " n.pettik
1 sibling, 1 reply; 5+ messages in thread
From: imeevma @ 2019-06-25 15:31 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/2] sql: add OP_Error opcode in VDBE
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE imeevma
@ 2019-06-26 16:11 ` n.pettik
0 siblings, 0 replies; 5+ messages in thread
From: n.pettik @ 2019-06-26 16:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: Imeev Mergen
> 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;
> +}
> +
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/2] sql: clean-up in case constraint creation failed
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed imeevma
@ 2019-06-26 16:11 ` n.pettik
0 siblings, 0 replies; 5+ messages in thread
From: n.pettik @ 2019-06-26 16:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: Imeev Mergen
> 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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-26 16:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-25 15:31 [tarantool-patches] [PATCH v1 0/2] sql: clean-up in case constraint creation failed imeevma
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE imeevma
2019-06-26 16:11 ` [tarantool-patches] " n.pettik
2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed imeevma
2019-06-26 16:11 ` [tarantool-patches] " n.pettik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox