[tarantool-patches] [PATCH v2 1/1] sql: cleanup on failed creation operation

[tarantool-patches] [PATCH v2 1/1] sql: cleanup on failed creation operation

[imeevma]

Some creation operations create objects even on fail. This is
wrong and should be fixed. This patch adds destructors for such
objects.

Closes #3592
---
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3592-cleanup-on-failed-create
Issue: https://github.com/tarantool/tarantool/issues/3592

 src/box/sql/build.c          | 109 ++++++++++++++++++++++++++++++++++++++++++-
 src/box/sql/sqliteInt.h      |   4 ++
 src/box/sql/vdbe.c           |  55 +++++++++++++++++++---
 test/sql/drop-table.result   |  38 +++++++++++++++
 test/sql/drop-table.test.lua |  19 ++++++++
 5 files changed, 218 insertions(+), 7 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a1e16b2..96d0624 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,6 +54,78 @@
 #include "box/schema.h"
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
+#include "fiber.h"
+
+/**
+ * Element of list that contains some information about records
+ * which were inserted into system spaces.
+ */
+struct saved_records
+{
+	/* A link in record list. */
+	struct rlist link;
+	/* Id of space in which record were inserted. */
+	int space_id;
+	/* First register of key of record. */
+	int record_key;
+	/* Length of key of record. */
+	int record_key_len;
+	/* Register with table name.*/
+	int record_name;
+	/* Flags of this element of list. */
+	int flag;
+};
+
+/**
+ * List of all records that were inserted in system spaces in
+ * current statement.
+ */
+static struct rlist record_list;
+/**
+ * Flag to show if there were any saved records in current
+ * statement.
+ */
+static bool is_records_saved = false;
+
+/**
+ * Save inserted in system space record in list saved_records.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param record_key Register that contains first field of key.
+ * @param record_key_len Exact number of fields of key.
+ * @param record_name Register contains name of table which were
+ *        created when record was inserted into box.space._space.
+ * @param flag OPFLAG_NCHANGE or 0.
+ * @retval -1 memory error.
+ * @retval 0 success.
+ */
+static inline void
+save_record(Parse *parser, int space_id, int record_key, int record_key_len,
+	    int record_name, int flag)
+{
+	if (!is_records_saved) {
+		rlist_create(&record_list);
+		is_records_saved = true;
+	}
+	struct saved_records *saved_records =
+		region_alloc(&fiber()->gc, sizeof(*saved_records));
+	if (saved_records == NULL) {
+		diag_set(OutOfMemory, sizeof(*saved_records), "region_alloc",
+			 "saved_records");
+		parser->rc = SQL_TARANTOOL_ERROR;
+		parser->nErr++;
+		return;
+	}
+	saved_records->space_id = space_id;
+	saved_records->record_key = record_key;
+	saved_records->record_key_len = record_key_len;
+	saved_records->record_name = record_name;
+	saved_records->flag = flag | OPFLAG_DESTRUCTOR;
+	if (record_name != 0)
+		saved_records->flag |= OPFLAG_CLEAR_HASH;
+	rlist_add_entry(&record_list, saved_records, link);
+}
 
 void
 sql_finish_coding(struct Parse *parse_context)
@@ -62,6 +134,28 @@ sql_finish_coding(struct Parse *parse_context)
 	struct sqlite3 *db = parse_context->db;
 	struct Vdbe *v = sqlite3GetVdbe(parse_context);
 	sqlite3VdbeAddOp0(v, OP_Halt);
+	/**
+	 * Destructors for all created in current statement
+	 * spaces, indexes, sequences etc.
+	 */
+	if (is_records_saved) {
+		while (rlist_empty(&record_list) == 0) {
+			int record_reg = ++parse_context->nMem;
+			struct saved_records *saved_records =
+				rlist_shift_entry(&record_list,
+						  struct saved_records, link);
+			sqlite3VdbeAddOp3(v, OP_MakeRecord,
+					  saved_records->record_key,
+					  saved_records->record_key_len,
+					  record_reg);
+			sqlite3VdbeAddOp3(v, OP_SDelete,
+					  saved_records->space_id,
+					  record_reg,
+					  saved_records->record_name);
+			sqlite3VdbeChangeP5(v, saved_records->flag);
+		}
+	}
+	sqlite3VdbeAddOp0(v, OP_Error);
 	if (db->mallocFailed || parse_context->nErr != 0) {
 		if (parse_context->rc == SQLITE_OK)
 			parse_context->rc = SQLITE_ERROR;
@@ -1240,8 +1334,13 @@ createIndex(Parse * pParse, Index * pIndex, int iSpaceId, int iIndexId,
 	 * Non-NULL value means that index has been created via
 	 * separate CREATE INDEX statement.
 	 */
-	if (zSql != NULL)
+	if (zSql != NULL) {
 		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+		save_record(pParse, BOX_INDEX_ID, iFirstCol, 2, 0,
+			    OPFLAG_NCHANGE);
+	} else {
+		save_record(pParse, BOX_INDEX_ID, iFirstCol, 2, 0, 0);
+	}
 	return;
 error:
 	pParse->rc = SQL_TARANTOOL_ERROR;
@@ -1344,6 +1443,8 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
 	sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
 	sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
 	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, iFirstCol + 2,
+		    OPFLAG_NCHANGE);
 	return;
 error:
 	pParse->nErr++;
@@ -1550,6 +1651,8 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
 	sqlite3VdbeAddOp2(vdbe, OP_SInsert, BOX_FK_CONSTRAINT_ID,
 			  constr_tuple_reg + 9);
 	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
+	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2, 0,
+		    OPFLAG_NCHANGE);
 	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
 	return;
 error:
@@ -1732,12 +1835,16 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 						 p->def->name);
 		sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID,
 				  reg_seq_record);
+		save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1, 0,
+			    0);
 		/* Do an insertion into _space_sequence. */
 		int reg_space_seq_record =
 			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
 						      reg_seq_id);
 		sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
 				  reg_space_seq_record);
+		save_record(pParse, BOX_SPACE_SEQUENCE_ID,
+			    reg_space_seq_record + 1, 1, 0, 0);
 	}
 
 	/*
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 1d32c9a..fc479b1 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2843,6 +2843,10 @@ struct Parse {
 #define OPFLAG_PERMUTE       0x01	/* OP_Compare: use the permutation */
 #define OPFLAG_SAVEPOSITION  0x02	/* OP_Delete: keep cursor position */
 #define OPFLAG_AUXDELETE     0x04	/* OP_Delete: index in a DELETE op */
+/* OP_SDelete: work as destructor. */
+#define OPFLAG_DESTRUCTOR    0x02
+/* OP_SDelete: remove table from tblHash. */
+#define OPFLAG_CLEAR_HASH    0x04
 
 #define OPFLAG_SAME_FRAME    0x01	/* OP_FCopy: use same frame for source
 					 * register
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index a5b907d..595d7d4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -610,6 +610,17 @@ int sqlite3VdbeExec(Vdbe *p)
 	u64 start;                 /* CPU clock count at start of opcode */
 #endif
 	struct session *user_session = current_session();
+	/*
+	 * Opcode to where we should jump on error in OP_SInsert.
+	 */
+	int op_clean_on_error = p->nOp - 1;
+	/*
+	 * On error in OP_SInsert rc is saved here before
+	 * clean-up. After clean-up this values is retrieved and
+	 * rc set. If error happened during clean-up this value
+	 * neglected.
+	 */
+	int saved_rc = 0;
 	/*** INSERT STACK UNION HERE ***/
 
 	assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies this */
@@ -663,10 +674,14 @@ int sqlite3VdbeExec(Vdbe *p)
 	sqlite3EndBenignMalloc();
 #endif
 	for(pOp=&aOp[p->pc]; 1; pOp++) {
-		/* Errors are detected by individual opcodes, with an immediate
-		 * jumps to abort_due_to_error.
+		/**
+		 * Errors are detected by individual opcodes, with
+		 * an immediate jumps to abort_due_to_error. Only
+		 * exception - opcode OP_SInsert in which on error
+		 * jump should be on jump_to_destructors. In this
+		 * case rc saved in saved_rc.
 		 */
-		assert(rc==SQLITE_OK);
+		assert(rc==SQLITE_OK || saved_rc != 0);
 
 		assert(pOp>=aOp && pOp<&aOp[p->nOp]);
 #ifdef VDBE_PROFILE
@@ -845,6 +860,21 @@ case OP_Gosub: {            /* jump */
 			jump_to_p2:
 	pOp = &aOp[pOp->p2 - 1];
 	break;
+			jump_to_destructors:
+	saved_rc = rc;
+	pOp = &aOp[op_clean_on_error - 1];
+	break;
+}
+
+/* Opcode:  Error * * * * *
+ *
+ * Retrieve saved rc and jump to abort_due_to_error. Should be
+ * located after destructors of created in current statement
+ * spaces, indexes etc.
+ */
+case OP_Error: {
+	rc = saved_rc;
+	goto abort_due_to_error;
 }
 
 /* Opcode:  Return P1 * * * *
@@ -4345,9 +4375,13 @@ case OP_SInsert: {
 	assert(space_is_system(space));
 	rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
 	if (rc)
-		goto abort_due_to_error;
+		goto jump_to_destructors;
 	if (pOp->p5 & OPFLAG_NCHANGE)
 		p->nChange++;
+	/* OP_SDelete - destructors of newly created record. */
+	op_clean_on_error--;
+	/* OP_MakeRecord - make key for destructor. */
+	op_clean_on_error--;
 	break;
 }
 
@@ -4371,8 +4405,17 @@ case OP_SDelete: {
 	rc = sql_delete_by_key(space, 0, pIn2->z, pIn2->n);
 	if (rc)
 		goto abort_due_to_error;
-	if (pOp->p5 & OPFLAG_NCHANGE)
-		p->nChange++;
+	if ((pOp->p5 & OPFLAG_DESTRUCTOR) == 0) {
+		if (pOp->p5 & OPFLAG_NCHANGE)
+			p->nChange++;
+	} else {
+		if ((pOp->p5 & OPFLAG_CLEAR_HASH) != 0) {
+			pIn3 = &aMem[pOp->p3];
+			sqlite3HashInsert(&db->pSchema->tblHash, pIn3->z, 0);
+		}
+		if (pOp->p5 & OPFLAG_NCHANGE)
+			p->nChange--;
+	}
 	break;
 }
 
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..e0349cc 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,41 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 -- DROP TABLE should do the job
 -- Debug
 -- require("console").start()
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+--
+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: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('drop table t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..7a27416 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,22 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
 
 -- Debug
 -- require("console").start()
+
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+--
+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: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+-- Error: no such table.
+box.sql.execute('drop table t1')
+box.session.su('admin')
+box.schema.user.drop('tmp')
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 12 comments, my review
fixes on the branch and at the end of the email.

On 07/09/2018 21:04, imeevma@tarantool.org wrote:
> Some creation operations create objects even on fail. This is
> wrong and should be fixed. This patch adds destructors for such
> objects.
> 
> Closes #3592
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3592-cleanup-on-failed-create
> Issue: https://github.com/tarantool/tarantool/issues/3592
> 
>   src/box/sql/build.c          | 109 ++++++++++++++++++++++++++++++++++++++++++-
>   src/box/sql/sqliteInt.h      |   4 ++
>   src/box/sql/vdbe.c           |  55 +++++++++++++++++++---
>   test/sql/drop-table.result   |  38 +++++++++++++++
>   test/sql/drop-table.test.lua |  19 ++++++++
>   5 files changed, 218 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a1e16b2..96d0624 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -54,6 +54,78 @@
>   #include "box/schema.h"
>   #include "box/tuple_format.h"
>   #include "box/coll_id_cache.h"
> +#include "fiber.h"
> +
> +/**
> + * Element of list that contains some information about records
> + * which were inserted into system spaces.
> + */
> +struct saved_records

1. One struct contains info about one record, it is not? So why
'saved_records' instead of 'saved_record'?

> +{
> +	/* A link in record list. */

2. Double ** for out of function comments.

> +	struct rlist link;
> +	/* Id of space in which record were inserted. */
> +	int space_id;

3. Space id is uint32.

> +	/* First register of key of record. */
> +	int record_key;
> +	/* Length of key of record. */
> +	int record_key_len;

4. What is len? Strlen? Here it is number of registers as
I understand.

> +	/* Register with table name.*/
> +	int record_name;
> +	/* Flags of this element of list. */
> +	int flag;
> +};
> +
> +/**
> + * List of all records that were inserted in system spaces in
> + * current statement.
> + */
> +static struct rlist record_list;

5. Please, do not use any global variables to store query-specific
information. It is a part of parser.

> +/**
> + * Flag to show if there were any saved records in current
> + * statement.
> + */
> +static bool is_records_saved = false;

6. rlist_empty().

> +
> +/**
> + * Save inserted in system space record in list saved_records.
> + *
> + * @param parser SQL Parser object.
> + * @param space_id Id of table in which record is inserted.
> + * @param record_key Register that contains first field of key.
> + * @param record_key_len Exact number of fields of key.
> + * @param record_name Register contains name of table which were
> + *        created when record was inserted into box.space._space.
> + * @param flag OPFLAG_NCHANGE or 0.

7. Why do you need any flags for rollback of a statement? As I
understand, if a DDL statement fails, a user will receive only
error. No meta is supposed to be in a response, so NCHANGE (flag
to increment meta rowcount) is not needed, it is not?

If you bother that this counter will affect next requests - it
will not. The counter is reset on each new statement.

> + * @retval -1 memory error.
> + * @retval 0 success.
> + */
> +static inline void
> +save_record(Parse *parser, int space_id, int record_key, int record_key_len,
> +	    int record_name, int flag)
> +{
> +	if (!is_records_saved) {
> +		rlist_create(&record_list);

8. Just create it always together with parser.

> +		is_records_saved = true;
> +	}
> +	struct saved_records *saved_records =
> +		region_alloc(&fiber()->gc, sizeof(*saved_records));
> +	if (saved_records == NULL) {
> +		diag_set(OutOfMemory, sizeof(*saved_records), "region_alloc",
> +			 "saved_records");
> +		parser->rc = SQL_TARANTOOL_ERROR;
> +		parser->nErr++;
> +		return;
> +	}
> +	saved_records->space_id = space_id;
> +	saved_records->record_key = record_key;
> +	saved_records->record_key_len = record_key_len;
> +	saved_records->record_name = record_name;
> +	saved_records->flag = flag | OPFLAG_DESTRUCTOR;
> +	if (record_name != 0)
> +		saved_records->flag |= OPFLAG_CLEAR_HASH;
> +	rlist_add_entry(&record_list, saved_records, link);
> +}
>   
>   void
>   sql_finish_coding(struct Parse *parse_context)
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 1d32c9a..fc479b1 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2843,6 +2843,10 @@ struct Parse {
>   #define OPFLAG_PERMUTE       0x01	/* OP_Compare: use the permutation */
>   #define OPFLAG_SAVEPOSITION  0x02	/* OP_Delete: keep cursor position */
>   #define OPFLAG_AUXDELETE     0x04	/* OP_Delete: index in a DELETE op */
> +/* OP_SDelete: work as destructor. */
> +#define OPFLAG_DESTRUCTOR    0x02
> +/* OP_SDelete: remove table from tblHash. */

9. Forget about this flag. It is not needed. You can do
whatever you want with rowcount if a result is error anyway.

> +#define OPFLAG_CLEAR_HASH    0x04
>   
>   #define OPFLAG_SAME_FRAME    0x01	/* OP_FCopy: use same frame for source
>   					 * register
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a5b907d..595d7d4 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -610,6 +610,17 @@ int sqlite3VdbeExec(Vdbe *p)
>   	u64 start;                 /* CPU clock count at start of opcode */
>   #endif
>   	struct session *user_session = current_session();
> +	/*
> +	 * Opcode to where we should jump on error in OP_SInsert.
> +	 */
> +	int op_clean_on_error = p->nOp - 1;
> +	/*
> +	 * On error in OP_SInsert rc is saved here before
> +	 * clean-up. After clean-up this values is retrieved and
> +	 * rc set. If error happened during clean-up this value
> +	 * neglected.
> +	 */
> +	int saved_rc = 0;

10. As I remember, we've decided to use jump (like goto jump_to_p2)
from sinsert to start iteration through destructors and OP_Halt to
finish them. Please, do not change or use OP_Gosub and do not
introduce OP_Error. Use OP_Halt only and jumps. The vdbe code should
look like:

     insert into _space, on error jump to p3/p2 = ::error_N::
     insert into _index, on error jump to p3/p2 = ::error_N-1::
     ...
     success op_halt
::error1::
     process error 1 (sdelete, etc)
::error2::
     process error 2
     ...
::errorN::
     op_halt with error

Also I see that you add OP_Error always, even if there is nothing
to delete (in sql_finish_coding) - why?

You can argue that you do not know error opcode numbers before you
have created them to do jumps to p2/p3 but you could generate
dummy jump targets during op_sinsert creation, remember those
opcode numbers in saved_records list and change their p3/p2 during
error section generation when you already know error opcode numbers.

11. You do not need to save rc. It is always SQL_TARANTOOL_ERROR
(if it is not, then we have another bug that should be fixed).

>   	/*** INSERT STACK UNION HERE ***/
>   
>   	assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies this */> diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
> index 08f2496..e0349cc 100644
> --- a/test/sql/drop-table.result
> +++ b/test/sql/drop-table.result
> @@ -33,3 +33,41 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
>   -- DROP TABLE should do the job
>   -- Debug
>   -- require("console").start()
> +--
> +-- gh-3592: segmentation fault when table with error during
> +-- creation is dropped.
> +--
> +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')

12. What is going on here? Why it is so complex? Why the previous
lines in the same file work without a special user and rights?

> +---
> +...
> +--
> +-- Error: user do not have rights to write in box.space._index.
> +-- Space that was already created should be automatically dropped.
> +--
> +box.sql.execute('create table t1 (id int primary key, a int)')
> +---
> +- error: Write access to space '_index' is denied for user 'tmp'
> +...
> +-- Error: no such table.
> +box.sql.execute('drop table t1')
> +---
> +- error: 'no such table: T1'
> +...
> +box.session.su('admin')
> +---
> +...
> +box.schema.user.drop('tmp')
> +---
> +...

=====================================================================

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 96d062440..1db65dcfe 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -62,17 +62,17 @@
   */
  struct saved_records
  {
-	/* A link in record list. */
+	/** A link in a record list. */
  	struct rlist link;
-	/* Id of space in which record were inserted. */
-	int space_id;
-	/* First register of key of record. */
+	/** Id of space in which the record was inserted. */
+	uint32_t space_id;
+	/** First register of key of the record. */
  	int record_key;
-	/* Length of key of record. */
+	/** Length of key of the record. */
  	int record_key_len;
-	/* Register with table name.*/
+	/** Register with table name.*/
  	int record_name;
-	/* Flags of this element of list. */
+	/** Flags of this element of list. */
  	int flag;
  };
  
@@ -101,8 +101,8 @@ static bool is_records_saved = false;
   * @retval 0 success.
   */
  static inline void
-save_record(Parse *parser, int space_id, int record_key, int record_key_len,
-	    int record_name, int flag)
+save_record(struct Parse *parser, uint32_t space_id, int record_key,
+	    int record_key_len, int record_name, int flag)
  {
  	if (!is_records_saved) {
  		rlist_create(&record_list);

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Imeev Mergen]

Hello! Thanks for review!


On 09/18/2018 07:18 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 12 comments, my review
> fixes on the branch and at the end of the email.
>
> On 07/09/2018 21:04, imeevma@tarantool.org wrote:
>> Some creation operations create objects even on fail. This is
>> wrong and should be fixed. This patch adds destructors for such
>> objects.
>>
>> Closes #3592
>> ---
>> Branch: 
>> https://github.com/tarantool/tarantool/tree/imeevma/gh-3592-cleanup-on-failed-create
>> Issue: https://github.com/tarantool/tarantool/issues/3592
>>
>>   src/box/sql/build.c          | 109 
>> ++++++++++++++++++++++++++++++++++++++++++-
>>   src/box/sql/sqliteInt.h      |   4 ++
>>   src/box/sql/vdbe.c           |  55 +++++++++++++++++++---
>>   test/sql/drop-table.result   |  38 +++++++++++++++
>>   test/sql/drop-table.test.lua |  19 ++++++++
>>   5 files changed, 218 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index a1e16b2..96d0624 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -54,6 +54,78 @@
>>   #include "box/schema.h"
>>   #include "box/tuple_format.h"
>>   #include "box/coll_id_cache.h"
>> +#include "fiber.h"
>> +
>> +/**
>> + * Element of list that contains some information about records
>> + * which were inserted into system spaces.
>> + */
>> +struct saved_records
>
> 1. One struct contains info about one record, it is not? So why
> 'saved_records' instead of 'saved_record'?
Fixed.
>
>> +{
>> +    /* A link in record list. */
>
> 2. Double ** for out of function comments.
Squashed.
>
>> +    struct rlist link;
>> +    /* Id of space in which record were inserted. */
>> +    int space_id;
>
> 3. Space id is uint32.
Squashed.
>
>> +    /* First register of key of record. */
>> +    int record_key;
>> +    /* Length of key of record. */
>> +    int record_key_len;
>
> 4. What is len? Strlen? Here it is number of registers as
> I understand.
Fixed.
>
>> +    /* Register with table name.*/
>> +    int record_name;
>> +    /* Flags of this element of list. */
>> +    int flag;
>> +};
>> +
>> +/**
>> + * List of all records that were inserted in system spaces in
>> + * current statement.
>> + */
>> +static struct rlist record_list;
>
> 5. Please, do not use any global variables to store query-specific
> information. It is a part of parser.
Done.
>
>> +/**
>> + * Flag to show if there were any saved records in current
>> + * statement.
>> + */
>> +static bool is_records_saved = false;
>
> 6. rlist_empty().
Done.
>
>> +
>> +/**
>> + * Save inserted in system space record in list saved_records.
>> + *
>> + * @param parser SQL Parser object.
>> + * @param space_id Id of table in which record is inserted.
>> + * @param record_key Register that contains first field of key.
>> + * @param record_key_len Exact number of fields of key.
>> + * @param record_name Register contains name of table which were
>> + *        created when record was inserted into box.space._space.
>> + * @param flag OPFLAG_NCHANGE or 0.
>
> 7. Why do you need any flags for rollback of a statement? As I
> understand, if a DDL statement fails, a user will receive only
> error. No meta is supposed to be in a response, so NCHANGE (flag
> to increment meta rowcount) is not needed, it is not?
>
> If you bother that this counter will affect next requests - it
> will not. The counter is reset on each new statement.
Done.
>
>> + * @retval -1 memory error.
>> + * @retval 0 success.
>> + */
>> +static inline void
>> +save_record(Parse *parser, int space_id, int record_key, int 
>> record_key_len,
>> +        int record_name, int flag)
>> +{
>> +    if (!is_records_saved) {
>> +        rlist_create(&record_list);
>
> 8. Just create it always together with parser.
Done.
>
>> +        is_records_saved = true;
>> +    }
>> +    struct saved_records *saved_records =
>> +        region_alloc(&fiber()->gc, sizeof(*saved_records));
>> +    if (saved_records == NULL) {
>> +        diag_set(OutOfMemory, sizeof(*saved_records), "region_alloc",
>> +             "saved_records");
>> +        parser->rc = SQL_TARANTOOL_ERROR;
>> +        parser->nErr++;
>> +        return;
>> +    }
>> +    saved_records->space_id = space_id;
>> +    saved_records->record_key = record_key;
>> +    saved_records->record_key_len = record_key_len;
>> +    saved_records->record_name = record_name;
>> +    saved_records->flag = flag | OPFLAG_DESTRUCTOR;
>> +    if (record_name != 0)
>> +        saved_records->flag |= OPFLAG_CLEAR_HASH;
>> +    rlist_add_entry(&record_list, saved_records, link);
>> +}
>>     void
>>   sql_finish_coding(struct Parse *parse_context)
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 1d32c9a..fc479b1 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -2843,6 +2843,10 @@ struct Parse {
>>   #define OPFLAG_PERMUTE       0x01    /* OP_Compare: use the 
>> permutation */
>>   #define OPFLAG_SAVEPOSITION  0x02    /* OP_Delete: keep cursor 
>> position */
>>   #define OPFLAG_AUXDELETE     0x04    /* OP_Delete: index in a 
>> DELETE op */
>> +/* OP_SDelete: work as destructor. */
>> +#define OPFLAG_DESTRUCTOR    0x02
>> +/* OP_SDelete: remove table from tblHash. */
>
> 9. Forget about this flag. It is not needed. You can do
> whatever you want with rowcount if a result is error anyway.
Done.
>
>> +#define OPFLAG_CLEAR_HASH    0x04
>>     #define OPFLAG_SAME_FRAME    0x01    /* OP_FCopy: use same frame 
>> for source
>>                        * register
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index a5b907d..595d7d4 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -610,6 +610,17 @@ int sqlite3VdbeExec(Vdbe *p)
>>       u64 start;                 /* CPU clock count at start of 
>> opcode */
>>   #endif
>>       struct session *user_session = current_session();
>> +    /*
>> +     * Opcode to where we should jump on error in OP_SInsert.
>> +     */
>> +    int op_clean_on_error = p->nOp - 1;
>> +    /*
>> +     * On error in OP_SInsert rc is saved here before
>> +     * clean-up. After clean-up this values is retrieved and
>> +     * rc set. If error happened during clean-up this value
>> +     * neglected.
>> +     */
>> +    int saved_rc = 0;
>
> 10. As I remember, we've decided to use jump (like goto jump_to_p2)
> from sinsert to start iteration through destructors and OP_Halt to
> finish them. Please, do not change or use OP_Gosub and do not
> introduce OP_Error. Use OP_Halt only and jumps. The vdbe code should
> look like:
>
>     insert into _space, on error jump to p3/p2 = ::error_N::
>     insert into _index, on error jump to p3/p2 = ::error_N-1::
>     ...
>     success op_halt
> ::error1::
>     process error 1 (sdelete, etc)
> ::error2::
>     process error 2
>     ...
> ::errorN::
>     op_halt with error
>
> Also I see that you add OP_Error always, even if there is nothing
> to delete (in sql_finish_coding) - why?
>
> You can argue that you do not know error opcode numbers before you
> have created them to do jumps to p2/p3 but you could generate
> dummy jump targets during op_sinsert creation, remember those
> opcode numbers in saved_records list and change their p3/p2 during
> error section generation when you already know error opcode numbers.
Done. To make it work this way I added some functionality on
register P3 of OP_Halt operation. I think it would be too complicated
to do it another way.
>
> 11. You do not need to save rc. It is always SQL_TARANTOOL_ERROR
> (if it is not, then we have another bug that should be fixed).
>
>>       /*** INSERT STACK UNION HERE ***/
>>         assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies 
>> this */> diff --git a/test/sql/drop-table.result 
>> b/test/sql/drop-table.result
>> index 08f2496..e0349cc 100644
>> --- a/test/sql/drop-table.result
>> +++ b/test/sql/drop-table.result
>> @@ -33,3 +33,41 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
>> 222, 'c3', 444)")
>>   -- DROP TABLE should do the job
>>   -- Debug
>>   -- require("console").start()
>> +--
>> +-- gh-3592: segmentation fault when table with error during
>> +-- creation is dropped.
>> +--
>> +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')
Fixed.
>
> 12. What is going on here? Why it is so complex? Why the previous
> lines in the same file work without a special user and rights?
>
>> +---
>> +...
>> +--
>> +-- Error: user do not have rights to write in box.space._index.
>> +-- Space that was already created should be automatically dropped.
>> +--
>> +box.sql.execute('create table t1 (id int primary key, a int)')
>> +---
>> +- error: Write access to space '_index' is denied for user 'tmp'
>> +...
>> +-- Error: no such table.
>> +box.sql.execute('drop table t1')
>> +---
>> +- error: 'no such table: T1'
>> +...
>> +box.session.su('admin')
>> +---
>> +...
>> +box.schema.user.drop('tmp')
>> +---
>> +...
Discussed, comment added.
>
> =====================================================================
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 96d062440..1db65dcfe 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -62,17 +62,17 @@
>   */
>  struct saved_records
>  {
> -    /* A link in record list. */
> +    /** A link in a record list. */
>      struct rlist link;
> -    /* Id of space in which record were inserted. */
> -    int space_id;
> -    /* First register of key of record. */
> +    /** Id of space in which the record was inserted. */
> +    uint32_t space_id;
> +    /** First register of key of the record. */
>      int record_key;
> -    /* Length of key of record. */
> +    /** Length of key of the record. */
>      int record_key_len;
> -    /* Register with table name.*/
> +    /** Register with table name.*/
>      int record_name;
> -    /* Flags of this element of list. */
> +    /** Flags of this element of list. */
>      int flag;
>  };
>
> @@ -101,8 +101,8 @@ static bool is_records_saved = false;
>   * @retval 0 success.
>   */
>  static inline void
> -save_record(Parse *parser, int space_id, int record_key, int 
> record_key_len,
> -        int record_name, int flag)
> +save_record(struct Parse *parser, uint32_t space_id, int record_key,
> +        int record_key_len, int record_name, int flag)
>  {
>      if (!is_records_saved) {
>          rlist_create(&record_list);

commit 1f18fa692305f28891f0dbb87e74e863e0c002c3
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Aug 31 15:50:17 2018 +0300

     sql: cleanup on failed creation operation

     Some creation operations create objects even on fail. This is
     wrong and should be fixed. This patch adds destructors for such
     objects.

     Closes #3592

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 60b49df..028a1c5 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,6 +54,54 @@
  #include "box/schema.h"
  #include "box/tuple_format.h"
  #include "box/coll_id_cache.h"
+#include "fiber.h"
+
+/**
+ * Structure that contains some information about record that was
+ * inserted into system space.
+ */
+struct saved_record
+{
+    /** A link in a record list. */
+    struct rlist link;
+    /** Id of space in which the record was inserted. */
+    uint32_t space_id;
+    /** First register of the key of the record. */
+    int reg_key;
+    /** Number of registers the key consists of. */
+    int reg_key_count;
+    /** The number of SInsert operation. */
+    int nOp;
+};
+
+/**
+ * Save inserted in system space record in list.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param reg_key Register that contains first field of the key.
+ * @param reg_key_count Exact number of fields of the key.
+ * @param nOp_after The number of operation that follows SInsert.
+ */
+static inline void
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,
+        int reg_key_count, int nOp_after)
+{
+    struct saved_record *record =
+        region_alloc(&fiber()->gc, sizeof(*record));
+    if (record == NULL) {
+        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
+             "record");
+        parser->rc = SQL_TARANTOOL_ERROR;
+        parser->nErr++;
+        return;
+    }
+    record->space_id = space_id;
+    record->reg_key = reg_key;
+    record->reg_key_count = reg_key_count;
+    record->nOp = nOp_after - 1;
+    rlist_add_entry(&parser->record_list, record, link);
+}

  void
  sql_finish_coding(struct Parse *parse_context)
@@ -62,6 +110,22 @@ sql_finish_coding(struct Parse *parse_context)
      struct sqlite3 *db = parse_context->db;
      struct Vdbe *v = sqlite3GetVdbe(parse_context);
      sqlite3VdbeAddOp0(v, OP_Halt);
+    /**
+     * Destructors for all created in current statement
+     * spaces, indexes, sequences etc.
+     */
+    while (rlist_empty(&parse_context->record_list) == 0) {
+        int record_reg = ++parse_context->nMem;
+        struct saved_record *record =
+            rlist_shift_entry(&parse_context->record_list,
+                      struct saved_record, link);
+        sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,
+                  record->reg_key_count, record_reg);
+        sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, record_reg);
+        /* Set P2 of SInsert. */
+        sqlite3VdbeChangeP2(v, record->nOp, v->nOp);
+    }
+    sqlite3VdbeAddOp3(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 1);
      if (db->mallocFailed || parse_context->nErr != 0) {
          if (parse_context->rc == SQLITE_OK)
              parse_context->rc = SQLITE_ERROR;
@@ -1101,13 +1165,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 != NULL)
          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp);
      return;
  error:
      parse->rc = SQL_TARANTOOL_ERROR;
@@ -1165,8 +1230,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);
      return;
  error:
      pParse->nErr++;
@@ -1340,9 +1406,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);
      sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
      return;
  error:
@@ -1487,14 +1555,18 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
context */
          int reg_seq_record =
              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);
          /* Do an insertion into _space_sequence. */
          int reg_space_seq_record =
              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);
      }
      /* Code creation of FK constraints, if any. */
      struct fkey_parse *fk_parse;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e98e845..a4b65eb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
      memset(parser, 0, sizeof(struct Parse));
      parser->db = db;
      rlist_create(&parser->new_fkey);
+    rlist_create(&parser->record_list);
      region_create(&parser->region, &cord()->slabc);
  }

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f56090d..c5becbd 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2764,6 +2764,11 @@ struct Parse {
       * Foreign key constraint appeared in CREATE TABLE stmt.
       */
      struct rlist new_fkey;
+    /**
+     * List of all records that were inserted in system spaces
+     * in current statement.
+     */
+    struct rlist record_list;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** True, if table to be created has AUTOINCREMENT PK. */
      bool is_new_table_autoinc;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0efc4dd..c90cf1c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -943,7 +943,7 @@ case OP_HaltIfNull: {      /* in3 */
      FALLTHROUGH;
  }

-/* Opcode:  Halt P1 P2 * P4 P5
+/* Opcode:  Halt P1 P2 P3 P4 P5
   *
   * Exit immediately.  All open cursors, etc are closed
   * automatically.
@@ -951,7 +951,9 @@ case OP_HaltIfNull: {      /* in3 */
   * P1 is the result code returned by sqlite3_exec(),
   * sqlite3_reset(), or sqlite3_finalize().  For a normal halt,
   * this should be SQLITE_OK (0).
- * For errors, it can be some other value.  If P1!=0 then P2 will
+ * For errors, it can be some other value.
+ * If P1 == SQL_TARANTOOL_ERROR and P3 != 0 then just go to
+ * abort_due_to_error. If P1!=0 and P3 == 0 then P2 will
   * determine whether or not to rollback the current transaction.
   * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
   * if P2==ON_CONFLICT_ACTION_ROLLBACK.  If
@@ -1005,6 +1007,10 @@ case OP_Halt: {
          pOp = &aOp[pcx];
          break;
      }
+    if (pOp->p1 == SQL_TARANTOOL_ERROR && pOp->p3 != 0) {
+        rc = SQL_TARANTOOL_ERROR;
+        goto abort_due_to_error;
+    }
      p->rc = pOp->p1;
      p->errorAction = (u8)pOp->p2;
      p->pc = pcx;
@@ -4318,8 +4324,8 @@ case OP_IdxInsert: {        /* in2 */
      break;
  }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SInsert P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P3], on error goto P2
   *
   * This opcode is used only during DDL routine.
   * In contrast to ordinary insertion, insertion to system spaces
@@ -4332,15 +4338,15 @@ case OP_IdxInsert: {        /* in2 */
   */
  case OP_SInsert: {
      assert(pOp->p1 > 0);
-    assert(pOp->p2 >= 0);
+    assert(pOp->p2 > 0);
+    assert(pOp->p3 >= 0);

-    pIn2 = &aMem[pOp->p2];
+    pIn3 = &aMem[pOp->p3];
      struct space *space = space_by_id(pOp->p1);
      assert(space != NULL);
      assert(space_is_system(space));
-    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
-    if (rc)
-        goto abort_due_to_error;
+    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
+        goto jump_to_p2;
      if (pOp->p5 & OPFLAG_NCHANGE)
          p->nChange++;
      break;
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..636f3e0 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,43 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- DROP TABLE should do the job
  -- Debug
  -- require("console").start()
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+-- We should grant user enough rights to create space, but not
+-- enough to create index.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create', 'universe')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('drop table t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..ba47716 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")

  -- Debug
  -- require("console").start()
+
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+-- We should grant user enough rights to create space, but not
+-- enough to create index.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create', 'universe')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+-- Error: no such table.
+box.sql.execute('drop table t1')
+box.session.su('admin')
+box.schema.user.drop('tmp')

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Vladislav Shpilevoy]

Hi! See 4 comments below, my review fixes at the end of
the email and on the branch.

> commit 1f18fa692305f28891f0dbb87e74e863e0c002c3
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Aug 31 15:50:17 2018 +0300
> 
>      sql: cleanup on failed creation operation
> 
>      Some creation operations create objects even on fail. This is
>      wrong and should be fixed. This patch adds destructors for such
>      objects.
> 
>      Closes #3592
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 60b49df..028a1c5 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -62,6 +110,22 @@ sql_finish_coding(struct Parse *parse_context)
>       struct sqlite3 *db = parse_context->db;
>       struct Vdbe *v = sqlite3GetVdbe(parse_context);
>       sqlite3VdbeAddOp0(v, OP_Halt);
> +    /**
> +     * Destructors for all created in current statement
> +     * spaces, indexes, sequences etc.
> +     */
> +    while (rlist_empty(&parse_context->record_list) == 0) {

1. Why not just rlist_foreach_entry/rlist_foreach_entry_reverse
instead of while + shift? You do not need to clean this list -
it is on a region anyway.

> +        int record_reg = ++parse_context->nMem;
> +        struct saved_record *record =
> +            rlist_shift_entry(&parse_context->record_list,
> +                      struct saved_record, link);
> +        sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,
> +                  record->reg_key_count, record_reg);
> +        sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, record_reg);
> +        /* Set P2 of SInsert. */
> +        sqlite3VdbeChangeP2(v, record->nOp, v->nOp);
> +    }
> +    sqlite3VdbeAddOp3(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 1);

2. The same as for the previous review: it is not okay to add a new
opcode to Vdbe always without calling attention to does it have
something to rollback or not. Please, add it only when you have
rollback section.

>       if (db->mallocFailed || parse_context->nErr != 0) {
>           if (parse_context->rc == SQLITE_OK)
>               parse_context->rc = SQLITE_ERROR;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 0efc4dd..c90cf1c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1005,6 +1007,10 @@ case OP_Halt: {
>           pOp = &aOp[pcx];
>           break;
>       }
> +    if (pOp->p1 == SQL_TARANTOOL_ERROR && pOp->p3 != 0) {
> +        rc = SQL_TARANTOOL_ERROR;
> +        goto abort_due_to_error;
> +    }

3. You do not need this check here on the hot path and p3
neither. See my review fixes.

>       p->rc = pOp->p1;
>       p->errorAction = (u8)pOp->p2;
>       p->pc = pcx;

4. I tried this test:

     box.sql.execute('CREATE TABLE test1 (id INT PRIMARY KEY)')
     box.sql.execute('EXPLAIN CREATE TABLE test2 (id INT PRIMARY KEY REFERENCES test1, a INT UNIQUE, b INT UNIQUE)')

And this is what I got in result (shortened pseudo vdbe here and
full at the end of the letter):

     insert into _space or goto error1
     insert into _index or goto error2
     insert into _fk_constaints or goto error3
     return ok

::error4::
     drop fk
::error3::
     drop index
::error2::
     drop space
::error1::
     return error

As you can see, the last ::error4:: is never
used, so I guess you can omit generation of last
error processing. And if the saved_records list
has only one record, it is the same as empty one.

It is okay that error4 is not called and should
not exist because there is nothing after fkey
creation that can fail and lead to rollback of
the DDL request.

My review fixes:

===============================================

commit 92c396d9e6fe5397e0acd1f3c45675208c6a04f4
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Sep 20 21:09:48 2018 +0200

     Review fixes

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 028a1c5b1..ebeeb1f20 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -70,8 +70,8 @@ struct saved_record
  	int reg_key;
  	/** Number of registers the key consists of. */
  	int reg_key_count;
-	/** The number of SInsert operation. */
-	int nOp;
+	/** The number of the OP_SInsert operation. */
+	int insertion_opcode;
  };
  
  /**
@@ -81,11 +81,11 @@ struct saved_record
   * @param space_id Id of table in which record is inserted.
   * @param reg_key Register that contains first field of the key.
   * @param reg_key_count Exact number of fields of the key.
- * @param nOp_after The number of operation that follows SInsert.
+ * @param insertion_opcode Number of OP_SInsert opcode.
   */
  static inline void
  save_record(struct Parse *parser, uint32_t space_id, int reg_key,
-	    int reg_key_count, int nOp_after)
+	    int reg_key_count, int insertion_opcode)
  {
  	struct saved_record *record =
  		region_alloc(&fiber()->gc, sizeof(*record));
@@ -99,7 +99,7 @@ save_record(struct Parse *parser, uint32_t space_id, int reg_key,
  	record->space_id = space_id;
  	record->reg_key = reg_key;
  	record->reg_key_count = reg_key_count;
-	record->nOp = nOp_after - 1;
+	record->insertion_opcode = insertion_opcode;
  	rlist_add_entry(&parser->record_list, record, link);
  }
  
@@ -123,9 +123,9 @@ sql_finish_coding(struct Parse *parse_context)
  				  record->reg_key_count, record_reg);
  		sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, record_reg);
  		/* Set P2 of SInsert. */
-		sqlite3VdbeChangeP2(v, record->nOp, v->nOp);
+		sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
  	}
-	sqlite3VdbeAddOp3(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 1);
+	sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
  	if (db->mallocFailed || parse_context->nErr != 0) {
  		if (parse_context->rc == SQLITE_OK)
  			parse_context->rc = SQLITE_ERROR;
@@ -1172,7 +1172,7 @@ vdbe_emit_create_index(struct Parse *parse, struct space_def *def,
  	 */
  	if (idx_def->opts.sql != NULL)
  		sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-	save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp);
+	save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
  	return;
  error:
  	parse->rc = SQL_TARANTOOL_ERROR;
@@ -1232,7 +1232,7 @@ createSpace(Parse * pParse, int iSpaceId, char *zStmt)
  	sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
  	sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);
  	sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
-	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp);
+	save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1);
  	return;
  error:
  	pParse->nErr++;
@@ -1410,7 +1410,7 @@ vdbe_emit_fkey_create(struct Parse *parse_context, const struct fkey_def *fk)
  			  constr_tuple_reg + 9);
  	sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
  	save_record(parse_context, BOX_FK_CONSTRAINT_ID, constr_tuple_reg, 2,
-		    vdbe->nOp);
+		    vdbe->nOp - 1);
  	sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
  	return;
  error:
@@ -1558,7 +1558,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
  		sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
  				  reg_seq_record);
  		save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
-			    v->nOp);
+			    v->nOp - 1);
  		/* Do an insertion into _space_sequence. */
  		int reg_space_seq_record =
  			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
@@ -1566,7 +1566,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
  		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);
+			    reg_space_seq_record + 1, 1, v->nOp - 1);
  	}
  	/* Code creation of FK constraints, if any. */
  	struct fkey_parse *fk_parse;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c90cf1cf4..fc959bdaf 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -943,7 +943,7 @@ case OP_HaltIfNull: {      /* in3 */
  	FALLTHROUGH;
  }
  
-/* Opcode:  Halt P1 P2 P3 P4 P5
+/* Opcode:  Halt P1 P2 * P4 P5
   *
   * Exit immediately.  All open cursors, etc are closed
   * automatically.
@@ -951,9 +951,7 @@ case OP_HaltIfNull: {      /* in3 */
   * P1 is the result code returned by sqlite3_exec(),
   * sqlite3_reset(), or sqlite3_finalize().  For a normal halt,
   * this should be SQLITE_OK (0).
- * For errors, it can be some other value.
- * If P1 == SQL_TARANTOOL_ERROR and P3 != 0 then just go to
- * abort_due_to_error. If P1!=0 and P3 == 0 then P2 will
+ * For errors, it can be some other value.  If P1!=0 then P2 will
   * determine whether or not to rollback the current transaction.
   * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
   * if P2==ON_CONFLICT_ACTION_ROLLBACK.  If
@@ -1007,17 +1005,17 @@ case OP_Halt: {
  		pOp = &aOp[pcx];
  		break;
  	}
-	if (pOp->p1 == SQL_TARANTOOL_ERROR && pOp->p3 != 0) {
-		rc = SQL_TARANTOOL_ERROR;
-		goto abort_due_to_error;
-	}
  	p->rc = pOp->p1;
  	p->errorAction = (u8)pOp->p2;
  	p->pc = pcx;
  	if (p->rc) {
  		if (p->rc == SQL_TARANTOOL_ERROR) {
-			assert(pOp->p4.z != NULL);
-			box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+			if (pOp->p4.z == NULL) {
+				assert(! diag_is_empty(diag_get()));
+			} else {
+				box_error_set(__FILE__, __LINE__, pOp->p5,
+					      pOp->p4.z);
+			}
  		} else if (pOp->p5 != 0) {
  			static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
  							       "FOREIGN KEY" };

Full Vdbe from comment 4.

===============================================

---
- - [0, 'Init', 0, 1, 0, '', '00', 'Start at 1']
   - [1, 'IncMaxid', 1, 0, 0, '', '00', '']
   - [2, 'SCopy', 1, 2, 0, '', '00', 'r[2]=r[1]']
   - [3, 'Integer', 1, 3, 0, '', '00', 'r[3]=1']
   - [4, 'String8', 0, 4, 0, 'TEST2', '00', 'r[4]=''TEST2''']
   - [5, 'String8', 0, 5, 0, 'memtx', '00', 'r[5]=''memtx''']
   - [6, 'Integer', 3, 6, 0, '', '00', 'r[6]=3']
   - [7, 'Blob', 91, 7, 77, !!binary gaNzcWzZVENSRUFURSBUQUJMRSB0ZXN0MiAoaWQgSU5UIFBSSU1BUlkgS0VZIFJFRkVSRU5DRVMgdGVzdDEsIGEgSU5UIFVOSVFVRSwgYiBJTlQgVU5JUVVFKZOFpG5hbWWiSUSkdHlwZadpbnRlZ2VyqGFmZmluaXR5RKtpc19udWxsYWJsZcKvbnVsbGFibGVfYWN0aW9upWFib3J0haRuYW1loUGkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZYWkbmFtZaFCpHR5cGWmc2NhbGFyqGFmZmluaXR5RKtpc19udWxsYWJsZcOvbnVsbGFibGVfYWN0aW9upG5vbmU=,
     '00', !!binary cls3XT2Bo3NxbNlUQ1JFQVRFIFRBQkxFIHRlc3QyIChpZCBJTlQgUFJJTUFSWSBLRVkgUkVGRVJFTkNFUyB0ZXN0MSwgYSBJTlQgVU5JUVVFLCBiIElOVCBVTklRVUUpk4WkbmFtZaJJRKR0eXBlp2ludGVnZXKoYWZmaW5pdHlEq2lzX251bGxhYmxlwq9udWxsYWJsZV9hY3Rpb26lYWJvcnSFpG5hbWWhQaR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lhaRuYW1loUKkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZSAobGVuPTkxLCBzdWJ0eXBlPTc3KQ==]
   - [8, 'Blob', 196, 8, 77, !!binary k4WkbmFtZaJJRKR0eXBlp2ludGVnZXKoYWZmaW5pdHlEq2lzX251bGxhYmxlwq9udWxsYWJsZV9hY3Rpb26lYWJvcnSFpG5hbWWhQaR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lhaRuYW1loUKkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZQ==,
     '00', !!binary cls4XT2ThaRuYW1loklEpHR5cGWnaW50ZWdlcqhhZmZpbml0eUSraXNfbnVsbGFibGXCr251bGxhYmxlX2FjdGlvbqVhYm9ydIWkbmFtZaFBpHR5cGWmc2NhbGFyqGFmZmluaXR5RKtpc19udWxsYWJsZcOvbnVsbGFibGVfYWN0aW9upG5vbmWFpG5hbWWhQqR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lIChsZW49MTk2LCBzdWJ0eXBlPTc3KQ==]
   - [9, 'MakeRecord', 2, 7, 9, '', '00', 'r[9]=mkrec(r[2..8])']
   - [10, 'SInsert', 280, 61, 9, '', '01', 'space id = 280, key = r[9], on error goto
       61']
   - [11, 'SCopy', 1, 10, 0, '', '00', 'r[10]=r[1]']
   - [12, 'Integer', 0, 11, 0, '', '00', 'r[11]=0']
   - [13, 'String8', 0, 12, 0, 'pk_unnamed_TEST2_1', '00', 'r[12]=''pk_unnamed_TEST2_1''']
   - [14, 'String8', 0, 13, 0, 'tree', '00', 'r[13]=''tree''']
   - [15, 'Blob', 14, 14, 77, !!binary gqZ1bmlxdWXDo3NxbKCRhaR0eXBlp2ludGVnZXKlZmllbGQ=,
     '00', !!binary clsxNF09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlp2ludGVnZXKlZmllbGQgKGxlbj0xNCwgc3VidHlwZT03Nyk=]
   - [16, 'Blob', 72, 15, 77, !!binary kYWkdHlwZadpbnRlZ2VypWZpZWxk, '00', !!binary clsxNV09kYWkdHlwZadpbnRlZ2VypWZpZWxkIChsZW49NzIsIHN1YnR5cGU9Nzcp]
   - [17, 'MakeRecord', 10, 6, 16, '', '00', 'r[16]=mkrec(r[10..15])']
   - [18, 'SInsert', 288, 59, 16, '', '00', 'space id = 288, key = r[16], on error
       goto 59']
   - [19, 'SCopy', 1, 17, 0, '', '00', 'r[17]=r[1]']
   - [20, 'Integer', 1, 18, 0, '', '00', 'r[18]=1']
   - [21, 'String8', 0, 19, 0, 'unique_unnamed_TEST2_2', '00', 'r[19]=''unique_unnamed_TEST2_2''']
   - [22, 'String8', 0, 20, 0, 'tree', '00', 'r[20]=''tree''']
   - [23, 'Blob', 14, 21, 77, !!binary gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAGraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNj,
     '00', !!binary clsyMV09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAGraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNjIChsZW49MTQsIHN1YnR5cGU9Nzcp]
   - [24, 'Blob', 70, 22, 77, !!binary kYWkdHlwZaZzY2FsYXKlZmllbGQBq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYw==,
     '00', !!binary clsyMl09kYWkdHlwZaZzY2FsYXKlZmllbGQBq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYyAobGVuPTcwLCBzdWJ0eXBlPTc3KQ==]
   - [25, 'MakeRecord', 17, 6, 23, '', '00', 'r[23]=mkrec(r[17..22])']
   - [26, 'SInsert', 288, 57, 23, '', '00', 'space id = 288, key = r[23], on error
       goto 57']
   - [27, 'SCopy', 1, 24, 0, '', '00', 'r[24]=r[1]']
   - [28, 'Integer', 2, 25, 0, '', '00', 'r[25]=2']
   - [29, 'String8', 0, 26, 0, 'unique_unnamed_TEST2_3', '00', 'r[26]=''unique_unnamed_TEST2_3''']
   - [30, 'String8', 0, 27, 0, 'tree', '00', 'r[27]=''tree''']
   - [31, 'Blob', 14, 28, 77, !!binary gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAKraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNj,
     '00', !!binary clsyOF09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAKraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNjIChsZW49MTQsIHN1YnR5cGU9Nzcp]
   - [32, 'Blob', 70, 29, 77, !!binary kYWkdHlwZaZzY2FsYXKlZmllbGQCq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYw==,
     '00', !!binary clsyOV09kYWkdHlwZaZzY2FsYXKlZmllbGQCq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYyAobGVuPTcwLCBzdWJ0eXBlPTc3KQ==]
   - [33, 'MakeRecord', 24, 6, 30, '', '00', 'r[30]=mkrec(r[24..29])']
   - [34, 'SInsert', 288, 55, 30, '', '00', 'space id = 288, key = r[30], on error
       goto 55']
   - [35, 'String8', 0, 31, 0, 'FK_CONSTRAINT_1_TEST2', '00', 'r[31]=''FK_CONSTRAINT_1_TEST2''']
   - [36, 'SCopy', 1, 32, 0, '', '00', 'r[32]=r[1]']
   - [37, 'Integer', 512, 33, 0, '', '00', 'r[33]=512']
   - [38, 'OpenWrite', 0, 0, 0, 'space<name=_fk_constraint>', '20', 'index id = 0,
       space ptr = space<name=_fk_constraint>']
   - [39, 'NoConflict', 0, 42, 31, '2', '00', 'key=r[31..32]']
   - [40, 'Halt', 21, 0, 0, 'Constraint FK_CONSTRAINT_1_TEST2 already exists', 'aa',
     '']
   - [41, 'Close', 0, 0, 0, '', '00', '']
   - [42, 'Bool', 0, 34, 0, '0', '00', 'r[34]=0']
   - [43, 'String8', 0, 35, 0, 'simple', '00', 'r[35]=''simple''']
   - [44, 'String8', 0, 36, 0, 'no_action', '00', 'r[36]=''no_action''']
   - [45, 'String8', 0, 37, 0, 'no_action', '00', 'r[37]=''no_action''']
   - [46, 'Blob', 2, 38, 77, !!binary kQ==, '00', !!binary clszOF09kSAobGVuPTIsIHN1YnR5cGU9Nzcp]
   - [47, 'Blob', 2, 39, 77, !!binary kQ==, '00', !!binary clszOV09kSAobGVuPTIsIHN1YnR5cGU9Nzcp]
   - [48, 'MakeRecord', 31, 9, 40, '', '00', 'r[40]=mkrec(r[31..39])']
   - [49, 'SInsert', 356, 53, 40, '', '01', 'space id = 356, key = r[40], on error
       goto 53']
   - [50, 'Halt', 0, 0, 0, '', '00', '']
   - [51, 'MakeRecord', 31, 2, 41, '', '00', 'r[41]=mkrec(r[31..32])']
   - [52, 'SDelete', 356, 41, 0, '', '00', 'space id = 356, key = r[41]']
   - [53, 'MakeRecord', 24, 2, 42, '', '00', 'r[42]=mkrec(r[24..25])']
   - [54, 'SDelete', 288, 42, 0, '', '00', 'space id = 288, key = r[42]']
   - [55, 'MakeRecord', 17, 2, 43, '', '00', 'r[43]=mkrec(r[17..18])']
   - [56, 'SDelete', 288, 43, 0, '', '00', 'space id = 288, key = r[43]']
   - [57, 'MakeRecord', 10, 2, 44, '', '00', 'r[44]=mkrec(r[10..11])']
   - [58, 'SDelete', 288, 44, 0, '', '00', 'space id = 288, key = r[44]']
   - [59, 'MakeRecord', 2, 1, 45, '', '00', 'r[45]=mkrec(r[2])']
   - [60, 'SDelete', 280, 45, 0, '', '00', 'space id = 280, key = r[45]']
   - [61, 'Halt', 21, 0, 1, '', '00', '']

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Imeev Mergen]

Hello! Thank you for review!


On 09/20/2018 10:14 PM, Vladislav Shpilevoy wrote:
> Hi! See 4 comments below, my review fixes at the end of
> the email and on the branch.
>
>> commit 1f18fa692305f28891f0dbb87e74e863e0c002c3
>> Author: Mergen Imeev <imeevma@gmail.com>
>> Date:   Fri Aug 31 15:50:17 2018 +0300
>>
>>      sql: cleanup on failed creation operation
>>
>>      Some creation operations create objects even on fail. This is
>>      wrong and should be fixed. This patch adds destructors for such
>>      objects.
>>
>>      Closes #3592
>>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 60b49df..028a1c5 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -62,6 +110,22 @@ sql_finish_coding(struct Parse *parse_context)
>>       struct sqlite3 *db = parse_context->db;
>>       struct Vdbe *v = sqlite3GetVdbe(parse_context);
>>       sqlite3VdbeAddOp0(v, OP_Halt);
>> +    /**
>> +     * Destructors for all created in current statement
>> +     * spaces, indexes, sequences etc.
>> +     */
>> +    while (rlist_empty(&parse_context->record_list) == 0) {
>
> 1. Why not just rlist_foreach_entry/rlist_foreach_entry_reverse
> instead of while + shift? You do not need to clean this list -
> it is on a region anyway.
Fixed.
>
>> +        int record_reg = ++parse_context->nMem;
>> +        struct saved_record *record =
>> + rlist_shift_entry(&parse_context->record_list,
>> +                      struct saved_record, link);
>> +        sqlite3VdbeAddOp3(v, OP_MakeRecord, record->reg_key,
>> +                  record->reg_key_count, record_reg);
>> +        sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, record_reg);
>> +        /* Set P2 of SInsert. */
>> +        sqlite3VdbeChangeP2(v, record->nOp, v->nOp);
>> +    }
>> +    sqlite3VdbeAddOp3(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 1);
>
> 2. The same as for the previous review: it is not okay to add a new
> opcode to Vdbe always without calling attention to does it have
> something to rollback or not. Please, add it only when you have
> rollback section.
Fixed.
>
>>       if (db->mallocFailed || parse_context->nErr != 0) {
>>           if (parse_context->rc == SQLITE_OK)
>>               parse_context->rc = SQLITE_ERROR;
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 0efc4dd..c90cf1c 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -1005,6 +1007,10 @@ case OP_Halt: {
>>           pOp = &aOp[pcx];
>>           break;
>>       }
>> +    if (pOp->p1 == SQL_TARANTOOL_ERROR && pOp->p3 != 0) {
>> +        rc = SQL_TARANTOOL_ERROR;
>> +        goto abort_due_to_error;
>> +    }
>
> 3. You do not need this check here on the hot path and p3
> neither. See my review fixes.
Squashed.
>
>>       p->rc = pOp->p1;
>>       p->errorAction = (u8)pOp->p2;
>>       p->pc = pcx;
>
> 4. I tried this test:
>
>     box.sql.execute('CREATE TABLE test1 (id INT PRIMARY KEY)')
>     box.sql.execute('EXPLAIN CREATE TABLE test2 (id INT PRIMARY KEY 
> REFERENCES test1, a INT UNIQUE, b INT UNIQUE)')
>
> And this is what I got in result (shortened pseudo vdbe here and
> full at the end of the letter):
>
>     insert into _space or goto error1
>     insert into _index or goto error2
>     insert into _fk_constaints or goto error3
>     return ok
>
> ::error4::
>     drop fk
> ::error3::
>     drop index
> ::error2::
>     drop space
> ::error1::
>     return error
>
> As you can see, the last ::error4:: is never
> used, so I guess you can omit generation of last
> error processing. And if the saved_records list
> has only one record, it is the same as empty one.
>
> It is okay that error4 is not called and should
> not exist because there is nothing after fkey
> creation that can fail and lead to rollback of
> the DDL request.
Fixed.
>
> My review fixes:
>
> ===============================================
>
> commit 92c396d9e6fe5397e0acd1f3c45675208c6a04f4
> Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Date:   Thu Sep 20 21:09:48 2018 +0200
>
>     Review fixes
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 028a1c5b1..ebeeb1f20 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -70,8 +70,8 @@ struct saved_record
>      int reg_key;
>      /** Number of registers the key consists of. */
>      int reg_key_count;
> -    /** The number of SInsert operation. */
> -    int nOp;
> +    /** The number of the OP_SInsert operation. */
> +    int insertion_opcode;
>  };
>
>  /**
> @@ -81,11 +81,11 @@ struct saved_record
>   * @param space_id Id of table in which record is inserted.
>   * @param reg_key Register that contains first field of the key.
>   * @param reg_key_count Exact number of fields of the key.
> - * @param nOp_after The number of operation that follows SInsert.
> + * @param insertion_opcode Number of OP_SInsert opcode.
>   */
>  static inline void
>  save_record(struct Parse *parser, uint32_t space_id, int reg_key,
> -        int reg_key_count, int nOp_after)
> +        int reg_key_count, int insertion_opcode)
>  {
>      struct saved_record *record =
>          region_alloc(&fiber()->gc, sizeof(*record));
> @@ -99,7 +99,7 @@ save_record(struct Parse *parser, uint32_t space_id, 
> int reg_key,
>      record->space_id = space_id;
>      record->reg_key = reg_key;
>      record->reg_key_count = reg_key_count;
> -    record->nOp = nOp_after - 1;
> +    record->insertion_opcode = insertion_opcode;
>      rlist_add_entry(&parser->record_list, record, link);
>  }
>
> @@ -123,9 +123,9 @@ sql_finish_coding(struct Parse *parse_context)
>                    record->reg_key_count, record_reg);
>          sqlite3VdbeAddOp2(v, OP_SDelete, record->space_id, record_reg);
>          /* Set P2 of SInsert. */
> -        sqlite3VdbeChangeP2(v, record->nOp, v->nOp);
> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
>      }
> -    sqlite3VdbeAddOp3(v, OP_Halt, SQL_TARANTOOL_ERROR, 0, 1);
> +    sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
>      if (db->mallocFailed || parse_context->nErr != 0) {
>          if (parse_context->rc == SQLITE_OK)
>              parse_context->rc = SQLITE_ERROR;
> @@ -1172,7 +1172,7 @@ vdbe_emit_create_index(struct Parse *parse, 
> struct space_def *def,
>       */
>      if (idx_def->opts.sql != NULL)
>          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> -    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp);
> +    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
>      return;
>  error:
>      parse->rc = SQL_TARANTOOL_ERROR;
> @@ -1232,7 +1232,7 @@ createSpace(Parse * pParse, int iSpaceId, char 
> *zStmt)
>      sqlite3VdbeAddOp3(v, OP_MakeRecord, iFirstCol, 7, iRecord);
>      sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);
>      sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> -    save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp);
> +    save_record(pParse, BOX_SPACE_ID, iFirstCol, 1, v->nOp - 1);
>      return;
>  error:
>      pParse->nErr++;
> @@ -1410,7 +1410,7 @@ vdbe_emit_fkey_create(struct Parse 
> *parse_context, const struct fkey_def *fk)
>                constr_tuple_reg + 9);
>      sqlite3VdbeChangeP5(vdbe, OPFLAG_NCHANGE);
>      save_record(parse_context, BOX_FK_CONSTRAINT_ID, 
> constr_tuple_reg, 2,
> -            vdbe->nOp);
> +            vdbe->nOp - 1);
>      sqlite3ReleaseTempRange(parse_context, constr_tuple_reg, 10);
>      return;
>  error:
> @@ -1558,7 +1558,7 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
> context */
>          sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SEQUENCE_ID, 0,
>                    reg_seq_record);
>          save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
> -                v->nOp);
> +                v->nOp - 1);
>          /* Do an insertion into _space_sequence. */
>          int reg_space_seq_record =
>              emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
> @@ -1566,7 +1566,7 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
> context */
>          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);
> +                reg_space_seq_record + 1, 1, v->nOp - 1);
>      }
>      /* Code creation of FK constraints, if any. */
>      struct fkey_parse *fk_parse;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index c90cf1cf4..fc959bdaf 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -943,7 +943,7 @@ case OP_HaltIfNull: {      /* in3 */
>      FALLTHROUGH;
>  }
>
> -/* Opcode:  Halt P1 P2 P3 P4 P5
> +/* Opcode:  Halt P1 P2 * P4 P5
>   *
>   * Exit immediately.  All open cursors, etc are closed
>   * automatically.
> @@ -951,9 +951,7 @@ case OP_HaltIfNull: {      /* in3 */
>   * P1 is the result code returned by sqlite3_exec(),
>   * sqlite3_reset(), or sqlite3_finalize().  For a normal halt,
>   * this should be SQLITE_OK (0).
> - * For errors, it can be some other value.
> - * If P1 == SQL_TARANTOOL_ERROR and P3 != 0 then just go to
> - * abort_due_to_error. If P1!=0 and P3 == 0 then P2 will
> + * For errors, it can be some other value.  If P1!=0 then P2 will
>   * determine whether or not to rollback the current transaction.
>   * Do not rollback if P2==ON_CONFLICT_ACTION_FAIL. Do the rollback
>   * if P2==ON_CONFLICT_ACTION_ROLLBACK.  If
> @@ -1007,17 +1005,17 @@ case OP_Halt: {
>          pOp = &aOp[pcx];
>          break;
>      }
> -    if (pOp->p1 == SQL_TARANTOOL_ERROR && pOp->p3 != 0) {
> -        rc = SQL_TARANTOOL_ERROR;
> -        goto abort_due_to_error;
> -    }
>      p->rc = pOp->p1;
>      p->errorAction = (u8)pOp->p2;
>      p->pc = pcx;
>      if (p->rc) {
>          if (p->rc == SQL_TARANTOOL_ERROR) {
> -            assert(pOp->p4.z != NULL);
> -            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> +            if (pOp->p4.z == NULL) {
> +                assert(! diag_is_empty(diag_get()));
> +            } else {
> +                box_error_set(__FILE__, __LINE__, pOp->p5,
> +                          pOp->p4.z);
> +            }
>          } else if (pOp->p5 != 0) {
>              static const char * const azType[] = { "NOT NULL", 
> "UNIQUE", "CHECK",
>                                     "FOREIGN KEY" };
>
> Full Vdbe from comment 4.
>
> ===============================================
>
> ---
> - - [0, 'Init', 0, 1, 0, '', '00', 'Start at 1']
>   - [1, 'IncMaxid', 1, 0, 0, '', '00', '']
>   - [2, 'SCopy', 1, 2, 0, '', '00', 'r[2]=r[1]']
>   - [3, 'Integer', 1, 3, 0, '', '00', 'r[3]=1']
>   - [4, 'String8', 0, 4, 0, 'TEST2', '00', 'r[4]=''TEST2''']
>   - [5, 'String8', 0, 5, 0, 'memtx', '00', 'r[5]=''memtx''']
>   - [6, 'Integer', 3, 6, 0, '', '00', 'r[6]=3']
>   - [7, 'Blob', 91, 7, 77, !!binary 
> gaNzcWzZVENSRUFURSBUQUJMRSB0ZXN0MiAoaWQgSU5UIFBSSU1BUlkgS0VZIFJFRkVSRU5DRVMgdGVzdDEsIGEgSU5UIFVOSVFVRSwgYiBJTlQgVU5JUVVFKZOFpG5hbWWiSUSkdHlwZadpbnRlZ2VyqGFmZmluaXR5RKtpc19udWxsYWJsZcKvbnVsbGFibGVfYWN0aW9upWFib3J0haRuYW1loUGkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZYWkbmFtZaFCpHR5cGWmc2NhbGFyqGFmZmluaXR5RKtpc19udWxsYWJsZcOvbnVsbGFibGVfYWN0aW9upG5vbmU=,
>     '00', !!binary 
> cls3XT2Bo3NxbNlUQ1JFQVRFIFRBQkxFIHRlc3QyIChpZCBJTlQgUFJJTUFSWSBLRVkgUkVGRVJFTkNFUyB0ZXN0MSwgYSBJTlQgVU5JUVVFLCBiIElOVCBVTklRVUUpk4WkbmFtZaJJRKR0eXBlp2ludGVnZXKoYWZmaW5pdHlEq2lzX251bGxhYmxlwq9udWxsYWJsZV9hY3Rpb26lYWJvcnSFpG5hbWWhQaR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lhaRuYW1loUKkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZSAobGVuPTkxLCBzdWJ0eXBlPTc3KQ==]
>   - [8, 'Blob', 196, 8, 77, !!binary 
> k4WkbmFtZaJJRKR0eXBlp2ludGVnZXKoYWZmaW5pdHlEq2lzX251bGxhYmxlwq9udWxsYWJsZV9hY3Rpb26lYWJvcnSFpG5hbWWhQaR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lhaRuYW1loUKkdHlwZaZzY2FsYXKoYWZmaW5pdHlEq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZQ==,
>     '00', !!binary 
> cls4XT2ThaRuYW1loklEpHR5cGWnaW50ZWdlcqhhZmZpbml0eUSraXNfbnVsbGFibGXCr251bGxhYmxlX2FjdGlvbqVhYm9ydIWkbmFtZaFBpHR5cGWmc2NhbGFyqGFmZmluaXR5RKtpc19udWxsYWJsZcOvbnVsbGFibGVfYWN0aW9upG5vbmWFpG5hbWWhQqR0eXBlpnNjYWxhcqhhZmZpbml0eUSraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lIChsZW49MTk2LCBzdWJ0eXBlPTc3KQ==]
>   - [9, 'MakeRecord', 2, 7, 9, '', '00', 'r[9]=mkrec(r[2..8])']
>   - [10, 'SInsert', 280, 61, 9, '', '01', 'space id = 280, key = r[9], 
> on error goto
>       61']
>   - [11, 'SCopy', 1, 10, 0, '', '00', 'r[10]=r[1]']
>   - [12, 'Integer', 0, 11, 0, '', '00', 'r[11]=0']
>   - [13, 'String8', 0, 12, 0, 'pk_unnamed_TEST2_1', '00', 
> 'r[12]=''pk_unnamed_TEST2_1''']
>   - [14, 'String8', 0, 13, 0, 'tree', '00', 'r[13]=''tree''']
>   - [15, 'Blob', 14, 14, 77, !!binary 
> gqZ1bmlxdWXDo3NxbKCRhaR0eXBlp2ludGVnZXKlZmllbGQ=,
>     '00', !!binary 
> clsxNF09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlp2ludGVnZXKlZmllbGQgKGxlbj0xNCwgc3VidHlwZT03Nyk=]
>   - [16, 'Blob', 72, 15, 77, !!binary kYWkdHlwZadpbnRlZ2VypWZpZWxk, 
> '00', !!binary 
> clsxNV09kYWkdHlwZadpbnRlZ2VypWZpZWxkIChsZW49NzIsIHN1YnR5cGU9Nzcp]
>   - [17, 'MakeRecord', 10, 6, 16, '', '00', 'r[16]=mkrec(r[10..15])']
>   - [18, 'SInsert', 288, 59, 16, '', '00', 'space id = 288, key = 
> r[16], on error
>       goto 59']
>   - [19, 'SCopy', 1, 17, 0, '', '00', 'r[17]=r[1]']
>   - [20, 'Integer', 1, 18, 0, '', '00', 'r[18]=1']
>   - [21, 'String8', 0, 19, 0, 'unique_unnamed_TEST2_2', '00', 
> 'r[19]=''unique_unnamed_TEST2_2''']
>   - [22, 'String8', 0, 20, 0, 'tree', '00', 'r[20]=''tree''']
>   - [23, 'Blob', 14, 21, 77, !!binary 
> gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAGraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNj,
>     '00', !!binary 
> clsyMV09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAGraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNjIChsZW49MTQsIHN1YnR5cGU9Nzcp]
>   - [24, 'Blob', 70, 22, 77, !!binary 
> kYWkdHlwZaZzY2FsYXKlZmllbGQBq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYw==,
>     '00', !!binary 
> clsyMl09kYWkdHlwZaZzY2FsYXKlZmllbGQBq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYyAobGVuPTcwLCBzdWJ0eXBlPTc3KQ==]
>   - [25, 'MakeRecord', 17, 6, 23, '', '00', 'r[23]=mkrec(r[17..22])']
>   - [26, 'SInsert', 288, 57, 23, '', '00', 'space id = 288, key = 
> r[23], on error
>       goto 57']
>   - [27, 'SCopy', 1, 24, 0, '', '00', 'r[24]=r[1]']
>   - [28, 'Integer', 2, 25, 0, '', '00', 'r[25]=2']
>   - [29, 'String8', 0, 26, 0, 'unique_unnamed_TEST2_3', '00', 
> 'r[26]=''unique_unnamed_TEST2_3''']
>   - [30, 'String8', 0, 27, 0, 'tree', '00', 'r[27]=''tree''']
>   - [31, 'Blob', 14, 28, 77, !!binary 
> gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAKraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNj,
>     '00', !!binary 
> clsyOF09gqZ1bmlxdWXDo3NxbKCRhaR0eXBlpnNjYWxhcqVmaWVsZAKraXNfbnVsbGFibGXDr251bGxhYmxlX2FjdGlvbqRub25lqnNvcnRfb3JkZXKjYXNjIChsZW49MTQsIHN1YnR5cGU9Nzcp]
>   - [32, 'Blob', 70, 29, 77, !!binary 
> kYWkdHlwZaZzY2FsYXKlZmllbGQCq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYw==,
>     '00', !!binary 
> clsyOV09kYWkdHlwZaZzY2FsYXKlZmllbGQCq2lzX251bGxhYmxlw69udWxsYWJsZV9hY3Rpb26kbm9uZapzb3J0X29yZGVyo2FzYyAobGVuPTcwLCBzdWJ0eXBlPTc3KQ==]
>   - [33, 'MakeRecord', 24, 6, 30, '', '00', 'r[30]=mkrec(r[24..29])']
>   - [34, 'SInsert', 288, 55, 30, '', '00', 'space id = 288, key = 
> r[30], on error
>       goto 55']
>   - [35, 'String8', 0, 31, 0, 'FK_CONSTRAINT_1_TEST2', '00', 
> 'r[31]=''FK_CONSTRAINT_1_TEST2''']
>   - [36, 'SCopy', 1, 32, 0, '', '00', 'r[32]=r[1]']
>   - [37, 'Integer', 512, 33, 0, '', '00', 'r[33]=512']
>   - [38, 'OpenWrite', 0, 0, 0, 'space<name=_fk_constraint>', '20', 
> 'index id = 0,
>       space ptr = space<name=_fk_constraint>']
>   - [39, 'NoConflict', 0, 42, 31, '2', '00', 'key=r[31..32]']
>   - [40, 'Halt', 21, 0, 0, 'Constraint FK_CONSTRAINT_1_TEST2 already 
> exists', 'aa',
>     '']
>   - [41, 'Close', 0, 0, 0, '', '00', '']
>   - [42, 'Bool', 0, 34, 0, '0', '00', 'r[34]=0']
>   - [43, 'String8', 0, 35, 0, 'simple', '00', 'r[35]=''simple''']
>   - [44, 'String8', 0, 36, 0, 'no_action', '00', 'r[36]=''no_action''']
>   - [45, 'String8', 0, 37, 0, 'no_action', '00', 'r[37]=''no_action''']
>   - [46, 'Blob', 2, 38, 77, !!binary kQ==, '00', !!binary 
> clszOF09kSAobGVuPTIsIHN1YnR5cGU9Nzcp]
>   - [47, 'Blob', 2, 39, 77, !!binary kQ==, '00', !!binary 
> clszOV09kSAobGVuPTIsIHN1YnR5cGU9Nzcp]
>   - [48, 'MakeRecord', 31, 9, 40, '', '00', 'r[40]=mkrec(r[31..39])']
>   - [49, 'SInsert', 356, 53, 40, '', '01', 'space id = 356, key = 
> r[40], on error
>       goto 53']
>   - [50, 'Halt', 0, 0, 0, '', '00', '']
>   - [51, 'MakeRecord', 31, 2, 41, '', '00', 'r[41]=mkrec(r[31..32])']
>   - [52, 'SDelete', 356, 41, 0, '', '00', 'space id = 356, key = r[41]']
>   - [53, 'MakeRecord', 24, 2, 42, '', '00', 'r[42]=mkrec(r[24..25])']
>   - [54, 'SDelete', 288, 42, 0, '', '00', 'space id = 288, key = r[42]']
>   - [55, 'MakeRecord', 17, 2, 43, '', '00', 'r[43]=mkrec(r[17..18])']
>   - [56, 'SDelete', 288, 43, 0, '', '00', 'space id = 288, key = r[43]']
>   - [57, 'MakeRecord', 10, 2, 44, '', '00', 'r[44]=mkrec(r[10..11])']
>   - [58, 'SDelete', 288, 44, 0, '', '00', 'space id = 288, key = r[44]']
>   - [59, 'MakeRecord', 2, 1, 45, '', '00', 'r[45]=mkrec(r[2])']
>   - [60, 'SDelete', 280, 45, 0, '', '00', 'space id = 280, key = r[45]']
>   - [61, 'Halt', 21, 0, 1, '', '00', '']
>
>
Patch with new fixes:

commit fe8415a79d401b741dcb565d34eb56495223f8b6
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Aug 31 15:50:17 2018 +0300

     sql: cleanup on failed creation operation

     Some creation operations create objects even on fail. This is
     wrong and should be fixed. This patch adds destructors for such
     objects.

     Closes #3592

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 60b49df..2ac86ab 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,6 +54,54 @@
  #include "box/schema.h"
  #include "box/tuple_format.h"
  #include "box/coll_id_cache.h"
+#include "fiber.h"
+
+/**
+ * Structure that contains some information about record that was
+ * inserted into system space.
+ */
+struct saved_record
+{
+    /** A link in a record list. */
+    struct rlist link;
+    /** Id of space in which the record was inserted. */
+    uint32_t space_id;
+    /** First register of the key of the record. */
+    int reg_key;
+    /** Number of registers the key consists of. */
+    int reg_key_count;
+    /** The number of the OP_SInsert operation. */
+    int insertion_opcode;
+};
+
+/**
+ * Save inserted in system space record in list.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param reg_key Register that contains first field of the key.
+ * @param reg_key_count Exact number of fields of the key.
+ * @param insertion_opcode Number of OP_SInsert opcode.
+ */
+static inline void
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,
+        int reg_key_count, int insertion_opcode)
+{
+    struct saved_record *record =
+        region_alloc(&fiber()->gc, sizeof(*record));
+    if (record == NULL) {
+        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
+             "record");
+        parser->rc = SQL_TARANTOOL_ERROR;
+        parser->nErr++;
+        return;
+    }
+    record->space_id = space_id;
+    record->reg_key = reg_key;
+    record->reg_key_count = reg_key_count;
+    record->insertion_opcode = insertion_opcode;
+    rlist_add_entry(&parser->record_list, record, link);
+}

  void
  sql_finish_coding(struct Parse *parse_context)
@@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
      struct sqlite3 *db = parse_context->db;
      struct Vdbe *v = 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) == 0) {
+        struct saved_record *record =
+            rlist_shift_entry(&parse_context->record_list,
+                      struct saved_record, link);
+        /* Set P2 of SInsert. */
+        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
+        rlist_foreach_entry(record, &parse_context->record_list, link) {
+            int record_reg = ++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);
+            /* Set P2 of SInsert. */
+            sqlite3VdbeChangeP2(v, record->insertion_opcode,
+                        v->nOp);
+        }
+        sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
+    }
      if (db->mallocFailed || parse_context->nErr != 0) {
          if (parse_context->rc == SQLITE_OK)
              parse_context->rc = SQLITE_ERROR;
@@ -1101,13 +1172,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 != NULL)
          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
      return;
  error:
      parse->rc = SQL_TARANTOOL_ERROR;
@@ -1165,8 +1237,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 +1413,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 +1562,18 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
context */
          int reg_seq_record =
              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 =
              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);
      }
      /* Code creation of FK constraints, if any. */
      struct fkey_parse *fk_parse;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e98e845..a4b65eb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
      memset(parser, 0, sizeof(struct Parse));
      parser->db = db;
      rlist_create(&parser->new_fkey);
+    rlist_create(&parser->record_list);
      region_create(&parser->region, &cord()->slabc);
  }

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f56090d..c5becbd 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2764,6 +2764,11 @@ struct Parse {
       * Foreign key constraint appeared in CREATE TABLE stmt.
       */
      struct rlist new_fkey;
+    /**
+     * List of all records that were inserted in system spaces
+     * in current statement.
+     */
+    struct rlist record_list;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** True, if table to be created has AUTOINCREMENT PK. */
      bool is_new_table_autoinc;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 0efc4dd..fc959bd 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1010,8 +1010,12 @@ case OP_Halt: {
      p->pc = pcx;
      if (p->rc) {
          if (p->rc == SQL_TARANTOOL_ERROR) {
-            assert(pOp->p4.z != NULL);
-            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+            if (pOp->p4.z == NULL) {
+                assert(! diag_is_empty(diag_get()));
+            } else {
+                box_error_set(__FILE__, __LINE__, pOp->p5,
+                          pOp->p4.z);
+            }
          } else if (pOp->p5 != 0) {
              static const char * const azType[] = { "NOT NULL", 
"UNIQUE", "CHECK",
                                     "FOREIGN KEY" };
@@ -4318,8 +4322,8 @@ case OP_IdxInsert: {        /* in2 */
      break;
  }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SInsert P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P3], on error goto P2
   *
   * This opcode is used only during DDL routine.
   * In contrast to ordinary insertion, insertion to system spaces
@@ -4332,15 +4336,15 @@ case OP_IdxInsert: {        /* in2 */
   */
  case OP_SInsert: {
      assert(pOp->p1 > 0);
-    assert(pOp->p2 >= 0);
+    assert(pOp->p2 > 0);
+    assert(pOp->p3 >= 0);

-    pIn2 = &aMem[pOp->p2];
+    pIn3 = &aMem[pOp->p3];
      struct space *space = space_by_id(pOp->p1);
      assert(space != NULL);
      assert(space_is_system(space));
-    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
-    if (rc)
-        goto abort_due_to_error;
+    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
+        goto jump_to_p2;
      if (pOp->p5 & OPFLAG_NCHANGE)
          p->nChange++;
      break;
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..636f3e0 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,43 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- DROP TABLE should do the job
  -- Debug
  -- require("console").start()
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+-- We should grant user enough rights to create space, but not
+-- enough to create index.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create', 'universe')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('drop table t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..ba47716 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")

  -- Debug
  -- require("console").start()
+
+--
+-- gh-3592: segmentation fault when table with error during
+-- creation is dropped.
+-- We should grant user enough rights to create space, but not
+-- enough to create index.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create', 'universe')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+-- Error: no such table.
+box.sql.execute('drop table t1')
+box.session.su('admin')
+box.schema.user.drop('tmp')

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! LGTM. Nikita,
please, review it.

> Patch with new fixes:
> 
> commit fe8415a79d401b741dcb565d34eb56495223f8b6
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Aug 31 15:50:17 2018 +0300
> 
>      sql: cleanup on failed creation operation
> 
>      Some creation operations create objects even on fail. This is
>      wrong and should be fixed. This patch adds destructors for such
>      objects.
> 
>      Closes #3592
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 60b49df..2ac86ab 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -54,6 +54,54 @@
>   #include "box/schema.h"
>   #include "box/tuple_format.h"
>   #include "box/coll_id_cache.h"
> +#include "fiber.h"
> +
> +/**
> + * Structure that contains some information about record that was
> + * inserted into system space.
> + */
> +struct saved_record
> +{
> +    /** A link in a record list. */
> +    struct rlist link;
> +    /** Id of space in which the record was inserted. */
> +    uint32_t space_id;
> +    /** First register of the key of the record. */
> +    int reg_key;
> +    /** Number of registers the key consists of. */
> +    int reg_key_count;
> +    /** The number of the OP_SInsert operation. */
> +    int insertion_opcode;
> +};
> +
> +/**
> + * Save inserted in system space record in list.
> + *
> + * @param parser SQL Parser object.
> + * @param space_id Id of table in which record is inserted.
> + * @param reg_key Register that contains first field of the key.
> + * @param reg_key_count Exact number of fields of the key.
> + * @param insertion_opcode Number of OP_SInsert opcode.
> + */
> +static inline void
> +save_record(struct Parse *parser, uint32_t space_id, int reg_key,
> +        int reg_key_count, int insertion_opcode)
> +{
> +    struct saved_record *record =
> +        region_alloc(&fiber()->gc, sizeof(*record));
> +    if (record == NULL) {
> +        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
> +             "record");
> +        parser->rc = SQL_TARANTOOL_ERROR;
> +        parser->nErr++;
> +        return;
> +    }
> +    record->space_id = space_id;
> +    record->reg_key = reg_key;
> +    record->reg_key_count = reg_key_count;
> +    record->insertion_opcode = insertion_opcode;
> +    rlist_add_entry(&parser->record_list, record, link);
> +}
> 
>   void
>   sql_finish_coding(struct Parse *parse_context)
> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>       struct sqlite3 *db = parse_context->db;
>       struct Vdbe *v = 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) == 0) {
> +        struct saved_record *record =
> +            rlist_shift_entry(&parse_context->record_list,
> +                      struct saved_record, link);
> +        /* Set P2 of SInsert. */
> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
> +        rlist_foreach_entry(record, &parse_context->record_list, link) {
> +            int record_reg = ++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);
> +            /* Set P2 of SInsert. */
> +            sqlite3VdbeChangeP2(v, record->insertion_opcode,
> +                        v->nOp);
> +        }
> +        sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
> +    }
>       if (db->mallocFailed || parse_context->nErr != 0) {
>           if (parse_context->rc == SQLITE_OK)
>               parse_context->rc = SQLITE_ERROR;
> @@ -1101,13 +1172,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 != NULL)
>           sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> +    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
>       return;
>   error:
>       parse->rc = SQL_TARANTOOL_ERROR;
> @@ -1165,8 +1237,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 +1413,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 +1562,18 @@ sqlite3EndTable(Parse * pParse,    /* Parse context */
>           int reg_seq_record =
>               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 =
>               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);
>       }
>       /* Code creation of FK constraints, if any. */
>       struct fkey_parse *fk_parse;
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e98e845..a4b65eb 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
>       memset(parser, 0, sizeof(struct Parse));
>       parser->db = db;
>       rlist_create(&parser->new_fkey);
> +    rlist_create(&parser->record_list);
>       region_create(&parser->region, &cord()->slabc);
>   }
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index f56090d..c5becbd 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2764,6 +2764,11 @@ struct Parse {
>        * Foreign key constraint appeared in CREATE TABLE stmt.
>        */
>       struct rlist new_fkey;
> +    /**
> +     * List of all records that were inserted in system spaces
> +     * in current statement.
> +     */
> +    struct rlist record_list;
>       bool initiateTTrans;    /* Initiate Tarantool transaction */
>       /** True, if table to be created has AUTOINCREMENT PK. */
>       bool is_new_table_autoinc;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 0efc4dd..fc959bd 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1010,8 +1010,12 @@ case OP_Halt: {
>       p->pc = pcx;
>       if (p->rc) {
>           if (p->rc == SQL_TARANTOOL_ERROR) {
> -            assert(pOp->p4.z != NULL);
> -            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> +            if (pOp->p4.z == NULL) {
> +                assert(! diag_is_empty(diag_get()));
> +            } else {
> +                box_error_set(__FILE__, __LINE__, pOp->p5,
> +                          pOp->p4.z);
> +            }
>           } else if (pOp->p5 != 0) {
>               static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
>                                      "FOREIGN KEY" };
> @@ -4318,8 +4322,8 @@ case OP_IdxInsert: {        /* in2 */
>       break;
>   }
> 
> -/* Opcode: SInsert P1 P2 * * P5
> - * Synopsis: space id = P1, key = r[P2]
> +/* Opcode: SInsert P1 P2 P3 * P5
> + * Synopsis: space id = P1, key = r[P3], on error goto P2
>    *
>    * This opcode is used only during DDL routine.
>    * In contrast to ordinary insertion, insertion to system spaces
> @@ -4332,15 +4336,15 @@ case OP_IdxInsert: {        /* in2 */
>    */
>   case OP_SInsert: {
>       assert(pOp->p1 > 0);
> -    assert(pOp->p2 >= 0);
> +    assert(pOp->p2 > 0);
> +    assert(pOp->p3 >= 0);
> 
> -    pIn2 = &aMem[pOp->p2];
> +    pIn3 = &aMem[pOp->p3];
>       struct space *space = space_by_id(pOp->p1);
>       assert(space != NULL);
>       assert(space_is_system(space));
> -    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
> -    if (rc)
> -        goto abort_due_to_error;
> +    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
> +        goto jump_to_p2;
>       if (pOp->p5 & OPFLAG_NCHANGE)
>           p->nChange++;
>       break;
> diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
> index 08f2496..636f3e0 100644
> --- a/test/sql/drop-table.result
> +++ b/test/sql/drop-table.result
> @@ -33,3 +33,43 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
>   -- DROP TABLE should do the job
>   -- Debug
>   -- require("console").start()
> +--
> +-- gh-3592: segmentation fault when table with error during
> +-- creation is dropped.
> +-- We should grant user enough rights to create space, but not
> +-- enough to create index.
> +--
> +box.schema.user.create('tmp')
> +---
> +...
> +box.schema.user.grant('tmp', 'create', 'universe')
> +---
> +...
> +box.schema.user.grant('tmp', 'write', 'space', '_space')
> +---
> +...
> +box.schema.user.grant('tmp', 'write', 'space', '_schema')
> +---
> +...
> +box.session.su('tmp')
> +---
> +...
> +--
> +-- Error: user do not have rights to write in box.space._index.
> +-- Space that was already created should be automatically dropped.
> +--
> +box.sql.execute('create table t1 (id int primary key, a int)')
> +---
> +- error: Write access to space '_index' is denied for user 'tmp'
> +...
> +-- Error: no such table.
> +box.sql.execute('drop table t1')
> +---
> +- error: 'no such table: T1'
> +...
> +box.session.su('admin')
> +---
> +...
> +box.schema.user.drop('tmp')
> +---
> +...
> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
> index 9663074..ba47716 100644
> --- a/test/sql/drop-table.test.lua
> +++ b/test/sql/drop-table.test.lua
> @@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
> 
>   -- Debug
>   -- require("console").start()
> +
> +--
> +-- gh-3592: segmentation fault when table with error during
> +-- creation is dropped.
> +-- We should grant user enough rights to create space, but not
> +-- enough to create index.
> +--
> +box.schema.user.create('tmp')
> +box.schema.user.grant('tmp', 'create', 'universe')
> +box.schema.user.grant('tmp', 'write', 'space', '_space')
> +box.schema.user.grant('tmp', 'write', 'space', '_schema')
> +box.session.su('tmp')
> +--
> +-- Error: user do not have rights to write in box.space._index.
> +-- Space that was already created should be automatically dropped.
> +--
> +box.sql.execute('create table t1 (id int primary key, a int)')
> +-- Error: no such table.
> +box.sql.execute('drop table t1')
> +box.session.su('admin')
> +box.schema.user.drop('tmp')
> 

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 7785 bytes --]


> commit fe8415a79d401b741dcb565d34eb56495223f8b6
> Author: Mergen Imeev <imeevma@gmail.com <mailto:imeevma@gmail.com>>
> Date:   Fri Aug 31 15:50:17 2018 +0300
> 
>     sql: cleanup on failed creation operation
> 
>     Some creation operations create objects even on fail. This is
>     wrong and should be fixed. This patch adds destructors for such
>     objects.

Kind of poor explanation. Where those destructors are used?
Which objects are you talking about? AFAIU ‘some creation operations’
may refer only to table creation; in other cases only one object created
per SQL statement: CREATE INDEX -> one index, CREATE TRIGGER ->
one trigger, ADD CONSTRAINT -> one constraint.
For this reason, you don’t need to add to list of records indexes and fk
constraints when it comes to separate CREATE INDEX/ADD CONSTRAINT
statements. Extend your explanation and provide examples, please.

> 
>     Closes #3592
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 60b49df..2ac86ab 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> +/**
> + * Save inserted in system space record in list.
> + *
> + * @param parser SQL Parser object.
> + * @param space_id Id of table in which record is inserted.
> + * @param reg_key Register that contains first field of the key.
> + * @param reg_key_count Exact number of fields of the key.
> + * @param insertion_opcode Number of OP_SInsert opcode.
> + */
> +static inline void
> +save_record(struct Parse *parser, uint32_t space_id, int reg_key,
> +        int reg_key_count, int insertion_opcode)
> +{
> +    struct saved_record *record =
> +        region_alloc(&fiber()->gc, sizeof(*record));

Why do you use global region? You can use parser’s one.

>  void
>  sql_finish_coding(struct Parse *parse_context)
> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>      struct sqlite3 *db = parse_context->db;
>      struct Vdbe *v = 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) == 0) {

Nit: if(! rlist_empty()) 

Also, lets add comments directly to VDBE program indicating labels
for each clean-up jumps (VdbeComment). It would significantly
simplify debugging of VDBE programs.

> +        struct saved_record *record =
> +            rlist_shift_entry(&parse_context->record_list,
> +                      struct saved_record, link);
> +        /* Set P2 of SInsert. */
> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
> +        rlist_foreach_entry(record, &parse_context->record_list, link) {
> +            int record_reg = ++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);
> +            /* Set P2 of SInsert. */
> +            sqlite3VdbeChangeP2(v, record->insertion_opcode,
> +                        v->nOp);
> +        }
> +        sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
> +    }
>      /* Code creation of FK constraints, if any. */
>      struct fkey_parse *fk_parse;
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index e98e845..a4b65eb 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
>      memset(parser, 0, sizeof(struct Parse));
>      parser->db = db;
>      rlist_create(&parser->new_fkey);
> +    rlist_create(&parser->record_list);
>      region_create(&parser->region, &cord()->slabc);
>  }
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index f56090d..c5becbd 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2764,6 +2764,11 @@ struct Parse {
>       * Foreign key constraint appeared in CREATE TABLE stmt.
>       */
>      struct rlist new_fkey;
> +    /**
> +     * List of all records that were inserted in system spaces
> +     * in current statement.
> +     */
> +    struct rlist record_list;
>      bool initiateTTrans;    /* Initiate Tarantool transaction */
>      /** True, if table to be created has AUTOINCREMENT PK. */
>      bool is_new_table_autoinc;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 0efc4dd..fc959bd 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1010,8 +1010,12 @@ case OP_Halt: {
>      p->pc = pcx;
>      if (p->rc) {
>          if (p->rc == SQL_TARANTOOL_ERROR) {
> -            assert(pOp->p4.z != NULL);
> -            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
> +            if (pOp->p4.z == NULL) {
> +                assert(! diag_is_empty(diag_get()));
> +            } else {
> +                box_error_set(__FILE__, __LINE__, pOp->p5,
> +                          pOp->p4.z);
> +            }
>          } else if (pOp->p5 != 0) {
>              static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
>                                     "FOREIGN KEY" };
> @@ -4318,8 +4322,8 @@ case OP_IdxInsert: {        /* in2 */
>      break;
>  }
> 
> -/* Opcode: SInsert P1 P2 * * P5
> - * Synopsis: space id = P1, key = r[P2]
> +/* Opcode: SInsert P1 P2 P3 * P5
> + * Synopsis: space id = P1, key = r[P3], on error goto P2

It would be better to swap P2 and P3: if you did so, you wouldn’t need to
explicitly set 0 as second arg since all unspecified args are nullified by default.
For instance:

sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);

sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);

On the other hand, I see that many opcodes use exactly p2 operand to
process jump and label jump_to_p2. So, if you don’t want to break this
convention, you may leave it as is.

> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
> index 9663074..ba47716 100644
> --- a/test/sql/drop-table.test.lua
> +++ b/test/sql/drop-table.test.lua
> @@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 222, 'c3', 444)")
> 
>  -- Debug
>  -- require("console").start()
> +
> +--
> +-- gh-3592: segmentation fault when table with error during
> +-- creation is dropped.
> +-- We should grant user enough rights to create space, but not
> +-- enough to create index.
> +--
> +box.schema.user.create('tmp')
> +box.schema.user.grant('tmp', 'create', 'universe')
> +box.schema.user.grant('tmp', 'write', 'space', '_space')
> +box.schema.user.grant('tmp', 'write', 'space', '_schema')
> +box.session.su('tmp')
> +--
> +-- Error: user do not have rights to write in box.space._index.
> +-- Space that was already created should be automatically dropped.
> +--
> +box.sql.execute('create table t1 (id int primary key, a int)')
> +-- Error: no such table.
> +box.sql.execute('drop table t1')
> +box.session.su('admin')
> +box.schema.user.drop('tmp’)

Em, and where are tests for current feature? This test checks only one
particular case. AFAIU #3592 is only result of absence of cleanup on
table creation (original issue should be https://github.com/tarantool/tarantool/issues/3499
but it is marked as ‘doc’. Now your patch changes this behaviour again).
Anyway, add test cases when CREATE TABLE fails on VDBE execution,
e.g. half of unique indexes or referencing constraints are created, but
smth goes wrong and remains of execution are eliminated correctly.


[-- Attachment #2: Type: text/html, Size: 99371 bytes --]

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 24032 bytes --]

Hello! Thank you for review! New patch below.


On 10/01/2018 04:05 PM, n.pettik wrote:
>
>> commit fe8415a79d401b741dcb565d34eb56495223f8b6
>> Author: Mergen Imeev <imeevma@gmail.com <mailto:imeevma@gmail.com>>
>> Date:   Fri Aug 31 15:50:17 2018 +0300
>>
>>     sql: cleanup on failed creation operation
>>
>>     Some creation operations create objects even on fail. This is
>>     wrong and should be fixed. This patch adds destructors for such
>>     objects.
>
> Kind of poor explanation. Where those destructors are used?
> Which objects are you talking about? AFAIU ‘some creation operations’
> may refer only to table creation; in other cases only one object created
> per SQL statement: CREATE INDEX -> one index, CREATE TRIGGER ->
> one trigger, ADD CONSTRAINT -> one constraint.
> For this reason, you don’t need to add to list of records indexes and fk
> constraints when it comes to separate CREATE INDEX/ADD CONSTRAINT
> statements. Extend your explanation and provide examples, please.
Fixed.
>
>>
>>     Closes #3592
>>
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 60b49df..2ac86ab 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> +/**
>> + * Save inserted in system space record in list.
>> + *
>> + * @param parser SQL Parser object.
>> + * @param space_id Id of table in which record is inserted.
>> + * @param reg_key Register that contains first field of the key.
>> + * @param reg_key_count Exact number of fields of the key.
>> + * @param insertion_opcode Number of OP_SInsert opcode.
>> + */
>> +static inline void
>> +save_record(struct Parse *parser, uint32_t space_id, int reg_key,
>> +        int reg_key_count, int insertion_opcode)
>> +{
>> +    struct saved_record *record =
>> + region_alloc(&fiber()->gc, sizeof(*record));
>
> Why do you use global region? You can use parser’s one.
Fixed.
>
>>  void
>>  sql_finish_coding(struct Parse *parse_context)
>> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>>      struct sqlite3 *db = parse_context->db;
>>      struct Vdbe *v = 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) == 0) {
>
> Nit: if(! rlist_empty())
Fixed.
>
> Also, lets add comments directly to VDBE program indicating labels
> for each clean-up jumps (VdbeComment). It would significantly
> simplify debugging of VDBE programs.
Done.
>
>> +        struct saved_record *record =
>> + rlist_shift_entry(&parse_context->record_list,
>> +                      struct saved_record, link);
>> +        /* Set P2 of SInsert. */
>> + sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
>> + rlist_foreach_entry(record, &parse_context->record_list, link) {
>> +            int record_reg = ++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);
>> +            /* Set P2 of SInsert. */
>> + sqlite3VdbeChangeP2(v, record->insertion_opcode,
>> + v->nOp);
>> +        }
>> +        sqlite3VdbeAddOp2(v, OP_Halt, SQL_TARANTOOL_ERROR, 0);
>> +    }
>>      /* Code creation of FK constraints, if any. */
>>      struct fkey_parse *fk_parse;
>> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
>> index e98e845..a4b65eb 100644
>> --- a/src/box/sql/prepare.c
>> +++ b/src/box/sql/prepare.c
>> @@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
>>      memset(parser, 0, sizeof(struct Parse));
>>      parser->db = db;
>> rlist_create(&parser->new_fkey);
>> + rlist_create(&parser->record_list);
>> region_create(&parser->region, &cord()->slabc);
>>  }
>>
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index f56090d..c5becbd 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -2764,6 +2764,11 @@ struct Parse {
>>       * Foreign key constraint appeared in CREATE TABLE stmt.
>>       */
>>      struct rlist new_fkey;
>> +    /**
>> +     * List of all records that were inserted in system spaces
>> +     * in current statement.
>> +     */
>> +    struct rlist record_list;
>>      bool initiateTTrans; /* Initiate Tarantool transaction */
>>      /** True, if table to be created has AUTOINCREMENT PK. */
>>      bool is_new_table_autoinc;
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 0efc4dd..fc959bd 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -1010,8 +1010,12 @@ case OP_Halt: {
>>      p->pc = pcx;
>>      if (p->rc) {
>>          if (p->rc == SQL_TARANTOOL_ERROR) {
>> - assert(pOp->p4.z != NULL);
>> - box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
>> +            if (pOp->p4.z == NULL) {
>> +                assert(! diag_is_empty(diag_get()));
>> +            } else {
>> + box_error_set(__FILE__, __LINE__, pOp->p5,
>> + pOp->p4.z);
>> +            }
>>          } else if (pOp->p5 != 0) {
>>              static const char * const azType[] = { "NOT NULL", 
>> "UNIQUE", "CHECK",
>>        "FOREIGN KEY" };
>> @@ -4318,8 +4322,8 @@ case OP_IdxInsert: {        /* in2 */
>>      break;
>>  }
>>
>> -/* Opcode: SInsert P1 P2 * * P5
>> - * Synopsis: space id = P1, key = r[P2]
>> +/* Opcode: SInsert P1 P2 P3 * P5
>> + * Synopsis: space id = P1, key = r[P3], on error goto P2
>
> It would be better to swap P2 and P3: if you did so, you wouldn’t need to
> explicitly set 0 as second arg since all unspecified args are 
> nullified by default.
> For instance:
>
> sqlite3VdbeAddOp3(v, OP_SInsert, BOX_SPACE_ID, 0, iRecord);
>
> sqlite3VdbeAddOp2(v, OP_SInsert, BOX_SPACE_ID, iRecord);
>
> On the other hand, I see that many opcodes use exactly p2 operand to
> process jump and label jump_to_p2. So, if you don’t want to break this
> convention, you may leave it as is.
I left as it was before review. I think it would be better to don't break
mentioned convention.
>
>> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
>> index 9663074..ba47716 100644
>> --- a/test/sql/drop-table.test.lua
>> +++ b/test/sql/drop-table.test.lua
>> @@ -25,3 +25,24 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
>> 222, 'c3', 444)")
>>
>>  -- Debug
>>  -- require("console").start()
>> +
>> +--
>> +-- gh-3592: segmentation fault when table with error during
>> +-- creation is dropped.
>> +-- We should grant user enough rights to create space, but not
>> +-- enough to create index.
>> +--
>> +box.schema.user.create('tmp')
>> +box.schema.user.grant('tmp', 'create', 'universe')
>> +box.schema.user.grant('tmp', 'write', 'space', '_space')
>> +box.schema.user.grant('tmp', 'write', 'space', '_schema')
>> +box.session.su('tmp')
>> +--
>> +-- Error: user do not have rights to write in box.space._index.
>> +-- Space that was already created should be automatically dropped.
>> +--
>> +box.sql.execute('create table t1 (id int primary key, a int)')
>> +-- Error: no such table.
>> +box.sql.execute('drop table t1')
>> +box.session.su('admin')
>> +box.schema.user.drop('tmp’)
>
> Em, and where are tests for current feature? This test checks only one
> particular case. AFAIU #3592 is only result of absence of cleanup on
> table creation (original issue should be 
> https://github.com/tarantool/tarantool/issues/3499
> but it is marked as ‘doc’. Now your patch changes this behaviour again).
> Anyway, add test cases when CREATE TABLE fails on VDBE execution,
> e.g. half of unique indexes or referencing constraints are created, but
> smth goes wrong and remains of execution are eliminated correctly.
Done.

New patch:

commit f2fcfec99dfb785c5eb4da6846949cdc5f78d22a
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Aug 31 15:50:17 2018 +0300

     sql: clean-up on failed CREATE TABLE

     In case statement "CREATE TABLE ..." fails it can left some
     records in system spaces that shouldn't be there. These records
     won't be left behind after this patch.

     @TarantoolBot document
     Title: Clean up after failure of CREATE TABLE
     Usually CREATE TABLE creates no less than two objects which are
     space and index. If creation of index (or any object after space)
     failed, created space (and other created objects) won't be deleted
     though operation failed. Now these objects will be deleted
     properly.

     Closes #3592

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
@@ -55,6 +55,53 @@
  #include "box/tuple_format.h"
  #include "box/coll_id_cache.h"

+/**
+ * Structure that contains information about record that was
+ * inserted into system space.
+ */
+struct saved_record
+{
+    /** A link in a record list. */
+    struct rlist link;
+    /** Id of space in which the record was inserted. */
+    uint32_t space_id;
+    /** First register of the key of the record. */
+    int reg_key;
+    /** Number of registers the key consists of. */
+    int reg_key_count;
+    /** The number of the OP_SInsert operation. */
+    int insertion_opcode;
+};
+
+/**
+ * Save inserted in system space record in list.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param reg_key Register that contains first field of the key.
+ * @param reg_key_count Exact number of fields of the key.
+ * @param insertion_opcode Number of OP_SInsert opcode.
+ */
+static inline void
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,
+        int reg_key_count, int insertion_opcode)
+{
+    struct saved_record *record =
+        region_alloc(&parser->region, sizeof(*record));
+    if (record == NULL) {
+        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
+             "record");
+        parser->rc = SQL_TARANTOOL_ERROR;
+        parser->nErr++;
+        return;
+    }
+    record->space_id = space_id;
+    record->reg_key = reg_key;
+    record->reg_key_count = reg_key_count;
+    record->insertion_opcode = insertion_opcode;
+    rlist_add_entry(&parser->record_list, record, link);
+}
+
  void
  sql_finish_coding(struct Parse *parse_context)
  {
@@ -62,6 +109,40 @@ sql_finish_coding(struct Parse *parse_context)
      struct sqlite3 *db = parse_context->db;
      struct Vdbe *v = 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) == 0) {
+        struct saved_record *record =
+            rlist_shift_entry(&parse_context->record_list,
+                      struct saved_record, link);
+        /* Set P2 of SInsert. */
+        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
+        char *comment = "Delete entry from %s if CREATE TABLE fails";
+        rlist_foreach_entry(record, &parse_context->record_list, link) {
+            int record_reg = ++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 = 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 != 0) {
          if (parse_context->rc == SQLITE_OK)
              parse_context->rc = 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 != NULL)
          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
      return;
  error:
      parse->rc = 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 =
              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 =
              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);
      }
      /* Code creation of FK constraints, if any. */
      struct fkey_parse *fk_parse;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e98e845..a4b65eb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
      memset(parser, 0, sizeof(struct Parse));
      parser->db = db;
      rlist_create(&parser->new_fkey);
+    rlist_create(&parser->record_list);
      region_create(&parser->region, &cord()->slabc);
  }

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 744b660..d8eb516 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2765,6 +2765,11 @@ struct Parse {
       * Foreign key constraint appeared in CREATE TABLE stmt.
       */
      struct rlist new_fkey;
+    /**
+     * List of all records that were inserted in system spaces
+     * in current statement.
+     */
+    struct rlist record_list;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** True, if table to be created has AUTOINCREMENT PK. */
      bool is_new_table_autoinc;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..1bec2fa 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1010,8 +1010,12 @@ case OP_Halt: {
      p->pc = pcx;
      if (p->rc) {
          if (p->rc == SQL_TARANTOOL_ERROR) {
-            assert(pOp->p4.z != NULL);
-            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+            if (pOp->p4.z == NULL) {
+                assert(! diag_is_empty(diag_get()));
+            } else {
+                box_error_set(__FILE__, __LINE__, pOp->p5,
+                          pOp->p4.z);
+            }
          } else if (pOp->p5 != 0) {
              static const char * const azType[] = { "NOT NULL", 
"UNIQUE", "CHECK",
                                     "FOREIGN KEY" };
@@ -4308,8 +4312,8 @@ case OP_IdxInsert: {
      break;
  }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SInsert P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P3], on error goto P2
   *
   * This opcode is used only during DDL routine.
   * In contrast to ordinary insertion, insertion to system spaces
@@ -4322,15 +4326,15 @@ case OP_IdxInsert: {
   */
  case OP_SInsert: {
      assert(pOp->p1 > 0);
-    assert(pOp->p2 >= 0);
+    assert(pOp->p2 > 0);
+    assert(pOp->p3 >= 0);

-    pIn2 = &aMem[pOp->p2];
+    pIn3 = &aMem[pOp->p3];
      struct space *space = space_by_id(pOp->p1);
      assert(space != NULL);
      assert(space_is_system(space));
-    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
-    if (rc)
-        goto abort_due_to_error;
+    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
+        goto jump_to_p2;
      if (pOp->p5 & OPFLAG_NCHANGE)
          p->nChange++;
      break;
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..1659604 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,105 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- DROP TABLE should do the job
  -- Debug
  -- require("console").start()
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create', 'universe')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+---
+...
+index_count = #box.space._index:select()
+---
+...
+sequence_count = #box.space._sequence:select()
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('drop table t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+--
+-- 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)')
+---
+- error: Write access to space '_sequence' is denied for user 'tmp'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+box.schema.user.drop('tmp')
+---
+...
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)")

  -- Debug
  -- require("console").start()
+
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create', 'universe')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+index_count = #box.space._index:select()
+sequence_count = #box.space._sequence:select()
+
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('create table t1 (id int primary key, a int)')
+-- Error: no such table.
+box.sql.execute('drop table t1')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #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)')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+box.schema.user.drop('tmp')


[-- Attachment #2: Type: text/html, Size: 146119 bytes --]

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[n.pettik]

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.

>> 
>>>  void
>>>  sql_finish_coding(struct Parse *parse_context)
>>> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>>>      struct sqlite3 *db = parse_context->db;
>>>      struct Vdbe *v = 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) == 0) {
>> 
>> Nit: if(! rlist_empty()) 
> 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 = parse_context->db;
>      struct Vdbe *v = 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) == 0) {
> +        struct saved_record *record =
> +            rlist_shift_entry(&parse_context->record_list,
> +                      struct saved_record, link);
> +        /* Set P2 of SInsert. */
> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
> +        char *comment = "Delete entry from %s if CREATE TABLE fails”;

Nit: const char *.

> +        rlist_foreach_entry(record, &parse_context->record_list, link) {
> +            int record_reg = ++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 = 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 != 0) {
>          if (parse_context->rc == SQLITE_OK)
>              parse_context->rc = 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 != NULL)
>          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
> +    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);

As I sad you don’t 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 != 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’t insist on this change tho,
it is up to you. 

>      return;
>  error:
>      parse->rc = 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 =
>              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 =
>              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’t 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 != 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 == #box.space._space:select()
> +index_count == #box.space._index:select()
> +sequence_count == #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)’)

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 == #box.space._space:select()
> +index_count == #box.space._index:select()
> +sequence_count == #box.space._sequence:select()
> +
> +box.schema.user.drop('tmp’)

I see no tests involving FK constraints. Add them as well.

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 34284 bytes --]

Hello! Thank you for review! New patch and diff between last two
patches below.


On 10/09/2018 05:15 PM, n.pettik wrote:
> 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.
It is better now:
https://travis-ci.org/tarantool/tarantool/builds/439677737

Test box-tap/feedback_daemon.test.lua failed but
there is ticket:
https://github.com/tarantool/tarantool/issues/3558

>>>>   void
>>>>   sql_finish_coding(struct Parse *parse_context)
>>>> @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context)
>>>>       struct sqlite3 *db = parse_context->db;
>>>>       struct Vdbe *v = 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) == 0) {
>>> Nit: if(! rlist_empty())
>> Fixed.
> No, it is not fixed.
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 = parse_context->db;
>>       struct Vdbe *v = 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) == 0) {
>> +        struct saved_record *record =
>> +            rlist_shift_entry(&parse_context->record_list,
>> +                      struct saved_record, link);
>> +        /* Set P2 of SInsert. */
>> +        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
>> +        char *comment = "Delete entry from %s if CREATE TABLE fails”;
> Nit: const char *.
Fixed.
>> +        rlist_foreach_entry(record, &parse_context->record_list, link) {
>> +            int record_reg = ++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 = 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 != 0) {
>>           if (parse_context->rc == SQLITE_OK)
>>               parse_context->rc = 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 != NULL)
>>           sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
>> +    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
> As I sad you don’t 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 != 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’t insist on this change tho,
> it is up to you.
Discussed and decided to left as it is.
>>       return;
>>   error:
>>       parse->rc = 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 =
>>               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 =
>>               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’t 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 != NULL) {
> +               save_record(parse_context, BOX_FK_CONSTRAINT_ID,
> +                           constr_tuple_reg, 2, vdbe->nOp - 1);
> +       }
Discussed and decided to left as it is.
>> 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 == #box.space._space:select()
>> +index_count == #box.space._index:select()
>> +sequence_count == #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)’)
> Nit: for SQL statements please use uppercase.
Fixed.
>> +
>> +box.session.su('admin')
>> +
>> +--
>> +-- Check that _space, _index and _sequence have the same number of
>> +-- records.
>> +--
>> +space_count == #box.space._space:select()
>> +index_count == #box.space._index:select()
>> +sequence_count == #box.space._sequence:select()
>> +
>> +box.schema.user.drop('tmp’)
> I see no tests involving FK constraints. Add them as well.
>
Added.

*Diff between two last patches:*

diff --git a/6a97ff9..28b488c b/28b488c
index 6a97ff9..28b488c 100644
--- a/6a97ff9..28b488c
+++ b/28b488c
@@ -119,26 +119,28 @@ sql_finish_coding(struct Parse *parse_context)
       * anything as these statements create no more than one
       * record.
       */
-    if (rlist_empty(&parse_context->record_list) == 0) {
+    if (!rlist_empty(&parse_context->record_list)) {
          struct saved_record *record =
              rlist_shift_entry(&parse_context->record_list,
                        struct saved_record, link);
          /* Set P2 of SInsert. */
          sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
-        char *comment = "Delete entry from %s if CREATE TABLE fails";
+        MAYBE_UNUSED const char *comment =
+            "Delete entry from %s if CREATE TABLE fails";
          rlist_foreach_entry(record, &parse_context->record_list, link) {
              int record_reg = ++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 = space_by_id(record->space_id);
+            MAYBE_UNUSED struct space *space =
+                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);
+        sqlite3VdbeAddOp1(v, OP_Halt, SQL_TARANTOOL_ERROR);
          VdbeComment((v,
                   "Exit with an error if CREATE statement fails"));
      }
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 1659604..43da275 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -42,7 +42,7 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  box.schema.user.create('tmp')
  ---
  ...
-box.schema.user.grant('tmp', 'create', 'universe')
+box.schema.user.grant('tmp', 'create, read', 'universe')
  ---
  ...
  box.schema.user.grant('tmp', 'write', 'space', '_space')
@@ -68,12 +68,12 @@ box.session.su('tmp')
  -- Error: user do not have rights to write in box.space._index.
  -- Space that was already created should be automatically dropped.
  --
-box.sql.execute('create table t1 (id int primary key, a int)')
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
  ---
  - error: Write access to space '_index' is denied for user 'tmp'
  ...
  -- Error: no such table.
-box.sql.execute('drop table t1')
+box.sql.execute('DROP TABLE t1')
  ---
  - error: 'no such table: T1'
  ...
@@ -109,7 +109,7 @@ 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)')
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT, a 
UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')
  ---
  - error: Write access to space '_sequence' is denied for user 'tmp'
  ...
@@ -132,6 +132,48 @@ sequence_count == #box.space._sequence:select()
  ---
  - true
  ...
+--
+-- Give user right to write in _sequence. Still have not enough
+-- rights to write in _fk_constraint.
+--
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+---
+...
+box.session.su('tmp')
+---
+...
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+---
+...
+--
+-- Error: user do not have rights to write in _fk_constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+---
+- error: Write access to space '_fk_constraint' is denied for user 'tmp'
+...
+box.sql.execute('DROP TABLE t3;')
+---
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
  box.schema.user.drop('tmp')
  ---
  ...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 1bc8894..f86e3d8 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -33,7 +33,7 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- create index.
  --
  box.schema.user.create('tmp')
-box.schema.user.grant('tmp', 'create', 'universe')
+box.schema.user.grant('tmp', 'create, read', 'universe')
  box.schema.user.grant('tmp', 'write', 'space', '_space')
  box.schema.user.grant('tmp', 'write', 'space', '_schema')

@@ -47,9 +47,9 @@ box.session.su('tmp')
  -- Error: user do not have rights to write in box.space._index.
  -- Space that was already created should be automatically dropped.
  --
-box.sql.execute('create table t1 (id int primary key, a int)')
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
  -- Error: no such table.
-box.sql.execute('drop table t1')
+box.sql.execute('DROP TABLE t1')

  box.session.su('admin')

@@ -71,7 +71,31 @@ 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)')
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY AUTOINCREMENT, a 
UNIQUE, b UNIQUE, c UNIQUE, d UNIQUE)')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+--
+-- Give user right to write in _sequence. Still have not enough
+-- rights to write in _fk_constraint.
+--
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+box.session.su('tmp')
+
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+--
+-- Error: user do not have rights to write in _fk_constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+box.sql.execute('DROP TABLE t3;')

  box.session.su('admin')


*New patch:*

commit 876ac107e293bd184e247d0e8e299765cb986f9f
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Aug 31 15:50:17 2018 +0300

     sql: clean-up on failed CREATE TABLE

     In case statement "CREATE TABLE ..." fails it can left some
     records in system spaces that shouldn't be there. These records
     won't be left behind after this patch.

     @TarantoolBot document
     Title: Clean up after failure of CREATE TABLE
     Usually CREATE TABLE creates no less than two objects which are
     space and index. If creation of index (or any object after space)
     failed, created space (and other created objects) won't be deleted
     though operation failed. Now these objects will be deleted
     properly.

     Closes #3592

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a806fb4..28b488c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -55,6 +55,53 @@
  #include "box/tuple_format.h"
  #include "box/coll_id_cache.h"

+/**
+ * Structure that contains information about record that was
+ * inserted into system space.
+ */
+struct saved_record
+{
+    /** A link in a record list. */
+    struct rlist link;
+    /** Id of space in which the record was inserted. */
+    uint32_t space_id;
+    /** First register of the key of the record. */
+    int reg_key;
+    /** Number of registers the key consists of. */
+    int reg_key_count;
+    /** The number of the OP_SInsert operation. */
+    int insertion_opcode;
+};
+
+/**
+ * Save inserted in system space record in list.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param reg_key Register that contains first field of the key.
+ * @param reg_key_count Exact number of fields of the key.
+ * @param insertion_opcode Number of OP_SInsert opcode.
+ */
+static inline void
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,
+        int reg_key_count, int insertion_opcode)
+{
+    struct saved_record *record =
+        region_alloc(&parser->region, sizeof(*record));
+    if (record == NULL) {
+        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
+             "record");
+        parser->rc = SQL_TARANTOOL_ERROR;
+        parser->nErr++;
+        return;
+    }
+    record->space_id = space_id;
+    record->reg_key = reg_key;
+    record->reg_key_count = reg_key_count;
+    record->insertion_opcode = insertion_opcode;
+    rlist_add_entry(&parser->record_list, record, link);
+}
+
  void
  sql_finish_coding(struct Parse *parse_context)
  {
@@ -62,6 +109,42 @@ sql_finish_coding(struct Parse *parse_context)
      struct sqlite3 *db = parse_context->db;
      struct Vdbe *v = 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)) {
+        struct saved_record *record =
+            rlist_shift_entry(&parse_context->record_list,
+                      struct saved_record, link);
+        /* Set P2 of SInsert. */
+        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
+        MAYBE_UNUSED const char *comment =
+            "Delete entry from %s if CREATE TABLE fails";
+        rlist_foreach_entry(record, &parse_context->record_list, link) {
+            int record_reg = ++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);
+            MAYBE_UNUSED struct space *space =
+                space_by_id(record->space_id);
+            VdbeComment((v, comment, space_name(space)));
+            /* Set P2 of SInsert. */
+            sqlite3VdbeChangeP2(v, record->insertion_opcode,
+                        v->nOp);
+        }
+        sqlite3VdbeAddOp1(v, OP_Halt, SQL_TARANTOOL_ERROR);
+        VdbeComment((v,
+                 "Exit with an error if CREATE statement fails"));
+    }
+
      if (db->mallocFailed || parse_context->nErr != 0) {
          if (parse_context->rc == SQLITE_OK)
              parse_context->rc = SQLITE_ERROR;
@@ -1101,13 +1184,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 != NULL)
          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
      return;
  error:
      parse->rc = SQL_TARANTOOL_ERROR;
@@ -1165,8 +1249,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 +1425,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 +1574,18 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
context */
          int reg_seq_record =
              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 =
              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);
      }
      /* Code creation of FK constraints, if any. */
      struct fkey_parse *fk_parse;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e98e845..a4b65eb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
      memset(parser, 0, sizeof(struct Parse));
      parser->db = db;
      rlist_create(&parser->new_fkey);
+    rlist_create(&parser->record_list);
      region_create(&parser->region, &cord()->slabc);
  }

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 744b660..d8eb516 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2765,6 +2765,11 @@ struct Parse {
       * Foreign key constraint appeared in CREATE TABLE stmt.
       */
      struct rlist new_fkey;
+    /**
+     * List of all records that were inserted in system spaces
+     * in current statement.
+     */
+    struct rlist record_list;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** True, if table to be created has AUTOINCREMENT PK. */
      bool is_new_table_autoinc;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..1bec2fa 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1010,8 +1010,12 @@ case OP_Halt: {
      p->pc = pcx;
      if (p->rc) {
          if (p->rc == SQL_TARANTOOL_ERROR) {
-            assert(pOp->p4.z != NULL);
-            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+            if (pOp->p4.z == NULL) {
+                assert(! diag_is_empty(diag_get()));
+            } else {
+                box_error_set(__FILE__, __LINE__, pOp->p5,
+                          pOp->p4.z);
+            }
          } else if (pOp->p5 != 0) {
              static const char * const azType[] = { "NOT NULL", 
"UNIQUE", "CHECK",
                                     "FOREIGN KEY" };
@@ -4308,8 +4312,8 @@ case OP_IdxInsert: {
      break;
  }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SInsert P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P3], on error goto P2
   *
   * This opcode is used only during DDL routine.
   * In contrast to ordinary insertion, insertion to system spaces
@@ -4322,15 +4326,15 @@ case OP_IdxInsert: {
   */
  case OP_SInsert: {
      assert(pOp->p1 > 0);
-    assert(pOp->p2 >= 0);
+    assert(pOp->p2 > 0);
+    assert(pOp->p3 >= 0);

-    pIn2 = &aMem[pOp->p2];
+    pIn3 = &aMem[pOp->p3];
      struct space *space = space_by_id(pOp->p1);
      assert(space != NULL);
      assert(space_is_system(space));
-    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
-    if (rc)
-        goto abort_due_to_error;
+    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
+        goto jump_to_p2;
      if (pOp->p5 & OPFLAG_NCHANGE)
          p->nChange++;
      break;
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..43da275 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,147 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- DROP TABLE should do the job
  -- Debug
  -- require("console").start()
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create, read', 'universe')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+---
+...
+index_count = #box.space._index:select()
+---
+...
+sequence_count = #box.space._sequence:select()
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('DROP TABLE t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+--
+-- 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)')
+---
+- error: Write access to space '_sequence' is denied for user 'tmp'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+--
+-- Give user right to write in _sequence. Still have not enough
+-- rights to write in _fk_constraint.
+--
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+---
+...
+box.session.su('tmp')
+---
+...
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+---
+...
+--
+-- Error: user do not have rights to write in _fk_constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+---
+- error: Write access to space '_fk_constraint' is denied for user 'tmp'
+...
+box.sql.execute('DROP TABLE t3;')
+---
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..f86e3d8 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,86 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")

  -- Debug
  -- require("console").start()
+
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create, read', 'universe')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+index_count = #box.space._index:select()
+sequence_count = #box.space._sequence:select()
+
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
+-- Error: no such table.
+box.sql.execute('DROP TABLE t1')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #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)')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+--
+-- Give user right to write in _sequence. Still have not enough
+-- rights to write in _fk_constraint.
+--
+box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+box.session.su('tmp')
+
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+--
+-- Error: user do not have rights to write in _fk_constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+box.sql.execute('DROP TABLE t3;')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+box.schema.user.drop('tmp')


[-- Attachment #2: Type: text/html, Size: 42139 bytes --]

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]


>>> +--
>>> +-- Check that _space, _index and _sequence have the same number of
>>> +-- records.
>>> +--
>>> +space_count == #box.space._space:select()
>>> +index_count == #box.space._index:select()
>>> +sequence_count == #box.space._sequence:select()
>>> +
>>> +box.schema.user.drop('tmp’)
>> I see no tests involving FK constraints. Add them as well.
>> 
> Added.


> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
> index 1bc8894..f86e3d8 100644
> --- a/test/sql/drop-table.test.lua
> +++ b/test/sql/drop-table.test.lua
> 
> +-- Give user right to write in _sequence. Still have not enough
> +-- rights to write in _fk_constraint.
> +--
> +box.schema.user.grant('tmp', 'write', 'space', '_sequence')
> +box.session.su('tmp')
> +
> +box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
> +--
> +-- Error: user do not have rights to write in _fk_constraint.
> +--
> +box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
> +box.sql.execute('DROP TABLE t3;’)

You misunderstood me a bit. I mean following test:

fk_count = #box.space._fk_constraints:select()

box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, a INT UNIQUE, c INT REFERENCES t3);’)

Here creation of last FK fails - for instance due to type mismatch.
Then, number of created FK constraints (and indexes) should be unchanged:

fk_count == #box.space._fk_constraints:select()
index_count == #box.space._fk_constraints:select()

Note that FK constraints are created after indexes.


[-- Attachment #2: Type: text/html, Size: 15578 bytes --]

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 22800 bytes --]

Thank you for review! Diff between two last patches and new patch
below.


On 10/11/2018 06:09 PM, n.pettik wrote:
>
>>>> +--
>>>> +-- Check that _space, _index and _sequence have the same number of
>>>> +-- records.
>>>> +--
>>>> +space_count == #box.space._space:select()
>>>> +index_count == #box.space._index:select()
>>>> +sequence_count == #box.space._sequence:select()
>>>> +
>>>> +box.schema.user.drop('tmp’)
>>> I see no tests involving FK constraints. Add them as well.
>>>
>> Added.
>
>
>> diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
>> index 1bc8894..f86e3d8 100644
>> --- a/test/sql/drop-table.test.lua
>> +++ b/test/sql/drop-table.test.lua
>>
>> +-- Give user right to write in _sequence. Still have not enough
>> +-- rights to write in _fk_constraint.
>> +--
>> +box.schema.user.grant('tmp', 'write', 'space', '_sequence')
>> +box.session.su('tmp')
>> +
>> +box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
>> +--
>> +-- Error: user do not have rights to write in _fk_constraint.
>> +--
>> +box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
>> +box.sql.execute('DROP TABLE t3;’)
>
> You misunderstood me a bit. I mean following test:
>
> fk_count = #box.space._fk_constraints:select()
>
> box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, 
> a INT UNIQUE, c INT REFERENCES t3);’)
>
> Here creation of last FK fails - for instance due to type mismatch.
> Then, number of created FK constraints (and indexes) should be unchanged:
>
> fk_count == #box.space._fk_constraints:select()
> index_count == #box.space._fk_constraints:select()
>
> Note that FK constraints are created after indexes.
>
Fixed - changed last test.

*Diff between last two patches:*

diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 43da275..297799e 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -132,11 +132,13 @@ sequence_count == #box.space._sequence:select()
  ---
  - true
  ...
+fk_constraint_count = #box.space._fk_constraint:select()
+---
+...
  --
--- Give user right to write in _sequence. Still have not enough
--- rights to write in _fk_constraint.
+-- Check that clean-up works fine after another error.
  --
-box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+box.schema.user.grant('tmp', 'write', 'space')
  ---
  ...
  box.session.su('tmp')
@@ -146,18 +148,16 @@ box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY 
KEY);')
  ---
  ...
  --
--- Error: user do not have rights to write in _fk_constraint.
+-- Error: Failed to create foreign key constraint.
  --
-box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, a 
INT UNIQUE, c INT REFERENCES t3);')
  ---
-- error: Write access to space '_fk_constraint' is denied for user 'tmp'
+- error: 'Failed to create foreign key constraint 
''FK_CONSTRAINT_2_T4'': field type
+    mismatch'
  ...
  box.sql.execute('DROP TABLE t3;')
  ---
  ...
-box.session.su('admin')
----
-...
  --
  -- Check that _space, _index and _sequence have the same number of
  -- records.
@@ -174,6 +174,13 @@ sequence_count == #box.space._sequence:select()
  ---
  - true
  ...
+fk_constraint_count == #box.space._fk_constraint:select()
+---
+- true
+...
+box.session.su('admin')
+---
+...
  box.schema.user.drop('tmp')
  ---
  ...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index f86e3d8..b1c5253 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -83,22 +83,21 @@ space_count == #box.space._space:select()
  index_count == #box.space._index:select()
  sequence_count == #box.space._sequence:select()

+fk_constraint_count = #box.space._fk_constraint:select()
+
  --
--- Give user right to write in _sequence. Still have not enough
--- rights to write in _fk_constraint.
+-- Check that clean-up works fine after another error.
  --
-box.schema.user.grant('tmp', 'write', 'space', '_sequence')
+box.schema.user.grant('tmp', 'write', 'space')
  box.session.su('tmp')

  box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
  --
--- Error: user do not have rights to write in _fk_constraint.
+-- Error: Failed to create foreign key constraint.
  --
-box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3);')
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, a 
INT UNIQUE, c INT REFERENCES t3);')
  box.sql.execute('DROP TABLE t3;')

-box.session.su('admin')
-
  --
  -- Check that _space, _index and _sequence have the same number of
  -- records.
@@ -106,5 +105,8 @@ box.session.su('admin')
  space_count == #box.space._space:select()
  index_count == #box.space._index:select()
  sequence_count == #box.space._sequence:select()
+fk_constraint_count == #box.space._fk_constraint:select()
+
+box.session.su('admin')

  box.schema.user.drop('tmp')

*New patch:*

commit f9092f3fd17676bfe220bf9d31aa4ed9a648590c
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Aug 31 15:50:17 2018 +0300

     sql: clean-up on failed CREATE TABLE

     In case statement "CREATE TABLE ..." fails it can left some
     records in system spaces that shouldn't be there. These records
     won't be left behind after this patch.

     @TarantoolBot document
     Title: Clean up after failure of CREATE TABLE
     Usually CREATE TABLE creates no less than two objects which are
     space and index. If creation of index (or any object after space)
     failed, created space (and other created objects) won't be deleted
     though operation failed. Now these objects will be deleted
     properly.

     Closes #3592

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a806fb4..28b488c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -55,6 +55,53 @@
  #include "box/tuple_format.h"
  #include "box/coll_id_cache.h"

+/**
+ * Structure that contains information about record that was
+ * inserted into system space.
+ */
+struct saved_record
+{
+    /** A link in a record list. */
+    struct rlist link;
+    /** Id of space in which the record was inserted. */
+    uint32_t space_id;
+    /** First register of the key of the record. */
+    int reg_key;
+    /** Number of registers the key consists of. */
+    int reg_key_count;
+    /** The number of the OP_SInsert operation. */
+    int insertion_opcode;
+};
+
+/**
+ * Save inserted in system space record in list.
+ *
+ * @param parser SQL Parser object.
+ * @param space_id Id of table in which record is inserted.
+ * @param reg_key Register that contains first field of the key.
+ * @param reg_key_count Exact number of fields of the key.
+ * @param insertion_opcode Number of OP_SInsert opcode.
+ */
+static inline void
+save_record(struct Parse *parser, uint32_t space_id, int reg_key,
+        int reg_key_count, int insertion_opcode)
+{
+    struct saved_record *record =
+        region_alloc(&parser->region, sizeof(*record));
+    if (record == NULL) {
+        diag_set(OutOfMemory, sizeof(*record), "region_alloc",
+             "record");
+        parser->rc = SQL_TARANTOOL_ERROR;
+        parser->nErr++;
+        return;
+    }
+    record->space_id = space_id;
+    record->reg_key = reg_key;
+    record->reg_key_count = reg_key_count;
+    record->insertion_opcode = insertion_opcode;
+    rlist_add_entry(&parser->record_list, record, link);
+}
+
  void
  sql_finish_coding(struct Parse *parse_context)
  {
@@ -62,6 +109,42 @@ sql_finish_coding(struct Parse *parse_context)
      struct sqlite3 *db = parse_context->db;
      struct Vdbe *v = 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)) {
+        struct saved_record *record =
+            rlist_shift_entry(&parse_context->record_list,
+                      struct saved_record, link);
+        /* Set P2 of SInsert. */
+        sqlite3VdbeChangeP2(v, record->insertion_opcode, v->nOp);
+        MAYBE_UNUSED const char *comment =
+            "Delete entry from %s if CREATE TABLE fails";
+        rlist_foreach_entry(record, &parse_context->record_list, link) {
+            int record_reg = ++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);
+            MAYBE_UNUSED struct space *space =
+                space_by_id(record->space_id);
+            VdbeComment((v, comment, space_name(space)));
+            /* Set P2 of SInsert. */
+            sqlite3VdbeChangeP2(v, record->insertion_opcode,
+                        v->nOp);
+        }
+        sqlite3VdbeAddOp1(v, OP_Halt, SQL_TARANTOOL_ERROR);
+        VdbeComment((v,
+                 "Exit with an error if CREATE statement fails"));
+    }
+
      if (db->mallocFailed || parse_context->nErr != 0) {
          if (parse_context->rc == SQLITE_OK)
              parse_context->rc = SQLITE_ERROR;
@@ -1101,13 +1184,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 != NULL)
          sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE);
+    save_record(parse, BOX_INDEX_ID, entry_reg, 2, v->nOp - 1);
      return;
  error:
      parse->rc = SQL_TARANTOOL_ERROR;
@@ -1165,8 +1249,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 +1425,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 +1574,18 @@ sqlite3EndTable(Parse * pParse,    /* Parse 
context */
          int reg_seq_record =
              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 =
              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);
      }
      /* Code creation of FK constraints, if any. */
      struct fkey_parse *fk_parse;
diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
index e98e845..a4b65eb 100644
--- a/src/box/sql/prepare.c
+++ b/src/box/sql/prepare.c
@@ -274,6 +274,7 @@ sql_parser_create(struct Parse *parser, sqlite3 *db)
      memset(parser, 0, sizeof(struct Parse));
      parser->db = db;
      rlist_create(&parser->new_fkey);
+    rlist_create(&parser->record_list);
      region_create(&parser->region, &cord()->slabc);
  }

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 744b660..d8eb516 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2765,6 +2765,11 @@ struct Parse {
       * Foreign key constraint appeared in CREATE TABLE stmt.
       */
      struct rlist new_fkey;
+    /**
+     * List of all records that were inserted in system spaces
+     * in current statement.
+     */
+    struct rlist record_list;
      bool initiateTTrans;    /* Initiate Tarantool transaction */
      /** True, if table to be created has AUTOINCREMENT PK. */
      bool is_new_table_autoinc;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..1bec2fa 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1010,8 +1010,12 @@ case OP_Halt: {
      p->pc = pcx;
      if (p->rc) {
          if (p->rc == SQL_TARANTOOL_ERROR) {
-            assert(pOp->p4.z != NULL);
-            box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z);
+            if (pOp->p4.z == NULL) {
+                assert(! diag_is_empty(diag_get()));
+            } else {
+                box_error_set(__FILE__, __LINE__, pOp->p5,
+                          pOp->p4.z);
+            }
          } else if (pOp->p5 != 0) {
              static const char * const azType[] = { "NOT NULL", 
"UNIQUE", "CHECK",
                                     "FOREIGN KEY" };
@@ -4308,8 +4312,8 @@ case OP_IdxInsert: {
      break;
  }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id = P1, key = r[P2]
+/* Opcode: SInsert P1 P2 P3 * P5
+ * Synopsis: space id = P1, key = r[P3], on error goto P2
   *
   * This opcode is used only during DDL routine.
   * In contrast to ordinary insertion, insertion to system spaces
@@ -4322,15 +4326,15 @@ case OP_IdxInsert: {
   */
  case OP_SInsert: {
      assert(pOp->p1 > 0);
-    assert(pOp->p2 >= 0);
+    assert(pOp->p2 > 0);
+    assert(pOp->p3 >= 0);

-    pIn2 = &aMem[pOp->p2];
+    pIn3 = &aMem[pOp->p3];
      struct space *space = space_by_id(pOp->p1);
      assert(space != NULL);
      assert(space_is_system(space));
-    rc = tarantoolSqlite3Insert(space, pIn2->z, pIn2->z + pIn2->n);
-    if (rc)
-        goto abort_due_to_error;
+    if (tarantoolSqlite3Insert(space, pIn3->z, pIn3->z + pIn3->n) != 0)
+        goto jump_to_p2;
      if (pOp->p5 & OPFLAG_NCHANGE)
          p->nChange++;
      break;
diff --git a/test/sql/drop-table.result b/test/sql/drop-table.result
index 08f2496..297799e 100644
--- a/test/sql/drop-table.result
+++ b/test/sql/drop-table.result
@@ -33,3 +33,154 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")
  -- DROP TABLE should do the job
  -- Debug
  -- require("console").start()
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+---
+...
+box.schema.user.grant('tmp', 'create, read', 'universe')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+---
+...
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+---
+...
+index_count = #box.space._index:select()
+---
+...
+sequence_count = #box.space._sequence:select()
+---
+...
+box.session.su('tmp')
+---
+...
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
+---
+- error: Write access to space '_index' is denied for user 'tmp'
+...
+-- Error: no such table.
+box.sql.execute('DROP TABLE t1')
+---
+- error: 'no such table: T1'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+--
+-- 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)')
+---
+- error: Write access to space '_sequence' is denied for user 'tmp'
+...
+box.session.su('admin')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+fk_constraint_count = #box.space._fk_constraint:select()
+---
+...
+--
+-- Check that clean-up works fine after another error.
+--
+box.schema.user.grant('tmp', 'write', 'space')
+---
+...
+box.session.su('tmp')
+---
+...
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+---
+...
+--
+-- Error: Failed to create foreign key constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, a 
INT UNIQUE, c INT REFERENCES t3);')
+---
+- error: 'Failed to create foreign key constraint 
''FK_CONSTRAINT_2_T4'': field type
+    mismatch'
+...
+box.sql.execute('DROP TABLE t3;')
+---
+...
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+---
+- true
+...
+index_count == #box.space._index:select()
+---
+- true
+...
+sequence_count == #box.space._sequence:select()
+---
+- true
+...
+fk_constraint_count == #box.space._fk_constraint:select()
+---
+- true
+...
+box.session.su('admin')
+---
+...
+box.schema.user.drop('tmp')
+---
+...
diff --git a/test/sql/drop-table.test.lua b/test/sql/drop-table.test.lua
index 9663074..b1c5253 100644
--- a/test/sql/drop-table.test.lua
+++ b/test/sql/drop-table.test.lua
@@ -25,3 +25,88 @@ box.sql.execute("INSERT INTO zzzoobar VALUES (111, 
222, 'c3', 444)")

  -- Debug
  -- require("console").start()
+
+--
+-- gh-3592: clean-up garbage on failed CREATE TABLE statement.
+--
+-- Let user have enough rights to create space, but not enough to
+-- create index.
+--
+box.schema.user.create('tmp')
+box.schema.user.grant('tmp', 'create, read', 'universe')
+box.schema.user.grant('tmp', 'write', 'space', '_space')
+box.schema.user.grant('tmp', 'write', 'space', '_schema')
+
+-- Number of records in _space, _index, _sequence:
+space_count = #box.space._space:select()
+index_count = #box.space._index:select()
+sequence_count = #box.space._sequence:select()
+
+box.session.su('tmp')
+--
+-- Error: user do not have rights to write in box.space._index.
+-- Space that was already created should be automatically dropped.
+--
+box.sql.execute('CREATE TABLE t1 (id INT PRIMARY KEY, a INT)')
+-- Error: no such table.
+box.sql.execute('DROP TABLE t1')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #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)')
+
+box.session.su('admin')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+
+fk_constraint_count = #box.space._fk_constraint:select()
+
+--
+-- Check that clean-up works fine after another error.
+--
+box.schema.user.grant('tmp', 'write', 'space')
+box.session.su('tmp')
+
+box.sql.execute('CREATE TABLE t3(a INTEGER PRIMARY KEY);')
+--
+-- Error: Failed to create foreign key constraint.
+--
+box.sql.execute('CREATE TABLE t4(x INTEGER PRIMARY KEY REFERENCES t3, a 
INT UNIQUE, c INT REFERENCES t3);')
+box.sql.execute('DROP TABLE t3;')
+
+--
+-- Check that _space, _index and _sequence have the same number of
+-- records.
+--
+space_count == #box.space._space:select()
+index_count == #box.space._index:select()
+sequence_count == #box.space._sequence:select()
+fk_constraint_count == #box.space._fk_constraint:select()
+
+box.session.su('admin')
+
+box.schema.user.drop('tmp')


[-- Attachment #2: Type: text/html, Size: 46624 bytes --]

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[n.pettik]

Now LGTM.

[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation

[Kirill Yukhin]

Hello,
On 07 Sep 22:04, imeevma@tarantool.org wrote:
> Some creation operations create objects even on fail. This is
> wrong and should be fixed. This patch adds destructors for such
> objects.
> 
> Closes #3592
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3592-cleanup-on-failed-create
> Issue: https://github.com/tarantool/tarantool/issues/3592
I've checked your patch into 2.1 branch.

--
Regards, Kirill Yukhin