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 B6DF52BE40 for ; Wed, 26 Sep 2018 12:08:09 -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 XVHs32bII4AW for ; Wed, 26 Sep 2018 12:08:09 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 AE87F2BE38 for ; Wed, 26 Sep 2018 12:08:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO References: <4041f49f7a62d12d9a6c8a9a452ed4cac267b1e3.1537204943.git.imeevma@gmail.com> From: Imeev Mergen Message-ID: <47c0c54e-1b2f-088c-6a0e-6d460b1cdcd6@tarantool.org> Date: Wed, 26 Sep 2018 19:08:06 +0300 MIME-Version: 1.0 In-Reply-To: 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! Thanks for review! Two patches placed below. On 09/20/2018 07:36 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! See 10 comments below. > > On 17/09/2018 19:23, imeevma@tarantool.org wrote: >> According to documentation some JDBC functions have an ability to >> return all ids that were generated in executed INSERT statement. >> This patch gives a way to implements such functinality. > > 1. Typos: 'way to implements', 'functinality'. Fixed. > >> >> Closes #2618 >> --- >> Branch: >> https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids >> Issue: https://github.com/tarantool/tarantool/issues/2618 >> >>   src/box/execute.c        |  17 ++++- >>   src/box/execute.h        |   1 + >>   src/box/lua/net_box.c    |  16 ++++- >>   src/box/sequence.c       |  37 ++++++++++ >>   src/box/sql/sqliteInt.h  |   4 ++ >>   src/box/sql/vdbeaux.c    |  29 ++++++++ >>   src/box/txn.c            |   5 ++ >>   src/box/txn.h            |   6 ++ >>   test/sql/errinj.result   |   3 +- >>   test/sql/iproto.result   | 171 >> ++++++++++++++++++++++++++++++++++++++--------- >>   test/sql/iproto.test.lua |  15 +++++ >>   11 files changed, 266 insertions(+), 38 deletions(-) >> >> diff --git a/src/box/execute.c b/src/box/execute.c >> index 24459b4..8e4bc80 100644 >> --- a/src/box/execute.c >> +++ b/src/box/execute.c >> @@ -641,14 +641,27 @@ err: >>               goto err; >>           int changes = sqlite3_changes(db); >>           int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + >> -               mp_sizeof_uint(changes); >> +               mp_sizeof_uint(changes) + >> +               mp_sizeof_uint(SQL_INFO_GENERATED_IDS) + >> +               mp_sizeof_array(db->generated_ids_count); > > 2. When you use '_count', the noun should be in the singular. Fixed. > >> +        for (uint64_t i = 0; i < db->generated_ids_count; ++i) { >> +            size += db->generated_ids_array[i] >= 0 ? > > 3. Type can be omitted in a variable name (no _array, but s). Fixed. > >> + mp_sizeof_uint(db->generated_ids_array[i]) : >> +                mp_sizeof_int(db->generated_ids_array[i]); >> +        } >>           char *buf = obuf_alloc(out, size); >>           if (buf == NULL) { >>               diag_set(OutOfMemory, size, "obuf_alloc", "buf"); >> -            goto err; > > 4. Why? I was wrong, fixed. > >>           } >>           buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT); >>           buf = mp_encode_uint(buf, changes); >> +        buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS); > > 5. I do not think we should send empty array. Maybe it is better to > omit the key in such a case? Done. > >> +        buf = mp_encode_array(buf, db->generated_ids_count); >> +        for (uint64_t i = 0; i < db->generated_ids_count; ++i) { >> +            buf = db->generated_ids_array[i] < 0 ? >> +                  mp_encode_int(buf, db->generated_ids_array[i]) : >> +                  mp_encode_uint(buf, db->generated_ids_array[i]); >> +        } >>       } >>       iproto_reply_sql(out, &header_svp, response->sync, schema_version, >>                keys); >> diff --git a/src/box/sequence.c b/src/box/sequence.c >> index 35b7605..234509c 100644 >> --- a/src/box/sequence.c >> +++ b/src/box/sequence.c >> @@ -178,6 +180,37 @@ sequence_update(struct sequence *seq, int64_t >> value) >>       return 0; >>   } >>   +/* >> + * Save generated value into txn. >> + * >> + * \param value value to save. >> + * \retval 0 Success. >> + * \retval -1 Error. >> + */ >> +static inline int >> +save_value(int64_t value) >> +{ >> +    struct txn *txn = in_txn(); >> +    if (txn == NULL) >> +        return 0; >> +    if (txn->generated_ids.size == txn->generated_ids.capacity) { > > 6. Why do you store generated ids in box txn instead of vdbe? As I know, > after commit/rollback txn object is freed so it does not matter where do > you store ids array - on the heap or on a region - txn.generated_ids > attribute will be invalid after commit/rollback. What is more, we do not > need ids from non-SQL requests - they are never used. > > Last time we discussed that you should store result of sequence_next from > OP_NextAutoincValue. And at the same place you can save the result into > Vdbe generated ids array. This will work the same way as your current > implementation, but will not slow down non-SQL requests. Done. > >> +        txn->generated_ids.capacity = txn->generated_ids.capacity > 0 ? >> +                          txn->generated_ids.capacity * 2 : >> +                          SEQUENCE_GENERATED_IDS_MIN_LEN; >> +        uint64_t capacity = txn->generated_ids.capacity * >> +                    sizeof(*txn->generated_ids.array); >> +        txn->generated_ids.array = realloc(txn->generated_ids.array, >> +                           capacity); >> +        if (txn->generated_ids.array == NULL) { >> +            diag_set(OutOfMemory, txn->generated_ids.capacity, >> +                 "realloc", "txn->generated_ids.array"); >> +            return -1; >> +        } >> +    } >> +    txn->generated_ids.array[txn->generated_ids.size++] = value; >> +    return 0; >> +} >> + >>   int >>   sequence_next(struct sequence *seq, int64_t *result) >>   { >> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h >> index 1d32c9a..7d41ece 100644 >> --- a/src/box/sql/sqliteInt.h >> +++ b/src/box/sql/sqliteInt.h >> @@ -1525,6 +1525,10 @@ struct sqlite3 { >>       u8 mTrace;        /* zero or more SQLITE_TRACE flags */ >>       u32 magic;        /* Magic number for detect library misuse */ >>       int nChange;        /* Value returned by sqlite3_changes() */ >> +    /* Number of generated ids. */ >> +    uint64_t generated_ids_count; >> +    /* Array of generated ids. */ >> +    int64_t *generated_ids_array; > > 7. Do not store ids in multiple places, and moreover in sqlite3 that > will be > removed in future. Use struct Vdbe. Ids array is per-request attribute. Done. > >>       int nTotalChange;    /* Value returned by >> sqlite3_total_changes() */ >>       int aLimit[SQLITE_N_LIMIT];    /* Limits */ >>       int nMaxSorterMmap;    /* Maximum size of regions mapped by >> sorter */ >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index 3b0c90c..7b5d6b9 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -2303,6 +2303,29 @@ sql_savepoint(Vdbe *p, const char *zName) >>   } >>     /* >> + * Move array of generated ids into db. >> + * >> + * \param db db to save array to. >> + * \retval 0 Success. >> + * \retval -1 Error. > > 8. Use @, not \. Fixed. > > 9. You should not have this function. Ids must be stored in Vdbe. Done. > >> + */ >> +static inline int >> +move_generated_ids_in_db(sqlite3 *db) >> +{ >> +    struct txn *txn = in_txn(); >> +    if (txn == NULL) >> +        return 0; >> +    uint64_t size = txn->generated_ids.size * >> +            sizeof(*txn->generated_ids.array); >> +    db->generated_ids_count = txn->generated_ids.size; >> +    db->generated_ids_array = malloc(size); >> +    if (db->generated_ids_array == NULL) >> +        return -1; >> +    memcpy(db->generated_ids_array, txn->generated_ids.array, size); >> +    return 0; >> +} >> + >> +/* >>    * This routine is called the when a VDBE tries to halt.  If the VDBE >>    * has made changes and is in autocommit mode, then commit those >>    * changes.  If a rollback is needed, then do the rollback. > 10. I still think that we should not realloc a huge monolith array of > ids. > My proposal is to use a list of small arrays. When a last chunk in the > list > becomes full, we allocate a new chunk, append it to the list and use > it for > next ids. But you can discuss it with Kirill if you do not agree. Done. New patch: sql: move autoincrement in vdbe commit 45a5c014743f9c116c632fd5d6007114d6d45602 Author: Mergen Imeev Date:   Thu Sep 20 21:54:32 2018 +0300     sql: move autoincrement in vdbe     This patch expands changes made in issue #2981. Now NULL in     primary key always replaced by generated value during VDBE     execution. It allows us to get all generated ids with ease.     Part of #2618     Closes #3670 diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c index 03f4e1b..987e3f4 100644 --- a/src/box/sql/insert.c +++ b/src/box/sql/insert.c @@ -655,9 +655,7 @@ sqlite3Insert(Parse * pParse,    /* Parser context */              if (j < 0 || nColumn == 0                  || (pColumn && j >= pColumn->nId)) {                  if (i == (int) autoinc_fieldno) { -                    sqlite3VdbeAddOp2(v, -                              OP_NextAutoincValue, -                              pTab->def->id, +                    sqlite3VdbeAddOp2(v, OP_Null, 0,                                iRegStore);                      continue;                  } @@ -669,77 +667,35 @@ sqlite3Insert(Parse * pParse,    /* Parser context */                                dflt,                                iRegStore);              } else if (useTempTable) { -                if (i == (int) autoinc_fieldno) { -                    int regTmp = ++pParse->nMem; -                    /* Emit code which doesn't override -                     * autoinc-ed value with select result -                     * in case if result is NULL value. -                     */ -                    sqlite3VdbeAddOp3(v, OP_Column, srcTab, -                              j, regTmp); -                    sqlite3VdbeAddOp2(v, OP_FCopy, regTmp, -                              iRegStore); -                    sqlite3VdbeChangeP3(v, -1, -                                OPFLAG_SAME_FRAME | -                                OPFLAG_NOOP_IF_NULL); -                } else { -                    sqlite3VdbeAddOp3(v, OP_Column, srcTab, -                              j, iRegStore); -                } +                sqlite3VdbeAddOp3(v, OP_Column, srcTab, +                          j, iRegStore);              } else if (pSelect) {                  if (regFromSelect != regData) { -                    if (i == (int) autoinc_fieldno) { -                        /* Emit code which doesn't override -                         * autoinc-ed value with select result -                         * in case that result is NULL -                         */ -                        sqlite3VdbeAddOp2(v, OP_FCopy, -                                  regFromSelect -                                  + j, -                                  iRegStore); -                        sqlite3VdbeChangeP3(v, -1, -                                    OPFLAG_SAME_FRAME -                                    | -                                    OPFLAG_NOOP_IF_NULL); -                    } else { -                        sqlite3VdbeAddOp2(v, OP_SCopy, -                                  regFromSelect -                                  + j, -                                  iRegStore); -                    } +                    sqlite3VdbeAddOp2(v, OP_SCopy, +                              regFromSelect + j, +                              iRegStore);                  }              } else { -                  if (i == (int) autoinc_fieldno) {                      if (pList->a[j].pExpr->op == TK_NULL) {                          sqlite3VdbeAddOp2(v, OP_Null, 0, iRegStore);                          continue;                      } - -                    if (pList->a[j].pExpr->op == -                        TK_REGISTER) { -                        /* Emit code which doesn't override -                         * autoinc-ed value with select result -                         * in case that result is NULL -                         */ -                        sqlite3VdbeAddOp2(v, OP_FCopy, -                                  pList->a[j]. -                                  pExpr->iTable, -                                  iRegStore); -                        sqlite3VdbeChangeP3(v, -1, -                                    OPFLAG_SAME_FRAME -                                    | -                                    OPFLAG_NOOP_IF_NULL); -                        continue; -                    }                  } -                  sqlite3ExprCode(pParse, pList->a[j].pExpr,                          iRegStore);              }          }          /* +         * Replace NULL in PK with autoincrement by +         * generated value. +         */ +        if ((int) autoinc_fieldno >= 0) { +            sqlite3VdbeAddOp2(v, OP_NextAutoincValue, pTab->def->id, +                      regData + (int) autoinc_fieldno); +        } +        /*           * Generate code to check constraints and process           * final insertion.           */ diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 00ffb0c..3d0548b 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1156,6 +1156,10 @@ case OP_NextAutoincValue: {      assert(pOp->p1 > 0);      assert(pOp->p2 > 0); +    pIn2 = &p->aMem[pOp->p2]; +    if ((pIn2->flags & MEM_Null) == 0) +        break; +      struct space *space = space_by_id(pOp->p1);      if (space == NULL) {          rc = SQL_TARANTOOL_ERROR; @@ -3730,44 +3734,6 @@ case OP_NextIdEphemeral: {      break;  } -/* Opcode: FCopy P1 P2 P3 * * - * Synopsis: reg[P2@cur_frame]= reg[P1@root_frame(OPFLAG_SAME_FRAME)] - * - * Copy integer value of register P1 in root frame in to register P2 of current - * frame. If current frame is topmost - copy within signle frame. - * Source register must hold integer value. - * - * If P3's flag OPFLAG_SAME_FRAME is set, do shallow copy of register within - * same frame, still making sure the value is integer. - * - * If P3's flag OPFLAG_NOOP_IF_NULL is set, then do nothing if reg[P1] is NULL - */ -case OP_FCopy: {     /* out2 */ -    VdbeFrame *pFrame; -    Mem *pIn1, *pOut; -    if (p->pFrame && ((pOp->p3 & OPFLAG_SAME_FRAME) == 0)) { -        for(pFrame=p->pFrame; pFrame->pParent; pFrame=pFrame->pParent); -        pIn1 = &pFrame->aMem[pOp->p1]; -    } else { -        pIn1 = &aMem[pOp->p1]; -    } - -    if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { -        pOut = &aMem[pOp->p2]; -        if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; -        /* Flag is set and register is NULL -> do nothing  */ -    } else { -        assert(memIsValid(pIn1)); -        assert(pIn1->flags &  MEM_Int); - -        pOut = &aMem[pOp->p2]; -        MemSetTypeFlag(pOut, MEM_Int); - -        pOut->u.i = pIn1->u.i; -    } -    break; -} -  /* Opcode: Delete P1 P2 P3 P4 P5   *   * Delete the record at which the P1 cursor is currently pointing. diff --git a/test/sql/gh-2981-check-autoinc.result b/test/sql/gh-2981-check-autoinc.result index b0f55e6..40bb71c 100644 --- a/test/sql/gh-2981-check-autoinc.result +++ b/test/sql/gh-2981-check-autoinc.result @@ -19,6 +19,9 @@ box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEG  box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));");  ---  ... +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));"); +--- +...  box.sql.execute("insert into t1 values (18, null);")  ---  ... @@ -47,6 +50,13 @@ box.sql.execute("insert into t3(s2) values (null)")  ---  - error: 'CHECK constraint failed: T3'  ... +box.sql.execute("insert into t4 values (18, null);") +--- +... +box.sql.execute("insert into t4 values (null, null);") +--- +- error: 'CHECK constraint failed: T4' +...  box.sql.execute("DROP TABLE t1")  ---  ... @@ -56,3 +66,35 @@ box.sql.execute("DROP TABLE t2")  box.sql.execute("DROP TABLE t3")  ---  ... +box.sql.execute("DROP TABLE t4") +--- +... +-- +-- gh-3670: Assertion with large number in autoincrement column. +-- +box.sql.execute("CREATE TABLE t5 (s1 INTEGER PRIMARY KEY AUTOINCREMENT);") +--- +... +box.sql.execute("insert into t5 values (2147483647);") +--- +... +box.sql.execute("insert into t5 select max(s1)*max(s1)*max(s1) from t5;") +--- +- error: datatype mismatch +... +box.sql.execute("CREATE TABLE t6 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 CHAR);") +--- +... +box.sql.execute("insert into t6 values (1, 'a');") +--- +... +box.sql.execute("insert into t6 select s2, s2 from t6;") +--- +- error: datatype mismatch +... +box.sql.execute("DROP TABLE t5") +--- +... +box.sql.execute("DROP TABLE t6") +--- +... diff --git a/test/sql/gh-2981-check-autoinc.test.lua b/test/sql/gh-2981-check-autoinc.test.lua index 98a5fb4..6fabf0e 100644 --- a/test/sql/gh-2981-check-autoinc.test.lua +++ b/test/sql/gh-2981-check-autoinc.test.lua @@ -7,6 +7,7 @@ box.cfg{}  box.sql.execute("CREATE TABLE t1 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));");  box.sql.execute("CREATE TABLE t2 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19 AND s1 <> 25));");  box.sql.execute("CREATE TABLE t3 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 < 10));"); +box.sql.execute("CREATE TABLE t4 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 INTEGER, CHECK (s1 <> 19));");  box.sql.execute("insert into t1 values (18, null);")  box.sql.execute("insert into t1(s2) values (null);") @@ -19,7 +20,24 @@ box.sql.execute("insert into t2(s2) values (null);")  box.sql.execute("insert into t3 values (9, null)")  box.sql.execute("insert into t3(s2) values (null)") +box.sql.execute("insert into t4 values (18, null);") +box.sql.execute("insert into t4 values (null, null);") +  box.sql.execute("DROP TABLE t1")  box.sql.execute("DROP TABLE t2")  box.sql.execute("DROP TABLE t3") +box.sql.execute("DROP TABLE t4") + +-- +-- gh-3670: Assertion with large number in autoincrement column. +-- +box.sql.execute("CREATE TABLE t5 (s1 INTEGER PRIMARY KEY AUTOINCREMENT);") +box.sql.execute("insert into t5 values (2147483647);") +box.sql.execute("insert into t5 select max(s1)*max(s1)*max(s1) from t5;") + +box.sql.execute("CREATE TABLE t6 (s1 INTEGER PRIMARY KEY AUTOINCREMENT, s2 CHAR);") +box.sql.execute("insert into t6 values (1, 'a');") +box.sql.execute("insert into t6 select s2, s2 from t6;") +box.sql.execute("DROP TABLE t5") +box.sql.execute("DROP TABLE t6") New patch: sql: return all generated ids via IPROTO commit eba2e082f177a6c557731645a40661dce2422d14 Author: Mergen Imeev Date:   Mon Sep 24 22:28:43 2018 +0300     sql: return all generated ids via IPROTO     According to documentation some JDBC functions have an ability to     return all ids that were generated in executed INSERT statement.     This patch gives a way to implement such functionality.     Closes #2618 diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..f06e4fb 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@  #include "schema.h"  #include "port.h"  #include "tuple.h" +#include "sql/vdbeInt.h"  const char *sql_type_strs[] = {      NULL, @@ -640,13 +641,45 @@ err:          if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)              goto err;          int changes = sqlite3_changes(db); +        struct rlist *id_list = &((struct Vdbe *)stmt)->generated_ids; +        uint64_t count = 0;          int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +                 mp_sizeof_uint(changes); +        if (rlist_empty(id_list) == 0) { +            struct id_array *id_array; +            rlist_foreach_entry(id_array, id_list, link) { +                int64_t *ids = id_array->ids; +                for (uint64_t i = 0; i < id_array->count; ++i) { +                    size += ids[i] >= 0 ? +                        mp_sizeof_uint(ids[i]) : +                        mp_sizeof_int(ids[i]); +                } +                count += id_array->count; +            } +        } +        if (count > 0) { +            size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) + +                mp_sizeof_array(count); +        } +          char *buf = obuf_alloc(out, size);          if (buf == NULL) {              diag_set(OutOfMemory, size, "obuf_alloc", "buf");              goto err;          } +        if (count > 0) { +            buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS); +            buf = mp_encode_array(buf, count); +            struct id_array *id_array; +            rlist_foreach_entry(id_array, id_list, link) { +                int64_t *ids = id_array->ids; +                for (uint64_t i = 0; i < id_array->count; ++i) { +                    buf = ids[i] >= 0 ? +                        mp_encode_uint(buf, ids[i]) : +                        mp_encode_int(buf, ids[i]); +                } +            } +        }          buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);          buf = mp_encode_uint(buf, changes);      } diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..614d3d0 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" {  /** Keys of IPROTO_SQL_INFO map. */  enum sql_info_key {      SQL_INFO_ROW_COUNT = 0, +    SQL_INFO_GENERATED_IDS = 1,      sql_info_key_MAX,  }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index a928a4c..9f4088a 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -668,14 +668,30 @@ static void  netbox_decode_sql_info(struct lua_State *L, const char **data)  {      uint32_t map_size = mp_decode_map(data); -    /* Only SQL_INFO_ROW_COUNT is available. */      assert(map_size == 1);      (void) map_size; +    lua_newtable(L);      uint32_t key = mp_decode_uint(data); +    /* +     * If SQL_INFO_GENERATED_IDS in data then it should be +     * just before SQL_INFO_ROW_COUNT. +     */ +    if (key == SQL_INFO_GENERATED_IDS) { +        uint64_t count = mp_decode_array(data); +        assert (count > 0); +        lua_createtable(L, 0, count); +        lua_setfield(L, -2, "generated_ids"); +        lua_getfield(L, -1, "generated_ids"); +        for (uint32_t j = 0; j < count; ++j) { +            int64_t value = mp_decode_uint(data); +            lua_pushinteger(L, value); +            lua_rawseti(L, -2, j + 1); +        } +        lua_pop(L, 1); +        key = mp_decode_uint(data); +    }      assert(key == SQL_INFO_ROW_COUNT); -    (void) key;      uint32_t row_count = mp_decode_uint(data); -    lua_createtable(L, 0, 1);      lua_pushinteger(L, row_count);      lua_setfield(L, -2, "rowcount");  } diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 3d0548b..53544fc 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -1177,6 +1177,22 @@ case OP_NextAutoincValue: {      pOut->flags = MEM_Int;      pOut->u.i = value; +    /* Save new id to return it via IProto. */ +    struct id_array *id_array; +    if (rlist_empty(&p->generated_ids) != 0) { +        id_array = malloc(sizeof(*id_array)); +        memset(id_array, 0, sizeof(*id_array)); +        rlist_add_entry(&p->generated_ids, id_array, link); +    } else { +        id_array = rlist_last_entry(&p->generated_ids, struct id_array, +                        link); +    } +    if (id_array->count == GENERATED_ID_ARRAY_SIZE) { +        id_array = malloc(sizeof(*id_array)); +        memset(id_array, 0, sizeof(*id_array)); +        rlist_add_entry(&p->generated_ids, id_array, link); +    } +    id_array->ids[id_array->count++] = value;      break;  } diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ce97f49..95f6c60 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -331,6 +331,25 @@ struct ScanStatus {      char *zName;        /* Name of table or index */  }; +enum +{ +    /** Capacity of array of ids. */ +    GENERATED_ID_ARRAY_SIZE = 16, +}; + +/** + * An element of list of generated ids. + */ +struct id_array +{ +    /** A link in a generated id array list. */ +    struct rlist link; +    /** Number of elements in array. */ +    uint64_t count; +    /** Array of generated ids. */ +    int64_t ids[GENERATED_ID_ARRAY_SIZE]; +}; +  /*   * An instance of the virtual machine.  This structure contains the complete   * state of the virtual machine. @@ -355,6 +374,9 @@ struct Vdbe {      i64 nFkConstraint;    /* Number of imm. FK constraints this VM */      uint32_t schema_ver;    /* Schema version at the moment of VDBE creation. */ +    /* List of arrays of generated ids. */ +    struct rlist generated_ids; +      /*       * In recursive triggers we can execute INSERT/UPDATE OR IGNORE       * statements. If IGNORE error action happened inside a trigger, diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 54edf1b..a177a18 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -57,6 +57,8 @@ sqlite3VdbeCreate(Parse * pParse)          return 0;      memset(p, 0, sizeof(Vdbe));      p->db = db; +    /* Init list of generated ids. */ +    rlist_create(&p->generated_ids);      if (db->pVdbe) {          db->pVdbe->pPrev = p;      } @@ -2746,6 +2748,12 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)      vdbeFreeOpArray(db, p->aOp, p->nOp);      sqlite3DbFree(db, p->aColName);      sqlite3DbFree(db, p->zSql); +    while (rlist_empty(&p->generated_ids) == 0) { +        struct id_array *id_array = +            rlist_shift_entry(&p->generated_ids, struct id_array, +                      link); +        free(id_array); +    }  #ifdef SQLITE_ENABLE_STMT_SCANSTATUS      {          int i; diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af474bc..70cca32 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -580,6 +580,86 @@ cn:close()  box.sql.execute('drop table test')  ---  ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return all ids generated in current INSERT statement. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +--- +... +cn = remote.connect(box.cfg.listen) +--- +... +cn:execute('insert into test values (1, 1)') +--- +- rowcount: 1 +... +cn:execute('insert into test values (null, 2)') +--- +- generated_ids: +  - 2 +  rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1') +--- +- rowcount: 1 +... +cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)') +--- +- generated_ids: +  - 101 +  - 121 +  rowcount: 4 +... +cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)') +--- +- generated_ids: +  - 122 +  - 123 +  - 124 +  - 125 +  - 126 +  rowcount: 5 +... +cn:execute('replace into test values (null, 1), (null, 1)') +--- +- generated_ids: +  - 127 +  - 128 +  rowcount: 2 +... +cn:execute('select * from test') +--- +- metadata: +  - name: ID +  - name: A +  rows: +  - [1, 11] +  - [2, 2] +  - [100, 1] +  - [101, 1] +  - [120, 1] +  - [121, 1] +  - [122, 1] +  - [123, 1] +  - [124, 1] +  - [125, 1] +  - [126, 1] +  - [127, 1] +  - [128, 1] +... +cn:execute('create table test2 (id integer primary key, a integer)') +--- +- rowcount: 1 +... +cn:execute('drop table test2') +--- +- rowcount: 1 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +...  box.schema.user.revoke('guest', 'read,write,execute', 'universe')  ---  ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index 220331b..fea51c9 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -201,6 +201,22 @@ future4:wait_result()  cn:close()  box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return all ids generated in current INSERT statement. +box.sql.execute('create table test (id integer primary key autoincrement, a integer)') +cn = remote.connect(box.cfg.listen) +cn:execute('insert into test values (1, 1)') +cn:execute('insert into test values (null, 2)') +cn:execute('update test set a = 11 where id == 1') +cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)') +cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)') +cn:execute('replace into test values (null, 1), (null, 1)') +cn:execute('select * from test') +cn:execute('create table test2 (id integer primary key, a integer)') +cn:execute('drop table test2') +cn:close() +box.sql.execute('drop table test') +  box.schema.user.revoke('guest', 'read,write,execute', 'universe')  space = nil