* [tarantool-patches] [PATCH v4 1/1] sql: return all generated ids via IPROTO
@ 2018-09-17 17:23 imeevma
2018-09-20 16:36 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 3+ messages in thread
From: imeevma @ 2018-09-17 17:23 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
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.
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);
+ for (uint64_t i = 0; i < db->generated_ids_count; ++i) {
+ size += db->generated_ids_array[i] >= 0 ?
+ 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;
}
buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
buf = mp_encode_uint(buf, changes);
+ buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+ 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/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..d1dce92 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,26 @@ 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;
uint32_t 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);
+ key = mp_decode_uint(data);
+ assert(key == SQL_INFO_GENERATED_IDS);
+ uint64_t count = mp_decode_array(data);
+ lua_newtable(L);
lua_pushinteger(L, row_count);
lua_setfield(L, -2, "rowcount");
+ 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);
}
static int
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
@@ -38,6 +38,7 @@
#include <small/mempool.h>
#include <msgpuck/msgpuck.h>
+#include "txn.h"
#include "diag.h"
#include "error.h"
#include "errcode.h"
@@ -52,6 +53,7 @@
enum {
SEQUENCE_HASH_SEED = 13U,
SEQUENCE_DATA_EXTENT_SIZE = 512,
+ SEQUENCE_GENERATED_IDS_MIN_LEN = 16,
};
/** Sequence state. */
@@ -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) {
+ 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)
{
@@ -194,6 +227,8 @@ sequence_next(struct sequence *seq, int64_t *result)
new_data) == light_sequence_end)
return -1;
*result = def->start;
+ if(save_value(*result) != 0)
+ return -1;
return 0;
}
old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +263,8 @@ done:
new_data, &old_data) == light_sequence_end)
unreachable();
*result = value;
+ if(save_value(*result) != 0)
+ return -1;
return 0;
overflow:
if (!def->cycle) {
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;
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.
+ */
+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.
@@ -2420,6 +2443,7 @@ sqlite3VdbeHalt(Vdbe * p)
* key constraints to hold up the transaction. This means a commit
* is required.
*/
+ move_generated_ids_in_db(db);
rc = box_txn_commit() ==
0 ? SQLITE_OK :
SQL_TARANTOOL_ERROR;
@@ -2751,6 +2775,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)
sqlite3DbFree(db, p->pVList);
sqlite3DbFree(db, p->pFree);
}
+ /* Free allocated for generated ids array. */
+ db->generated_ids_count = 0;
+ free(db->generated_ids_array);
+ db->generated_ids_array = NULL;
+
vdbeFreeOpArray(db, p->aOp, p->nOp);
sqlite3DbFree(db, p->aColName);
sqlite3DbFree(db, p->zSql);
diff --git a/src/box/txn.c b/src/box/txn.c
index 617ceb8..9af8e4a 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -150,6 +150,9 @@ txn_begin(bool is_autocommit)
txn->engine = NULL;
txn->engine_tx = NULL;
txn->psql_txn = NULL;
+ txn->generated_ids.size = 0;
+ txn->generated_ids.capacity = 0;
+ txn->generated_ids.array = NULL;
/* fiber_on_yield/fiber_on_stop initialized by engine on demand */
fiber_set_txn(fiber(), txn);
return txn;
@@ -353,6 +356,7 @@ txn_commit(struct txn *txn)
stailq_foreach_entry(stmt, &txn->stmts, next)
txn_stmt_unref_tuples(stmt);
+ free(txn->generated_ids.array);
TRASH(txn);
/** Free volatile txn memory. */
fiber_gc();
@@ -395,6 +399,7 @@ txn_rollback()
stailq_foreach_entry(stmt, &txn->stmts, next)
txn_stmt_unref_tuples(stmt);
+ free(txn->generated_ids.array);
TRASH(txn);
/** Free volatile txn memory. */
fiber_gc();
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..a77189f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -168,6 +168,12 @@ struct txn {
/** Commit and rollback triggers */
struct rlist on_commit, on_rollback;
struct sql_txn *psql_txn;
+ /* Save all generated ids. */
+ struct {
+ int64_t size;
+ int64_t capacity;
+ int64_t *array;
+ } generated_ids;
};
/* Pointer to the current transaction (if any) */
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index a0ba60f..70aba9d 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -73,7 +73,8 @@ while f1:status() ~= 'dead' do fiber.sleep(0) end
...
insert_res
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
select_res
---
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af474bc..dd3fee1 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -69,19 +69,23 @@ type(ret.rows[1])
-- Operation with rowcount result.
cn:execute('insert into test values (10, 11, NULL)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('delete from test where a = 5')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('insert into test values (11, 12, NULL), (12, 12, NULL), (13, 12, NULL)')
---
-- rowcount: 3
+- generated_ids: []
+ rowcount: 3
...
cn:execute('delete from test where a = 12')
---
-- rowcount: 3
+- generated_ids: []
+ rowcount: 3
...
-- SQL errors.
cn:execute('insert into not_existing_table values ("kek")')
@@ -330,7 +334,8 @@ cn:execute('select :value', parameters)
-- gh-2608 SQL iproto DDL
cn:execute('create table test2(id primary key, a, b, c)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
box.space.TEST2.name
---
@@ -338,7 +343,8 @@ box.space.TEST2.name
...
cn:execute('insert into test2 values (1, 1, 1, 1)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('select * from test2')
---
@@ -352,7 +358,8 @@ cn:execute('select * from test2')
...
cn:execute('create index test2_a_b_index on test2(a, b)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
#box.space.TEST2.index
---
@@ -360,7 +367,8 @@ cn:execute('create index test2_a_b_index on test2(a, b)')
...
cn:execute('drop table test2')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
box.space.TEST2
---
@@ -370,100 +378,121 @@ box.space.TEST2
-- Test CREATE [IF NOT EXISTS] TABLE.
cn:execute('create table test3(id primary key, a, b)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
-- Rowcount = 1, although two tuples were created:
-- for _space and for _index.
cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)')
---
-- rowcount: 3
+- generated_ids: []
+ rowcount: 3
...
cn:execute('create table if not exists test3(id primary key)')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
-- Test CREATE VIEW [IF NOT EXISTS] and
-- DROP VIEW [IF EXISTS].
cn:execute('create view test3_view(id) as select id from test3')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('create view if not exists test3_view(id) as select id from test3')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
cn:execute('drop view test3_view')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('drop view if exists test3_view')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
-- Test CREATE INDEX [IF NOT EXISTS] and
-- DROP INDEX [IF EXISTS].
cn:execute('create index test3_sec on test3(a, b)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('create index if not exists test3_sec on test3(a, b)')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
cn:execute('drop index test3_sec on test3')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('drop index if exists test3_sec on test3')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
-- Test CREATE TRIGGER [IF NOT EXISTS] and
-- DROP TRIGGER [IF EXISTS].
cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
cn:execute('drop trigger trig')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('drop trigger if exists trig')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
-- Test DROP TABLE [IF EXISTS].
-- Create more indexes, triggers and _truncate tuple.
cn:execute('create index idx1 on test3(a)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('create index idx2 on test3(b)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
box.space.TEST3:truncate()
---
...
cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)')
---
-- rowcount: 3
+- generated_ids: []
+ rowcount: 3
...
cn:execute('drop table test3')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
cn:execute('drop table if exists test3')
---
-- rowcount: 0
+- generated_ids: []
+ rowcount: 0
...
--
-- gh-2948: sql: remove unnecessary templates for binding
@@ -535,7 +564,8 @@ cn = remote.connect(box.cfg.listen)
...
cn:execute('create table test (id integer primary key, a integer, b integer)')
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
future1 = cn:execute('insert into test values (1, 1, 1)', nil, nil, {is_async = true})
---
@@ -548,7 +578,8 @@ future3 = cn:execute('insert into test values (2, 2, 2), (3, 3, 3)', nil, nil, {
...
future1:wait_result()
---
-- rowcount: 1
+- generated_ids: []
+ rowcount: 1
...
future2:wait_result()
---
@@ -558,7 +589,8 @@ future2:wait_result()
...
future3:wait_result()
---
-- rowcount: 2
+- generated_ids: []
+ rowcount: 2
...
future4 = cn:execute('select * from test', nil, nil, {is_async = true})
---
@@ -580,6 +612,81 @@ 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)')
+---
+- generated_ids: []
+ 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')
+---
+- generated_ids: []
+ 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('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]
+...
+cn:execute('create table test2 (id integer primary key, a integer)')
+---
+- generated_ids: []
+ rowcount: 1
+...
+cn:execute('drop table test2')
+---
+- generated_ids: []
+ 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..6517cde 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -201,6 +201,21 @@ 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('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
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO
2018-09-17 17:23 [tarantool-patches] [PATCH v4 1/1] sql: return all generated ids via IPROTO imeevma
@ 2018-09-20 16:36 ` Vladislav Shpilevoy
2018-09-26 16:08 ` Imeev Mergen
0 siblings, 1 reply; 3+ messages in thread
From: Vladislav Shpilevoy @ 2018-09-20 16:36 UTC (permalink / raw)
To: imeevma, tarantool-patches
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'.
>
> 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.
> + 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).
> + 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?
> }
> 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?
> + 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.
> + 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.
> 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 \.
9. You should not have this function. Ids must be stored in Vdbe.
> + */
> +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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH v4 1/1] sql: return all generated ids via IPROTO
2018-09-20 16:36 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-09-26 16:08 ` Imeev Mergen
0 siblings, 0 replies; 3+ messages in thread
From: Imeev Mergen @ 2018-09-26 16:08 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-26 16:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-17 17:23 [tarantool-patches] [PATCH v4 1/1] sql: return all generated ids via IPROTO imeevma
2018-09-20 16:36 ` [tarantool-patches] " Vladislav Shpilevoy
2018-09-26 16:08 ` Imeev Mergen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox