[tarantool-patches] Re: [PATCH v1 1/1] sql: cleanup on failed table creation

[Vladislav Shpilevoy]

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 at 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')
>>
>