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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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')
>