[tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See 6 comments below.

> commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1
> Author: Mergen Imeev <imeevma@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]
> +...