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 1E4DE2F8E7 for ; Fri, 2 Nov 2018 14:52:16 -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 1sX2DLWjePSE for ; Fri, 2 Nov 2018 14:52:16 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (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 C23132F6E7 for ; Fri, 2 Nov 2018 14:52:14 -0400 (EDT) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH v8 2/3] sql: return all generated ids via IPROTO References: <53b98187eba28a890db153e97d9c0a2b3759d1dd.1540832830.git.imeevma@gmail.com> <6167E418-CCCC-4596-9E6C-F04C02045456@tarantool.org> Message-ID: <81dc397f-aa49-15b2-5416-1f11931a11e5@tarantool.org> Date: Fri, 2 Nov 2018 21:52:10 +0300 MIME-Version: 1.0 In-Reply-To: <6167E418-CCCC-4596-9E6C-F04C02045456@tarantool.org> Content-Type: multipart/alternative; boundary="------------35B158489B476B8C7ED56780" Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org Cc: Vladislav Shpilevoy This is a multi-part message in MIME format. --------------35B158489B476B8C7ED56780 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Hi! Thank you for review! My answer, diff between patches and new patch below. Changes were made only in commit named by "sql: return all generated ids via IPROTO". After changes were made and diff saved I rebased this branch to current 2.1. After this new patch description were formed, so it is a bit different from old patch with applied diff. On 10/30/18 5:30 PM, n.pettik wrote: >> On 29 Oct 2018, at 20:33,imeevma@tarantool.org wrote: >> >> According to documentation some JDBC functions have an ability to >> return all ids that were generated in executed INSERT statement. >> This patch gives a way to implement such functionality. > I guess you should be more specific in describing such things. > I don’t ask you to dive into details, but explaining main idea of > implementation would definitely help reviewer to do his/her job. A bit extended this message. Not sure if it is enough. >> Closes #2618 >> — >> diff --git a/src/box/execute.c b/src/box/execute.c >> index 24459b4..ebf3962 100644 >> --- a/src/box/execute.c >> +++ b/src/box/execute.c >> @@ -42,6 +42,7 @@ >> #include "schema.h" >> #include "port.h" >> #include "tuple.h" >> +#include "sql/vdbe.h" >> >> >> diff --git a/src/box/execute.h b/src/box/execute.h >> index f21393b..614d3d0 100644 >> --- a/src/box/execute.h >> +++ b/src/box/execute.h >> @@ -42,6 +42,7 @@ extern "C" { >> /** Keys of IPROTO_SQL_INFO map. */ >> enum sql_info_key { >> SQL_INFO_ROW_COUNT = 0, >> + SQL_INFO_GENERATED_IDS = 1, > I vote for _AUTOINCREMENT_IDS, see comments below. > Anyway, in JDBC it is called GENERATED_KEYS. So, we either > follow JDBC convention or use our naming. In the latter case, > AUTOINCREMENT_IDS sounds more solid. Renamed to SQL_INFO_AUTOINCREMENT_IDS. >> sql_info_key_MAX, >> }; >> >> /** >> + * Append a new sequence value to Vdbe. List of automatically >> + * generated identifiers is returned as metadata of SQL response. >> + * >> + * @param Vdbe VDBE to save id in. >> + * @param id ID to save in VDBE. >> + * @retval 0 Success. >> + * @retval -1 Error. > It looks terrible. Does this function really need comment at all? Removed. >> + */ >> +int >> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id); >> >> + >> +int >> +vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id) >> +{ >> + assert(vdbe != NULL); >> + struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry)); >> + if (id_entry == NULL) { >> + diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”); > Out of 80. > >> + return -1; >> + } >> + id_entry->id = id; >> + stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link); >> + return 0; >> +} >> + >> /* >> * Execute as much of a VDBE program as we can. >> * This is the core of sqlite3_step(). >> @@ -2995,8 +3015,9 @@ case OP_TransactionBegin: { >> * If there is no active transaction, raise an error. >> */ >> case OP_TransactionCommit: { >> - if (box_txn()) { >> - if (box_txn_commit() != 0) { >> + struct txn *txn = in_txn(); >> + if (txn != NULL) { >> + if (txn_commit(txn) != 0) { > From first sigh it doesn’t look like diff related to patch. > Add comment pointing out that txn_commit() doesn’t > invoke fiber_gc() anymore and VDBE takes care of it. Extended comment of this opcode. >> /** >> * Prepare given VDBE to execution: initialize structs connected >> @@ -197,6 +198,15 @@ sql_alloc_txn(); >> int >> sql_vdbe_prepare(struct Vdbe *vdbe); >> >> +/** >> + * Return pointer to list of generated ids. >> + * >> + * @param vdbe VDBE to get list of generated ids from. >> + * @retval List of generated ids. >> + */ >> +struct stailq * >> +vdbe_generated_ids(struct Vdbe *vdbe); > As for me, “generated_ids” seems to be too general name… > I would call it “autoincrement_ids" or sort of. Renamed to autoinc_id_list here. >> + >> int sqlite3VdbeAddOp0(Vdbe *, int); >> int sqlite3VdbeAddOp1(Vdbe *, int, int); >> int sqlite3VdbeAddOp2(Vdbe *, int, int, int); >> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h >> index ce97f49..7a7d3de 100644 >> --- a/src/box/sql/vdbeInt.h >> +++ b/src/box/sql/vdbeInt.h >> @@ -367,6 +367,11 @@ struct Vdbe { >> struct sql_txn *psql_txn; >> /** The auto-commit flag. */ >> bool auto_commit; >> + /** >> + * List of ids generated in current VDBE. It is returned >> + * as metadata of SQL response. >> + */ >> + struct stailq generated_ids; > Again: “autoincrement_ids”. Term “generated_ids” can be confused > with entity ids. For example, during execution of > statement VDBE operates on space id, index id etc (IMHO). Renamed to autoinc_id_list here. >> /* When allocating a new Vdbe object, all of the fields below should be >> * initialized to zero or NULL >> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c >> index 6e42d05..ae73f25 100644 >> --- a/src/box/sql/vdbeaux.c >> +++ b/src/box/sql/vdbeaux.c >> @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse) >> return 0; >> memset(p, 0, sizeof(Vdbe)); >> p->db = db; >> + stailq_create(&p->generated_ids); >> if (db->pVdbe) { >> db->pVdbe->pPrev = p; >> } >> >> @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p) >> * key constraints to hold up the transaction. This means a commit >> * is required. >> */ >> - rc = box_txn_commit() == >> - 0 ? SQLITE_OK : >> - SQL_TARANTOOL_ERROR; >> + rc = (in_txn() == NULL || >> + txn_commit(in_txn()) == 0) ? >> + SQLITE_OK : SQL_TARANTOOL_ERROR; >> closeCursorsAndFree(p); >> } >> if (rc == SQLITE_BUSY && !p->pDelFrame) { >> @@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p) >> } >> p->magic = VDBE_MAGIC_DEAD; >> p->db = 0; >> + if (in_txn() == NULL) >> + fiber_gc(); > Describe this change (in code, obviously): > “VDBE is responsible for releasing region …" > and so on. Added comment. >> sqlite3DbFree(db, p); >> } >> >> diff --git a/src/box/txn.h b/src/box/txn.h >> index e74c14d..87b55da 100644 >> --- a/src/box/txn.h >> +++ b/src/box/txn.h >> @@ -49,6 +49,7 @@ struct engine; >> struct space; >> struct tuple; >> struct xrow_header; >> +struct Vdbe; >> >> enum { >> /** >> @@ -117,6 +118,19 @@ struct sql_txn { >> * VDBE to the next in the same transaction. >> */ >> uint32_t fk_deferred_count; >> + /** Current VDBE. */ >> + struct Vdbe *vdbe; >> +}; >> + >> +/** >> + * An element of list of autogenerated ids, being returned as SQL >> + * response metadata. >> + */ >> +struct id_entry { > “id_entry” is too common name, be more specific pls. Renamed to struct autoinc_id_entry. Still, name of variable is 'id_entry'. >> + /** A link in a generated id list. */ >> + struct stailq_entry link; >> + /** Generated id. */ >> + int64_t id; > These comments are too obvious, they only contaminate > source code. Removed these comments. >> }; >> >> /** >> * FFI bindings: do not throw exceptions, do not accept extra >> diff --git a/test/sql/iproto.result b/test/sql/iproto.result >> index af474bc..af4fc12 100644 >> --- a/test/sql/iproto.result >> +++ b/test/sql/iproto.result >> @@ -580,6 +580,98 @@ cn:close() >> box.sql.execute('drop table test') >> --- >> ... >> +-- gh-2618 Return generated columns after INSERT in IPROTO. >> +-- Return all ids generated in current INSERT statement. > Add pls case where generated ids are less than 0. Added. *Diff between versions:* diff --git a/src/box/execute.c b/src/box/execute.c index ebf3962..64c0be6 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -638,24 +638,24 @@ err:   } else {     keys = 1;     assert(port_tuple->size == 0); -   struct stailq *generated_ids = -     vdbe_generated_ids((struct Vdbe *)stmt); -   uint32_t map_size = stailq_empty(generated_ids) ? 1 : 2; +   struct stailq *autoinc_id_list = +     vdbe_autoinc_id_list((struct Vdbe *)stmt); +   uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;     if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)       goto err;     uint64_t id_count = 0;     int changes = sqlite3_changes(db);     int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +          mp_sizeof_uint(changes); -   if (!stailq_empty(generated_ids)) { -     struct id_entry *id_entry; -     stailq_foreach_entry(id_entry, generated_ids, link) { +   if (!stailq_empty(autoinc_id_list)) { +     struct autoinc_id_entry *id_entry; +     stailq_foreach_entry(id_entry, autoinc_id_list, link) {         size += id_entry->id >= 0 ?           mp_sizeof_uint(id_entry->id) :           mp_sizeof_int(id_entry->id);         id_count++;       } -     size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) + +     size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +         mp_sizeof_array(id_count);     }     char *buf = obuf_alloc(out, size); @@ -665,11 +665,11 @@ err:     }     buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);     buf = mp_encode_uint(buf, changes); -   if (!stailq_empty(generated_ids)) { -     buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS); +   if (!stailq_empty(autoinc_id_list)) { +     buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);       buf = mp_encode_array(buf, id_count); -     struct id_entry *id_entry; -     stailq_foreach_entry(id_entry, generated_ids, link) { +     struct autoinc_id_entry *id_entry; +     stailq_foreach_entry(id_entry, autoinc_id_list, link) {         buf = id_entry->id >= 0 ?               mp_encode_uint(buf, id_entry->id) :               mp_encode_int(buf, id_entry->id); diff --git a/src/box/execute.h b/src/box/execute.h index 614d3d0..77bfd79 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,7 +42,7 @@ extern "C" {  /** Keys of IPROTO_SQL_INFO map. */  enum sql_info_key {   SQL_INFO_ROW_COUNT = 0, - SQL_INFO_GENERATED_IDS = 1, + SQL_INFO_AUTOINCREMENT_IDS = 1,   sql_info_key_MAX,  }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 5a137e0..836ed6e 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -680,11 +680,11 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)   lua_setfield(L, -2, "rowcount");   /*    * If data have two elements then second is -  * SQL_INFO_GENERATED_IDS. +  * SQL_INFO_AUTOINCREMENT_IDS.    */   if (map_size == 2) {     key = mp_decode_uint(data); -   assert(key == SQL_INFO_GENERATED_IDS); +   assert(key == SQL_INFO_AUTOINCREMENT_IDS);     (void)key;     uint64_t count = mp_decode_array(data);     assert(count > 0); @@ -695,7 +695,7 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)       luaL_pushint64(L, id);       lua_rawseti(L, -2, j + 1);     } -   lua_setfield(L, -2, "generated_ids"); +   lua_setfield(L, -2, "autoincrement_ids");   }  } diff --git a/src/box/sequence.c b/src/box/sequence.c index 9a87a56..c9828c0 100644 --- a/src/box/sequence.c +++ b/src/box/sequence.c @@ -196,7 +196,7 @@ sequence_next(struct sequence *seq, int64_t *result)       return -1;     *result = def->start;     if (txn_vdbe() != NULL && -       vdbe_add_new_generated_id(txn_vdbe(), *result) != 0) +       vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)       return -1;     return 0;   } @@ -233,7 +233,7 @@ done:     unreachable();   *result = value;   if (txn_vdbe() != NULL && -     vdbe_add_new_generated_id(txn_vdbe(), value) != 0) +     vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)     return -1;   return 0;  overflow: diff --git a/src/box/sequence.h b/src/box/sequence.h index 7180ead..4419427 100644 --- a/src/box/sequence.h +++ b/src/box/sequence.h @@ -140,17 +140,8 @@ sequence_next(struct sequence *seq, int64_t *result);  int  access_check_sequence(struct sequence *seq); -/** - * Append a new sequence value to Vdbe. List of automatically - * generated identifiers is returned as metadata of SQL response. - * - * @param Vdbe VDBE to save id in. - * @param id ID to save in VDBE. - * @retval 0 Success. - * @retval -1 Error. - */  int -vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id); +vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);  /**   * Create an iterator over sequence data. diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 1e732a9..dd763e7 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -579,22 +579,24 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)  }  struct stailq * -vdbe_generated_ids(struct Vdbe *vdbe) +vdbe_autoinc_id_list(struct Vdbe *vdbe)  { - return &vdbe->generated_ids; + return &vdbe->autoinc_id_list;  }  int -vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id) +vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)  {   assert(vdbe != NULL); - struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry)); + struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc, +              sizeof(*id_entry));   if (id_entry == NULL) { -   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry"); +   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", +      "id_entry");     return -1;   }   id_entry->id = id; - stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link); + stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);   return 0;  } @@ -3014,6 +3016,9 @@ case OP_TransactionBegin: {   *   * Commit Tarantool's transaction.   * If there is no active transaction, raise an error. + * After txn was commited VDBE should take care of region as + * txn_commit() doesn't invoke fiber_gc(). Region is needed to + * get information of autogenerated ids during sql response dump.   */  case OP_TransactionCommit: {   struct txn *txn = in_txn(); diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index ed7fb2f..9546d42 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -205,7 +205,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe);   * @retval List of generated ids.   */  struct stailq * -vdbe_generated_ids(struct Vdbe *vdbe); +vdbe_autoinc_id_list(struct Vdbe *vdbe);  int sqlite3VdbeAddOp0(Vdbe *, int);  int sqlite3VdbeAddOp1(Vdbe *, int, int); diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 19b35b7..d16a51e 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -369,7 +369,7 @@ struct Vdbe {    * List of ids generated in current VDBE. It is returned    * as metadata of SQL response.    */ - struct stailq generated_ids; + struct stailq autoinc_id_list;   /* When allocating a new Vdbe object, all of the fields below should be    * initialized to zero or NULL diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 3efe252..78cf2c4 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -57,7 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)     return 0;   memset(p, 0, sizeof(Vdbe));   p->db = db; - stailq_create(&p->generated_ids); + stailq_create(&p->autoinc_id_list);   if (db->pVdbe) {     db->pVdbe->pPrev = p;   } @@ -2779,6 +2779,10 @@ sqlite3VdbeDelete(Vdbe * p)   }   p->magic = VDBE_MAGIC_DEAD;   p->db = 0; + /* +  * VDBE is responsible for releasing region after txn +  * was commited. +  */   if (in_txn() == NULL)     fiber_gc();   sqlite3DbFree(db, p); diff --git a/src/box/txn.h b/src/box/txn.h index 87b55da..de5cb0d 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -126,10 +126,8 @@ struct sql_txn {   * An element of list of autogenerated ids, being returned as SQL   * response metadata.   */ -struct id_entry { - /** A link in a generated id list. */ +struct autoinc_id_entry {   struct stailq_entry link; - /** Generated id. */   int64_t id;  }; diff --git a/test/sql/iproto.result b/test/sql/iproto.result index af4fc12..06cb4b9 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -37,6 +37,9 @@ box.sql.execute('select * from test')  box.schema.user.grant('guest','read,write,execute', 'universe')  ---  ... +box.schema.user.grant('guest', 'create', 'space') +--- +...  cn = remote.connect(box.cfg.listen)  ---  ... @@ -594,7 +597,7 @@ cn:execute('insert into test values (1, 1)')  ...  cn:execute('insert into test values (null, 2)')  --- -- generated_ids: +- autoincrement_ids:    - 2    rowcount: 1  ... @@ -604,14 +607,14 @@ cn:execute('update test set a = 11 where id == 1')  ...  cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')  --- -- generated_ids: +- autoincrement_ids:    - 101    - 121    rowcount: 4  ...  cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')  --- -- generated_ids: +- autoincrement_ids:    - 122    - 123    - 124 @@ -654,12 +657,27 @@ _ = box.space.TEST:on_replace(push_id)  ...  cn:execute('insert into test values (null, 1)')  --- -- generated_ids: +- autoincrement_ids:    - 127    - 1    - 2    rowcount: 1  ... +box.sql.execute('create table test3 (id int primary key autoincrement)') +--- +... +box.schema.sequence.alter('TEST3', {min=-10000, step=-10}) +--- +... +cn:execute('insert into TEST3 values (null), (null), (null), (null)') +--- +- autoincrement_ids: +  - 1 +  - -9 +  - -19 +  - -29 +  rowcount: 4 +...  cn:close()  ---  ... @@ -672,9 +690,15 @@ s:drop()  sq:drop()  ---  ... +box.sql.execute('drop table test3') +--- +...  box.schema.user.revoke('guest', 'read,write,execute', 'universe')  ---  ... +box.schema.user.revoke('guest', 'create', 'space') +--- +...  space = nil  ---  ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index a2caa57..51ac43b 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -10,6 +10,7 @@ space:replace{4, 5, '6'}  space:replace{7, 8.5, '9'}  box.sql.execute('select * from test')  box.schema.user.grant('guest','read,write,execute', 'universe') +box.schema.user.grant('guest', 'create', 'space')  cn = remote.connect(box.cfg.listen)  cn:ping() @@ -220,12 +221,18 @@ function push_id() s:replace{box.NULL} s:replace{box.NULL} end  _ = box.space.TEST:on_replace(push_id)  cn:execute('insert into test values (null, 1)') +box.sql.execute('create table test3 (id int primary key autoincrement)') +box.schema.sequence.alter('TEST3', {min=-10000, step=-10}) +cn:execute('insert into TEST3 values (null), (null), (null), (null)') +  cn:close()  box.sql.execute('drop table test')  s:drop()  sq:drop() +box.sql.execute('drop table test3')  box.schema.user.revoke('guest', 'read,write,execute', 'universe') +box.schema.user.revoke('guest', 'create', 'space')  space = nil  -- Cleanup xlog *New version:* commit 58f70a2cc6bfbed679f1601b09ba1a5d9db05872 Author: Mergen Imeev Date:   Wed Sep 26 22:07:54 2018 +0300     sql: return all generated ids via IPROTO     According to documentation some JDBC functions have an ability to     return all ids that were generated in executed INSERT statement.     This patch gives a way to implement such functionality. After     this patch all ids autogenerated during VDBE execution will be     saved and returned through IPROTO.     Closes #2618 diff --git a/src/box/execute.c b/src/box/execute.c index 24459b4..64c0be6 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -42,6 +42,7 @@  #include "schema.h"  #include "port.h"  #include "tuple.h" +#include "sql/vdbe.h"  const char *sql_type_strs[] = {      NULL, @@ -637,11 +638,26 @@ err:      } else {          keys = 1;          assert(port_tuple->size == 0); -        if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0) +        struct stailq *autoinc_id_list = +            vdbe_autoinc_id_list((struct Vdbe *)stmt); +        uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2; +        if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)              goto err; +        uint64_t id_count = 0;          int changes = sqlite3_changes(db);          int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +                 mp_sizeof_uint(changes); +        if (!stailq_empty(autoinc_id_list)) { +            struct autoinc_id_entry *id_entry; +            stailq_foreach_entry(id_entry, autoinc_id_list, link) { +                size += id_entry->id >= 0 ? +                    mp_sizeof_uint(id_entry->id) : +                    mp_sizeof_int(id_entry->id); +                id_count++; +            } +            size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) + +                mp_sizeof_array(id_count); +        }          char *buf = obuf_alloc(out, size);          if (buf == NULL) {              diag_set(OutOfMemory, size, "obuf_alloc", "buf"); @@ -649,6 +665,16 @@ err:          }          buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);          buf = mp_encode_uint(buf, changes); +        if (!stailq_empty(autoinc_id_list)) { +            buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS); +            buf = mp_encode_array(buf, id_count); +            struct autoinc_id_entry *id_entry; +            stailq_foreach_entry(id_entry, autoinc_id_list, link) { +                buf = id_entry->id >= 0 ? +                      mp_encode_uint(buf, id_entry->id) : +                      mp_encode_int(buf, id_entry->id); +            } +        }      }      iproto_reply_sql(out, &header_svp, response->sync, schema_version,               keys); diff --git a/src/box/execute.h b/src/box/execute.h index f21393b..77bfd79 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -42,6 +42,7 @@ extern "C" {  /** Keys of IPROTO_SQL_INFO map. */  enum sql_info_key {      SQL_INFO_ROW_COUNT = 0, +    SQL_INFO_AUTOINCREMENT_IDS = 1,      sql_info_key_MAX,  }; diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index a928a4c..836ed6e 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -668,16 +668,35 @@ 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; +    assert(map_size == 1 || map_size == 2); +    lua_newtable(L); +    /* +     * First element in data is SQL_INFO_ROW_COUNT. +     */      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);      lua_pushinteger(L, row_count);      lua_setfield(L, -2, "rowcount"); +    /* +     * If data have two elements then second is +     * SQL_INFO_AUTOINCREMENT_IDS. +     */ +    if (map_size == 2) { +        key = mp_decode_uint(data); +        assert(key == SQL_INFO_AUTOINCREMENT_IDS); +        (void)key; +        uint64_t count = mp_decode_array(data); +        assert(count > 0); +        lua_createtable(L, 0, count); +        for (uint32_t j = 0; j < count; ++j) { +            int64_t id; +            mp_read_int64(data, &id); +            luaL_pushint64(L, id); +            lua_rawseti(L, -2, j + 1); +        } +        lua_setfield(L, -2, "autoincrement_ids"); +    }  }  static int diff --git a/src/box/sequence.c b/src/box/sequence.c index 35b7605..c9828c0 100644 --- a/src/box/sequence.c +++ b/src/box/sequence.c @@ -38,6 +38,7 @@  #include  #include +#include "txn.h"  #include "diag.h"  #include "error.h"  #include "errcode.h" @@ -194,6 +195,9 @@ sequence_next(struct sequence *seq, int64_t *result)                        new_data) == light_sequence_end)              return -1;          *result = def->start; +        if (txn_vdbe() != NULL && +            vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0) +            return -1;          return 0;      }      old_data = light_sequence_get(&sequence_data_index, pos); @@ -228,6 +232,9 @@ done:                     new_data, &old_data) == light_sequence_end)          unreachable();      *result = value; +    if (txn_vdbe() != NULL && +        vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0) +        return -1;      return 0;  overflow:      if (!def->cycle) { diff --git a/src/box/sequence.h b/src/box/sequence.h index 0f6c8da..4419427 100644 --- a/src/box/sequence.h +++ b/src/box/sequence.h @@ -43,6 +43,7 @@ extern "C" {  #endif /* defined(__cplusplus) */  struct iterator; +struct Vdbe;  /** Sequence metadata. */  struct sequence_def { @@ -139,6 +140,9 @@ sequence_next(struct sequence *seq, int64_t *result);  int  access_check_sequence(struct sequence *seq); +int +vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id); +  /**   * Create an iterator over sequence data.   * diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index d6d6b7e..78fd28a 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -578,6 +578,28 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)      }  } +struct stailq * +vdbe_autoinc_id_list(struct Vdbe *vdbe) +{ +    return &vdbe->autoinc_id_list; +} + +int +vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id) +{ +    assert(vdbe != NULL); +    struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc, +                             sizeof(*id_entry)); +    if (id_entry == NULL) { +        diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", +             "id_entry"); +        return -1; +    } +    id_entry->id = id; +    stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link); +    return 0; +} +  /*   * Execute as much of a VDBE program as we can.   * This is the core of sqlite3_step(). @@ -2997,10 +3019,14 @@ case OP_TransactionBegin: {   *   * Commit Tarantool's transaction.   * If there is no active transaction, raise an error. + * After txn was commited VDBE should take care of region as + * txn_commit() doesn't invoke fiber_gc(). Region is needed to + * get information of autogenerated ids during sql response dump.   */  case OP_TransactionCommit: { -    if (box_txn()) { -        if (box_txn_commit() != 0) { +    struct txn *txn = in_txn(); +    if (txn != NULL) { +        if (txn_commit(txn) != 0) {              rc = SQL_TARANTOOL_ERROR;              goto abort_due_to_error;          } @@ -3044,7 +3070,7 @@ case OP_TransactionRollback: {   */  case OP_TTransaction: {      if (!box_txn()) { -        if (box_txn_begin() != 0) { +        if (sql_txn_begin(p) != 0) {              rc = SQL_TARANTOOL_ERROR;              goto abort_due_to_error;          } diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h index d5da5d5..9546d42 100644 --- a/src/box/sql/vdbe.h +++ b/src/box/sql/vdbe.h @@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);   * Allocate and initialize SQL-specific struct which completes   * original Tarantool's txn struct using region allocator.   * + * @param v Vdbe with which associate this transaction.   * @retval NULL on OOM, new psql_txn struct on success.   **/  struct sql_txn * -sql_alloc_txn(); +sql_alloc_txn(struct Vdbe *v);  /**   * Prepare given VDBE to execution: initialize structs connected @@ -197,6 +198,15 @@ sql_alloc_txn();  int  sql_vdbe_prepare(struct Vdbe *vdbe); +/** + * Return pointer to list of generated ids. + * + * @param vdbe VDBE to get list of generated ids from. + * @retval List of generated ids. + */ +struct stailq * +vdbe_autoinc_id_list(struct Vdbe *vdbe); +  int sqlite3VdbeAddOp0(Vdbe *, int);  int sqlite3VdbeAddOp1(Vdbe *, int, int);  int sqlite3VdbeAddOp2(Vdbe *, int, int, int); diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index ce97f49..cfb327a 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -367,6 +367,11 @@ struct Vdbe {      struct sql_txn *psql_txn;      /** The auto-commit flag. */      bool auto_commit; +    /** +     * List of ids generated in current VDBE. It is returned +     * as metadata of SQL response. +     */ +    struct stailq autoinc_id_list;      /* When allocating a new Vdbe object, all of the fields below should be       * initialized to zero or NULL diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index 6e42d05..1e72a4d 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)          return 0;      memset(p, 0, sizeof(Vdbe));      p->db = db; +    stailq_create(&p->autoinc_id_list);      if (db->pVdbe) {          db->pVdbe->pPrev = p;      } @@ -75,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse)  }  struct sql_txn * -sql_alloc_txn() +sql_alloc_txn(struct Vdbe *v)  {      struct sql_txn *txn = region_alloc_object(&fiber()->gc,                            struct sql_txn); @@ -84,6 +85,8 @@ sql_alloc_txn()               "struct sql_txn");          return NULL;      } +    txn->vdbe = v; +    v->psql_txn = txn;      txn->pSavepoint = NULL;      txn->fk_deferred_count = 0;      return txn; @@ -103,11 +106,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)           * check FK violations, at least now.           */          if (txn->psql_txn == NULL) { -            txn->psql_txn = sql_alloc_txn(); +            txn->psql_txn = sql_alloc_txn(vdbe);              if (txn->psql_txn == NULL)                  return -1; +        } else { +            vdbe->psql_txn = txn->psql_txn;          } -        vdbe->psql_txn = txn->psql_txn;      } else {          vdbe->psql_txn = NULL;      } @@ -2261,12 +2265,11 @@ sql_txn_begin(Vdbe *p)      struct txn *ptxn = txn_begin(false);      if (ptxn == NULL)          return -1; -    ptxn->psql_txn = sql_alloc_txn(); +    ptxn->psql_txn = sql_alloc_txn(p);      if (ptxn->psql_txn == NULL) {          box_txn_rollback();          return -1;      } -    p->psql_txn = ptxn->psql_txn;      return 0;  } @@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)                       * key constraints to hold up the transaction. This means a commit                       * is required.                       */ -                    rc = box_txn_commit() == -                            0 ? SQLITE_OK : -                            SQL_TARANTOOL_ERROR; +                    rc = (in_txn() == NULL || +                          txn_commit(in_txn()) == 0) ? +                         SQLITE_OK : SQL_TARANTOOL_ERROR;                      closeCursorsAndFree(p);                  }                  if (rc == SQLITE_BUSY && !p->pDelFrame) { @@ -2780,6 +2783,12 @@ sqlite3VdbeDelete(Vdbe * p)      }      p->magic = VDBE_MAGIC_DEAD;      p->db = 0; +    /* +     * VDBE is responsible for releasing region after txn +     * was commited. +     */ +    if (in_txn() == NULL) +        fiber_gc();      sqlite3DbFree(db, p);  } diff --git a/src/box/txn.h b/src/box/txn.h index e74c14d..de5cb0d 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -49,6 +49,7 @@ struct engine;  struct space;  struct tuple;  struct xrow_header; +struct Vdbe;  enum {      /** @@ -117,6 +118,17 @@ struct sql_txn {       * VDBE to the next in the same transaction.       */      uint32_t fk_deferred_count; +    /** Current VDBE. */ +    struct Vdbe *vdbe; +}; + +/** + * An element of list of autogenerated ids, being returned as SQL + * response metadata. + */ +struct autoinc_id_entry { +    struct stailq_entry link; +    int64_t id;  };  struct txn { @@ -337,6 +349,20 @@ txn_last_stmt(struct txn *txn)  void  txn_on_stop(struct trigger *trigger, void *event); +/** + * Return VDBE that is being currently executed. + * + * @retval VDBE that is being currently executed. + * @retval NULL Either txn or ptxn_sql or vdbe is NULL; + */ +static inline struct Vdbe * +txn_vdbe() +{ +    struct txn *txn = in_txn(); +    if (txn == NULL || txn->psql_txn == NULL) +        return NULL; +    return txn->psql_txn->vdbe; +}  /**   * FFI bindings: do not throw exceptions, do not accept extra diff --git a/test/sql/iproto.result b/test/sql/iproto.result index ee34f6f..06cb4b9 100644 --- a/test/sql/iproto.result +++ b/test/sql/iproto.result @@ -583,6 +583,116 @@ cn:close()  box.sql.execute('drop table test')  ---  ... +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return all ids generated in current INSERT statement. +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)') +--- +- rowcount: 1 +... +cn:execute('insert into test values (null, 2)') +--- +- autoincrement_ids: +  - 2 +  rowcount: 1 +... +cn:execute('update test set a = 11 where id == 1') +--- +- rowcount: 1 +... +cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)') +--- +- autoincrement_ids: +  - 101 +  - 121 +  rowcount: 4 +... +cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)') +--- +- autoincrement_ids: +  - 122 +  - 123 +  - 124 +  - 125 +  - 126 +  rowcount: 5 +... +cn:execute('select * from test') +--- +- metadata: +  - name: ID +  - name: A +  rows: +  - [1, 11] +  - [2, 2] +  - [100, 1] +  - [101, 1] +  - [120, 1] +  - [121, 1] +  - [122, 1] +  - [123, 1] +  - [124, 1] +  - [125, 1] +  - [126, 1] +... +s = box.schema.create_space('test2', {engine = engine}) +--- +... +sq = box.schema.sequence.create('test2') +--- +... +pk = s:create_index('pk', {sequence = 'test2'}) +--- +... +function push_id() s:replace{box.NULL} s:replace{box.NULL} end +--- +... +_ = box.space.TEST:on_replace(push_id) +--- +... +cn:execute('insert into test values (null, 1)') +--- +- autoincrement_ids: +  - 127 +  - 1 +  - 2 +  rowcount: 1 +... +box.sql.execute('create table test3 (id int primary key autoincrement)') +--- +... +box.schema.sequence.alter('TEST3', {min=-10000, step=-10}) +--- +... +cn:execute('insert into TEST3 values (null), (null), (null), (null)') +--- +- autoincrement_ids: +  - 1 +  - -9 +  - -19 +  - -29 +  rowcount: 4 +... +cn:close() +--- +... +box.sql.execute('drop table test') +--- +... +s:drop() +--- +... +sq:drop() +--- +... +box.sql.execute('drop table test3') +--- +...  box.schema.user.revoke('guest', 'read,write,execute', 'universe')  ---  ... diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua index b065590..51ac43b 100644 --- a/test/sql/iproto.test.lua +++ b/test/sql/iproto.test.lua @@ -202,6 +202,35 @@ future4:wait_result()  cn:close()  box.sql.execute('drop table test') +-- gh-2618 Return generated columns after INSERT in IPROTO. +-- Return all ids generated in current INSERT statement. +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)') +cn:execute('insert into test values (null, 2)') +cn:execute('update test set a = 11 where id == 1') +cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)') +cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)') +cn:execute('select * from test') + +s = box.schema.create_space('test2', {engine = engine}) +sq = box.schema.sequence.create('test2') +pk = s:create_index('pk', {sequence = 'test2'}) +function push_id() s:replace{box.NULL} s:replace{box.NULL} end +_ = box.space.TEST:on_replace(push_id) +cn:execute('insert into test values (null, 1)') + +box.sql.execute('create table test3 (id int primary key autoincrement)') +box.schema.sequence.alter('TEST3', {min=-10000, step=-10}) +cn:execute('insert into TEST3 values (null), (null), (null), (null)') + +cn:close() +box.sql.execute('drop table test') +s:drop() +sq:drop() +box.sql.execute('drop table test3') +  box.schema.user.revoke('guest', 'read,write,execute', 'universe')  box.schema.user.revoke('guest', 'create', 'space')  space = nil --------------35B158489B476B8C7ED56780 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit Hi! Thank you for review! My answer, diff between patches and new
patch below. Changes were made only in commit named by "sql:
return all generated ids via IPROTO". After changes were made and
diff saved I rebased this branch to current 2.1. After this new
patch description were formed, so it is a bit different from
old patch with applied diff.

On 10/30/18 5:30 PM, n.pettik wrote:


      
On 29 Oct 2018, at 20:33, imeevma@tarantool.org wrote:

According to documentation some JDBC functions have an ability to
return all ids that were generated in executed INSERT statement.
This patch gives a way to implement such functionality.
I guess you should be more specific in describing such things.
I don’t ask you to dive into details, but explaining main idea of
implementation would definitely help reviewer to do his/her job.
A bit extended this message. Not sure if it is enough.

      
Closes #2618
—
diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..ebf3962 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -42,6 +42,7 @@
#include "schema.h"
#include "port.h"
#include "tuple.h"
+#include "sql/vdbe.h"


diff --git a/src/box/execute.h b/src/box/execute.h
index f21393b..614d3d0 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,6 +42,7 @@ extern "C" {
/** Keys of IPROTO_SQL_INFO map. */
enum sql_info_key {
	SQL_INFO_ROW_COUNT = 0,
+	SQL_INFO_GENERATED_IDS = 1,
I vote for _AUTOINCREMENT_IDS, see comments below.
Anyway, in JDBC it is called GENERATED_KEYS. So, we either
follow JDBC convention or use our naming. In the latter case,
AUTOINCREMENT_IDS sounds more solid.
Renamed to SQL_INFO_AUTOINCREMENT_IDS.

      
	sql_info_key_MAX,
};

/**
+ * Append a new sequence value to Vdbe. List of automatically
+ * generated identifiers is returned as metadata of SQL response.
+ *
+ * @param Vdbe VDBE to save id in.
+ * @param id ID to save in VDBE.
+ * @retval 0 Success.
+ * @retval -1 Error.
It looks terrible. Does this function really need comment at all?
Removed.

      
+ */
+int
+vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);

+
+int
+vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
+{
+	assert(vdbe != NULL);
+	struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
+	if (id_entry == NULL) {
+		diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry”);
Out of 80.

+		return -1;
+	}
+	id_entry->id = id;
+	stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
+	return 0;
+}
+
/*
 * Execute as much of a VDBE program as we can.
 * This is the core of sqlite3_step().
@@ -2995,8 +3015,9 @@ case OP_TransactionBegin: {
 * If there is no active transaction, raise an error.
 */
case OP_TransactionCommit: {
-	if (box_txn()) {
-		if (box_txn_commit() != 0) {
+	struct txn *txn = in_txn();
+	if (txn != NULL) {
+		if (txn_commit(txn) != 0) {
From first sigh it doesn’t look like diff related to patch.
Add comment pointing out that txn_commit() doesn’t
invoke fiber_gc() anymore and VDBE takes care of it.
Extended comment of this opcode.

      
/**
 * Prepare given VDBE to execution: initialize structs connected
@@ -197,6 +198,15 @@ sql_alloc_txn();
int
sql_vdbe_prepare(struct Vdbe *vdbe);

+/**
+ * Return pointer to list of generated ids.
+ *
+ * @param vdbe VDBE to get list of generated ids from.
+ * @retval List of generated ids.
+ */
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe);
As for me, “generated_ids” seems to be too general name…
I would call it “autoincrement_ids" or sort of.
Renamed to autoinc_id_list here.

      
+
int sqlite3VdbeAddOp0(Vdbe *, int);
int sqlite3VdbeAddOp1(Vdbe *, int, int);
int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..7a7d3de 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,6 +367,11 @@ struct Vdbe {
	struct sql_txn *psql_txn;
	/** The auto-commit flag. */
	bool auto_commit;
+	/**
+	 * List of ids generated in current VDBE. It is returned
+	 * as metadata of SQL response.
+	 */
+	struct stailq generated_ids;
Again: “autoincrement_ids”. Term “generated_ids” can be confused
with entity ids. For example, during execution of <CREATE TABLE>
statement VDBE operates on space id, index id etc (IMHO).
Renamed to autoinc_id_list here.

      
	/* When allocating a new Vdbe object, all of the fields below should be
	 * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d05..ae73f25 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
		return 0;
	memset(p, 0, sizeof(Vdbe));
	p->db = db;
+	stailq_create(&p->generated_ids);
	if (db->pVdbe) {
		db->pVdbe->pPrev = p;
	}

@@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
					 * key constraints to hold up the transaction. This means a commit
					 * is required.
					 */
-					rc = box_txn_commit() ==
-						    0 ? SQLITE_OK :
-						    SQL_TARANTOOL_ERROR;
+					rc = (in_txn() == NULL ||
+					      txn_commit(in_txn()) == 0) ?
+					     SQLITE_OK : SQL_TARANTOOL_ERROR;
					closeCursorsAndFree(p);
				}
				if (rc == SQLITE_BUSY && !p->pDelFrame) {
@@ -2780,6 +2783,8 @@ sqlite3VdbeDelete(Vdbe * p)
	}
	p->magic = VDBE_MAGIC_DEAD;
	p->db = 0;
+	if (in_txn() == NULL)
+		fiber_gc();
Describe this change (in code, obviously):
“VDBE is responsible for releasing region …"
and so on.
Added comment.

      
	sqlite3DbFree(db, p);
}

diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..87b55da 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,7 @@ struct engine;
struct space;
struct tuple;
struct xrow_header;
+struct Vdbe;

enum {
	/**
@@ -117,6 +118,19 @@ struct sql_txn {
	 * VDBE to the next in the same transaction.
	 */
	uint32_t fk_deferred_count;
+	/** Current VDBE. */
+	struct Vdbe *vdbe;
+};
+
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct id_entry {
“id_entry” is too common name, be more specific pls. 
Renamed to struct autoinc_id_entry. Still, name of variable is
'id_entry'.

      
+	/** A link in a generated id list. */
+	struct stailq_entry link;
+	/** Generated id. */
+	int64_t id;
These comments are too obvious, they only contaminate
source code. 
Removed these comments.

      
};

/**
 * FFI bindings: do not throw exceptions, do not accept extra
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af474bc..af4fc12 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -580,6 +580,98 @@ cn:close()
box.sql.execute('drop table test')
---
...
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
Add pls case where generated ids are less than 0.

Added.


Diff between versions:

diff --git a/src/box/execute.c b/src/box/execute.c
index ebf3962..64c0be6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -638,24 +638,24 @@ err:
  } else {
    keys = 1;
    assert(port_tuple->size == 0);
-   struct stailq *generated_ids =
-     vdbe_generated_ids((struct Vdbe *)stmt);
-   uint32_t map_size = stailq_empty(generated_ids) ? 1 : 2;
+   struct stailq *autoinc_id_list =
+     vdbe_autoinc_id_list((struct Vdbe *)stmt);
+   uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
    if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
      goto err;
    uint64_t id_count = 0;
    int changes = sqlite3_changes(db);
    int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
         mp_sizeof_uint(changes);
-   if (!stailq_empty(generated_ids)) {
-     struct id_entry *id_entry;
-     stailq_foreach_entry(id_entry, generated_ids, link) {
+   if (!stailq_empty(autoinc_id_list)) {
+     struct autoinc_id_entry *id_entry;
+     stailq_foreach_entry(id_entry, autoinc_id_list, link) {
        size += id_entry->id >= 0 ?
          mp_sizeof_uint(id_entry->id) :
          mp_sizeof_int(id_entry->id);
        id_count++;
      }
-     size += mp_sizeof_uint(SQL_INFO_GENERATED_IDS) +
+     size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
        mp_sizeof_array(id_count);
    }
    char *buf = obuf_alloc(out, size);
@@ -665,11 +665,11 @@ err:
    }
    buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
    buf = mp_encode_uint(buf, changes);
-   if (!stailq_empty(generated_ids)) {
-     buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+   if (!stailq_empty(autoinc_id_list)) {
+     buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
      buf = mp_encode_array(buf, id_count);
-     struct id_entry *id_entry;
-     stailq_foreach_entry(id_entry, generated_ids, link) {
+     struct autoinc_id_entry *id_entry;
+     stailq_foreach_entry(id_entry, autoinc_id_list, link) {
        buf = id_entry->id >= 0 ?
              mp_encode_uint(buf, id_entry->id) :
              mp_encode_int(buf, id_entry->id);
diff --git a/src/box/execute.h b/src/box/execute.h
index 614d3d0..77bfd79 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,7 +42,7 @@ extern "C" {
 /** Keys of IPROTO_SQL_INFO map. */
 enum sql_info_key {
  SQL_INFO_ROW_COUNT = 0,
- SQL_INFO_GENERATED_IDS = 1,
+ SQL_INFO_AUTOINCREMENT_IDS = 1,
  sql_info_key_MAX,
 };
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 5a137e0..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -680,11 +680,11 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
  lua_setfield(L, -2, "rowcount");
  /*
   * If data have two elements then second is
-  * SQL_INFO_GENERATED_IDS.
+  * SQL_INFO_AUTOINCREMENT_IDS.
   */
  if (map_size == 2) {
    key = mp_decode_uint(data);
-   assert(key == SQL_INFO_GENERATED_IDS);
+   assert(key == SQL_INFO_AUTOINCREMENT_IDS);
    (void)key;
    uint64_t count = mp_decode_array(data);
    assert(count > 0);
@@ -695,7 +695,7 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
      luaL_pushint64(L, id);
      lua_rawseti(L, -2, j + 1);
    }
-   lua_setfield(L, -2, "generated_ids");
+   lua_setfield(L, -2, "autoincrement_ids");
  }
 }
 
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 9a87a56..c9828c0 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -196,7 +196,7 @@ sequence_next(struct sequence *seq, int64_t *result)
      return -1;
    *result = def->start;
    if (txn_vdbe() != NULL &&
-       vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+       vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
      return -1;
    return 0;
  }
@@ -233,7 +233,7 @@ done:
    unreachable();
  *result = value;
  if (txn_vdbe() != NULL &&
-     vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
+     vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
    return -1;
  return 0;
 overflow:
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 7180ead..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -140,17 +140,8 @@ sequence_next(struct sequence *seq, int64_t *result);
 int
 access_check_sequence(struct sequence *seq);
 
-/**
- * Append a new sequence value to Vdbe. List of automatically
- * generated identifiers is returned as metadata of SQL response.
- *
- * @param Vdbe VDBE to save id in.
- * @param id ID to save in VDBE.
- * @retval 0 Success.
- * @retval -1 Error.
- */
 int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id);
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
 
 /**
  * Create an iterator over sequence data.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1e732a9..dd763e7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -579,22 +579,24 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
 }
 
 struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe)
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
 {
- return &vdbe->generated_ids;
+ return &vdbe->autoinc_id_list;
 }
 
 int
-vdbe_add_new_generated_id(struct Vdbe *vdbe, int64_t id)
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 {
  assert(vdbe != NULL);
- struct id_entry *id_entry = region_alloc(&fiber()->gc, sizeof(*id_entry));
+ struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+              sizeof(*id_entry));
  if (id_entry == NULL) {
-   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc", "id_entry");
+   diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+      "id_entry");
    return -1;
  }
  id_entry->id = id;
- stailq_add_tail_entry(vdbe_generated_ids(vdbe), id_entry, link);
+ stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
  return 0;
 }
 
@@ -3014,6 +3016,9 @@ case OP_TransactionBegin: {
  *
  * Commit Tarantool's transaction.
  * If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
  */
 case OP_TransactionCommit: {
  struct txn *txn = in_txn();
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index ed7fb2f..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -205,7 +205,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe);
  * @retval List of generated ids.
  */
 struct stailq *
-vdbe_generated_ids(struct Vdbe *vdbe);
+vdbe_autoinc_id_list(struct Vdbe *vdbe);
 
 int sqlite3VdbeAddOp0(Vdbe *, int);
 int sqlite3VdbeAddOp1(Vdbe *, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 19b35b7..d16a51e 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -369,7 +369,7 @@ struct Vdbe {
   * List of ids generated in current VDBE. It is returned
   * as metadata of SQL response.
   */
- struct stailq generated_ids;
+ struct stailq autoinc_id_list;
 
  /* When allocating a new Vdbe object, all of the fields below should be
   * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 3efe252..78cf2c4 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,7 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
    return 0;
  memset(p, 0, sizeof(Vdbe));
  p->db = db;
- stailq_create(&p->generated_ids);
+ stailq_create(&p->autoinc_id_list);
  if (db->pVdbe) {
    db->pVdbe->pPrev = p;
  }
@@ -2779,6 +2779,10 @@ sqlite3VdbeDelete(Vdbe * p)
  }
  p->magic = VDBE_MAGIC_DEAD;
  p->db = 0;
+ /*
+  * VDBE is responsible for releasing region after txn
+  * was commited.
+  */
  if (in_txn() == NULL)
    fiber_gc();
  sqlite3DbFree(db, p);
diff --git a/src/box/txn.h b/src/box/txn.h
index 87b55da..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -126,10 +126,8 @@ struct sql_txn {
  * An element of list of autogenerated ids, being returned as SQL
  * response metadata.
  */
-struct id_entry {
- /** A link in a generated id list. */
+struct autoinc_id_entry {
  struct stailq_entry link;
- /** Generated id. */
  int64_t id;
 };
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index af4fc12..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -37,6 +37,9 @@ box.sql.execute('select * from test')
 box.schema.user.grant('guest','read,write,execute', 'universe')
 ---
 ...
+box.schema.user.grant('guest', 'create', 'space')
+---
+...
 cn = remote.connect(box.cfg.listen)
 ---
 ...
@@ -594,7 +597,7 @@ cn:execute('insert into test values (1, 1)')
 ...
 cn:execute('insert into test values (null, 2)')
 ---
-- generated_ids:
+- autoincrement_ids:
   - 2
   rowcount: 1
 ...
@@ -604,14 +607,14 @@ cn:execute('update test set a = 11 where id == 1')
 ...
 cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')
 ---
-- generated_ids:
+- autoincrement_ids:
   - 101
   - 121
   rowcount: 4
 ...
 cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
 ---
-- generated_ids:
+- autoincrement_ids:
   - 122
   - 123
   - 124
@@ -654,12 +657,27 @@ _ = box.space.TEST:on_replace(push_id)
 ...
 cn:execute('insert into test values (null, 1)')
 ---
-- generated_ids:
+- autoincrement_ids:
   - 127
   - 1
   - 2
   rowcount: 1
 ...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+  - 1
+  - -9
+  - -19
+  - -29
+  rowcount: 4
+...
 cn:close()
 ---
 ...
@@ -672,9 +690,15 @@ s:drop()
 sq:drop()
 ---
 ...
+box.sql.execute('drop table test3')
+---
+...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
+box.schema.user.revoke('guest', 'create', 'space')
+---
+...
 space = nil
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index a2caa57..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -10,6 +10,7 @@ space:replace{4, 5, '6'}
 space:replace{7, 8.5, '9'}
 box.sql.execute('select * from test')
 box.schema.user.grant('guest','read,write,execute', 'universe')
+box.schema.user.grant('guest', 'create', 'space')
 cn = remote.connect(box.cfg.listen)
 cn:ping()
 
@@ -220,12 +221,18 @@ function push_id() s:replace{box.NULL} s:replace{box.NULL} end
 _ = box.space.TEST:on_replace(push_id)
 cn:execute('insert into test values (null, 1)')
 
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
 cn:close()
 box.sql.execute('drop table test')
 s:drop()
 sq:drop()
+box.sql.execute('drop table test3')
 
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+box.schema.user.revoke('guest', 'create', 'space')
 space = nil
 
 -- Cleanup xlog

New version:

commit 58f70a2cc6bfbed679f1601b09ba1a5d9db05872
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Sep 26 22:07:54 2018 +0300

    sql: return all generated ids via IPROTO
   
    According to documentation some JDBC functions have an ability to
    return all ids that were generated in executed INSERT statement.
    This patch gives a way to implement such functionality. After
    this patch all ids autogenerated during VDBE execution will be
    saved and returned through IPROTO.
   
    Closes #2618

diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4..64c0be6 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -42,6 +42,7 @@
 #include "schema.h"
 #include "port.h"
 #include "tuple.h"
+#include "sql/vdbe.h"
 
 const char *sql_type_strs[] = {
     NULL,
@@ -637,11 +638,26 @@ err:
     } else {
         keys = 1;
         assert(port_tuple->size == 0);
-        if (iproto_reply_map_key(out, 1, IPROTO_SQL_INFO) != 0)
+        struct stailq *autoinc_id_list =
+            vdbe_autoinc_id_list((struct Vdbe *)stmt);
+        uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2;
+        if (iproto_reply_map_key(out, map_size, IPROTO_SQL_INFO) != 0)
             goto err;
+        uint64_t id_count = 0;
         int changes = sqlite3_changes(db);
         int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) +
                mp_sizeof_uint(changes);
+        if (!stailq_empty(autoinc_id_list)) {
+            struct autoinc_id_entry *id_entry;
+            stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+                size += id_entry->id >= 0 ?
+                    mp_sizeof_uint(id_entry->id) :
+                    mp_sizeof_int(id_entry->id);
+                id_count++;
+            }
+            size += mp_sizeof_uint(SQL_INFO_AUTOINCREMENT_IDS) +
+                mp_sizeof_array(id_count);
+        }
         char *buf = obuf_alloc(out, size);
         if (buf == NULL) {
             diag_set(OutOfMemory, size, "obuf_alloc", "buf");
@@ -649,6 +665,16 @@ err:
         }
         buf = mp_encode_uint(buf, SQL_INFO_ROW_COUNT);
         buf = mp_encode_uint(buf, changes);
+        if (!stailq_empty(autoinc_id_list)) {
+            buf = mp_encode_uint(buf, SQL_INFO_AUTOINCREMENT_IDS);
+            buf = mp_encode_array(buf, id_count);
+            struct autoinc_id_entry *id_entry;
+            stailq_foreach_entry(id_entry, autoinc_id_list, link) {
+                buf = id_entry->id >= 0 ?
+                      mp_encode_uint(buf, id_entry->id) :
+                      mp_encode_int(buf, id_entry->id);
+            }
+        }
     }
     iproto_reply_sql(out, &header_svp, response->sync, schema_version,
              keys);
diff --git a/src/box/execute.h b/src/box/execute.h
index f21393b..77bfd79 100644
--- a/src/box/execute.h
+++ b/src/box/execute.h
@@ -42,6 +42,7 @@ extern "C" {
 /** Keys of IPROTO_SQL_INFO map. */
 enum sql_info_key {
     SQL_INFO_ROW_COUNT = 0,
+    SQL_INFO_AUTOINCREMENT_IDS = 1,
     sql_info_key_MAX,
 };
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..836ed6e 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -668,16 +668,35 @@ 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;
+    assert(map_size == 1 || map_size == 2);
+    lua_newtable(L);
+    /*
+     * First element in data is SQL_INFO_ROW_COUNT.
+     */
     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);
     lua_pushinteger(L, row_count);
     lua_setfield(L, -2, "rowcount");
+    /*
+     * If data have two elements then second is
+     * SQL_INFO_AUTOINCREMENT_IDS.
+     */
+    if (map_size == 2) {
+        key = mp_decode_uint(data);
+        assert(key == SQL_INFO_AUTOINCREMENT_IDS);
+        (void)key;
+        uint64_t count = mp_decode_array(data);
+        assert(count > 0);
+        lua_createtable(L, 0, count);
+        for (uint32_t j = 0; j < count; ++j) {
+            int64_t id;
+            mp_read_int64(data, &id);
+            luaL_pushint64(L, id);
+            lua_rawseti(L, -2, j + 1);
+        }
+        lua_setfield(L, -2, "autoincrement_ids");
+    }
 }
 
 static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..c9828c0 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -38,6 +38,7 @@
 #include <small/mempool.h>
 #include <msgpuck/msgpuck.h>
 
+#include "txn.h"
 #include "diag.h"
 #include "error.h"
 #include "errcode.h"
@@ -194,6 +195,9 @@ sequence_next(struct sequence *seq, int64_t *result)
                       new_data) == light_sequence_end)
             return -1;
         *result = def->start;
+        if (txn_vdbe() != NULL &&
+            vdbe_add_new_autoinc_id(txn_vdbe(), *result) != 0)
+            return -1;
         return 0;
     }
     old_data = light_sequence_get(&sequence_data_index, pos);
@@ -228,6 +232,9 @@ done:
                    new_data, &old_data) == light_sequence_end)
         unreachable();
     *result = value;
+    if (txn_vdbe() != NULL &&
+        vdbe_add_new_autoinc_id(txn_vdbe(), value) != 0)
+        return -1;
     return 0;
 overflow:
     if (!def->cycle) {
diff --git a/src/box/sequence.h b/src/box/sequence.h
index 0f6c8da..4419427 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -43,6 +43,7 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 struct iterator;
+struct Vdbe;
 
 /** Sequence metadata. */
 struct sequence_def {
@@ -139,6 +140,9 @@ sequence_next(struct sequence *seq, int64_t *result);
 int
 access_check_sequence(struct sequence *seq);
 
+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id);
+
 /**
  * Create an iterator over sequence data.
  *
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index d6d6b7e..78fd28a 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,28 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
     }
 }
 
+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe)
+{
+    return &vdbe->autoinc_id_list;
+}
+
+int
+vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
+{
+    assert(vdbe != NULL);
+    struct autoinc_id_entry *id_entry = region_alloc(&fiber()->gc,
+                             sizeof(*id_entry));
+    if (id_entry == NULL) {
+        diag_set(OutOfMemory, sizeof(*id_entry), "region_alloc",
+             "id_entry");
+        return -1;
+    }
+    id_entry->id = id;
+    stailq_add_tail_entry(vdbe_autoinc_id_list(vdbe), id_entry, link);
+    return 0;
+}
+
 /*
  * Execute as much of a VDBE program as we can.
  * This is the core of sqlite3_step().
@@ -2997,10 +3019,14 @@ case OP_TransactionBegin: {
  *
  * Commit Tarantool's transaction.
  * If there is no active transaction, raise an error.
+ * After txn was commited VDBE should take care of region as
+ * txn_commit() doesn't invoke fiber_gc(). Region is needed to
+ * get information of autogenerated ids during sql response dump.
  */
 case OP_TransactionCommit: {
-    if (box_txn()) {
-        if (box_txn_commit() != 0) {
+    struct txn *txn = in_txn();
+    if (txn != NULL) {
+        if (txn_commit(txn) != 0) {
             rc = SQL_TARANTOOL_ERROR;
             goto abort_due_to_error;
         }
@@ -3044,7 +3070,7 @@ case OP_TransactionRollback: {
  */
 case OP_TTransaction: {
     if (!box_txn()) {
-        if (box_txn_begin() != 0) {
+        if (sql_txn_begin(p) != 0) {
             rc = SQL_TARANTOOL_ERROR;
             goto abort_due_to_error;
         }
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index d5da5d5..9546d42 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -179,10 +179,11 @@ Vdbe *sqlite3VdbeCreate(Parse *);
  * Allocate and initialize SQL-specific struct which completes
  * original Tarantool's txn struct using region allocator.
  *
+ * @param v Vdbe with which associate this transaction.
  * @retval NULL on OOM, new psql_txn struct on success.
  **/
 struct sql_txn *
-sql_alloc_txn();
+sql_alloc_txn(struct Vdbe *v);
 
 /**
  * Prepare given VDBE to execution: initialize structs connected
@@ -197,6 +198,15 @@ sql_alloc_txn();
 int
 sql_vdbe_prepare(struct Vdbe *vdbe);
 
+/**
+ * Return pointer to list of generated ids.
+ *
+ * @param vdbe VDBE to get list of generated ids from.
+ * @retval List of generated ids.
+ */
+struct stailq *
+vdbe_autoinc_id_list(struct Vdbe *vdbe);
+
 int sqlite3VdbeAddOp0(Vdbe *, int);
 int sqlite3VdbeAddOp1(Vdbe *, int, int);
 int sqlite3VdbeAddOp2(Vdbe *, int, int, int);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index ce97f49..cfb327a 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,6 +367,11 @@ struct Vdbe {
     struct sql_txn *psql_txn;
     /** The auto-commit flag. */
     bool auto_commit;
+    /**
+     * List of ids generated in current VDBE. It is returned
+     * as metadata of SQL response.
+     */
+    struct stailq autoinc_id_list;
 
     /* When allocating a new Vdbe object, all of the fields below should be
      * initialized to zero or NULL
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d05..1e72a4d 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,6 +57,7 @@ sqlite3VdbeCreate(Parse * pParse)
         return 0;
     memset(p, 0, sizeof(Vdbe));
     p->db = db;
+    stailq_create(&p->autoinc_id_list);
     if (db->pVdbe) {
         db->pVdbe->pPrev = p;
     }
@@ -75,7 +76,7 @@ sqlite3VdbeCreate(Parse * pParse)
 }
 
 struct sql_txn *
-sql_alloc_txn()
+sql_alloc_txn(struct Vdbe *v)
 {
     struct sql_txn *txn = region_alloc_object(&fiber()->gc,
                           struct sql_txn);
@@ -84,6 +85,8 @@ sql_alloc_txn()
              "struct sql_txn");
         return NULL;
     }
+    txn->vdbe = v;
+    v->psql_txn = txn;
     txn->pSavepoint = NULL;
     txn->fk_deferred_count = 0;
     return txn;
@@ -103,11 +106,12 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
          * check FK violations, at least now.
          */
         if (txn->psql_txn == NULL) {
-            txn->psql_txn = sql_alloc_txn();
+            txn->psql_txn = sql_alloc_txn(vdbe);
             if (txn->psql_txn == NULL)
                 return -1;
+        } else {
+            vdbe->psql_txn = txn->psql_txn;
         }
-        vdbe->psql_txn = txn->psql_txn;
     } else {
         vdbe->psql_txn = NULL;
     }
@@ -2261,12 +2265,11 @@ sql_txn_begin(Vdbe *p)
     struct txn *ptxn = txn_begin(false);
     if (ptxn == NULL)
         return -1;
-    ptxn->psql_txn = sql_alloc_txn();
+    ptxn->psql_txn = sql_alloc_txn(p);
     if (ptxn->psql_txn == NULL) {
         box_txn_rollback();
         return -1;
     }
-    p->psql_txn = ptxn->psql_txn;
     return 0;
 }
 
@@ -2414,9 +2417,9 @@ sqlite3VdbeHalt(Vdbe * p)
                      * key constraints to hold up the transaction. This means a commit
                      * is required.
                      */
-                    rc = box_txn_commit() ==
-                            0 ? SQLITE_OK :
-                            SQL_TARANTOOL_ERROR;
+                    rc = (in_txn() == NULL ||
+                          txn_commit(in_txn()) == 0) ?
+                         SQLITE_OK : SQL_TARANTOOL_ERROR;
                     closeCursorsAndFree(p);
                 }
                 if (rc == SQLITE_BUSY && !p->pDelFrame) {
@@ -2780,6 +2783,12 @@ sqlite3VdbeDelete(Vdbe * p)
     }
     p->magic = VDBE_MAGIC_DEAD;
     p->db = 0;
+    /*
+     * VDBE is responsible for releasing region after txn
+     * was commited.
+     */
+    if (in_txn() == NULL)
+        fiber_gc();
     sqlite3DbFree(db, p);
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..de5cb0d 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,7 @@ struct engine;
 struct space;
 struct tuple;
 struct xrow_header;
+struct Vdbe;
 
 enum {
     /**
@@ -117,6 +118,17 @@ struct sql_txn {
      * VDBE to the next in the same transaction.
      */
     uint32_t fk_deferred_count;
+    /** Current VDBE. */
+    struct Vdbe *vdbe;
+};
+
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct autoinc_id_entry {
+    struct stailq_entry link;
+    int64_t id;
 };
 
 struct txn {
@@ -337,6 +349,20 @@ txn_last_stmt(struct txn *txn)
 void
 txn_on_stop(struct trigger *trigger, void *event);
 
+/**
+ * Return VDBE that is being currently executed.
+ *
+ * @retval VDBE that is being currently executed.
+ * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
+ */
+static inline struct Vdbe *
+txn_vdbe()
+{
+    struct txn *txn = in_txn();
+    if (txn == NULL || txn->psql_txn == NULL)
+        return NULL;
+    return txn->psql_txn->vdbe;
+}
 
 /**
  * FFI bindings: do not throw exceptions, do not accept extra
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index ee34f6f..06cb4b9 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -583,6 +583,116 @@ cn:close()
 box.sql.execute('drop table test')
 ---
 ...
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+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)')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (null, 2)')
+---
+- autoincrement_ids:
+  - 2
+  rowcount: 1
+...
+cn:execute('update test set a = 11 where id == 1')
+---
+- rowcount: 1
+...
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')
+---
+- autoincrement_ids:
+  - 101
+  - 121
+  rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+---
+- autoincrement_ids:
+  - 122
+  - 123
+  - 124
+  - 125
+  - 126
+  rowcount: 5
+...
+cn:execute('select * from test')
+---
+- metadata:
+  - name: ID
+  - name: A
+  rows:
+  - [1, 11]
+  - [2, 2]
+  - [100, 1]
+  - [101, 1]
+  - [120, 1]
+  - [121, 1]
+  - [122, 1]
+  - [123, 1]
+  - [124, 1]
+  - [125, 1]
+  - [126, 1]
+...
+s = box.schema.create_space('test2', {engine = engine})
+---
+...
+sq = box.schema.sequence.create('test2')
+---
+...
+pk = s:create_index('pk', {sequence = 'test2'})
+---
+...
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+---
+...
+_ = box.space.TEST:on_replace(push_id)
+---
+...
+cn:execute('insert into test values (null, 1)')
+---
+- autoincrement_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
+...
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+---
+...
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+---
+...
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+---
+- autoincrement_ids:
+  - 1
+  - -9
+  - -19
+  - -29
+  rowcount: 4
+...
+cn:close()
+---
+...
+box.sql.execute('drop table test')
+---
+...
+s:drop()
+---
+...
+sq:drop()
+---
+...
+box.sql.execute('drop table test3')
+---
+...
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index b065590..51ac43b 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -202,6 +202,35 @@ future4:wait_result()
 cn:close()
 box.sql.execute('drop table test')
 
+-- gh-2618 Return generated columns after INSERT in IPROTO.
+-- Return all ids generated in current INSERT statement.
+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)')
+cn:execute('insert into test values (null, 2)')
+cn:execute('update test set a = 11 where id == 1')
+cn:execute('insert into test values (100, 1), (null, 1), (120, 1), (null, 1)')
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+cn:execute('select * from test')
+
+s = box.schema.create_space('test2', {engine = engine})
+sq = box.schema.sequence.create('test2')
+pk = s:create_index('pk', {sequence = 'test2'})
+function push_id() s:replace{box.NULL} s:replace{box.NULL} end
+_ = box.space.TEST:on_replace(push_id)
+cn:execute('insert into test values (null, 1)')
+
+box.sql.execute('create table test3 (id int primary key autoincrement)')
+box.schema.sequence.alter('TEST3', {min=-10000, step=-10})
+cn:execute('insert into TEST3 values (null), (null), (null), (null)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+box.sql.execute('drop table test3')
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 box.schema.user.revoke('guest', 'create', 'space')
 space = nil

--------------35B158489B476B8C7ED56780--