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