[tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Sep 18 19:18:47 MSK 2018
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 at 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);
More information about the Tarantool-patches
mailing list