* [tarantool-patches] [PATCH v5 1/1] sql: return all generated ids via IPROTO
@ 2018-09-28 16:53 imeevma
2018-09-29 4:36 ` [tarantool-patches] " Konstantin Osipov
2018-10-03 19:19 ` Vladislav Shpilevoy
0 siblings, 2 replies; 7+ messages in thread
From: imeevma @ 2018-09-28 16:53 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
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
---
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 | 24 ++++++++++++++++
src/box/execute.h | 1 +
src/box/lua/net_box.c | 21 ++++++++++++--
src/box/sequence.c | 29 ++++++++++++++++++++
src/box/sql/vdbe.c | 2 +-
src/box/sql/vdbe.h | 3 +-
src/box/sql/vdbeInt.h | 3 ++
src/box/sql/vdbeaux.c | 15 ++++++++--
src/box/txn.h | 13 +++++++++
test/sql/iproto.result | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
test/sql/iproto.test.lua | 15 ++++++++++
11 files changed, 190 insertions(+), 7 deletions(-)
diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..d9fe33f 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,
@@ -639,14 +640,37 @@ err:
assert(port_tuple->size == 0);
if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
goto err;
+ struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
+ 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(id_list) == 0) {
+ struct id_entry *id_entry;
+ stailq_foreach_entry(id_entry, 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) +
+ mp_sizeof_array(id_count);
+ }
char *buf = obuf_alloc(out, size);
if (buf == NULL) {
diag_set(OutOfMemory, size, "obuf_alloc", "buf");
goto err;
}
+ if (id_count > 0) {
+ buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+ buf = mp_encode_array(buf, id_count);
+ struct id_entry *id_entry;
+ stailq_foreach_entry(id_entry, id_list, link) {
+ buf = id_entry->id >= 0 ?
+ mp_encode_uint(buf, id_entry->id) :
+ mp_encode_int(buf, id_entry->id);
+ }
+ }
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..5d2ac78 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **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/sequence.c b/src/box/sequence.c
index 35b7605..ed6b2da 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"
@@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value)
return 0;
}
+/**
+ * Save generated id in VDBE.
+ *
+ * @param value ID to save in VDBE.
+ * @retval 0 Success.
+ * @retval -1 Error.
+ */
+static inline int
+add_new_id_in_vdbe(int64_t value)
+{
+ struct txn *txn = in_txn();
+ if (txn == NULL || txn->psql_txn == NULL)
+ return 0;
+ assert(txn->psql_txn->vdbe != NULL);
+ struct id_entry *id_entry = malloc(sizeof(*id_entry));
+ if (id_entry == NULL) {
+ diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
+ return -1;
+ }
+ id_entry->id = value;
+ stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);
+ return 0;
+}
+
int
sequence_next(struct sequence *seq, int64_t *result)
{
@@ -194,6 +219,8 @@ sequence_next(struct sequence *seq, int64_t *result)
new_data) == light_sequence_end)
return -1;
*result = def->start;
+ if(add_new_id_in_vdbe(*result) != 0)
+ return -1;
return 0;
}
old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +255,8 @@ done:
new_data, &old_data) == light_sequence_end)
unreachable();
*result = value;
+ if(add_new_id_in_vdbe(*result) != 0)
+ return -1;
return 0;
overflow:
if (!def->cycle) {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 00ffb0c..cbec9a2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3040,7 +3040,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..9b94183 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 vdbe VDBE that is being prepared or executed.
* @retval NULL on OOM, new psql_txn struct on success.
**/
struct sql_txn *
-sql_alloc_txn();
+sql_alloc_txn(struct Vdbe *vdbe);
/**
* Prepare given VDBE to execution: initialize structs connected
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..ca1794c 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -368,6 +368,9 @@ struct Vdbe {
/** The auto-commit flag. */
bool auto_commit;
+ /** List of id generated in current VDBE. */
+ struct stailq 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 54edf1b..98ddef7 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. */
+ stailq_create(&p->id_list);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
}
@@ -75,7 +77,7 @@ sqlite3VdbeCreate(Parse * pParse)
}
struct sql_txn *
-sql_alloc_txn()
+sql_alloc_txn(struct Vdbe *vdbe)
{
struct sql_txn *txn = region_alloc_object(&fiber()->gc,
struct sql_txn);
@@ -86,6 +88,8 @@ sql_alloc_txn()
}
txn->pSavepoint = NULL;
txn->fk_deferred_count = 0;
+ txn->vdbe = vdbe;
+ txn->id_list = &vdbe->id_list;
return txn;
}
@@ -103,7 +107,7 @@ 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;
}
@@ -2261,7 +2265,7 @@ 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;
@@ -2746,6 +2750,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)
vdbeFreeOpArray(db, p->aOp, p->nOp);
sqlite3DbFree(db, p->aColName);
sqlite3DbFree(db, p->zSql);
+ while (stailq_empty(&p->id_list) == 0) {
+ struct id_entry *id_entry =
+ stailq_shift_entry(&p->id_list, struct id_entry, link);
+ free(id_entry);
+ }
#ifdef SQLITE_ENABLE_STMT_SCANSTATUS
{
int i;
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..f8521c4 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -117,6 +117,19 @@ struct sql_txn {
* VDBE to the next in the same transaction.
*/
uint32_t fk_deferred_count;
+ /** Current VDBE. */
+ struct Vdbe *vdbe;
+ /** Pointer to list of generated ids of current VDBE.*/
+ struct stailq *id_list;
+};
+
+/** An element of list of generated ids. */
+struct id_entry
+{
+ /** A link in a generated id list. */
+ struct stailq_entry link;
+ /** Generated id. */
+ int64_t id;
};
struct txn {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af474bc..163fb6e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -580,6 +580,77 @@ 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('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)')
+---
+- 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..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] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-09-28 16:53 [tarantool-patches] [PATCH v5 1/1] sql: return all generated ids via IPROTO imeevma
@ 2018-09-29 4:36 ` Konstantin Osipov
2018-10-18 11:29 ` Imeev Mergen
2018-10-03 19:19 ` Vladislav Shpilevoy
1 sibling, 1 reply; 7+ messages in thread
From: Konstantin Osipov @ 2018-09-29 4:36 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
* imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:53]:
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..d9fe33f 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"
Ugh. You're including a header which ends with Int. Have you
thought what this Int means? It's not Integer.
>
> const char *sql_type_strs[] = {
> NULL,
> @@ -639,14 +640,37 @@ err:
> assert(port_tuple->size == 0);
> if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
> goto err;
> + struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
> + uint64_t id_count = 0;
The name is very general. autoinc_id_list would be more specific.
Why not simply call it generated_ids here and everywhere? This is
what these values are called in the standard.
> int changes = sqlite3_changes(db);
> int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
> mp_sizeof_uint(changes);
> + if (stailq_empty(id_list) == 0) {
What is the point of this == 0? Why are you comparing with an
integer here, not boolean?
> + struct id_entry *id_entry;
> + stailq_foreach_entry(id_entry, id_list, link) {
> + size += id_entry->id >= 0 ?
> + mp_sizeof_uint(id_entry->id) :
> + mp_sizeof_int(id_entry->id);
How can you get a negative id?
> + id_count++;
> + }
> + size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
> + mp_sizeof_array(id_count);
> + }
> char *buf = obuf_alloc(out, size);
> if (buf == NULL) {
> diag_set(OutOfMemory, size, "obuf_alloc", "buf");
> goto err;
> }
> + if (id_count > 0) {
> + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
> + buf = mp_encode_array(buf, id_count);
> + struct id_entry *id_entry;
> + stailq_foreach_entry(id_entry, id_list, link) {
> + buf = id_entry->id >= 0 ?
> + mp_encode_uint(buf, id_entry->id) :
> + mp_encode_int(buf, id_entry->id);
> + }
> + }
> 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..5d2ac78 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **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/sequence.c b/src/box/sequence.c
> index 35b7605..ed6b2da 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"
> @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value)
> return 0;
> }
>
> +/**
> + * Save generated id in VDBE.
> + *
> + * @param value ID to save in VDBE.
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
> +static inline int
> +add_new_id_in_vdbe(int64_t value)
subject-verb-object
> +{
> + struct txn *txn = in_txn();
> + if (txn == NULL || txn->psql_txn == NULL)
> + return 0;
> + assert(txn->psql_txn->vdbe != NULL);
> + struct id_entry *id_entry = malloc(sizeof(*id_entry));
Why malloc?
> + if (id_entry == NULL) {
> + diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
> + return -1;
> + }
> + id_entry->id = value;
> + stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);
> + return 0;
> +}
> +
> int
> sequence_next(struct sequence *seq, int64_t *result)
> {
> @@ -194,6 +219,8 @@ sequence_next(struct sequence *seq, int64_t *result)
> new_data) == light_sequence_end)
> return -1;
> *result = def->start;
> + if(add_new_id_in_vdbe(*result) != 0)
> + return -1;
> return 0;
> }
> old_data = light_sequence_get(&sequence_data_index, pos);
> @@ -228,6 +255,8 @@ done:
> new_data, &old_data) == light_sequence_end)
> unreachable();
> *result = value;
> + if(add_new_id_in_vdbe(*result) != 0)
> + return -1;
You continue to miss a space after if.
> return 0;
> overflow:
> if (!def->cycle) {
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 00ffb0c..cbec9a2 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3040,7 +3040,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..9b94183 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 vdbe VDBE that is being prepared or executed.
> * @retval NULL on OOM, new psql_txn struct on success.
> **/
> struct sql_txn *
> -sql_alloc_txn();
> +sql_alloc_txn(struct Vdbe *vdbe);
>
> /**
> * Prepare given VDBE to execution: initialize structs connected
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index ce97f49..ca1794c 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -368,6 +368,9 @@ struct Vdbe {
> /** The auto-commit flag. */
> bool auto_commit;
>
> + /** List of id generated in current VDBE. */
> + struct stailq 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 54edf1b..98ddef7 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. */
> + stailq_create(&p->id_list);
> if (db->pVdbe) {
> db->pVdbe->pPrev = p;
> }
> @@ -75,7 +77,7 @@ sqlite3VdbeCreate(Parse * pParse)
> }
>
> struct sql_txn *
> -sql_alloc_txn()
> +sql_alloc_txn(struct Vdbe *vdbe)
> {
> struct sql_txn *txn = region_alloc_object(&fiber()->gc,
> struct sql_txn);
> @@ -86,6 +88,8 @@ sql_alloc_txn()
> }
> txn->pSavepoint = NULL;
> txn->fk_deferred_count = 0;
> + txn->vdbe = vdbe;
> + txn->id_list = &vdbe->id_list;
Why do you need both?
> return txn;
> }
>
> @@ -103,7 +107,7 @@ 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;
> }
> @@ -2261,7 +2265,7 @@ 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;
> @@ -2746,6 +2750,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)
> vdbeFreeOpArray(db, p->aOp, p->nOp);
> sqlite3DbFree(db, p->aColName);
> sqlite3DbFree(db, p->zSql);
> + while (stailq_empty(&p->id_list) == 0) {
> + struct id_entry *id_entry =
> + stailq_shift_entry(&p->id_list, struct id_entry, link);
> + free(id_entry);
> + }
> #ifdef SQLITE_ENABLE_STMT_SCANSTATUS
> {
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-09-28 16:53 [tarantool-patches] [PATCH v5 1/1] sql: return all generated ids via IPROTO imeevma
2018-09-29 4:36 ` [tarantool-patches] " Konstantin Osipov
@ 2018-10-03 19:19 ` Vladislav Shpilevoy
2018-10-04 22:31 ` Konstantin Osipov
2018-10-18 11:25 ` Imeev Mergen
1 sibling, 2 replies; 7+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-03 19:19 UTC (permalink / raw)
To: tarantool-patches, imeevma
Hi! Thanks for the patch!
I see that Kostja slightly reviewed it and you did not
answer him, so I inlined here some of his comments to
agree/disagree with them.
See 8 comments below.
On 28/09/2018 19:53, 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 implement such functionality.
>
> 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 | 24 ++++++++++++++++
> src/box/execute.h | 1 +
> src/box/lua/net_box.c | 21 ++++++++++++--
> src/box/sequence.c | 29 ++++++++++++++++++++
> src/box/sql/vdbe.c | 2 +-
> src/box/sql/vdbe.h | 3 +-
> src/box/sql/vdbeInt.h | 3 ++
> src/box/sql/vdbeaux.c | 15 ++++++++--
> src/box/txn.h | 13 +++++++++
> test/sql/iproto.result | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
> test/sql/iproto.test.lua | 15 ++++++++++
> 11 files changed, 190 insertions(+), 7 deletions(-)
>
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..d9fe33f 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"
1. Kostja said:
>> Ugh. You're including a header which ends with Int. Have you
>> thought what this Int means? It's not Integer.
Strictly speaking, I agree with him. Src/box/execute is not
an SQL internal file. It even uses SQLite public API to
fetch names and types. I guess, you should implement a getter
for ids list like
const struct stailq *
vdbe_auto_ids_list(const struct Vdbe *v);
>
> const char *sql_type_strs[] = {
> NULL,
> @@ -639,14 +640,37 @@ err:
> assert(port_tuple->size == 0);
> if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
> goto err;
> + struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
> + 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(id_list) == 0) {
2. Kostja said:
>> What is the point of this == 0? Why are you comparing with an
>> integer here, not boolean?
I partially agree and I guess it is my fault that I am too harsh
about code style. You used here == 0 because stailq_empty()
returns an integer, but its name looks like a flag getter, so you
can use it as a boolean here.
> + struct id_entry *id_entry;
> + stailq_foreach_entry(id_entry, 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) +
> + mp_sizeof_array(id_count);
> + }> char *buf = obuf_alloc(out, size);
> if (buf == NULL) {
> diag_set(OutOfMemory, size, "obuf_alloc", "buf");
> goto err;
> }
> + if (id_count > 0) {
3. Lets be consistent: either you use !stailq_empty() in both
places, or id_count > 0.
> + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
> + buf = mp_encode_array(buf, id_count);
> + struct id_entry *id_entry;
> + stailq_foreach_entry(id_entry, id_list, link) {
> + buf = id_entry->id >= 0 ?
> + mp_encode_uint(buf, id_entry->id) :
> + mp_encode_int(buf, id_entry->id);
> + }
> + }
> buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
> buf = mp_encode_uint(buf, changes);
> }
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index a928a4c..5d2ac78 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
> /* Only SQL_INFO_ROW_COUNT is available. */
> assert(map_size == 1);
4. How does it work? map_size can be 2 now, and the comment is
outdated - not only SQL_INFO_ROW_COUNT is available.
> (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/sequence.c b/src/box/sequence.c
> index 35b7605..ed6b2da 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"
> @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value)
> return 0;
> }
>
> +/**
> + * Save generated id in VDBE.
> + *
> + * @param value ID to save in VDBE.
> + * @retval 0 Success.
> + * @retval -1 Error.
> + */
> +static inline int
> +add_new_id_in_vdbe(int64_t value)
5. Kostja said:
>> subject-verb-object
I agree. He means 'vdbe_add_new_id'. And in such a
case it is evidently a part of Vdbe API, so put it into
vdbe.h with this signature:
int
vdbe_add_new_auto_id(struct Vdbe *vdbe, int64_t id);
> +{
> + struct txn *txn = in_txn();
> + if (txn == NULL || txn->psql_txn == NULL)
> + return 0;
> + assert(txn->psql_txn->vdbe != NULL);
> + struct id_entry *id_entry = malloc(sizeof(*id_entry));
> + if (id_entry == NULL) {
> + diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
> + return -1;
> + }
> + id_entry->id = value;
> + stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);
6. If you have Vdbe, you do not need to store the list both in
Vdbe and sql_txn. Store it in Vdbe only.
7. Kostja said:
>> Why malloc?
It is understandable why malloc - txn after commit invalidates
the region memory. We neither can save ids on obuf directly
since while getting next id a yield is possible and the obuf
will be messed with another request. But guess I thought up a
solution. It is a bit tricky, so maybe it should be accounted
as a separate issue and this solved with malloc.
I propose to split txn_commit() into commit and region reset.
Txn_commit() goal is to write data to disk, not to destroy a
region. It saves us when a transaction is started and committed
by Vdbe. But what about autocommit transactions? I think, we
should get rid of autocommit transactions for Vdbe. Vdbe always
starts a transaction for DML and commits it, but without region
reset - it is a part of vdbe finalize routine (which already
exists actually).
This split of txn_commit logic allows to store anything on
a region during Vdbe work and not being afraid about a reset.
> + return 0;
> +}
> +
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index d5da5d5..9b94183 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 vdbe VDBE that is being prepared or executed.
> * @retval NULL on OOM, new psql_txn struct on success.
> **/
> struct sql_txn *
> -sql_alloc_txn();
> +sql_alloc_txn(struct Vdbe *vdbe);
8. I think, alloc should only alloc. sql_txn_begin should
put Vdbe into sql_txn.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-10-03 19:19 ` Vladislav Shpilevoy
@ 2018-10-04 22:31 ` Konstantin Osipov
2018-10-18 11:25 ` Imeev Mergen
1 sibling, 0 replies; 7+ messages in thread
From: Konstantin Osipov @ 2018-10-04 22:31 UTC (permalink / raw)
To: tarantool-patches; +Cc: imeevma
* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/10/04 13:33]:
> Hi! Thanks for the patch!
Hi Vlad,
Thank you for following up on my comments.
--
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-10-03 19:19 ` Vladislav Shpilevoy
2018-10-04 22:31 ` Konstantin Osipov
@ 2018-10-18 11:25 ` Imeev Mergen
2018-10-25 21:31 ` Vladislav Shpilevoy
1 sibling, 1 reply; 7+ messages in thread
From: Imeev Mergen @ 2018-10-18 11:25 UTC (permalink / raw)
To: Vladislav Shpilevoy, tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 24989 bytes --]
Hello! Thank you for review! New patch below. There won't be diff
between versions as I started this version before decided to save
this diff.
On 10/03/2018 10:19 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> I see that Kostja slightly reviewed it and you did not
> answer him, so I inlined here some of his comments to
> agree/disagree with them.
>
> See 8 comments below.
>
> On 28/09/2018 19:53, 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 implement such functionality.
>>
>> 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 | 24 ++++++++++++++++
>> src/box/execute.h | 1 +
>> src/box/lua/net_box.c | 21 ++++++++++++--
>> src/box/sequence.c | 29 ++++++++++++++++++++
>> src/box/sql/vdbe.c | 2 +-
>> src/box/sql/vdbe.h | 3 +-
>> src/box/sql/vdbeInt.h | 3 ++
>> src/box/sql/vdbeaux.c | 15 ++++++++--
>> src/box/txn.h | 13 +++++++++
>> test/sql/iproto.result | 71
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> test/sql/iproto.test.lua | 15 ++++++++++
>> 11 files changed, 190 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 24459b4..d9fe33f 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"
>
> 1. Kostja said:
>
>>> Ugh. You're including a header which ends with Int. Have you
>>> thought what this Int means? It's not Integer.
>
> Strictly speaking, I agree with him. Src/box/execute is not
> an SQL internal file. It even uses SQLite public API to
> fetch names and types. I guess, you should implement a getter
> for ids list like
>
> const struct stailq *
> vdbe_auto_ids_list(const struct Vdbe *v);
Fixed.
>
>> const char *sql_type_strs[] = {
>> NULL,
>> @@ -639,14 +640,37 @@ err:
>> assert(port_tuple->size == 0);
>> if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
>> goto err;
>> + struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
>> + 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(id_list) == 0) {
>
> 2. Kostja said:
>
>>> What is the point of this == 0? Why are you comparing with an
>>> integer here, not boolean?
>
> I partially agree and I guess it is my fault that I am too harsh
> about code style. You used here == 0 because stailq_empty()
> returns an integer, but its name looks like a flag getter, so you
> can use it as a boolean here.
Fixed.
>
>> + struct id_entry *id_entry;
>> + stailq_foreach_entry(id_entry, 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) +
>> + mp_sizeof_array(id_count);
>> + }> char *buf = obuf_alloc(out, size);
>> if (buf == NULL) {
>> diag_set(OutOfMemory, size, "obuf_alloc", "buf");
>> goto err;
>> }
>> + if (id_count > 0) {
>
> 3. Lets be consistent: either you use !stailq_empty() in both
> places, or id_count > 0.
Fixed.
>
>> + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
>> + buf = mp_encode_array(buf, id_count);
>> + struct id_entry *id_entry;
>> + stailq_foreach_entry(id_entry, id_list, link) {
>> + buf = id_entry->id >= 0 ?
>> + mp_encode_uint(buf, id_entry->id) :
>> + mp_encode_int(buf, id_entry->id);
>> + }
>> + }
>> buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
>> buf = mp_encode_uint(buf, changes);
>> }
>> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
>> index a928a4c..5d2ac78 100644
>> --- a/src/box/lua/net_box.c
>> +++ b/src/box/lua/net_box.c
>> @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L,
>> const char **data)
>> /* Only SQL_INFO_ROW_COUNT is available. */
>> assert(map_size == 1);
>
> 4. How does it work? map_size can be 2 now, and the comment is
> outdated - not only SQL_INFO_ROW_COUNT is available.
Fixed.
>
>> (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/sequence.c b/src/box/sequence.c
>> index 35b7605..ed6b2da 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"
>> @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t
>> value)
>> return 0;
>> }
>> +/**
>> + * Save generated id in VDBE.
>> + *
>> + * @param value ID to save in VDBE.
>> + * @retval 0 Success.
>> + * @retval -1 Error.
>> + */
>> +static inline int
>> +add_new_id_in_vdbe(int64_t value)
>
> 5. Kostja said:
>
>>> subject-verb-object
> I agree. He means 'vdbe_add_new_id'. And in such a
> case it is evidently a part of Vdbe API, so put it into
> vdbe.h with this signature:
>
> int
> vdbe_add_new_auto_id(struct Vdbe *vdbe, int64_t id);
Fixed.
>
>> +{
>> + struct txn *txn = in_txn();
>> + if (txn == NULL || txn->psql_txn == NULL)
>> + return 0;
>> + assert(txn->psql_txn->vdbe != NULL);
>> + struct id_entry *id_entry = malloc(sizeof(*id_entry));
>> + if (id_entry == NULL) {
>> + diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
>> + return -1;
>> + }
>> + id_entry->id = value;
>> + stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);
>
> 6. If you have Vdbe, you do not need to store the list both in
> Vdbe and sql_txn. Store it in Vdbe only.
Fixed.
>
> 7. Kostja said:
>
>>> Why malloc?
> It is understandable why malloc - txn after commit invalidates
> the region memory. We neither can save ids on obuf directly
> since while getting next id a yield is possible and the obuf
> will be messed with another request. But guess I thought up a
> solution. It is a bit tricky, so maybe it should be accounted
> as a separate issue and this solved with malloc.
>
> I propose to split txn_commit() into commit and region reset.
> Txn_commit() goal is to write data to disk, not to destroy a
> region. It saves us when a transaction is started and committed
> by Vdbe. But what about autocommit transactions? I think, we
> should get rid of autocommit transactions for Vdbe. Vdbe always
> starts a transaction for DML and commits it, but without region
> reset - it is a part of vdbe finalize routine (which already
> exists actually).
>
> This split of txn_commit logic allows to store anything on
> a region during Vdbe work and not being afraid about a reset.
>
>> + return 0;
>> +}
>> +
>> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
>> index d5da5d5..9b94183 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 vdbe VDBE that is being prepared or executed.
>> * @retval NULL on OOM, new psql_txn struct on success.
>> **/
>> struct sql_txn *
>> -sql_alloc_txn();
>> +sql_alloc_txn(struct Vdbe *vdbe);
>
> 8. I think, alloc should only alloc. sql_txn_begin should
> put Vdbe into sql_txn.
Done.
*New patch:*
commit 9dd5e9833aec54c819e6dae07d62624538f76246
Author: Mergen Imeev <imeevma@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.
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"
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 *generated_ids =
+ vdbe_generated_ids((struct Vdbe *)stmt);
+ uint32_t map_size = stailq_empty(generated_ids) ? 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) {
+ 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) +
+ 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(generated_ids)) {
+ buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+ buf = mp_encode_array(buf, id_count);
+ struct id_entry *id_entry;
+ stailq_foreach_entry(id_entry, generated_ids, 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..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..1a9b3f7 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,37 @@ 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_GENERATED_IDS.
+ */
+ if (map_size == 2) {
+ key = mp_decode_uint(data);
+ assert(key == SQL_INFO_GENERATED_IDS);
+ (void)key;
+ 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 id;
+ mp_read_int64(data, &id);
+ lua_pushinteger(L, id);
+ 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..b1c68e9 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_generated_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_generated_id(txn_vdbe(), *result) != 0)
+ return -1;
return 0;
overflow:
if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..d38fb2f 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 {
@@ -140,6 +141,17 @@ int
access_check_sequence(struct sequence *seq);
/**
+ * Add new generated id in VDBE.
+ *
+ * @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);
+
+/**
* Create an iterator over sequence data.
*
* The iterator creates a snapshot of sequence data and walks
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..7a3748e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,26 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
}
}
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe)
+{
+ return &vdbe->generated_ids;
+}
+
+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");
+ 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().
@@ -2996,7 +3016,7 @@ case OP_TransactionBegin: {
*/
case OP_TransactionCommit: {
if (box_txn()) {
- if (box_txn_commit() != 0) {
+ if (txn_commit(in_txn()) != 0) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
@@ -3040,7 +3060,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..8f877b3 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -197,6 +197,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);
+
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..37df5e2 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -368,6 +368,9 @@ struct Vdbe {
/** The auto-commit flag. */
bool auto_commit;
+ /** List of id generated in current VDBE. */
+ struct stailq generated_ids;
+
/* 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..0023dc6 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. */
+ stailq_create(&p->generated_ids);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
}
@@ -106,6 +108,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
txn->psql_txn = sql_alloc_txn();
if (txn->psql_txn == NULL)
return -1;
+ txn->psql_txn->vdbe = vdbe;
}
vdbe->psql_txn = txn->psql_txn;
} else {
@@ -2266,6 +2269,7 @@ sql_txn_begin(Vdbe *p)
box_txn_rollback();
return -1;
}
+ ptxn->psql_txn->vdbe = p;
p->psql_txn = ptxn->psql_txn;
return 0;
}
@@ -2414,9 +2418,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 +2784,8 @@ sqlite3VdbeDelete(Vdbe * p)
}
p->magic = VDBE_MAGIC_DEAD;
p->db = 0;
+ if (in_txn() == NULL)
+ fiber_gc();
sqlite3DbFree(db, p);
}
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..2acba9f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -117,6 +117,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 generated ids. */
+struct id_entry
+{
+ /** A link in a generated id list. */
+ struct stailq_entry link;
+ /** Generated id. */
+ int64_t id;
};
struct txn {
@@ -337,6 +348,20 @@ txn_last_stmt(struct txn *txn)
void
txn_on_stop(struct trigger *trigger, void *event);
+/**
+ * Return VDBE that is currently executed.
+ *
+ * @retval VDBE that is 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 af474bc..163fb6e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -580,6 +580,77 @@ 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('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)')
+---
+- 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..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
[-- Attachment #2: Type: text/html, Size: 33640 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-09-29 4:36 ` [tarantool-patches] " Konstantin Osipov
@ 2018-10-18 11:29 ` Imeev Mergen
0 siblings, 0 replies; 7+ messages in thread
From: Imeev Mergen @ 2018-10-18 11:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
[-- Attachment #1: Type: text/plain, Size: 24899 bytes --]
Hello! Thank you for review! New patch below.
On 09/29/2018 07:36 AM, Konstantin Osipov wrote:
> *imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:53]:
>> diff --git a/src/box/execute.c b/src/box/execute.c
>> index 24459b4..d9fe33f 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"
> Ugh. You're including a header which ends with Int. Have you
> thought what this Int means? It's not Integer.
Fixed.
>>
>> const char *sql_type_strs[] = {
>> NULL,
>> @@ -639,14 +640,37 @@ err:
>> assert(port_tuple->size == 0);
>> if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
>> goto err;
>> + struct stailq *id_list = &((struct Vdbe *)stmt)->id_list;
>> + uint64_t id_count = 0;
> The name is very general. autoinc_id_list would be more specific.
> Why not simply call it generated_ids here and everywhere? This is
> what these values are called in the standard.
Fixed.
>> int changes = sqlite3_changes(db);
>> int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
>> mp_sizeof_uint(changes);
>> + if (stailq_empty(id_list) == 0) {
> What is the point of this == 0? Why are you comparing with an
> integer here, not boolean?
Fixed.
>> + struct id_entry *id_entry;
>> + stailq_foreach_entry(id_entry, id_list, link) {
>> + size += id_entry->id >= 0 ?
>> + mp_sizeof_uint(id_entry->id) :
>> + mp_sizeof_int(id_entry->id);
> How can you get a negative id?
It is possible:
box.sql.execute('create table a (id int primary key autoincrement)')
box.schema.sequence.alter('A', {min=-10000, step=-1})
box.sql.execute('insert into a values (null)')
box.sql.execute('insert into a values (null)')
box.sql.execute('insert into a values (null)')
box.sql.execute('insert into a values (null)')
box.sql.execute('insert into a values (null)')
box.sql.execute('select * from a')
---
- - [-3]
- [-2]
- [-1]
- [0]
- [1]
...
>> + id_count++;
>> + }
>> + size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
>> + mp_sizeof_array(id_count);
>> + }
>> char *buf = obuf_alloc(out, size);
>> if (buf == NULL) {
>> diag_set(OutOfMemory, size, "obuf_alloc", "buf");
>> goto err;
>> }
>> + if (id_count > 0) {
>> + buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
>> + buf = mp_encode_array(buf, id_count);
>> + struct id_entry *id_entry;
>> + stailq_foreach_entry(id_entry, id_list, link) {
>> + buf = id_entry->id >= 0 ?
>> + mp_encode_uint(buf, id_entry->id) :
>> + mp_encode_int(buf, id_entry->id);
>> + }
>> + }
>> 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..5d2ac78 100644
>> --- a/src/box/lua/net_box.c
>> +++ b/src/box/lua/net_box.c
>> @@ -671,11 +671,28 @@ netbox_decode_sql_info(struct lua_State *L, const char **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/sequence.c b/src/box/sequence.c
>> index 35b7605..ed6b2da 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"
>> @@ -178,6 +179,30 @@ sequence_update(struct sequence *seq, int64_t value)
>> return 0;
>> }
>>
>> +/**
>> + * Save generated id in VDBE.
>> + *
>> + * @param value ID to save in VDBE.
>> + * @retval 0 Success.
>> + * @retval -1 Error.
>> + */
>> +static inline int
>> +add_new_id_in_vdbe(int64_t value)
> subject-verb-object
Fixed.
>> +{
>> + struct txn *txn = in_txn();
>> + if (txn == NULL || txn->psql_txn == NULL)
>> + return 0;
>> + assert(txn->psql_txn->vdbe != NULL);
>> + struct id_entry *id_entry = malloc(sizeof(*id_entry));
> Why malloc?
Fixed. To save ids in region fiber_gc() was moved from
txn_commit() to sqlite3VdbeDelete().
>> + if (id_entry == NULL) {
>> + diag_set(OutOfMemory, sizeof(*id_entry), "malloc", "id_entry");
>> + return -1;
>> + }
>> + id_entry->id = value;
>> + stailq_add_tail_entry(txn->psql_txn->id_list, id_entry, link);
>> + return 0;
>> +}
>> +
>> int
>> sequence_next(struct sequence *seq, int64_t *result)
>> {
>> @@ -194,6 +219,8 @@ sequence_next(struct sequence *seq, int64_t *result)
>> new_data) == light_sequence_end)
>> return -1;
>> *result = def->start;
>> + if(add_new_id_in_vdbe(*result) != 0)
>> + return -1;
>> return 0;
>> }
>> old_data = light_sequence_get(&sequence_data_index, pos);
>> @@ -228,6 +255,8 @@ done:
>> new_data, &old_data) == light_sequence_end)
>> unreachable();
>> *result = value;
>> + if(add_new_id_in_vdbe(*result) != 0)
>> + return -1;
> You continue to miss a space after if.
Fixed.
>> return 0;
>> overflow:
>> if (!def->cycle) {
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 00ffb0c..cbec9a2 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -3040,7 +3040,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..9b94183 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 vdbe VDBE that is being prepared or executed.
>> * @retval NULL on OOM, new psql_txn struct on success.
>> **/
>> struct sql_txn *
>> -sql_alloc_txn();
>> +sql_alloc_txn(struct Vdbe *vdbe);
>>
>> /**
>> * Prepare given VDBE to execution: initialize structs connected
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index ce97f49..ca1794c 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -368,6 +368,9 @@ struct Vdbe {
>> /** The auto-commit flag. */
>> bool auto_commit;
>>
>> + /** List of id generated in current VDBE. */
>> + struct stailq 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 54edf1b..98ddef7 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. */
>> + stailq_create(&p->id_list);
>> if (db->pVdbe) {
>> db->pVdbe->pPrev = p;
>> }
>> @@ -75,7 +77,7 @@ sqlite3VdbeCreate(Parse * pParse)
>> }
>>
>> struct sql_txn *
>> -sql_alloc_txn()
>> +sql_alloc_txn(struct Vdbe *vdbe)
>> {
>> struct sql_txn *txn = region_alloc_object(&fiber()->gc,
>> struct sql_txn);
>> @@ -86,6 +88,8 @@ sql_alloc_txn()
>> }
>> txn->pSavepoint = NULL;
>> txn->fk_deferred_count = 0;
>> + txn->vdbe = vdbe;
>> + txn->id_list = &vdbe->id_list;
> Why do you need both?
Fixed.
>> return txn;
>> }
>>
>> @@ -103,7 +107,7 @@ 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;
>> }
>> @@ -2261,7 +2265,7 @@ 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;
>> @@ -2746,6 +2750,11 @@ sqlite3VdbeClearObject(sqlite3 * db, Vdbe * p)
>> vdbeFreeOpArray(db, p->aOp, p->nOp);
>> sqlite3DbFree(db, p->aColName);
>> sqlite3DbFree(db, p->zSql);
>> + while (stailq_empty(&p->id_list) == 0) {
>> + struct id_entry *id_entry =
>> + stailq_shift_entry(&p->id_list, struct id_entry, link);
>> + free(id_entry);
>> + }
>> #ifdef SQLITE_ENABLE_STMT_SCANSTATUS
>> {
*New patch:*
commit 9dd5e9833aec54c819e6dae07d62624538f76246
Author: Mergen Imeev <imeevma@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.
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"
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 *generated_ids =
+ vdbe_generated_ids((struct Vdbe *)stmt);
+ uint32_t map_size = stailq_empty(generated_ids) ? 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) {
+ 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) +
+ 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(generated_ids)) {
+ buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+ buf = mp_encode_array(buf, id_count);
+ struct id_entry *id_entry;
+ stailq_foreach_entry(id_entry, generated_ids, 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..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..1a9b3f7 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,37 @@ 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_GENERATED_IDS.
+ */
+ if (map_size == 2) {
+ key = mp_decode_uint(data);
+ assert(key == SQL_INFO_GENERATED_IDS);
+ (void)key;
+ 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 id;
+ mp_read_int64(data, &id);
+ lua_pushinteger(L, id);
+ 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..b1c68e9 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_generated_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_generated_id(txn_vdbe(), *result) != 0)
+ return -1;
return 0;
overflow:
if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..d38fb2f 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 {
@@ -140,6 +141,17 @@ int
access_check_sequence(struct sequence *seq);
/**
+ * Add new generated id in VDBE.
+ *
+ * @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);
+
+/**
* Create an iterator over sequence data.
*
* The iterator creates a snapshot of sequence data and walks
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..7a3748e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,26 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
}
}
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe)
+{
+ return &vdbe->generated_ids;
+}
+
+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");
+ 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().
@@ -2996,7 +3016,7 @@ case OP_TransactionBegin: {
*/
case OP_TransactionCommit: {
if (box_txn()) {
- if (box_txn_commit() != 0) {
+ if (txn_commit(in_txn()) != 0) {
rc = SQL_TARANTOOL_ERROR;
goto abort_due_to_error;
}
@@ -3040,7 +3060,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..8f877b3 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -197,6 +197,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);
+
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..37df5e2 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -368,6 +368,9 @@ struct Vdbe {
/** The auto-commit flag. */
bool auto_commit;
+ /** List of id generated in current VDBE. */
+ struct stailq generated_ids;
+
/* 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..0023dc6 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. */
+ stailq_create(&p->generated_ids);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
}
@@ -106,6 +108,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
txn->psql_txn = sql_alloc_txn();
if (txn->psql_txn == NULL)
return -1;
+ txn->psql_txn->vdbe = vdbe;
}
vdbe->psql_txn = txn->psql_txn;
} else {
@@ -2266,6 +2269,7 @@ sql_txn_begin(Vdbe *p)
box_txn_rollback();
return -1;
}
+ ptxn->psql_txn->vdbe = p;
p->psql_txn = ptxn->psql_txn;
return 0;
}
@@ -2414,9 +2418,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 +2784,8 @@ sqlite3VdbeDelete(Vdbe * p)
}
p->magic = VDBE_MAGIC_DEAD;
p->db = 0;
+ if (in_txn() == NULL)
+ fiber_gc();
sqlite3DbFree(db, p);
}
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..2acba9f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -117,6 +117,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 generated ids. */
+struct id_entry
+{
+ /** A link in a generated id list. */
+ struct stailq_entry link;
+ /** Generated id. */
+ int64_t id;
};
struct txn {
@@ -337,6 +348,20 @@ txn_last_stmt(struct txn *txn)
void
txn_on_stop(struct trigger *trigger, void *event);
+/**
+ * Return VDBE that is currently executed.
+ *
+ * @retval VDBE that is 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 af474bc..163fb6e 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -580,6 +580,77 @@ 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('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)')
+---
+- 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..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
[-- Attachment #2: Type: text/html, Size: 30785 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO
2018-10-18 11:25 ` Imeev Mergen
@ 2018-10-25 21:31 ` Vladislav Shpilevoy
0 siblings, 0 replies; 7+ messages in thread
From: Vladislav Shpilevoy @ 2018-10-25 21:31 UTC (permalink / raw)
To: Imeev Mergen, tarantool-patches
Hi! Thanks for the fixes! I pushed more and
one additional commit to remove one attribute
from struct Vdbe. Please, look at both commits,
squash fixes if you are ok with them and send
v2 to Nikita (including my commit, yes).
My review fixes:
==================================================================
commit 45019a8dccb89a7f89b5e59e442c539aa0e6ef41
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date: Fri Oct 26 00:11:27 2018 +0300
Review fixes
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 1a9b3f740..4d79fdefb 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -685,19 +685,16 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
if (map_size == 2) {
key = mp_decode_uint(data);
assert(key == SQL_INFO_GENERATED_IDS);
- (void)key;
uint64_t count = mp_decode_array(data);
- assert (count > 0);
+ 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 id;
mp_read_int64(data, &id);
- lua_pushinteger(L, id);
+ luaL_pushint64(L, id);
lua_rawseti(L, -2, j + 1);
}
- lua_pop(L, 1);
+ lua_setfield(L, -2, "generated_ids");
}
}
diff --git a/src/box/sequence.c b/src/box/sequence.c
index b1c68e97e..9a87a56eb 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -233,7 +233,7 @@ done:
unreachable();
*result = value;
if (txn_vdbe() != NULL &&
- vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+ vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
return -1;
return 0;
overflow:
diff --git a/src/box/sequence.h b/src/box/sequence.h
index d38fb2f97..7180ead76 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -141,7 +141,8 @@ int
access_check_sequence(struct sequence *seq);
/**
- * Add new generated id in VDBE.
+ * 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.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a3748eae..2571492fc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3015,8 +3015,9 @@ case OP_TransactionBegin: {
* If there is no active transaction, raise an error.
*/
case OP_TransactionCommit: {
- if (box_txn()) {
- if (txn_commit(in_txn()) != 0) {
+ struct txn *txn = in_txn();
+ if (txn != NULL) {
+ if (txn_commit(txn) != 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 8f877b3a4..ed7fb2f49 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
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 37df5e290..7a7d3dec0 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,8 +367,10 @@ struct Vdbe {
struct sql_txn *psql_txn;
/** The auto-commit flag. */
bool auto_commit;
-
- /** List of id generated in current VDBE. */
+ /**
+ * List of ids generated in current VDBE. It is returned
+ * as metadata of SQL response.
+ */
struct stailq generated_ids;
/* When allocating a new Vdbe object, all of the fields below should be
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 0023dc6cd..ae73f2541 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,7 +57,6 @@ sqlite3VdbeCreate(Parse * pParse)
return 0;
memset(p, 0, sizeof(Vdbe));
p->db = db;
- /* Init list of generated ids. */
stailq_create(&p->generated_ids);
if (db->pVdbe) {
db->pVdbe->pPrev = p;
@@ -77,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);
@@ -86,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;
@@ -105,12 +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;
- txn->psql_txn->vdbe = vdbe;
+ } else {
+ vdbe->psql_txn = txn->psql_txn;
}
- vdbe->psql_txn = txn->psql_txn;
} else {
vdbe->psql_txn = NULL;
}
@@ -2264,13 +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;
}
- ptxn->psql_txn->vdbe = p;
- p->psql_txn = ptxn->psql_txn;
return 0;
}
diff --git a/src/box/txn.h b/src/box/txn.h
index 2acba9f87..12e9d1013 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -121,9 +121,11 @@ struct sql_txn {
struct Vdbe *vdbe;
};
-/** An element of list of generated ids. */
-struct id_entry
-{
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct id_entry {
/** A link in a generated id list. */
struct stailq_entry link;
/** Generated id. */
@@ -349,9 +351,9 @@ void
txn_on_stop(struct trigger *trigger, void *event);
/**
- * Return VDBE that is currently executed.
+ * Return VDBE that is being currently executed.
*
- * @retval VDBE that is currently executed.
+ * @retval VDBE that is being currently executed.
* @retval NULL Either txn or ptxn_sql or vdbe is NULL;
*/
static inline struct Vdbe *
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 163fb6ed7..af4fc124d 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -637,13 +637,28 @@ cn:execute('select * from test')
- [125, 1]
- [126, 1]
...
-cn:execute('create table test2 (id integer primary key, a integer)')
+s = box.schema.create_space('test2', {engine = engine})
---
-- rowcount: 1
...
-cn:execute('drop table test2')
+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)
---
-- rowcount: 1
+...
+cn:execute('insert into test values (null, 1)')
+---
+- generated_ids:
+ - 127
+ - 1
+ - 2
+ rowcount: 1
...
cn:close()
---
@@ -651,6 +666,12 @@ cn:close()
box.sql.execute('drop table test')
---
...
+s:drop()
+---
+...
+sq:drop()
+---
+...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 6517cdef2..a2caa57b1 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -204,6 +204,7 @@ 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)')
@@ -211,10 +212,18 @@ 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')
+
+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)')
+
cn:close()
box.sql.execute('drop table test')
+s:drop()
+sq:drop()
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
space = nil
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-25 21:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-28 16:53 [tarantool-patches] [PATCH v5 1/1] sql: return all generated ids via IPROTO imeevma
2018-09-29 4:36 ` [tarantool-patches] " Konstantin Osipov
2018-10-18 11:29 ` Imeev Mergen
2018-10-03 19:19 ` Vladislav Shpilevoy
2018-10-04 22:31 ` Konstantin Osipov
2018-10-18 11:25 ` Imeev Mergen
2018-10-25 21:31 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox