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 3B01E3163B for ; Wed, 26 Jun 2019 12:11:13 -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 hL1eESoPknia for ; Wed, 26 Jun 2019 12:11:13 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 E416C312C7 for ; Wed, 26 Jun 2019 12:11:12 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v1 2/2] sql: clean-up in case constraint creation failed From: "n.pettik" In-Reply-To: Date: Wed, 26 Jun 2019 19:11:11 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <61E497AB-D885-4AF0-9705-B975F54ADE97@tarantool.org> 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: tarantool-patches@freelists.org Cc: Imeev Mergen > On 25 Jun 2019, at 18:31, imeevma@tarantool.org wrote: >=20 > This patch makes VDBE run destructors before halting in case > constraint creation failed. >=20 > Closes #4183 tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT = ck1 CHECK(if > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t1, = CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT ck1 CHECK(id < = 0));") --- - error: 'Supplied key type of part 0 does not match index part type: = expected string' ... tarantool> box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT = ck1 CHECK(if > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t1, = CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t1, CONSTRAINT ck1 CHECK(id < = 0));") --- - error: Space 'T1' already exists ... Fix this bug. Also apply this diff containing explanations to the patch: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index d05ca0bc3..0b907b167 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -74,21 +74,29 @@ struct saved_record /** The number of the operation. */ int op_number; /** Flag to show that operation is SInsert. */ - bool is_sinsert; + bool is_insert; }; =20 /** - * 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 =3D 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 =3D reg_key; record->reg_key_count =3D reg_key_count; record->op_number =3D op_number; - record->is_sinsert =3D is_sinsert; + record->is_insert =3D is_insert_op; rlist_add_entry(&parser->record_list, record, link); } =20 @@ -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 =3D @@ -136,7 +145,7 @@ sql_finish_coding(struct Parse *parse_context) MAYBE_UNUSED const char *comment =3D "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 =3D ++parse_context->nMem; sqlVdbeAddOp3(v, OP_MakeRecord, = record->reg_key, record->reg_key_count, = rec_reg); @@ -146,7 +155,7 @@ sql_finish_coding(struct Parse *parse_context) space_by_id(record->space_id); VdbeComment((v, comment, = space_name(space))); } - /* Set P2 of operation. */ + /* Set jump target for OP_Error and OP_SInsert. = */ sqlVdbeChangeP2(v, record->op_number, v->nOp); } sqlVdbeAddOp1(v, OP_Halt, -1); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 445b58a62..73dc6e4d7 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4521,9 +4521,6 @@ extern int sqlSubProgramsRemaining; * The function allocates error and name resources for VDBE * itself. * - * This function should not be used after the OP_SInsert, since it - * can lead to garbage in the case of an error. - * * @param parser Parsing context. * @param space_id Space to lookup identifier. * @param index_id Index identifier of key. > diff --git a/test/sql/clear.test.lua b/test/sql/clear.test.lua > index c67a447..1495134 100644 > --- a/test/sql/clear.test.lua > +++ b/test/sql/clear.test.lua > @@ -27,5 +27,16 @@ box.execute("SELECT * from zoobar") > box.execute("DROP INDEX zoobar2 ON zoobar") > box.execute("DROP TABLE zoobar") >=20 > +-- > +-- 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));=E2=80=9D) if > 0 -> id > 0 Please, use correct condition in check constraint.