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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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);