[tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO
Imeev Mergen
imeevma at tarantool.org
Fri Nov 2 21:52:10 MSK 2018
Hi! Thank you for review! My answer, diff between patches and new
patch below. Changes were made only in commit named by "sql:
return all generated ids via IPROTO". After changes were made and
diff saved I rebased this branch to current 2.1. After this new
patch description were formed, so it is a bit different from
old patch with applied diff.
On 10/30/18 5:30 PM, n.pettik wrote:
>> On 29 Oct 2018, at 20:33,imeevma at 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 implement such functionality.
> I guess you should be more specific in describing such things.
> I don’t ask you to dive into details, but explaining main idea of
> implementation would definitely help reviewer to do his/her job.
A bit extended this message. Not sure if it is enough.
>> Closes #2618
>> —
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 24459b4..ebf3962 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/vdbe.h"
>>
>>
>> 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,
> I vote for _AUTOINCREMENT_IDS, see comments below.
> Anyway, in JDBC it is called GENERATED_KEYS. So, we either
> follow JDBC convention or use our naming. In the latter case,
> AUTOINCREMENT_IDS sounds more solid.
Renamed to SQL_INFO_AUTOINCREMENT_IDS.
>> sql_info_key_MAX,
>> };
>>
>> /**
>> + * Append a new sequence value to Vdbe. List of automatically
>> + * generated identifiers is returned as metadata of SQL response.
>> + *
>> + * @param Vdbe VDBE to save id in.
>> + * @param id ID to save in VDBE.
>> + * @retval 0 Success.
>> + * @retval -1 Error.
> It looks terrible. Does this function really need comment at all?
Removed.
>> + */
>> +int
>> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
>>
>> +
>> +int
>> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
>> +{
>> + assert(vdbe != NULL);
>> + struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
>> + if (id_entry == NULL) {
>> + diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”);
> Out of 80.
>
>> + return -1;
>> + }
>> + id_entry->id = id;
>> + stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
>> + return 0;
>> +}
>> +
>> /*
>> * Execute as much of a VDBE program as we can.
>> * This is the core of sqlite3_step().
>> @@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
>> * If there is no active transaction, raise an error.
>> */
>> case OP_TransactionCommit: {
>> - if (box_txn()) {
>> - if (box_txn_commit() != 0) {
>> + struct txn *txn = in_txn();
>> + if (txn != NULL) {
>> + if (txn_commit(txn) != 0) {
> From first sigh it doesn’t look like diff related to patch.
> Add comment pointing out that txn_commit() doesn’t
> invoke fiber_gc() anymore and VDBE takes care of it.
Extended comment of this opcode.
>> /**
>> * Prepare given VDBE to execution: initialize structs connected
>> @@ -197,6 +198,15 @@ sql_alloc_txn();
>> int
>> sql_vdbe_prepare(struct Vdbe *vdbe);
>>
>> +/**
>> + * Return pointer to list of generated ids.
>> + *
>> + * @param vdbe VDBE to get list of generated ids from.
>> + * @retval List of generated ids.
>> + */
>> +struct stailq *
>> +vdbe_generated_ids(struct Vdbe *vdbe);
> As for me, “generated_ids” seems to be too general name…
> I would call it “autoincrement_ids" or sort of.
Renamed to autoinc_id_list here.
>> +
>> int sqlite3VdbeAddOp0(Vdbe *, int);
>> int sqlite3VdbeAddOp1(Vdbe *, int, int);
>> int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index ce97f49..7a7d3de 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -367,6 +367,11 @@ struct Vdbe {
>> struct sql_txn *psql_txn;
>> /** The auto-commit flag. */
>> bool auto_commit;
>> + /**
>> + * List of ids generated in current VDBE. It is returned
>> + * as metadata of SQL response.
>> + */
>> + struct stailq generated_ids;
> Again: “autoincrement_ids”. Term “generated_ids” can be confused
> with entity ids. For example, during execution of <CREATE TABLE>
> statement VDBE operates on space id, index id etc (IMHO).
Renamed to autoinc_id_list here.
>> /* When allocating a new Vdbe object, all of the fields below should be
>> * initialized to zero or NULL
>> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
>> index 6e42d05..ae73f25 100644
>> --- a/src/box/sql/vdbeaux.c
>> +++ b/src/box/sql/vdbeaux.c
>> @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
>> return 0;
>> memset(p, 0, sizeof(Vdbe));
>> p->db = db;
>> + stailq_create(&p->generated_ids);
>> if (db->pVdbe) {
>> db->pVdbe->pPrev = p;
>> }
>>
>> @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
>> * key constraints to hold up the transaction. This means a commit
>> * is required.
>> */
>> - rc = box_txn_commit() ==
>> - 0 ? SQLITE_OK :
>> - SQL_TARANTOOL_ERROR;
>> + rc = (in_txn() == NULL ||
>> + txn_commit(in_txn()) == 0) ?
>> + SQLITE_OK : SQL_TARANTOOL_ERROR;
>> closeCursorsAndFree(p);
>> }
>> if (rc == SQLITE_BUSY && !p->pDelFrame) {
>> @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
>> }
>> p->magic = VDBE_MAGIC_DEAD;
>> p->db = 0;
>> + if (in_txn() == NULL)
>> + fiber_gc();
> Describe this change (in code, obviously):
> “VDBE is responsible for releasing region …"
> and so on.
Added comment.
>> sqlite3DbFree(db, p);
>> }
>>
>> diff --git a/src/box/txn.h b/src/box/txn.h
>> index e74c14d..87b55da 100644
>> --- a/src/box/txn.h
>> +++ b/src/box/txn.h
>> @@ -49,6 +49,7 @@ struct engine;
>> struct space;
>> struct tuple;
>> struct xrow_header;
>> +struct Vdbe;
>>
>> enum {
>> /**
>> @@ -117,6 +118,19 @@ struct sql_txn {
>> * VDBE to the next in the same transaction.
>> */
>> uint32_t fk_deferred_count;
>> + /** Current VDBE. */
>> + struct Vdbe *vdbe;
>> +};
>> +
>> +/**
>> + * An element of list of autogenerated ids, being returned as SQL
>> + * response metadata.
>> + */
>> +struct id_entry {
> “id_entry” is too common name, be more specific pls.
Renamed to struct autoinc_id_entry. Still, name of variable is
'id_entry'.
>> + /** A link in a generated id list. */
>> + struct stailq_entry link;
>> + /** Generated id. */
>> + int64_t id;
> These comments are too obvious, they only contaminate
> source code.
Removed these comments.
>> };
>>
>> /**
>> * FFI bindings: do not throw exceptions, do not accept extra
>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index af474bc..af4fc12 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -580,6 +580,98 @@ 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.
> Add pls case where generated ids are less than 0.
Added.
*Diff between versions:*
diff --git a/src/box/execute.c b/src/box/execute.c
index ebf3962..64c0be6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -638,24 +638,24 @@ err:
} else {
keys = 1;
assert(port_tuple->size == 0);
- struct stailq *generated_ids =
- vdbe_generated_ids((struct Vdbe *)stmt);
- uint32_t map_size = stailq_empty(generated_ids) ? 1 : 2;
+ struct stailq *autoinc_id_list =
+ vdbe_autoinc_id_list((struct Vdbe *)stmt);
+ uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
goto err;
uint64_t id_count = 0;
int changes = sqlite3_changes(db);
int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
mp_sizeof_uint(changes);
- if (!stailq_empty(generated_ids)) {
- struct id_entry *id_entry;
- stailq_foreach_entry(id_entry, generated_ids, link) {
+ if (!stailq_empty(autoinc_id_list)) {
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
size += id_entry->id >= 0 ?
mp_sizeof_uint(id_entry->id) :
mp_sizeof_int(id_entry->id);
id_count++;
}
- size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
+ size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
mp_sizeof_array(id_count);
}
char *buf = obuf_alloc(out, size);
@@ -665,11 +665,11 @@ err:
}
buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
buf = mp_encode_uint(buf, changes);
- if (!stailq_empty(generated_ids)) {
- buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+ if (!stailq_empty(autoinc_id_list)) {
+ buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
buf = mp_encode_array(buf, id_count);
- struct id_entry *id_entry;
- stailq_foreach_entry(id_entry, generated_ids, link) {
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
buf = id_entry->id >= 0 ?
mp_encode_uint(buf, id_entry->id) :
mp_encode_int(buf, id_entry->id);
diff --git a/src/box/execute.h b/src/box/execute.h
index 614d3d0..77bfd79 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,7 +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_AUTOINCREMENT_IDS = 1,
sql_info_key_MAX,
};
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 5a137e0..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -680,11 +680,11 @@ netbox_decode_sql_info(struct lua_State *L, const
char **data)
lua_setfield(L, -2, "rowcount");
/*
* If data have two elements then second is
- * SQL_INFO_GENERATED_IDS.
+ * SQL_INFO_AUTOINCREMENT_IDS.
*/
if (map_size == 2) {
key = mp_decode_uint(data);
- assert(key == SQL_INFO_GENERATED_IDS);
+ assert(key == SQL_INFO_AUTOINCREMENT_IDS);
(void)key;
uint64_t count = mp_decode_array(data);
assert(count > 0);
@@ -695,7 +695,7 @@ netbox_decode_sql_info(struct lua_State *L, const
char **data)
luaL_pushint64(L, id);
lua_rawseti(L, -2, j + 1);
}
- lua_setfield(L, -2, "generated_ids");
+ lua_setfield(L, -2, "autoincrement_ids");
}
}
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 9a87a56..c9828c0 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -196,7 +196,7 @@ sequence_next(struct sequence *seq, int64_t *result)
return -1;
*result = def->start;
if (txn_vdbe() != NULL &&
- vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+ vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
return -1;
return 0;
}
@@ -233,7 +233,7 @@ done:
unreachable();
*result = value;
if (txn_vdbe() != NULL &&
- vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
+ vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
return -1;
return 0;
overflow:
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 7180ead..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -140,17 +140,8 @@ sequence_next(struct sequence *seq, int64_t *result);
int
access_check_sequence(struct sequence *seq);
-/**
- * Append a new sequence value to Vdbe. List of automatically
- * generated identifiers is returned as metadata of SQL response.
- *
- * @param Vdbe VDBE to save id in.
- * @param id ID to save in VDBE.
- * @retval 0 Success.
- * @retval -1 Error.
- */
int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
/**
* Create an iterator over sequence data.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1e732a9..dd763e7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -579,22 +579,24 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
}
struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe)
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
{
- return &vdbe->generated_ids;
+ return &vdbe->autoinc_id_list;
}
int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
{
assert(vdbe != NULL);
- struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
+ struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+ sizeof(*id_entry));
if (id_entry == NULL) {
- diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry");
+ diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+ "id_entry");
return -1;
}
id_entry->id = id;
- stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
+ stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
return 0;
}
@@ -3014,6 +3016,9 @@ case OP_TransactionBegin: {
*
* Commit Tarantool's transaction.
* If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
*/
case OP_TransactionCommit: {
struct txn *txn = in_txn();
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index ed7fb2f..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -205,7 +205,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe);
* @retval List of generated ids.
*/
struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe);
+vdbe_autoinc_id_list(struct Vdbe *vdbe);
int sqlite3VdbeAddOp0(Vdbe *, int);
int sqlite3VdbeAddOp1(Vdbe *, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 19b35b7..d16a51e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -369,7 +369,7 @@ struct Vdbe {
* List of ids generated in current VDBE. It is returned
* as metadata of SQL response.
*/
- struct stailq generated_ids;
+ struct stailq autoinc_id_list;
/* When allocating a new Vdbe object, all of the fields below should be
* initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3efe252..78cf2c4 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,7 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
return 0;
memset(p, 0, sizeof(Vdbe));
p->db = db;
- stailq_create(&p->generated_ids);
+ stailq_create(&p->autoinc_id_list);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
}
@@ -2779,6 +2779,10 @@ sqlite3VdbeDelete(Vdbe * p)
}
p->magic = VDBE_MAGIC_DEAD;
p->db = 0;
+ /*
+ * VDBE is responsible for releasing region after txn
+ * was commited.
+ */
if (in_txn() == NULL)
fiber_gc();
sqlite3DbFree(db, p);
diff --git a/src/box/txn.h b/src/box/txn.h
index 87b55da..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -126,10 +126,8 @@ struct sql_txn {
* An element of list of autogenerated ids, being returned as SQL
* response metadata.
*/
-struct id_entry {
- /** A link in a generated id list. */
+struct autoinc_id_entry {
struct stailq_entry link;
- /** Generated id. */
int64_t id;
};
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af4fc12..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -37,6 +37,9 @@ box.sql.execute('select * from test')
box.schema.user.grant('guest','read,write,execute', 'universe')
---
...
+box.schema.user.grant('guest', 'create', 'space')
+---
+...
cn = remote.connect(box.cfg.listen)
---
...
@@ -594,7 +597,7 @@ cn:execute('insert into test values (1, 1)')
...
cn:execute('insert into test values (null, 2)')
---
-- generated_ids:
+- autoincrement_ids:
- 2
rowcount: 1
...
@@ -604,14 +607,14 @@ cn:execute('update test set a = 11 where id == 1')
...
cn:execute('insert into test values (100, 1), (null, 1), (120, 1),
(null, 1)')
---
-- generated_ids:
+- autoincrement_ids:
- 101
- 121
rowcount: 4
...
cn:execute('insert into test values (null, 1), (null, 1), (null, 1),
(null, 1), (null, 1)')
---
-- generated_ids:
+- autoincrement_ids:
- 122
- 123
- 124
@@ -654,12 +657,27 @@ _ = box.space.TEST:on_replace(push_id)
...
cn:execute('insert into test values (null, 1)')
---
-- generated_ids:
+- autoincrement_ids:
- 127
- 1
- 2
rowcount: 1
...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+ - 1
+ - -9
+ - -19
+ - -29
+ rowcount: 4
+...
cn:close()
---
...
@@ -672,9 +690,15 @@ s:drop()
sq:drop()
---
...
+box.sql.execute('drop table test3')
+---
+...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
+box.schema.user.revoke('guest', 'create', 'space')
+---
+...
space = nil
---
...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a2caa57..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -10,6 +10,7 @@ space:replace{4, 5, '6'}
space:replace{7, 8.5, '9'}
box.sql.execute('select * from test')
box.schema.user.grant('guest','read,write,execute', 'universe')
+box.schema.user.grant('guest', 'create', 'space')
cn = remote.connect(box.cfg.listen)
cn:ping()
@@ -220,12 +221,18 @@ function push_id() s:replace{box.NULL}
s:replace{box.NULL} end
_ = box.space.TEST:on_replace(push_id)
cn:execute('insert into test values (null, 1)')
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
cn:close()
box.sql.execute('drop table test')
s:drop()
sq:drop()
+box.sql.execute('drop table test3')
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+box.schema.user.revoke('guest', 'create', 'space')
space = nil
-- Cleanup xlog
*New version:*
commit 58f70a2cc6bfbed679f1601b09ba1a5d9db05872
Author: Mergen Imeev <imeevma at gmail.com>
Date: Wed Sep 26 22:07:54 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. After
this patch all ids autogenerated during VDBE execution will be
saved and returned through IPROTO.
Closes #2618
diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..64c0be6 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/vdbe.h"
const char *sql_type_strs[] = {
NULL,
@@ -637,11 +638,26 @@ err:
} else {
keys = 1;
assert(port_tuple->size == 0);
- if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
+ struct stailq *autoinc_id_list =
+ vdbe_autoinc_id_list((struct Vdbe *)stmt);
+ uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
+ if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
goto err;
+ uint64_t id_count = 0;
int changes = sqlite3_changes(db);
int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
mp_sizeof_uint(changes);
+ if (!stailq_empty(autoinc_id_list)) {
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+ size += id_entry->id >= 0 ?
+ mp_sizeof_uint(id_entry->id) :
+ mp_sizeof_int(id_entry->id);
+ id_count++;
+ }
+ size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
+ mp_sizeof_array(id_count);
+ }
char *buf = obuf_alloc(out, size);
if (buf == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "buf");
@@ -649,6 +665,16 @@ err:
}
buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
buf = mp_encode_uint(buf, changes);
+ if (!stailq_empty(autoinc_id_list)) {
+ buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
+ buf = mp_encode_array(buf, id_count);
+ struct autoinc_id_entry *id_entry;
+ stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+ buf = id_entry->id >= 0 ?
+ mp_encode_uint(buf, id_entry->id) :
+ mp_encode_int(buf, id_entry->id);
+ }
+ }
}
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..77bfd79 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_AUTOINCREMENT_IDS = 1,
sql_info_key_MAX,
};
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,35 @@ 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;
+ assert(map_size == 1 || map_size == 2);
+ lua_newtable(L);
+ /*
+ * First element in data is SQL_INFO_ROW_COUNT.
+ */
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);
lua_pushinteger(L, row_count);
lua_setfield(L, -2, "rowcount");
+ /*
+ * If data have two elements then second is
+ * SQL_INFO_AUTOINCREMENT_IDS.
+ */
+ if (map_size == 2) {
+ key = mp_decode_uint(data);
+ assert(key == SQL_INFO_AUTOINCREMENT_IDS);
+ (void)key;
+ uint64_t count = mp_decode_array(data);
+ assert(count > 0);
+ lua_createtable(L, 0, count);
+ for (uint32_t j = 0; j < count; ++j) {
+ int64_t id;
+ mp_read_int64(data, &id);
+ luaL_pushint64(L, id);
+ lua_rawseti(L, -2, j + 1);
+ }
+ lua_setfield(L, -2, "autoincrement_ids");
+ }
}
static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..c9828c0 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"
@@ -194,6 +195,9 @@ sequence_next(struct sequence *seq, int64_t *result)
new_data) == light_sequence_end)
return -1;
*result = def->start;
+ if (txn_vdbe() != NULL &&
+ vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
+ return -1;
return 0;
}
old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +232,9 @@ done:
new_data, &old_data) == light_sequence_end)
unreachable();
*result = value;
+ if (txn_vdbe() != NULL &&
+ vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
+ return -1;
return 0;
overflow:
if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -43,6 +43,7 @@ extern "C" {
#endif /* defined(__cplusplus) */
struct iterator;
+struct Vdbe;
/** Sequence metadata. */
struct sequence_def {
@@ -139,6 +140,9 @@ sequence_next(struct sequence *seq, int64_t *result);
int
access_check_sequence(struct sequence *seq);
+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
+
/**
* Create an iterator over sequence data.
*
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d6d6b7e..78fd28a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,28 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
}
}
+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
+{
+ return &vdbe->autoinc_id_list;
+}
+
+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
+{
+ assert(vdbe != NULL);
+ struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+ sizeof(*id_entry));
+ if (id_entry == NULL) {
+ diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+ "id_entry");
+ return -1;
+ }
+ id_entry->id = id;
+ stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
+ return 0;
+}
+
/*
* Execute as much of a VDBE program as we can.
* This is the core of sqlite3_step().
@@ -2997,10 +3019,14 @@ case OP_TransactionBegin: {
*
* Commit Tarantool's transaction.
* If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
*/
case OP_TransactionCommit: {
- if (box_txn()) {
- if (box_txn_commit() != 0) {
+ struct txn *txn = in_txn();
+ if (txn != NULL) {
+ if (txn_commit(txn) != 0) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
@@ -3044,7 +3070,7 @@ case OP_TransactionRollback: {
*/
case OP_TTransaction: {
if (!box_txn()) {
- if (box_txn_begin() != 0) {
+ if (sql_txn_begin(p) != 0) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index d5da5d5..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);
* Allocate and initialize SQL-specific struct which completes
* original Tarantool's txn struct using region allocator.
*
+ * @param v Vdbe with which associate this transaction.
* @retval NULL on OOM, new psql_txn struct on success.
**/
struct sql_txn *
-sql_alloc_txn();
+sql_alloc_txn(struct Vdbe *v);
/**
* Prepare given VDBE to execution: initialize structs connected
@@ -197,6 +198,15 @@ sql_alloc_txn();
int
sql_vdbe_prepare(struct Vdbe *vdbe);
+/**
+ * Return pointer to list of generated ids.
+ *
+ * @param vdbe VDBE to get list of generated ids from.
+ * @retval List of generated ids.
+ */
+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe);
+
int sqlite3VdbeAddOp0(Vdbe *, int);
int sqlite3VdbeAddOp1(Vdbe *, int, int);
int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..cfb327a 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,6 +367,11 @@ struct Vdbe {
struct sql_txn *psql_txn;
/** The auto-commit flag. */
bool auto_commit;
+ /**
+ * List of ids generated in current VDBE. It is returned
+ * as metadata of SQL response.
+ */
+ struct stailq autoinc_id_list;
/* When allocating a new Vdbe object, all of the fields below
should be
* initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d05..1e72a4d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
return 0;
memset(p, 0, sizeof(Vdbe));
p->db = db;
+ stailq_create(&p->autoinc_id_list);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
}
@@ -75,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse)
}
struct sql_txn *
-sql_alloc_txn()
+sql_alloc_txn(struct Vdbe *v)
{
struct sql_txn *txn = region_alloc_object(&fiber()->gc,
struct sql_txn);
@@ -84,6 +85,8 @@ sql_alloc_txn()
"struct sql_txn");
return NULL;
}
+ txn->vdbe = v;
+ v->psql_txn = txn;
txn->pSavepoint = NULL;
txn->fk_deferred_count = 0;
return txn;
@@ -103,11 +106,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
* check FK violations, at least now.
*/
if (txn->psql_txn == NULL) {
- txn->psql_txn = sql_alloc_txn();
+ txn->psql_txn = sql_alloc_txn(vdbe);
if (txn->psql_txn == NULL)
return -1;
+ } else {
+ vdbe->psql_txn = txn->psql_txn;
}
- vdbe->psql_txn = txn->psql_txn;
} else {
vdbe->psql_txn = NULL;
}
@@ -2261,12 +2265,11 @@ sql_txn_begin(Vdbe *p)
struct txn *ptxn = txn_begin(false);
if (ptxn == NULL)
return -1;
- ptxn->psql_txn = sql_alloc_txn();
+ ptxn->psql_txn = sql_alloc_txn(p);
if (ptxn->psql_txn == NULL) {
box_txn_rollback();
return -1;
}
- p->psql_txn = ptxn->psql_txn;
return 0;
}
@@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
* key constraints to hold up the transaction.
This means a commit
* is required.
*/
- rc = box_txn_commit() ==
- 0 ? SQLITE_OK :
- SQL_TARANTOOL_ERROR;
+ rc = (in_txn() == NULL ||
+ txn_commit(in_txn()) == 0) ?
+ SQLITE_OK : SQL_TARANTOOL_ERROR;
closeCursorsAndFree(p);
}
if (rc == SQLITE_BUSY && !p->pDelFrame) {
@@ -2780,6 +2783,12 @@ sqlite3VdbeDelete(Vdbe * p)
}
p->magic = VDBE_MAGIC_DEAD;
p->db = 0;
+ /*
+ * VDBE is responsible for releasing region after txn
+ * was commited.
+ */
+ if (in_txn() == NULL)
+ fiber_gc();
sqlite3DbFree(db, p);
}
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,7 @@ struct engine;
struct space;
struct tuple;
struct xrow_header;
+struct Vdbe;
enum {
/**
@@ -117,6 +118,17 @@ struct sql_txn {
* VDBE to the next in the same transaction.
*/
uint32_t fk_deferred_count;
+ /** Current VDBE. */
+ struct Vdbe *vdbe;
+};
+
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct autoinc_id_entry {
+ struct stailq_entry link;
+ int64_t id;
};
struct txn {
@@ -337,6 +349,20 @@ txn_last_stmt(struct txn *txn)
void
txn_on_stop(struct trigger *trigger, void *event);
+/**
+ * Return VDBE that is being currently executed.
+ *
+ * @retval VDBE that is being currently executed.
+ * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
+ */
+static inline struct Vdbe *
+txn_vdbe()
+{
+ struct txn *txn = in_txn();
+ if (txn == NULL || txn->psql_txn == NULL)
+ return NULL;
+ return txn->psql_txn->vdbe;
+}
/**
* FFI bindings: do not throw exceptions, do not accept extra
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index ee34f6f..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -583,6 +583,116 @@ 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)')
+---
+- autoincrement_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)')
+---
+- autoincrement_ids:
+ - 101
+ - 121
+ rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1),
(null, 1), (null, 1)')
+---
+- autoincrement_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]
+...
+s = box.schema.create_space('test2', {engine = engine})
+---
+...
+sq = box.schema.sequence.create('test2')
+---
+...
+pk = s:create_index('pk', {sequence = 'test2'})
+---
+...
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+---
+...
+_ = box.space.TEST:on_replace(push_id)
+---
+...
+cn:execute('insert into test values (null, 1)')
+---
+- autoincrement_ids:
+ - 127
+ - 1
+ - 2
+ rowcount: 1
+...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+ - 1
+ - -9
+ - -19
+ - -29
+ rowcount: 4
+...
+cn:close()
+---
+...
+box.sql.execute('drop table test')
+---
+...
+s:drop()
+---
+...
+sq:drop()
+---
+...
+box.sql.execute('drop table test3')
+---
+...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index b065590..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -202,6 +202,35 @@ 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')
+
+s = box.schema.create_space('test2', {engine = engine})
+sq = box.schema.sequence.create('test2')
+pk = s:create_index('pk', {sequence = 'test2'})
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+_ = box.space.TEST:on_replace(push_id)
+cn:execute('insert into test values (null, 1)')
+
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+box.sql.execute('drop table test3')
+
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
box.schema.user.revoke('guest', 'create', 'space')
space = nil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181102/54d85bee/attachment.html>
More information about the Tarantool-patches
mailing list