[tarantool-patches] Re: [PATCH v1 1/1] sql: return last_insert_id via IPROTO
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Aug 2 17:00:14 MSK 2018
Hi! Thanks for the patch! See 8 comments below.
On 02/08/2018 14:28, imeevma at 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')
> ---
> ...
More information about the Tarantool-patches
mailing list