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

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

Hi! Thanks for the patch! See 11 comments below.

On 03/08/2018 13:28, imeevma@tarantool.org wrote:
> After this patch client will get last_insert_id
> as additional metadata when he executes some
> queries.
> 
> Part of #2618.
> 
> @TarantoolBot document

1. Please, describe in the same request new iproto key
you have introduced.

> Title: SQL function last_insert_id.
> Function last_insert_id returns first autogenerated ID in
> last INSERT/REPLACE statement in current session. Return
> value of function is undetermined when more than one
> INSERT/REPLACE statements executed asynchronously.
> Example:
> CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER);
> INSERT INTO test VALUES (NULL, 1);
> SELECT last_insert_id();
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-generated-columns-and-values
> Issue: https://github.com/tarantool/tarantool/issues/2618
> 
>   src/box/execute.c             |  11 ++-
>   src/box/execute.h             |   1 +
>   src/box/lua/net_box.c         |  12 ++-
>   src/box/sequence.c            |  17 ++++
>   src/box/sequence.h            |   9 +++
>   src/box/session.cc            |   1 +
>   src/box/session.h             |   2 +
>   src/box/sql.c                 |   1 +
>   src/box/sql/func.c            |  18 +++++
>   src/box/sql/vdbe.c            |  33 ++++++++
>   test/sql-tap/insert3.test.lua |  27 ++++++-
>   test/sql/errinj.result        |   3 +-
>   test/sql/iproto.result        | 180 ++++++++++++++++++++++++++++++++++--------
>   test/sql/iproto.test.lua      |  19 +++++
>   14 files changed, 296 insertions(+), 38 deletions(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 308c9c7..0e24b47 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -667,16 +667,22 @@ 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);
> +	(void) key;

2. Now the key is used and its (void) guard can be removed,
it is not?

> +	int64_t last_insert_id = mp_typeof(**data) == MP_UINT ?
> +				 (int64_t) mp_decode_uint(data) :
> +				 mp_decode_int(data);
> +	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/sequence.c b/src/box/sequence.c
> index 35b7605..cefb3db 100644
> --- a/src/box/sequence.c
> +++ b/src/box/sequence.c
> @@ -340,3 +340,20 @@ sequence_data_iterator_create(void)
>   	light_sequence_iterator_freeze(&sequence_data_index, &iter->iter);
>   	return &iter->base;
>   }
> +
> +int64_t
> +sequence_get_value(struct sequence *seq)
> +{
> +	uint32_t key = seq->def->id;
> +	uint32_t hash = sequence_hash(key);
> +	uint32_t pos = light_sequence_find_key(&sequence_data_index, hash, key);
> +	/*
> +	 * Number 1 is often used as start value of sequences in
> +	 * general. We are goint to use it as default value.
> +	 */
> +	if (pos == light_sequence_end)
> +		return 1;

3. The same comment as in the previous review. 1 is not mandatory start
value and can be different in a common case, as well as 0, -1, 2 and any
other constant. Use sequence_def.start.

4. Please, when sending a new patch version, explicitly answer my
comments either in a new version email or in a response to my email. It
is hard to iterate through two emails (with the comments and with your
patch) at the same time.

> +	struct sequence_data data = light_sequence_get(&sequence_data_index,
> +						       pos);
> +	return data.value;
> +}
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 2b93c3d..d9752f3 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -48,6 +48,7 @@
>   #include "txn.h"
>   #include "space.h"
>   #include "space_def.h"
> +#include "sequence.h"

5. Why?

>   #include "index_def.h"
>   #include "tuple.h"
>   #include "fiber.h"
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 45056a7..eca69b7 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -38,6 +38,7 @@
>   #include "vdbeInt.h"
>   #include "version.h"
>   #include "coll.h"
> +#include "src/box/session.h"

6. For box/ dir we do not use src/ prefix.
Please, use 'box/session.h'.

>   #include <unicode/ustring.h>
>   #include <unicode/ucasemap.h>
>   #include <unicode/ucnv.h>
> @@ -601,6 +602,22 @@ changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2)
>   }
>   
>   /*
> + * Implementation of the last_insert_id() function. Returns first

7. As I said earlier, please, do not describe the function in a
comment like it is an object. Use imperative. I see that other
functions in the file use the same style, but do not follow it -
this style is sqlite's obsolete one.

> + * autogenerated ID in last INSERT/REPLACE in current session.
> + *
> + * @param context Context being used.
> + * @param not_used Unused.
> + * @param not_used2 Unused.
> + */
> +static void
> +last_insert_id(sqlite3_context *context, int not_used,
> +	       sqlite3_value **not_used2)
> +{
> +	UNUSED_PARAMETER2(not_used, not_used2);
> +	sqlite3_result_int(context, current_session()->sql_last_insert_id);
> +}
> +
> +/*
>    * Implementation of the total_changes() SQL function.  The return value is
>    * the same as the sqlite3_total_changes() API function.
>    */
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index ca89908..38cd501 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -567,6 +567,30 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
>   	}
>   }
>   
> +/**
> + * Check that new ID is autogenerated in current query. If it is
> + * this function sets sql_last_insert_id of current session as
> + * first autogenerated ID in last INSERT/REPLACE query executed
> + * in current session.
> + *
> + * @param space space to get sequence from.
> + * @retval true new id autogenerated.
> + * @retval false no new id autogenerated.
> + */
> +static inline bool
> +check_last_insert_id(struct space *space)
> +{
> +	int64_t new_id = 0;
> +	struct session *session = current_session();
> +	struct sequence *sequence = space->sequence;
> +	if (sequence != NULL)
> +		new_id = sequence_get_value(sequence);

8. If sequence == NULL, this function should not do anything,
it is not? I think, it is simpler to just inline it and
remove some of checks. See the comment 10.

> +	if (session->sql_last_insert_id == new_id)
> +		return false;
> +	session->sql_last_insert_id = new_id;
> +	return true;
> +}
> +
>   /*
>    * Execute as much of a VDBE program as we can.
>    * This is the core of sqlite3_step().
> @@ -599,6 +623,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 query.
> +	 */
> +	bool last_insert_id_is_set = false;

9. For flags we use 'is_' prefix.

>   	/*** INSERT STACK UNION HERE ***/
>   
>   	assert(p->magic==VDBE_MAGIC_RUN);  /* sqlite3_step() verifies this */
> @@ -4300,6 +4329,10 @@ case OP_IdxInsert: {        /* in2 */
>   				rc = tarantoolSqlite3Replace(pBtCur->space,
>   							     pIn2->z,
>   							     pIn2->z + pIn2->n);
> +			if (rc == 0 && !last_insert_id_is_set) {
> +				last_insert_id_is_set =
> +					check_last_insert_id(pBtCur->space);

10. I propose to rewrite it as

if (!last_insert_id_is_set && rc == 0 && space->sequence != NULL) {
	last_insert_id_is_set = true;
	user_session->sql_last_insert_id =
		sequence_get_value(space->sequence);
}

And remove check_last_insert_id.

> +			}
>   		} else if (pBtCur->curFlags & BTCF_TEphemCursor) {
>   			rc = tarantoolSqlite3EphemeralInsert(pBtCur->space,
>   							     pIn2->z,> @@ -580,6 +612,90 @@ cn:close()
>   box.sql.execute('drop table test')
>   ---
>   ...
> +-- gh-2618 Return generated columns after INSERT in IPROTO.
> +-- Return autogenerated id of first inserted tuple in last insert.

11. 'In last insert' of what or whither?