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 D335426B3B for ; Mon, 6 Aug 2018 11:24:38 -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 1R0dyBkgYEmJ for ; Mon, 6 Aug 2018 11:24:38 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 1BC5624E6F for ; Mon, 6 Aug 2018 11:24:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: return last_insert_id via IPROTO References: <8f24acf7f3f1583077ba1e40ba55a849ff8864a1.1533292030.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: Date: Mon, 6 Aug 2018 18:24:35 +0300 MIME-Version: 1.0 In-Reply-To: <8f24acf7f3f1583077ba1e40ba55a849ff8864a1.1533292030.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: imeevma@tarantool.org, tarantool-patches@freelists.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 > #include > #include > @@ -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?