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 DD7232B92B for ; Tue, 9 Oct 2018 10:15: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 QMjaYjAvcEs0 for ; Tue, 9 Oct 2018 10:15:13 -0400 (EDT) Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 067242B007 for ; Tue, 9 Oct 2018 10:15:12 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation From: "n.pettik" In-Reply-To: <51918bf1-14ea-45aa-4e1e-901d2ee9c532@tarantool.org> Date: Tue, 9 Oct 2018 17:15:10 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <1AF44E5C-F76B-4783-8578-FC4AFA437E66@tarantool.org> References: <433c416428f1ef575324236be22c27962db3f8b5.1536346889.git.imeevma@gmail.com> <74ed5f5b-d74f-0933-ccf0-c0d9861f0842@tarantool.org> <30e818b1-2916-58cc-9074-2764de0d6fad@tarantool.org> <80242351-a576-dde6-034e-f19c899a9cc9@tarantool.org> <51918bf1-14ea-45aa-4e1e-901d2ee9c532@tarantool.org> 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 Attach please next time diff between two versions, so as I can avoid to review whole patch again. Also, check Travis before you send patch: https://travis-ci.org/tarantool/tarantool/jobs/438801852 https://travis-ci.org/tarantool/tarantool/jobs/438801839 Build fails. >>=20 >>> void >>> sql_finish_coding(struct Parse *parse_context) >>> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context) >>> struct sqlite3 *db =3D parse_context->db; >>> struct Vdbe *v =3D sqlite3GetVdbe(parse_context); >>> sqlite3VdbeAddOp0(v, OP_Halt); >>> + /* >>> + * Destructors for all created in current statement >>> + * spaces, indexes, sequences etc. There is no need to >>> + * create destructor for last SInsert. >>> + */ >>> + if (rlist_empty(&parse_context->record_list) =3D=3D 0) { >>=20 >> Nit: if(! rlist_empty())=20 > Fixed. No, it is not fixed. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index a806fb4..6a97ff9 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > void > sql_finish_coding(struct Parse *parse_context) > { > @@ -62,6 +109,40 @@ sql_finish_coding(struct Parse *parse_context) > struct sqlite3 *db =3D parse_context->db; > struct Vdbe *v =3D sqlite3GetVdbe(parse_context); > sqlite3VdbeAddOp0(v, OP_Halt); > + /* > + * In case statement "CREATE TABLE ..." fails it can > + * left some records in system spaces that shouldn't be > + * there. To clean-up properly this code is added. Last > + * record isn't deleted because if statement fails than > + * 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. > + */ > + if (rlist_empty(&parse_context->record_list) =3D=3D 0) { > + struct saved_record *record =3D > + rlist_shift_entry(&parse_context->record_list, > + struct saved_record, link); > + /* Set P2 of SInsert. */ > + sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp); > + char *comment =3D "Delete entry from %s if CREATE TABLE = fails=E2=80=9D; Nit: const char *. > + rlist_foreach_entry(record, &parse_context->record_list, = link) { > + int record_reg =3D ++parse_context->nMem; > + sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key, > + record->reg_key_count, record_reg); > + sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, > + record_reg); > + struct space *space =3D space_by_id(record->space_id); > + VdbeComment((v, comment, space_name(space))); > + /* Set P2 of SInsert. */ > + sqlite3VdbeChangeP2(v, record->insertion_opcode, > + v->nOp); > + } > + sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0); > + VdbeComment((v, > + "Exit with an error if CREATE statement fails")); > + } > + > if (db->mallocFailed || parse_context->nErr !=3D 0) { > if (parse_context->rc =3D=3D SQLITE_OK) > parse_context->rc =3D SQLITE_ERROR; > @@ -1101,13 +1182,14 @@ vdbe_emit_create_index(struct Parse *parse, = struct space_def *def, > sqlite3VdbeAddOp4(v, OP_Blob, index_parts_sz, entry_reg + 5, > SQL_SUBTYPE_MSGPACK, index_parts, P4_STATIC); > sqlite3VdbeAddOp3(v, OP_MakeRecord, entry_reg, 6, tuple_reg); > - sqlite3VdbeAddOp2(v, OP_SInsert, BOX_INDEX_ID, tuple_reg); > + sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg); > /* > * Non-NULL value means that index has been created via > * separate CREATE INDEX statement. > */ > if (idx_def->opts.sql !=3D NULL) > sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > + save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1); As I sad you don=E2=80=99t need to add to this list index entries when = it comes to CREATE INDEX. You can tell index from unique constraint by existence of opts.sql string: +++ b/src/box/sql/build.c @@ -1185,11 +1185,15 @@ vdbe_emit_create_index(struct Parse *parse, = struct space_def *def, sqlite3VdbeAddOp3(v, OP_SInsert, BOX_INDEX_ID, 0, tuple_reg); /* * Non-NULL value means that index has been created via - * separate CREATE INDEX statement. + * separate CREATE INDEX statement. On the other hand, + * we need to register all indexes incoming in + * CREATE TABLE in order to remove them on table creation + * fail. */ if (idx_def->opts.sql !=3D NULL) sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); - save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1); + else + save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - = 1); return; However, in this case we need to jump to abort_due_to_error label during processing of OP_SInsert. I don=E2=80=99t insist on this change = tho, it is up to you.=20 > return; > error: > parse->rc =3D SQL_TARANTOOL_ERROR; > @@ -1165,8 +1247,9 @@ createSpace(Parse * pParse, int iSpaceId, char = *zStmt) > sqlite3VdbeAddOp4(v, OP_Blob, table_stmt_sz, iFirstCol + 6, > SQL_SUBTYPE_MSGPACK, table_stmt, P4_STATIC); > sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord); > - sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord); > + sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord); > sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > + save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1); > return; > error: > pParse->nErr++; > @@ -1340,9 +1423,11 @@ vdbe_emit_fkey_create(struct Parse = *parse_context, const struct fkey_def *fk) > parent_links, P4_DYNAMIC); > sqlite3VdbeAddOp3(vdbe, OP_MakeRecord, constr_tuple_reg, 9, > constr_tuple_reg + 9); > - sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, > + sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0, > constr_tuple_reg + 9); > sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE); > + save_record(parse_context, BOX_FK_CONSTRAINT_ID, = constr_tuple_reg, 2, > + vdbe->nOp - 1); > sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10); > return; > error: > @@ -1487,14 +1572,18 @@ sqlite3EndTable(Parse * pParse, /* Parse = context */ > int reg_seq_record =3D > emitNewSysSequenceRecord(pParse, reg_seq_id, > p->def->name); > - sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, > + sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0, > reg_seq_record); > + save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1, > + v->nOp - 1); > /* Do an insertion into _space_sequence. */ > int reg_space_seq_record =3D > emitNewSysSpaceSequenceRecord(pParse, reg_space_id, > reg_seq_id); > - sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, > + sqlite3VdbeAddOp3(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); If we are creating FK constraint with ALTER TABLE ADD CONSTRAINT, we don=E2=80=99t need to add it to list of things to be deleted (the = same as for index). @@ -1426,8 +1430,10 @@ vdbe_emit_fkey_create(struct Parse = *parse_context, const struct fkey_def *fk) sqlite3VdbeAddOp3(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID, 0, constr_tuple_reg + 9); sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE); - save_record(parse_context, BOX_FK_CONSTRAINT_ID, = constr_tuple_reg, 2, - vdbe->nOp - 1); + if (parse_context->pNewTable !=3D NULL) { + save_record(parse_context, BOX_FK_CONSTRAINT_ID, + constr_tuple_reg, 2, vdbe->nOp - 1); + } > diff --git a/test/sql/drop-table.test.lua = b/test/sql/drop-table.test.lua > index 9663074..1bc8894 100644 > --- a/test/sql/drop-table.test.lua > +++ b/test/sql/drop-table.test.lua > @@ -25,3 +25,62 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, = 222, 'c3', 444)") > + > +box.session.su('admin') > + > +-- > +-- Check that _space, _index and _sequence have the same number of > +-- records. > +-- > +space_count =3D=3D #box.space._space:select() > +index_count =3D=3D #box.space._index:select() > +sequence_count =3D=3D #box.space._sequence:select() > + > +-- > +-- Give user right to write in _index. Still have not enough > +-- rights to write in _sequence. > +-- > +box.schema.user.grant('tmp', 'write', 'space', '_index') > +box.session.su('tmp') > + > +-- > +-- Error: user do not have rights to write in _sequence. > +-- > +box.sql.execute('create table t2 (id int primary key autoincrement, a = unique, b unique, c unique, d unique)=E2=80=99) Nit: for SQL statements please use uppercase. > + > +box.session.su('admin') > + > +-- > +-- Check that _space, _index and _sequence have the same number of > +-- records. > +-- > +space_count =3D=3D #box.space._space:select() > +index_count =3D=3D #box.space._index:select() > +sequence_count =3D=3D #box.space._sequence:select() > + > +box.schema.user.drop('tmp=E2=80=99) I see no tests involving FK constraints. Add them as well.