From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9B0BE3138A for ; Tue, 25 Jun 2019 11:31:56 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Pwmj0-eMG9n3 for ; Tue, 25 Jun 2019 11:31:56 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 387E131385 for ; Tue, 25 Jun 2019 11:31:56 -0400 (EDT) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed Date: Tue, 25 Jun 2019 18:31:54 +0300 Message-Id: In-Reply-To: References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: korablev@tarantool.org Cc: tarantool-patches@freelists.org This patch makes VDBE run destructors before halting in case constraint creation failed. Closes #4183 --- src/box/sql/build.c | 64 +++++++++++++++++++++++++++--------------------- src/box/sql/sqlInt.h | 3 +++ test/sql/checks.result | 3 ++- test/sql/checks.test.lua | 2 +- test/sql/clear.result | 27 ++++++++++++++++++++ test/sql/clear.test.lua | 11 +++++++++ 6 files changed, 80 insertions(+), 30 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index c75e1d8..d05ca0b 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -71,8 +71,10 @@ struct saved_record int reg_key; /** Number of registers the key consists of. */ int reg_key_count; - /** The number of the OP_SInsert operation. */ - int insertion_opcode; + /** The number of the operation. */ + int op_number; + /** Flag to show that operation is SInsert. */ + bool is_sinsert; }; /** @@ -82,11 +84,11 @@ struct saved_record * @param space_id Id of table in which record is inserted. * @param reg_key Register that contains first field of the key. * @param reg_key_count Exact number of fields of the key. - * @param insertion_opcode Number of OP_SInsert opcode. + * @param op_number Number of the operation. */ static inline void save_record(struct Parse *parser, uint32_t space_id, int reg_key, - int reg_key_count, int insertion_opcode) + int reg_key_count, int op_number, bool is_sinsert) { struct saved_record *record = region_alloc(&parser->region, sizeof(*record)); @@ -99,7 +101,8 @@ save_record(struct Parse *parser, uint32_t space_id, int reg_key, record->space_id = space_id; record->reg_key = reg_key; record->reg_key_count = reg_key_count; - record->insertion_opcode = insertion_opcode; + record->op_number = op_number; + record->is_sinsert = is_sinsert; rlist_add_entry(&parser->record_list, record, link); } @@ -119,27 +122,32 @@ sql_finish_coding(struct Parse *parse_context) * other "CREATE ..." statements but it won't delete * anything as these statements create no more than one * record. + * + * Here it was assumed that after OP_Error there is at + * least one OP_SInsert. That means that the last opcode + * in the current list is OP_SInsert. */ if (!rlist_empty(&parse_context->record_list)) { struct saved_record *record = rlist_shift_entry(&parse_context->record_list, struct saved_record, link); - /* Set P2 of SInsert. */ - sqlVdbeChangeP2(v, record->insertion_opcode, v->nOp); + /* Set P2 of operation. */ + sqlVdbeChangeP2(v, record->op_number, v->nOp); MAYBE_UNUSED const char *comment = "Delete entry from %s if CREATE TABLE fails"; rlist_foreach_entry(record, &parse_context->record_list, link) { - int record_reg = ++parse_context->nMem; - sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key, - record->reg_key_count, record_reg); - sqlVdbeAddOp2(v, OP_SDelete, record->space_id, - record_reg); - MAYBE_UNUSED struct space *space = - space_by_id(record->space_id); - VdbeComment((v, comment, space_name(space))); - /* Set P2 of SInsert. */ - sqlVdbeChangeP2(v, record->insertion_opcode, - v->nOp); + if (record->is_sinsert) { + int rec_reg = ++parse_context->nMem; + sqlVdbeAddOp3(v, OP_MakeRecord, record->reg_key, + record->reg_key_count, rec_reg); + sqlVdbeAddOp2(v, OP_SDelete, record->space_id, + rec_reg); + MAYBE_UNUSED struct space *space = + space_by_id(record->space_id); + VdbeComment((v, comment, space_name(space))); + } + /* Set P2 of operation. */ + sqlVdbeChangeP2(v, record->op_number, v->nOp); } sqlVdbeAddOp1(v, OP_Halt, -1); VdbeComment((v, @@ -891,7 +899,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def, SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC); sqlVdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg); sqlVdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg); - save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1); + save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1, true); return; error: parse->is_aborted = true; @@ -950,7 +958,7 @@ vdbe_emit_space_create(struct Parse *pParse, int space_id_reg, sqlVdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, tuple_reg); sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, tuple_reg); sqlVdbeChangeP5(v, OPFLAG_NCHANGE); - save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1); + save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1, true); return; error: pParse->is_aborted = true; @@ -1074,17 +1082,17 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, vdbe_emit_open_cursor(parser, cursor, 0, space_by_id(BOX_CK_CONSTRAINT_ID)); sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); - sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 3, + sqlVdbeAddOp4Int(v, OP_NoConflict, cursor, v->nOp + 2, ck_constraint_reg, 2); sqlVdbeAddOp4(v, OP_Error, ER_CONSTRAINT_EXISTS, v->nOp + 1, 0, err, P4_STATIC); - sqlVdbeAddOp1(v, OP_Halt, -1); + save_record(parser, 0, 0, 0, v->nOp - 1, false); sqlVdbeAddOp1(v, OP_Close, cursor); sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0, ck_constraint_reg + 5); save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2, - v->nOp - 1); + v->nOp - 1, true); VdbeComment((v, "Create CK constraint %s", ck_def->name)); sqlReleaseTempRange(parser, ck_constraint_reg, 5); } @@ -1144,11 +1152,11 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, vdbe_emit_open_cursor(parse_context, cursor, 0, space_by_id(BOX_FK_CONSTRAINT_ID)); sqlVdbeChangeP5(vdbe, OPFLAG_SYSTEMSP); - sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 3, + sqlVdbeAddOp4Int(vdbe, OP_NoConflict, cursor, vdbe->nOp + 2, constr_tuple_reg, 2); sqlVdbeAddOp4(vdbe, OP_Error, ER_CONSTRAINT_EXISTS, vdbe->nOp + 1, 0, err, P4_STATIC); - sqlVdbeAddOp1(vdbe, OP_Halt, -1); + save_record(parse_context, 0, 0, 0, vdbe->nOp - 1, false); sqlVdbeAddOp1(vdbe, OP_Close, cursor); sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3); @@ -1199,7 +1207,7 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context, sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE); } save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2, - vdbe->nOp - 1); + vdbe->nOp - 1, true); sqlReleaseTempRange(parse_context, constr_tuple_reg, 10); return; error: @@ -1322,7 +1330,7 @@ sqlEndTable(struct Parse *pParse) sqlVdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0, reg_seq_record); save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1, - v->nOp - 1); + v->nOp - 1, true); /* Do an insertion into _space_sequence. */ int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse, reg_space_id, reg_seq_id, @@ -1330,7 +1338,7 @@ sqlEndTable(struct Parse *pParse) sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0, reg_space_seq_record); save_record(pParse, BOX_SPACE_SEQUENCE_ID, - reg_space_seq_record + 1, 1, v->nOp - 1); + reg_space_seq_record + 1, 1, v->nOp - 1, true); } /* Code creation of FK constraints, if any. */ struct fk_constraint_parse *fk_parse; diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 73dc6e4..445b58a 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4521,6 +4521,9 @@ extern int sqlSubProgramsRemaining; * The function allocates error and name resources for VDBE * itself. * + * This function should not be used after the OP_SInsert, since it + * can lead to garbage in the case of an error. + * * @param parser Parsing context. * @param space_id Space to lookup identifier. * @param index_id Index identifier of key. diff --git a/test/sql/checks.result b/test/sql/checks.result index bdcf507..5279ae0 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -251,8 +251,9 @@ box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), C --- - error: Constraint CK1 already exists ... -box.space.T2:drop() +box.space.T2 --- +- null ... box.space._ck_constraint:select() --- diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua index 50cb13c..cacba59 100644 --- a/test/sql/checks.test.lua +++ b/test/sql/checks.test.lua @@ -86,7 +86,7 @@ box.execute("INSERT INTO \"test\" VALUES(11, 11);") box.execute("INSERT INTO \"test\" VALUES(12, 11);") s:drop() box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))") -box.space.T2:drop() +box.space.T2 box.space._ck_constraint:select() -- diff --git a/test/sql/clear.result b/test/sql/clear.result index 0a3baa3..532a2a0 100644 --- a/test/sql/clear.result +++ b/test/sql/clear.result @@ -166,5 +166,32 @@ box.execute("DROP TABLE zoobar") --- - row_count: 1 ... +-- +-- gh-4183: Garbage in case of failed constraint creation. +-- +box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(if > 0), CONSTRAINT ck1 CHECK(id < 0));") +--- +- error: Constraint CK1 already exists +... +box.space.t1 +--- +- null +... +box.space._ck_constraint:select() +--- +- [] +... +box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);") +--- +- error: Constraint FK1 already exists +... +box.space.t2 +--- +- null +... +box.space._fk_constraint:select() +--- +- [] +... -- Debug -- require("console").start() diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua index c67a447..1495134 100644 --- a/test/sql/clear.test.lua +++ b/test/sql/clear.test.lua @@ -27,5 +27,16 @@ box.execute("SELECT * from zoobar") box.execute("DROP INDEX zoobar2 ON zoobar") box.execute("DROP TABLE zoobar") +-- +-- gh-4183: Garbage in case of failed constraint creation. +-- +box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(if > 0), CONSTRAINT ck1 CHECK(id < 0));") +box.space.t1 +box.space._ck_constraint:select() + +box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);") +box.space.t2 +box.space._fk_constraint:select() + -- Debug -- require("console").start() -- 2.7.4