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

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

Hi! Thanks for the fixes! See my 5 comments below.

> commit c87a9ae3b3446e016956f6b52d34677b3e321286
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Aug 17 13:24:58 2018 +0300
> 
>      sql: move autoincrement in vdbe
> 
>      This patch expands changes made in issue #2981. Now NULL
>      in primary key always replaced by generated value inside
>      of vdbe. It allows us to get last_insert_id with easy.
> 
>      Part of #2618
> 

1. This commit LGTM.

> 
> commit 010ec9b016ae6a49914b9e88544547ea9049e145
> 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
>      statements.
> 
>      Part of #2618
> 
>      @TarantoolBot document
>      Title: SQL function last_insert_id()
>      and IPROTO key last_insert_id.

2. Please, do not wrap Title section. TarantoolBot reads
a request line by line splitting by '\n'.

>      Function last_insert_id() returns first primary key value
>      autogenerated in last INSERT/REPLACE statement in current
>      session. User can have more than one session and this
>      function will work propertly for each one of them. Return

3. Typo: 'propertly'.

>      value of function is undetermined when more than one
>      INSERT/REPLACE statements executed asynchronously.
>      IPROTO key last_insert_id is a metadata returned through
>      IPROTO after such statements as INSERT, REPLACE, UPDATE
>      etc. Value of this key is equal to value returned by
>      function last_insert_id() executed after the statment.

4. Typo: 'statment'.

>      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/sql/vdbe.c b/src/box/sql/vdbe.c
> index 30cd6b9..f7fbd71 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -600,6 +600,11 @@ int sqlite3VdbeExec(Vdbe *p)
>       u64 start;                 /* CPU clock count at start of opcode */
>   #endif
>       struct session *user_session = current_session();
> +    /*
> +     * Field sql_last_insert_id of current session should be
> +     * set no more than once for each statement.
> +     */
> +    bool is_last_insert_id_set = false;

5. Kirill Sch. is implementing #2370 feature that allows (as I
hope for his implementation) to interrupt Vdbe on DML just like
for DQL (SELECT) to return each affected tuple. It means, that
VdbeExec will be called multiple times for a DML request. So
lets move this flag into struct Vdbe into the flags section
(next to 'isPrepareV2', 'expired', 'doingRerun' etc.)

Let me admonish you before you started against 'bft' type usage.
Use simple 'bool'.

>       /*** INSERT STACK UNION HERE ***/
> 
>       assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies this */