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
prev parent 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