* [tarantool-patches] [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE
2019-06-28 12:07 [tarantool-patches] [PATCH v2 0/3] sql: clean-up in case constraint creation failed imeevma
@ 2019-06-28 12:07 ` imeevma
2019-07-02 23:27 ` [tarantool-patches] " n.pettik
2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 2/3] sql: clean-up in case constraint creation failed imeevma
2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 3/3] sql: Fix destructors for constraints imeevma
2 siblings, 1 reply; 6+ messages in thread
From: imeevma @ 2019-06-28 12:07 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
Hi! Thank you for review! My answers and new patch below.
On 6/26/19 7:11 PM, n.pettik wrote:
>
>> 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?
>
Fixed.
>>
>> +/* Opcode: Error P1 P2 * P4 *
>
> I’d call it OP_DiagError or OP_SetDiag.
>
Fixed. Changed to 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
>
Fixed with new description.
>> + *
>> + * The P1 parameter is error code of the erroe. The P4 parameter
>
> -> is an error code to be set.
>
Fixed.
>> + * is a description of the error.
>
> -> is a text description.
>
Fixed.
>> + */
>> +case OP_Error: { /* jump */
>> + box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
>
> Why not simple diag_set() ?
>
I did it like this because using diag_set() would be a bit
troublesome if we decided to use an argument of a different type
or more than one argument.
New patch:
From 55b8f0d0ac93ed6df10a538e8485d9309c12f2b7 Mon Sep 17 00:00:00 2001
Date: Thu, 27 Jun 2019 16:28:30 +0300
Subject: [PATCH] sql: add OP_SetDiag opcode in VDBE
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
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 292168f..aab60dd 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3277,15 +3277,15 @@ 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 jmp = 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, jmp);
sqlVdbeAddOp1(v, OP_Close, cursor);
return 0;
}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 898d16c..d549c6b 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4347,8 +4347,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..b065de3 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -809,10 +809,10 @@ vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
int check_is_passed = sqlVdbeMakeLabel(v);
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);
+ const char *err = tt_sprintf(fmt, ck_constraint_name, expr_str);
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0,
+ sqlDbStrDup(parser->db, err), 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 a257e72..faa7c34 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);
@@ -5405,10 +5408,11 @@ vdbe_code_raise_on_multiple_rows(struct Parse *parser, int limit_reg, int end_ma
int r1 = sqlGetTempReg(parser);
sqlVdbeAddOp2(v, OP_Integer, 0, r1);
sqlVdbeAddOp3(v, OP_Ne, r1, end_mark, limit_reg);
- 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);
+ const char *err = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE),
+ "Expression subquery returned more than "
+ "1 row");
+ sqlVdbeAddOp4(v, OP_SetDiag, ER_SQL_EXECUTE, 0, 0, err, 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 c8887f9..ffb0df4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -885,6 +885,21 @@ case OP_Goto: { /* jump */
goto jump_to_p2;
}
+/* Opcode: SetDiag P1 P2 * P4 *
+ *
+ * Set an error. After that jump to address P2 if P2 is not 0,
+ * else go to the next instruction.
+ *
+ * The P1 parameter is an error code to be set. The P4 parameter
+ * is a text description.
+ */
+case OP_SetDiag: { /* jump */
+ box_error_set(__FILE__, __LINE__, pOp->p1, pOp->p4.z);
+ if (pOp->p2 == 0)
+ break;
+ goto jump_to_p2;
+}
+
/* Opcode: Gosub P1 P2 * * *
*
* Write the current address onto register P1
@@ -989,34 +1004,27 @@ case OP_Yield: { /* in1, jump */
break;
}
-/* Opcode: Halt P1 P2 P3 P4 *
+/* Opcode: Halt P1 P2 * * *
*
- * Exit immediately. All open cursors, etc are closed
+ * Exit immediately. All open cursors, etc are closed
* automatically.
*
- * P1 is the result code returned by sql_exec(),
- * sql_reset(), or sql_finalize(). For a normal halt,
- * this should be 0.
- * For errors, it can be some other value. If P1!=0 then P2 will
- * determine whether or not to rollback the current transaction.
- * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
- * if P2==ON_CONFLICT_ACTION_ROLLBACK. If
- * P2==ON_CONFLICT_ACTION_ABORT, then back out all changes that
- * 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.
+ * P1 is the result code. For a normal halt, this should be 0. For
+ * errors, it is -1. If P1 != 0 then P2 will determine whether or
+ * not to rollback the current transaction. Do not rollback if
+ * P2 is ON_CONFLICT_ACTION_FAIL. Do the rollback if P2 is
+ * ON_CONFLICT_ACTION_ROLLBACK. If P2 is ON_CONFLICT_ACTION_ABORT,
+ * then back out all changes that have occurred during this
+ * execution of the VDBE, but do not rollback the transaction.
*
- * 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.
+ * There is an implied "Halt 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.
*/
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 +1053,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;
^ permalink raw reply [flat|nested] 6+ messages in thread* [tarantool-patches] [PATCH v2 2/3] sql: clean-up in case constraint creation failed
2019-06-28 12:07 [tarantool-patches] [PATCH v2 0/3] sql: clean-up in case constraint creation failed imeevma
2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
@ 2019-06-28 12:07 ` imeevma
2019-06-28 12:07 ` [tarantool-patches] [PATCH v2 3/3] sql: Fix destructors for constraints imeevma
2 siblings, 0 replies; 6+ messages in thread
From: imeevma @ 2019-06-28 12:07 UTC (permalink / raw)
To: korablev; +Cc: tarantool-patches
Thank you for review! My answers and new patch below.
On 6/26/19 7:11 PM, n.pettik wrote:
>
>
>> 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:
>
Fixed in next 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.
>
>
Thank you. Fixed.
>> 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.
>
Sorry, fixed.
New patch:
From 34cdfac3f2acbfd77cd394481616041c863c1300 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 run destructors before halting in case
constraint creation failed.
Part of #4183
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index aab60dd..15b8162 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);
@@ -3283,6 +3306,8 @@ 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);
+ if (is_clean_needed)
+ save_record(parser, 0, 0, 0, v->nOp - 1, false);
sqlVdbeAddOp1(v, OP_Halt, -1);
}
sqlVdbeJumpHere(v, jmp);
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..4e10b0e 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -168,3 +168,30 @@ box.execute("DROP TABLE zoobar")
...
-- Debug
-- require("console").start()
+--
+-- gh-4183: Garbage in case of failed constraint creation.
+--
+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..4e212e3 100644
--- a/test/sql/clear.test.lua
+++ b/test/sql/clear.test.lua
@@ -29,3 +29,14 @@ box.execute("DROP TABLE zoobar")
-- Debug
-- require("console").start()
+
+--
+-- gh-4183: Garbage in case of failed constraint creation.
+--
+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] 6+ messages in thread