Tarantool development patches archive
 help / color / mirror / Atom feed
From: Imeev Mergen <imeevma@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO
Date: Wed, 26 Sep 2018 19:08:06 +0300	[thread overview]
Message-ID: <47c0c54e-1b2f-088c-6a0e-6d460b1cdcd6@tarantool.org> (raw)
In-Reply-To: <b255ff22-f157-903c-8a79-325f68f839f3@tarantool.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 '<noun>_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 <name>_array, but <name>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 <imeevma@gmail.com>
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 <imeevma@gmail.com>
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

      reply	other threads:[~2018-09-26 16:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-17 17:23 [tarantool-patches] " imeevma
2018-09-20 16:36 ` [tarantool-patches] " Vladislav Shpilevoy
2018-09-26 16:08   ` Imeev Mergen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=47c0c54e-1b2f-088c-6a0e-6d460b1cdcd6@tarantool.org \
    --to=imeevma@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox