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 EA82B2B5B3 for ; Mon, 1 Oct 2018 09:05:46 -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 E4mkNRsSgRKe for ; Mon, 1 Oct 2018 09:05:46 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 E741E2B5A8 for ; Mon, 1 Oct 2018 09:05:45 -0400 (EDT) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_54BB43A5-83DE-48BE-947A-19F4C708EE5B" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation Date: Mon, 1 Oct 2018 16:05:42 +0300 In-Reply-To: References: <433c416428f1ef575324236be22c27962db3f8b5.1536346889.git.imeevma@gmail.com> <74ed5f5b-d74f-0933-ccf0-c0d9861f0842@tarantool.org> <30e818b1-2916-58cc-9074-2764de0d6fad@tarantool.org> <80242351-a576-dde6-034e-f19c899a9cc9@tarantool.org> 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: tarantool-patches@freelists.org Cc: Imeev Mergen , Vladislav Shpilevoy --Apple-Mail=_54BB43A5-83DE-48BE-947A-19F4C708EE5B Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > commit fe8415a79d401b741dcb565d34eb56495223f8b6 > Author: Mergen Imeev > > Date: Fri Aug 31 15:50:17 2018 +0300 >=20 > sql: cleanup on failed creation operation >=20 > 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 =E2=80=98some creation = operations=E2=80=99 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=E2=80=99t 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. >=20 > Closes #3592 >=20 > 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 =3D > + region_alloc(&fiber()->gc, sizeof(*record)); Why do you use global region? You can use parser=E2=80=99s one. > void > sql_finish_coding(struct Parse *parse_context) > @@ -62,6 +110,29 @@ sql_finish_coding(struct Parse *parse_context) > struct sqlite3 *db =3D parse_context->db; > struct Vdbe *v =3D 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) =3D=3D 0) { Nit: if(! rlist_empty())=20 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 =3D > + 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 =3D ++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 =3D db; > rlist_create(&parser->new_fkey); > + rlist_create(&parser->record_list); > region_create(&parser->region, &cord()->slabc); > } >=20 > 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 =3D pcx; > if (p->rc) { > if (p->rc =3D=3D SQL_TARANTOOL_ERROR) { > - assert(pOp->p4.z !=3D NULL); > - box_error_set(__FILE__, __LINE__, pOp->p5, pOp->p4.z); > + if (pOp->p4.z =3D=3D NULL) { > + assert(! diag_is_empty(diag_get())); > + } else { > + box_error_set(__FILE__, __LINE__, pOp->p5, > + pOp->p4.z); > + } > } else if (pOp->p5 !=3D 0) { > static const char * const azType[] =3D { "NOT NULL", = "UNIQUE", "CHECK", > "FOREIGN KEY" }; > @@ -4318,8 +4322,8 @@ case OP_IdxInsert: { /* in2 */ > break; > } >=20 > -/* Opcode: SInsert P1 P2 * * P5 > - * Synopsis: space id =3D P1, key =3D r[P2] > +/* Opcode: SInsert P1 P2 P3 * P5 > + * Synopsis: space id =3D P1, key =3D r[P3], on error goto P2 It would be better to swap P2 and P3: if you did so, you wouldn=E2=80=99t = 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=E2=80=99t 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)") >=20 > -- 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=E2=80=99) 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 =E2=80=98doc=E2=80=99. 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. --Apple-Mail=_54BB43A5-83DE-48BE-947A-19F4C708EE5B Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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.

Kind of = poor explanation. Where those destructors are used?
Which = objects are you talking about? AFAIU =E2=80=98some creation = operations=E2=80=99
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=E2=80=99= 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 =3D
+    =     region_alloc(&fiber()->gc, = sizeof(*record));

Why do you use global region? You can use = parser=E2=80=99s one.

 void
 sql_finish_coding(struct = Parse *parse_context)
@@ -62,6 +110,29 @@ = sql_finish_coding(struct Parse *parse_context)
     struct sqlite3 *db =3D = parse_context->db;
     struct = Vdbe *v =3D 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) =3D=3D 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 =3D
+    =         = 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 =3D = ++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 =3D 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 =3D = pcx;
     if (p->rc) = {
     =     if (p->rc =3D=3D SQL_TARANTOOL_ERROR) {
-        =     assert(pOp->p4.z !=3D NULL);
-        =     box_error_set(__FILE__, __LINE__, pOp->p5, = pOp->p4.z);
+        =     if (pOp->p4.z =3D=3D NULL) {
+        =         assert(! = diag_is_empty(diag_get()));
+    =         } else {
+        =         box_error_set(__FILE__, __LINE__, = pOp->p5,
+        =             =       pOp->p4.z);
+        =     }
     =     } else if (pOp->p5 !=3D 0) {
         =     static const char * const azType[] =3D { "NOT NULL", = "UNIQUE", "CHECK",
     =             =             =        "FOREIGN KEY" };
@@ -4318,8 +4322,8 @@ case OP_IdxInsert: = {        /* in2 */
     break;
 }

-/* Opcode: SInsert P1 P2 * * P5
- * Synopsis: space id =3D P1, key =3D = r[P2]
+/* Opcode: SInsert P1 P2 P3 * = P5
+ * Synopsis: space id =3D P1, key =3D = r[P3], on error goto P2

It would be = better to swap P2 and P3: if you did so, you wouldn=E2=80=99t 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=E2=80=99t = 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=E2=80=99)
<= br class=3D"">
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 =E2=80=98doc=E2=80=99. 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.

= --Apple-Mail=_54BB43A5-83DE-48BE-947A-19F4C708EE5B--