From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Imeev Mergen <imeevma@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v1 2/2] sql: clean-up in case constraint creation failed Date: Wed, 26 Jun 2019 19:11:11 +0300 [thread overview] Message-ID: <61E497AB-D885-4AF0-9705-B975F54ADE97@tarantool.org> (raw) In-Reply-To: <f3e9676e11f26d4e84b81d88093d365669d0e39f.1561476468.git.imeevma@gmail.com> > 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: 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. > 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.
prev parent reply other threads:[~2019-06-26 16:11 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-25 15:31 [tarantool-patches] [PATCH v1 0/2] " imeevma 2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 1/2] sql: add OP_Error opcode in VDBE imeevma 2019-06-26 16:11 ` [tarantool-patches] " n.pettik 2019-06-25 15:31 ` [tarantool-patches] [PATCH v1 2/2] sql: clean-up in case constraint creation failed imeevma 2019-06-26 16:11 ` n.pettik [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=61E497AB-D885-4AF0-9705-B975F54ADE97@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v1 2/2] sql: clean-up in case constraint creation failed' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox