From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3290628ED3 for ; Thu, 2 Aug 2018 10:00:18 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nHQL3BJ8VdOh for ; Thu, 2 Aug 2018 10:00:18 -0400 (EDT) Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [94.100.177.95]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7018924A36 for ; Thu, 2 Aug 2018 10:00:17 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: return last_insert_id via IPROTO References: <7a1845fe1409d5f3f3f7c949753ae774d8c8e86b.1533209232.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <6ca58663-0536-19c5-2883-50a1ec14b282@tarantool.org> Date: Thu, 2 Aug 2018 17:00:14 +0300 MIME-Version: 1.0 In-Reply-To: <7a1845fe1409d5f3f3f7c949753ae774d8c8e86b.1533209232.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, imeevma@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') > --- > ...