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 313152A923 for ; Sat, 22 Sep 2018 05:22:03 -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 uaCZ-07DaULY for ; Sat, 22 Sep 2018 05:22:03 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 539E02A596 for ; Sat, 22 Sep 2018 05:22:02 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: cleanup on failed creation operation 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> From: Imeev Mergen Message-ID: Date: Sat, 22 Sep 2018 12:21:59 +0300 MIME-Version: 1.0 In-Reply-To: <80242351-a576-dde6-034e-f19c899a9cc9@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Vladislav Shpilevoy , tarantool-patches@freelists.org 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 >> 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 > 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', '20', > 'index id = 0, >       space ptr = space'] >   - [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 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')