[tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Aug 17 01:05:47 MSK 2018
Thanks for the fixes! See 6 comments below.
> commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1
> Author: Mergen Imeev <imeevma at gmail.com>
> Date: Tue Jul 31 16:43:38 2018 +0300
>
> sql: return last_insert_id via IPROTO
>
> After this patch client will get last_insert_id
> as additional metadata when he executes some
> queries.
>
> Part of #2618.
>
> @TarantoolBot document
> Title: SQL function last_insert_id()
> and IPROTO key last_insert_id.
> Function last_insert_id() returns first primary key value
> autogenerated in last INSERT/REPLACE statement in current
> session. Return value of function is undetermined when more
> than one INSERT/REPLACE statements executed asynchronously.
1. Please, note that only for a single session of a single user.
If a user opens multiple connections, then in each last_insert_id
will work properly.
> IPROTO key last_insert_id is a metadata returned through
> IPROTO after such queries as INSERT, REPLACE, UPDATE etc.
> Value of this key is equal to value returned by function
> last_insert_id() executed after the query.
> Example:
> CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER);
> INSERT INTO test VALUES (NULL, 1);
> SELECT last_insert_id();
>
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 308c9c7..83ba9b8 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -667,16 +667,21 @@ static void
> netbox_decode_sql_info(struct lua_State *L, const char **data)
> {
> uint32_t map_size = mp_decode_map(data);
> - /* Only SQL_INFO_ROW_COUNT is available. */
> assert(map_size == 1);
> (void) map_size;
> uint32_t key = mp_decode_uint(data);
> assert(key == SQL_INFO_ROW_COUNT);
> - (void) key;
> uint32_t row_count = mp_decode_uint(data);
> - lua_createtable(L, 0, 1);
> + key = mp_decode_uint(data);
> + assert(key == SQL_INFO_LAST_INSERT_ID);
> + int64_t last_insert_id = mp_typeof(**data) == MP_UINT ?
> + (int64_t) mp_decode_uint(data) :
> + mp_decode_int(data);
2. Why not mp_read_int64() ?
> + lua_createtable(L, 0, 2);
> lua_pushinteger(L, row_count);
> lua_setfield(L, -2, "rowcount");
> + lua_pushinteger(L, last_insert_id);
> + lua_setfield(L, -2, "last_insert_id");
> }
>
> static int
> diff --git a/src/box/session.h b/src/box/session.h
> index df1dcbc..3d4049c 100644
> --- a/src/box/session.h
> +++ b/src/box/session.h
> @@ -92,6 +92,8 @@ union session_meta {
> struct session {
> /** Session id. */
> uint64_t id;
> + /** First PK autogenerated in last INSERT/REPLACE query */
3. query -> statement. Sorry for nitpicking, it is Kostja O.'s
whim. But on the other hand it is literally more precise -
query is more likely about SELECT while statement concerns complex
multi-row DML constructions.
> + int64_t sql_last_insert_id;
> /** SQL Tarantool Default storage engine. */
> uint8_t sql_default_engine;
> /** SQL Connection flag for current user session */
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 13cb61e..8b5b0df 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse, /* Parser context */
> if (j < 0 || nColumn == 0
> || (pColumn && j >= pColumn->nId)) {
> if (i == pTab->iAutoIncPKey) {
> - sqlite3VdbeAddOp2(v,
> - OP_NextAutoincValue,
> - pTab->def->id,
> + sqlite3VdbeAddOp2(v, OP_Null, 0,
4. Can we fix the bug related to OP_NextAutoincValue in a
separate commit? So the patchset would consist of 2 commits.
And why for iAutoInc do you generate OP_Null here? As I
understand, here and on line 799 you are sure this column is
NULL and can put next_auto_inc explicitly with no writing
NULL and then replacing it in a separate opcode. It is not?
> iRegStore);
> continue;
> }
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index dc5146f..dceb8c0 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -3745,7 +3761,7 @@ case OP_FCopy: { /* out2 */
>
> if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) {
> pOut = &aMem[pOp->p2];
> - if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null;
> + if (pOut->flags & MEM_Int) pOut->flags = MEM_Null;
5. Why?
> /* Flag is set and register is NULL -> do nothing */
> } else {
> assert(memIsValid(pIn1));
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index af474bc..5fdf49b 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> @@ -580,6 +612,91 @@ cn:close()
> box.sql.execute('drop table test')
> ---
> ...
> +-- gh-2618 Return generated columns after INSERT in IPROTO.
> +-- Return first PK autogenerated in last INSERT/REPLACE in which
> +-- new PK was genereted in current session.
> +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)', nil, nil)
6. Why do you need last two arguments be nil? It is the same
as merely omit them.
> +---
> +- last_insert_id: 0
> + rowcount: 1
> +...
> +cn:execute('insert into test values (null, 2)', nil, nil)
> +---
> +- last_insert_id: 2
> + rowcount: 1
> +...
> +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil)
> +---
> +- last_insert_id: 3
> + rowcount: 2
> +...
> +cn:execute('insert into test values (17, 2)', nil, nil)
> +---
> +- last_insert_id: 3
> + rowcount: 1
> +...
> +cn:execute('update test set a = 11 where id == 1', nil, nil)
> +---
> +- last_insert_id: 3
> + rowcount: 1
> +...
> +cn:execute('update test set a = 12 where id == 2', nil, nil)
> +---
> +- last_insert_id: 3
> + rowcount: 1
> +...
> +cn:execute('update test set a = 13 where id == 3', nil, nil)
> +---
> +- last_insert_id: 3
> + rowcount: 1
> +...
> +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil)
> +---
> +- last_insert_id: 18
> + rowcount: 2
> +...
> +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil)
> +---
> +- last_insert_id: 19
> + rowcount: 2
> +...
> +cn:execute('select * from test', nil, nil)
> +---
> +- metadata:
> + - name: ID
> + - name: A
> + rows:
> + - [1, 11]
> + - [2, 12]
> + - [3, 13]
> + - [4, 44]
> + - [17, 2]
> + - [18, 100]
> + - [19, 2]
> + - [20, 3]
> +...
More information about the Tarantool-patches
mailing list