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 E95CD284C6 for ; Mon, 13 Aug 2018 10:30: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 B4y54zBcS9GT for ; Mon, 13 Aug 2018 10:30:56 -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 A80AC28422 for ; Mon, 13 Aug 2018 10:30:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: cleanup on failed table creation References: <7fa7510873713138b01c6568aaa5935ba70a5ba4.1533822416.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Mon, 13 Aug 2018 17:30:53 +0300 MIME-Version: 1.0 In-Reply-To: <7fa7510873713138b01c6568aaa5935ba70a5ba4.1533822416.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: imeevma@tarantool.org, tarantool-patches@freelists.org Hi! Thanks for the patch! See 2 comments below. On 09/08/2018 16:48, imeevma@tarantool.org wrote: > In some cases operation "create table" left some > created objects not deleted after operation fails. > After this patch all created objects will be deleted. > > Closes #3592. > --- > Branch: imeevma/gh-3592-cleanup-on-failed-create-table 1. Please, put here a full link to the branch. Including https://github etc. > Issue: https://github.com/tarantool/tarantool/issues/3592 > > src/box/sql/vdbe.c | 54 ++++++++++++++++++++++++++++++++++++++++++++ > test/sql/drop-table.result | 37 ++++++++++++++++++++++++++++++ > test/sql/drop-table.test.lua | 15 ++++++++++++ > 3 files changed, 106 insertions(+) > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ca89908..0583587 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -568,6 +568,23 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp) > } > > /* > + * Inserted in current query record. > + */ > +struct inserted_record 2. Have you tried to avoid introducing per-VdbeExec structure for this? I do not like list of such structures since it contradicts with Vdbe concept. For error checking we should generate error processing code instead of saving history somewhere. Now we have two programs actually: List of inserted records to rollback and Vdbe. There should be Vdbe only, I believe. Also, we allocate these records on each SInsert even if it is successful just to maybe rollback it, that is very unlikely case. I propose to generate error processing code as Vdbe opcode. The program have labels on which the context makes goto on error. For this we can add 2 arguments to OP_SInsert: label on which goto on error and label to goto on success and use it like this: -- Table creation. -- 1. op_sinsert; if insert into _space then goto 4; else goto 3; 2. op_sdelete; -- This code called by next opcodes delete from _space; -- on error via goto 2. It is 'destructor' -- of space creation. 3. op_halt; -- Index creation. -- 4. op_sinsert; if insert into _index then goto 7; else goto 2; 5. op_sdelete; -- This code is called by next opcodes delete from _index; -- on error via goto 5. This code destructs -- the index and then fallback to the space -- destruction via goto 6 below. 6. op_goto; goto 2; -- Secondary index creation. -- 7. op_sinsert; if insert into _index then goto 10; else goto 5; 8. op_sdelete -- All the same. Next code does goto 8 delete from _index; -- on error. 9. op_goto; goto 5; ... -- Next code should goto error_label on error. -- i-th index creation N. op_sinsert; if insert into _index then goto N + 3; else goto error_label; N + 1. op_sdelete delete from _index; N + 2. op_goto; goto error_label; ... This may look terrible and as a big patch, but it will not as I understand. It would be relatively easy patch. You can generate op_sdelete by the key you already have for op_sinsert. OP_Goto is already implemented. Goto from within of op_sinsert can be done via something like 'goto jump_to_p2'. > +{ > + /* Reference to previous record */ > + struct inserted_record *prev;> + /* Space in which record was inserted */ > + struct space *space; > + /* Length of packed primary key of record */ > + uint32_t len; > + /* Flag to decrease nChanges if needed */ > + bool is_changes_increased; > + /* Packed primary key of record */ > + char key[0]; > +}; > + > +/* > * Execute as much of a VDBE program as we can. > * This is the core of sqlite3_step(). > */ > @@ -599,6 +616,7 @@ int sqlite3VdbeExec(Vdbe *p) > u64 start; /* CPU clock count at start of opcode */ > #endif > struct session *user_session = current_session(); > + struct inserted_record *inserted_record = NULL; > /*** INSERT STACK UNION HERE ***/ > > assert(p->magic==VDBE_MAGIC_RUN); /* sqlite3_step() verifies this */ > @@ -4352,6 +4370,28 @@ case OP_SInsert: { > goto abort_due_to_error; > if (pOp->p5 & OPFLAG_NCHANGE) > p->nChange++; > + > + uint32_t key_size; > + char *key = tuple_extract_key_raw(pIn2->z, pIn2->z + pIn2->n, > + space->index[0]->def->key_def, > + &key_size); > + if (key == NULL) { > + rc = SQLITE_NOMEM_BKPT; > + goto abort_due_to_error; > + } > + struct inserted_record *tmp = > + (struct inserted_record *)malloc(sizeof(*tmp) + key_size); > + if (tmp == NULL) { > + rc = SQLITE_NOMEM_BKPT; > + goto abort_due_to_error; > + } > + tmp->prev = inserted_record; > + tmp->space = space; > + tmp->len = key_size; > + tmp->is_changes_increased = pOp->p5 & OPFLAG_NCHANGE; > + memcpy(tmp->key, key, key_size); > + > + inserted_record = tmp; > break; > } > > @@ -5344,6 +5384,15 @@ default: { /* This is really OP_Noop and OP_Explain */ > * an error of some kind. > */ > abort_due_to_error: > + while (inserted_record != NULL) { > + sql_delete_by_key(inserted_record->space, inserted_record->key, > + inserted_record->len); > + if (inserted_record->is_changes_increased) > + p->nChange--; > + struct inserted_record *tmp = inserted_record->prev; > + free(inserted_record); > + inserted_record = tmp; > + } > if (db->mallocFailed) rc = SQLITE_NOMEM_BKPT; > assert(rc); > if (p->zErrMsg==0 && rc!=SQLITE_IOERR_NOMEM) { > @@ -5372,6 +5421,11 @@ abort_due_to_error: > > /* This is the only way out of this procedure. */ > vdbe_return: > + while (inserted_record != NULL) { > + struct inserted_record *tmp = inserted_record->prev; > + free(inserted_record); > + inserted_record = tmp; > + } > testcase( nVmStep>0); > p->aCounter[SQLITE_STMTSTATUS_VM_STEP] += (int)nVmStep; > assert(rc!=SQLITE_OK || nExtraDelete==0 > diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result > index 08f2496..f1fdc17 100644 > --- a/test/sql/drop-table.result > +++ b/test/sql/drop-table.result > @@ -33,3 +33,40 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)") > -- DROP TABLE should do the job > -- Debug > -- require("console").start() > +-- gh-3592: net.box segmentation fault after "create table" with > +-- autoincrement > +test_run = require('test_run').new() > +--- > +... > +box.schema.user.create('tmp') > +--- > +... > +box.schema.user.grant('tmp', 'create', 'space') > +--- > +... > +box.schema.user.grant('tmp', 'write', 'space', '_space') > +--- > +... > +box.schema.user.grant('tmp', 'write', 'space', '_schema') > +--- > +... > +box.session.su('tmp') > +--- > +... > +-- Error - nothing should be created > +box.sql.execute('create table t (id integer primary key, a integer)') > +--- > +- error: Write access to space '_index' is denied for user 'tmp' > +... > +box.space.T > +--- > +- null > +... > +box.sql.execute('drop table t') > +--- > +- error: 'no such table: T' > +... > +test_run:cmd('restart server default'); > +box.schema.user.drop('tmp') > +--- > +... > diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua > index 9663074..f059619 100644 > --- a/test/sql/drop-table.test.lua > +++ b/test/sql/drop-table.test.lua > @@ -25,3 +25,18 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)") > > -- Debug > -- require("console").start() > + > +-- gh-3592: net.box segmentation fault after "create table" with > +-- autoincrement > +test_run = require('test_run').new() > +box.schema.user.create('tmp') > +box.schema.user.grant('tmp', 'create', 'space') > +box.schema.user.grant('tmp', 'write', 'space', '_space') > +box.schema.user.grant('tmp', 'write', 'space', '_schema') > +box.session.su('tmp') > +-- Error - nothing should be created > +box.sql.execute('create table t (id integer primary key, a integer)') > +box.space.T > +box.sql.execute('drop table t') > +test_run:cmd('restart server default'); > +box.schema.user.drop('tmp') >