* [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 0/3] sql: clean-up in case constraint creation failed @ 2019-07-04 10:49 imeevma 2019-07-04 10:49 ` [tarantool-patches] [PATCH v3 1/3] sql: add OP_SetDiag opcode in VDBE imeevma 0 siblings, 1 reply; 5+ messages in thread From: imeevma @ 2019-07-04 10:49 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_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 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-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 Hi! Thank you for review. New patch and diff between versions of the patch below. On 7/3/19 2:27 AM, n.pettik wrote: > >>>> + */ >>>> +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. > > Do not understand what you mean. Could you provide > some examples? > Sorry, I can't. But I have beed said so by Vlad: On 6/2/19 7:35 PM, Vladislav Shpilevoy wrote: > There was a concrete reason, why it was possible - because different > error codes have different arguments of various types, and the only way > to set an error at parsing stage is to allow to set arbitrary error > message to any error code. Without '...', va_arg etc. Besides, we could > use it to set correct line number and function name in future. Now you > use diag_set, which restricts us. > > So why do you need that patch? We will need to revert it when an error > appears requiring more than one argument, or an argument of not > const char * type. That will definitely happen. > It can be ween here: https://www.freelists.org/post/tarantool-patches/PATCH-v1-49-sql-rework-diag-set-in-OP-Halt,1 >> 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. > > What is destructor? You didn’t mention that now OP_Halt > doesn’t set errors at all from now. Please, I am asking you > not for the first time, provide decent commit messages. > Fixed. >> 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); > > Are these manipulations with labels related to patch? > ‘label’ or ‘addr' are more appropriate names IMHO. > I think that these manipulations should be done, since in this patch we should jump to different lengths depending on the value of no_error. This will be even more severe after the next patch. >> sqlVdbeAddOp1(v, OP_Close, cursor); >> return 0; >> } >> >> 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); > > Why did you rename error_msg to err? It’s redundant diff. > Fixed. >> + 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); >> } >> >> @@ -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”); > > Why do you enlarge diff providing meaningless renaming - error -> err? > Fixed. >> 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; >> +} >> + > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ffb0df4b3..f0d21dc88 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -885,19 +885,22 @@ case OP_Goto: { /* jump */ > goto jump_to_p2; > } > > -/* Opcode: SetDiag P1 P2 * P4 * > +/* 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. > + * 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. > * > - * The P1 parameter is an error code to be set. The P4 parameter > - * is a text description. > + * 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) > - break; > - goto jump_to_p2; > + if (pOp->p2 != 0) > + goto jump_to_p2; > + break; > } > > /* Opcode: Gosub P1 P2 * * * > Fixed. Diff: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index aab60dd..5eb11c2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3277,7 +3277,8 @@ 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 jmp = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, key_len); + int addr = sqlVdbeAddOp4Int(v, cond_opcode, cursor, 0, key_reg, + key_len); if (no_error) { sqlVdbeAddOp0(v, OP_Halt); } else { @@ -3285,7 +3286,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id, P4_DYNAMIC); sqlVdbeAddOp1(v, OP_Halt, -1); } - sqlVdbeJumpHere(v, jmp); + sqlVdbeJumpHere(v, addr); sqlVdbeAddOp1(v, OP_Close, cursor); return 0; } diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index b065de3..4386140 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -809,9 +809,9 @@ 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 *err = tt_sprintf(fmt, ck_constraint_name, expr_str); + const char *error_msg = tt_sprintf(fmt, ck_constraint_name, expr_str); sqlVdbeAddOp4(v, OP_SetDiag, ER_CK_CONSTRAINT_FAILED, 0, 0, - sqlDbStrDup(parser->db, err), P4_DYNAMIC); + 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); diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 301a4c4..bd144c1 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -5406,10 +5406,10 @@ 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 *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); + const char *error = tt_sprintf(tnt_errcode_desc(ER_SQL_EXECUTE), + "Expression subquery returned more "\ + "than 1 row"); + 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 ffb0df4..be78c9b 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -885,19 +885,22 @@ case OP_Goto: { /* jump */ goto jump_to_p2; } -/* Opcode: SetDiag P1 P2 * P4 * +/* 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. + * 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. * - * The P1 parameter is an error code to be set. The P4 parameter - * is a text description. + * 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) - break; - goto jump_to_p2; + if (pOp->p2 != 0) + goto jump_to_p2; + break; } /* Opcode: Gosub P1 P2 * * * @@ -1006,20 +1009,23 @@ case OP_Yield: { /* in1, jump */ /* 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. 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. - * - * 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. + * 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. + * + * 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. */ case OP_Halt: { VdbeFrame *pFrame; New patch: From 0a0f232c4c4c7068df8514230c149bc953a080a5 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 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 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 292168f..5eb11c2 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3277,15 +3277,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 d7104d8..d7d0375 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 c8887f9..be78c9b 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; ^ 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 1/3] sql: add OP_SetDiag opcode in VDBE imeevma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox