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

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

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

On 02/08/2018 14: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.

1. Please, write a documentation bot request.

> ---
> 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             |  12 +++-
>   src/box/execute.h             |   1 +
>   src/box/lua/net_box.c         |  15 +++-
>   src/box/sequence.c            |  13 ++++
>   src/box/sequence.h            |   9 +++
>   src/box/session.cc            |   1 +
>   src/box/session.h             |   2 +
>   src/box/sql.c                 |  17 +++++
>   src/box/sql/func.c            |  19 +++++
>   src/box/sql/sqliteInt.h       |  18 +++++
>   src/box/sql/vdbe.c            |  14 +++-
>   test/sql-tap/insert3.test.lua |  27 ++++++-
>   test/sql/errinj.result        |   3 +-
>   test/sql/iproto.result        | 162 +++++++++++++++++++++++++++++++++---------
>   test/sql/iproto.test.lua      |  16 +++++
>   15 files changed, 289 insertions(+), 40 deletions(-)
> 
> diff --git a/src/box/execute.c b/src/box/execute.c
> index 24459b4..f2b31c6 100644
> --- a/src/box/execute.c
> +++ b/src/box/execute.c
> @@ -640,8 +640,16 @@ err:
>   		if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
>   			goto err;
>   		int changes = sqlite3_changes(db);
> +		/*
> +		 * Even though last_insert_id is int64, it
> +		 * is easier to work with it as uint64.
> +		 */
> +		uint64_t last_insert_id = (uint64_t)get_last_insert_id();

2. Please, use int64, and do not encode negative values as uint. IProto
is being parsed by different connectors written by different users, not
only by builtin netbox.

> +
>   		int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
> -			   mp_sizeof_uint(changes);
> +			   mp_sizeof_uint(changes) +
> +			   mp_sizeof_uint(SQL_INFO_LAST_INSERT_ID) +
> +			   mp_sizeof_uint(last_insert_id);
>   		char *buf = obuf_alloc(out, size);
>   		if (buf == NULL) {
>   			diag_set(OutOfMemory, size, "obuf_alloc", "buf");
> diff --git a/src/box/sequence.c b/src/box/sequence.c
> index 35b7605..fe66931 100644
> --- a/src/box/sequence.c
> +++ b/src/box/sequence.c
> @@ -340,3 +340,16 @@ 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);
> +	if (pos == light_sequence_end)
> +		return 0;

3. Please, do not use 0. Start value can differ from 0.

> +	struct sequence_data data = light_sequence_get(&sequence_data_index,
> +						       pos);
> +	return data.value;
> +}
>   #if defined(__cplusplus)
>   } /* extern "C" */
>   #endif /* defined(__cplusplus) */
> diff --git a/src/box/session.cc b/src/box/session.cc
> index 64714cd..f19853e 100644
> --- a/src/box/session.cc
> +++ b/src/box/session.cc
> @@ -108,6 +108,7 @@ session_create(enum session_type type)
>   	session->type = type;
>   	session->sql_flags = default_flags;
>   	session->sql_default_engine = SQL_STORAGE_ENGINE_MEMTX;
> +	session->last_insert_id = 0;

4. Lets name it 'sql_last_insert_id', since it is SQL only feature.

>   
>   	/* For on_connect triggers. */
>   	credentials_init(&session->credentials, guest_user->auth_token,
> diff --git a/src/box/sql.c b/src/box/sql.c
> index d48c3cf..bb275eb 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1794,3 +1795,19 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,
>   	sql_parser_destroy(&parser);
>   	return rc;
>   }
> +
> +void
> +set_last_insert_id(struct space *space)

5. I think, this function is too specific and can be inlined in its
single usage place.

> +{
> +	struct session *session = current_session();
> +	session->last_insert_id = 0;
> +	struct sequence *sequence = space->sequence;
> +	if(sequence != NULL)
> +		session->last_insert_id = sequence_get_value(sequence);
> +}
> +
> +int64_t
> +get_last_insert_id(void)

6. Lets inline this one-liner in its usage places and remove.

> +{
> +	return current_session()->last_insert_id;
> +}
> @@ -580,6 +612,72 @@ cn:close()

7. What about tests on UPDATE, REPLACE that they do not affect the id?

>   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.
> +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)
> +---
> +- last_insert_id: 1
> +  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: 17
> +  rowcount: 1
> +...
> +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil)
> +---
> +- last_insert_id: 18
> +  rowcount: 2
> +...
> +future = cn:execute('select * from test', nil, nil, {is_async = true})

8. Why do you need this SELECT be async?

> +---
> +...
> +future:wait_result()
> +---
> +- metadata:
> +  - name: ID
> +  - name: A
> +  rows:
> +  - [1, 1]
> +  - [2, 2]
> +  - [3, 2]
> +  - [4, 3]
> +  - [17, 2]
> +  - [18, 2]
> +  - [19, 3]
> +...
> +cn:execute('create table test2 (id integer primary key, a integer)')
> +---
> +- last_insert_id: 18
> +  rowcount: 1
> +...
> +cn:execute('drop table test2')
> +---
> +- last_insert_id: 18
> +  rowcount: 1
> +...
> +cn:close()
> +---
> +...
> +box.sql.execute('drop table test')
> +---
> +...
>   box.schema.user.revoke('guest', 'read,write,execute', 'universe')
>   ---
>   ...