* [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed
@ 2019-07-15 15:16 imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: imeevma @ 2019-07-15 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: korablev, tarantool-patches
This patch makes VDBE run destructors before halting in case
constraint creation failed. This is done using new opcode
OP_SetDiag, 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.
Changes in v3:
1) Removed unnecessary changes.
2) Fixed commit-messages.
3) Now OP_Halt won't be added after OP_SetDiag in case clean-up
needed.
Mergen Imeev (3):
sql: add OP_SetDiag opcode in VDBE
sql: clean-up in case constraint creation failed
sql: use common registers instead of temp. for constraints data
src/box/sql/build.c | 117 ++++++++++++++++++++++++++++----------------
src/box/sql/expr.c | 6 ++-
src/box/sql/fk_constraint.c | 3 +-
src/box/sql/insert.c | 11 +++--
src/box/sql/select.c | 14 ++++--
src/box/sql/sqlInt.h | 2 +-
src/box/sql/trigger.c | 6 ++-
src/box/sql/vdbe.c | 31 +++++++-----
test/sql/checks.result | 3 +-
test/sql/checks.test.lua | 2 +-
test/sql/clear.result | 50 +++++++++++++++++++
test/sql/clear.test.lua | 23 +++++++++
12 files changed, 196 insertions(+), 72 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE
2019-07-15 15:16 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
@ 2019-07-15 15:16 ` imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data imeevma
2 siblings, 0 replies; 5+ messages in thread
From: imeevma @ 2019-07-15 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: korablev, tarantool-patches
To separate the error setting and execution halting, a new opcode
OP_SetDiag was created. The only functionality of the opcode is
the execution of diag_set(). It is important to note that
OP_SetDiag does not set is_aborted to true, so we can continue
working with other opcodes, if necessary. This function allows us
to perform cleanup in some special cases, for example, when
creating a constraint failed because of the creation of two or
more constraints with the same name in the same CREATE TABLE
statement.
Since now diag_set() is executed in OP_SetDiag, this functionality
has been removed from OP_Halt.
Needed for #4183
---
src/box/sql/build.c | 9 +++++----
src/box/sql/expr.c | 6 ++++--
src/box/sql/fk_constraint.c | 3 ++-
src/box/sql/insert.c | 11 ++++++-----
src/box/sql/select.c | 14 +++++++++-----
src/box/sql/vdbe.c | 31 ++++++++++++++++++++-----------
6 files changed, 46 insertions(+), 28 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 396de63..e56b9da 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3297,15 +3297,16 @@ 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 addr = 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, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
return 0;
}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 7673832..916b0a0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4339,8 +4339,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..4386140 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -810,9 +810,9 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
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);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
+ sqlDbStrDup(parser->db, error_msg), 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 7c8da25..bd144c1 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);
@@ -5406,7 +5409,8 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
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);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, error, 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 9f4ee7a..6a4a303 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,24 @@ case OP_Goto: { /* jump */
goto jump_to_p2;
}
+/* Opcode: SetDiag P1 P2 * P4 *
+ *
+ * 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.
+ *
+ * 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)
+ goto jump_to_p2;
+ break;
+}
+
/* Opcode: Gosub P1 P2 * * *
*
* Write the current address onto register P1
@@ -989,7 +1007,7 @@ case OP_Yield: { /* in1, jump */
break;
}
-/* Opcode: Halt P1 P2 P3 P4 *
+/* Opcode: Halt P1 P2 * * *
*
* Exit immediately. All open cursors, etc are closed
* automatically.
@@ -1005,11 +1023,6 @@ case OP_Yield: { /* in1, jump */
* 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.
- *
- * 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.
@@ -1017,6 +1030,7 @@ case OP_Yield: { /* in1, jump */
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 +1059,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;
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed
2019-07-15 15:16 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
@ 2019-07-15 15:16 ` imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data imeevma
2 siblings, 0 replies; 5+ messages in thread
From: imeevma @ 2019-07-15 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: korablev, tarantool-patches
This patch makes VDBE to perform a clean-up if the creation of a
constraint fails because of the creation of two or more
constraints of the same type with the same name and in the same
CREATE TABLE statement.
For example:
CREATE TABLE t1(
id INT PRIMARY KEY,
CONSTRAINT ck1 CHECK(id > 1),
CONSTRAINT ck1 CHECK(id < 10)
);
Part of #4183
---
src/box/sql/build.c | 94 ++++++++++++++++++++++++++++++------------------
src/box/sql/sqlInt.h | 2 +-
src/box/sql/trigger.c | 6 ++--
test/sql/checks.result | 3 +-
test/sql/checks.test.lua | 2 +-
test/sql/clear.result | 30 ++++++++++++++++
test/sql/clear.test.lua | 14 ++++++++
7 files changed, 112 insertions(+), 39 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index e56b9da..c071886 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,
@@ -911,7 +934,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;
@@ -970,7 +993,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;
@@ -1092,12 +1115,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);
}
@@ -1157,7 +1180,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,
@@ -1207,7 +1230,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:
@@ -1304,7 +1327,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);
@@ -1330,7 +1353,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,
@@ -1338,7 +1361,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;
@@ -1465,7 +1488,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),
@@ -1584,7 +1607,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;
}
@@ -1617,7 +1640,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);
@@ -3283,7 +3306,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);
@@ -3304,7 +3327,10 @@ 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);
- sqlVdbeAddOp1(v, OP_Halt, -1);
+ if (is_clean_needed)
+ save_record(parser, 0, 0, 0, v->nOp - 1, false);
+ else
+ sqlVdbeAddOp1(v, OP_Halt, -1);
}
sqlVdbeJumpHere(v, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 3b8c82d..4f5bad2 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4542,7 +4542,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..2034d82 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -168,3 +168,33 @@ box.execute("DROP TABLE zoobar")
...
-- Debug
-- require("console").start()
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+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..7ed18a4 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -29,3 +29,17 @@ box.execute("DROP TABLE zoobar")
-- Debug
-- require("console").start()
+
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+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()
--
2.7.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data
2019-07-15 15:16 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed imeevma
@ 2019-07-15 15:16 ` imeevma
2 siblings, 0 replies; 5+ messages in thread
From: imeevma @ 2019-07-15 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: korablev, tarantool-patches
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
---
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 c071886..2aefa2a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1098,7 +1098,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);
@@ -1122,7 +1127,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);
}
/**
@@ -1141,10 +1145,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;
@@ -1231,7 +1236,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 2034d82..2fb1761 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -198,3 +198,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 7ed18a4..4c58767 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -43,3 +43,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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed
2019-07-04 10:49 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
@ 2019-07-04 10:49 ` imeevma
0 siblings, 0 replies; 5+ messages in thread
From: imeevma @ 2019-07-04 10:49 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
Diff between patches and the new patch below. In addition to the
changes shown in diff, a new commit-message was written.
Diff between versions:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 12b6e5e..0dd2906 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3309,7 +3309,8 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
P4_DYNAMIC);
if (is_clean_needed)
save_record(parser, 0, 0, 0, v->nOp - 1, false);
- sqlVdbeAddOp1(v, OP_Halt, -1);
+ else
+ sqlVdbeAddOp1(v, OP_Halt, -1);
}
sqlVdbeJumpHere(v, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
diff --git a/test/sql/clear.result b/test/sql/clear.result
index 4e10b0e..2034d82 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -169,7 +169,10 @@ box.execute("DROP TABLE zoobar")
-- Debug
-- require("console").start()
--
--- gh-4183: Garbage in case of failed constraint creation.
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
--
box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
---
diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua
index 4e212e3..7ed18a4 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -31,7 +31,10 @@ box.execute("DROP TABLE zoobar")
-- require("console").start()
--
--- gh-4183: Garbage in case of failed constraint creation.
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
--
box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
box.space.t1
New version:
From 58bd81c8bac35962942d6e18948ed2d6b267366b 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 to perform a clean-up if the creation of a
constraint fails because of the creation of two or more
constraints of the same type with the same name and in the same
CREATE TABLE statement.
For example:
CREATE TABLE t1(
id INT PRIMARY KEY,
CONSTRAINT ck1 CHECK(id > 1),
CONSTRAINT ck1 CHECK(id < 10)
);
Part of #4183
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5eb11c2..0dd2906 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);
@@ -3284,7 +3307,10 @@ 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);
- sqlVdbeAddOp1(v, OP_Halt, -1);
+ if (is_clean_needed)
+ save_record(parser, 0, 0, 0, v->nOp - 1, false);
+ else
+ sqlVdbeAddOp1(v, OP_Halt, -1);
}
sqlVdbeJumpHere(v, addr);
sqlVdbeAddOp1(v, OP_Close, cursor);
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..2034d82 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -168,3 +168,33 @@ box.execute("DROP TABLE zoobar")
...
-- Debug
-- require("console").start()
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+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..7ed18a4 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -29,3 +29,17 @@ box.execute("DROP TABLE zoobar")
-- Debug
-- require("console").start()
+
+--
+-- gh-4183: Check if there is a garbage in case of failure to
+-- create a constraint, when more than one constraint of the same
+-- type is created with the same name and in the same
+-- CREATE TABLE statement.
+--
+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()
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-15 15:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 15:16 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 2/3] sql: clean-up in case constraint creation failed imeevma
2019-07-15 15:16 ` [tarantool-patches] [PATCH v3 3/3] sql: use common registers instead of temp. for constraints data imeevma
-- strict thread matches above, loose matches on Subject: below --
2019-07-04 10:49 [tarantool-patches] [PATCH v3 0/3] sql: clean-up in case constraint creation failed imeevma
2019-07-04 10:49 ` [tarantool-patches] [PATCH v3 2/3] " imeevma
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox