From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0B7502AD0A for ; Tue, 18 Sep 2018 12:18:56 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id W-2-txkvjHL8 for ; Tue, 18 Sep 2018 12:18:55 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 94AA321C71 for ; Tue, 18 Sep 2018 12:18:55 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation References: <433c416428f1ef575324236be22c27962db3f8b5.1536346889.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <74ed5f5b-d74f-0933-ccf0-c0d9861f0842@tarantool.org> Date: Tue, 18 Sep 2018 18:18:47 +0200 MIME-Version: 1.0 In-Reply-To: <433c416428f1ef575324236be22c27962db3f8b5.1536346889.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: imeevma@tarantool.org, tarantool-patches@freelists.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);