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 28EA228481 for ; Mon, 13 Aug 2018 11:10:58 -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 42-LJdoQbap2 for ; Mon, 13 Aug 2018 11:10:58 -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 B09792837D for ; Mon, 13 Aug 2018 11:10:57 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: cleanup on failed table creation From: Vladislav Shpilevoy References: <7fa7510873713138b01c6568aaa5935ba70a5ba4.1533822416.git.imeevma@gmail.com> Message-ID: <61b1ea80-2e3f-e5ad-5f35-1c1a13cfa0f9@tarantool.org> Date: Mon, 13 Aug 2018 18:10:55 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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 Also such error checking way can be useful to make this https://github.com/tarantool/tarantool/issues/3435 more correct. The way of 'if not exists' checking introduced in 3435 was not applicable for index/table creation exactly because of lack of destructors. On 13/08/2018 17:30, Vladislav Shpilevoy wrote: > 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') >> >