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 6B6AA29E5B for ; Thu, 16 Aug 2018 18:05:51 -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 bFnMC4ljTrKj for ; Thu, 16 Aug 2018 18:05:51 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 C823B278EB for ; Thu, 16 Aug 2018 18:05:50 -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: Fri, 17 Aug 2018 01:05:47 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Imeev Mergen , tarantool-patches@freelists.org Thanks for the fixes! See 6 comments below. > commit 2b292311e61f3c25a4c776bd75e3e6d241ff66a1 > Author: Mergen Imeev > Date:   Tue Jul 31 16:43:38 2018 +0300 > >     sql: return last_insert_id via IPROTO > >     After this patch client will get last_insert_id >     as additional metadata when he executes some >     queries. > >     Part of #2618. > >     @TarantoolBot document >     Title: SQL function last_insert_id() >     and IPROTO key last_insert_id. >     Function last_insert_id() returns first primary key value >     autogenerated in last INSERT/REPLACE statement in current >     session. Return value of function is undetermined when more >     than one INSERT/REPLACE statements executed asynchronously. 1. Please, note that only for a single session of a single user. If a user opens multiple connections, then in each last_insert_id will work properly. >     IPROTO key last_insert_id is a metadata returned through >     IPROTO after such queries as INSERT, REPLACE, UPDATE etc. >     Value of this key is equal to value returned by function >     last_insert_id() executed after the query. >     Example: >     CREATE TABLE test (id INTEGER PRIMARY KEY AUTOINCREMENT, a INTEGER); >     INSERT INTO test VALUES (NULL, 1); >     SELECT last_insert_id(); > > diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c > index 308c9c7..83ba9b8 100644 > --- a/src/box/lua/net_box.c > +++ b/src/box/lua/net_box.c > @@ -667,16 +667,21 @@ 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); > +    int64_t last_insert_id = mp_typeof(**data) == MP_UINT ? > +                 (int64_t) mp_decode_uint(data) : > +                 mp_decode_int(data); 2. Why not mp_read_int64() ? > +    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/session.h b/src/box/session.h > index df1dcbc..3d4049c 100644 > --- a/src/box/session.h > +++ b/src/box/session.h > @@ -92,6 +92,8 @@ union session_meta { >  struct session { >      /** Session id. */ >      uint64_t id; > +    /** First PK autogenerated in last INSERT/REPLACE query */ 3. query -> statement. Sorry for nitpicking, it is Kostja O.'s whim. But on the other hand it is literally more precise - query is more likely about SELECT while statement concerns complex multi-row DML constructions. > +    int64_t sql_last_insert_id; >      /** SQL Tarantool Default storage engine. */ >      uint8_t sql_default_engine; >      /** SQL Connection flag for current user session */ > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 13cb61e..8b5b0df 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -739,9 +739,7 @@ sqlite3Insert(Parse * pParse,    /* Parser context */ >              if (j < 0 || nColumn == 0 >                  || (pColumn && j >= pColumn->nId)) { >                  if (i == pTab->iAutoIncPKey) { > -                    sqlite3VdbeAddOp2(v, > -                              OP_NextAutoincValue, > -                              pTab->def->id, > +                    sqlite3VdbeAddOp2(v, OP_Null, 0, 4. Can we fix the bug related to OP_NextAutoincValue in a separate commit? So the patchset would consist of 2 commits. And why for iAutoInc do you generate OP_Null here? As I understand, here and on line 799 you are sure this column is NULL and can put next_auto_inc explicitly with no writing NULL and then replacing it in a separate opcode. It is not? >                                iRegStore); >                      continue; >                  } > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index dc5146f..dceb8c0 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3745,7 +3761,7 @@ case OP_FCopy: {     /* out2 */ > >      if ((pOp->p3 & OPFLAG_NOOP_IF_NULL) && (pIn1->flags & MEM_Null)) { >          pOut = &aMem[pOp->p2]; > -        if (pOut->flags & MEM_Undefined) pOut->flags = MEM_Null; > +        if (pOut->flags & MEM_Int) pOut->flags = MEM_Null; 5. Why? >          /* Flag is set and register is NULL -> do nothing  */ >      } else { >          assert(memIsValid(pIn1)); > diff --git a/test/sql/iproto.result b/test/sql/iproto.result > index af474bc..5fdf49b 100644 > --- a/test/sql/iproto.result > +++ b/test/sql/iproto.result > @@ -580,6 +612,91 @@ cn:close() >  box.sql.execute('drop table test') >  --- >  ... > +-- gh-2618 Return generated columns after INSERT in IPROTO. > +-- Return first PK autogenerated in last INSERT/REPLACE in which > +-- new PK was genereted in current session. > +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) 6. Why do you need last two arguments be nil? It is the same as merely omit them. > +--- > +- last_insert_id: 0 > +  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: 3 > +  rowcount: 1 > +... > +cn:execute('update test set a = 11 where id == 1', nil, nil) > +--- > +- last_insert_id: 3 > +  rowcount: 1 > +... > +cn:execute('update test set a = 12 where id == 2', nil, nil) > +--- > +- last_insert_id: 3 > +  rowcount: 1 > +... > +cn:execute('update test set a = 13 where id == 3', nil, nil) > +--- > +- last_insert_id: 3 > +  rowcount: 1 > +... > +cn:execute('replace into test values (4, 44), (null, 100)', nil, nil) > +--- > +- last_insert_id: 18 > +  rowcount: 2 > +... > +cn:execute('insert into test values (null, 2), (null, 3)', nil, nil) > +--- > +- last_insert_id: 19 > +  rowcount: 2 > +... > +cn:execute('select * from test', nil, nil) > +--- > +- metadata: > +  - name: ID > +  - name: A > +  rows: > +  - [1, 11] > +  - [2, 12] > +  - [3, 13] > +  - [4, 44] > +  - [17, 2] > +  - [18, 100] > +  - [19, 2] > +  - [20, 3] > +...