[tarantool-patches] Re: [PATCH v5 1/1] sql: return all generated ids via IPROTO

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! I pushed more and
one additional commit to remove one attribute
from struct Vdbe. Please, look at both commits,
squash fixes if you are ok with them and send
v2 to Nikita (including my commit, yes).

My review fixes:

==================================================================

commit 45019a8dccb89a7f89b5e59e442c539aa0e6ef41
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Fri Oct 26 00:11:27 2018 +0300

     Review fixes

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 1a9b3f740..4d79fdefb 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -685,19 +685,16 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
  	if (map_size == 2) {
  		key = mp_decode_uint(data);
  		assert(key == SQL_INFO_GENERATED_IDS);
-		(void)key;
  		uint64_t count = mp_decode_array(data);
-		assert (count > 0);
+		assert(count > 0);
  		lua_createtable(L, 0, count);
-		lua_setfield(L, -2, "generated_ids");
-		lua_getfield(L, -1, "generated_ids");
  		for (uint32_t j = 0; j < count; ++j) {
  			int64_t id;
  			mp_read_int64(data, &id);
-			lua_pushinteger(L, id);
+			luaL_pushint64(L, id);
  			lua_rawseti(L, -2, j + 1);
  		}
-		lua_pop(L, 1);
+		lua_setfield(L, -2, "generated_ids");
  	}
  }
  
diff --git a/src/box/sequence.c b/src/box/sequence.c
index b1c68e97e..9a87a56eb 100644
--- a/src/box/sequence.c
+++ b/src/box/sequence.c
@@ -233,7 +233,7 @@ done:
  		unreachable();
  	*result = value;
  	if (txn_vdbe() != NULL &&
-	    vdbe_add_new_generated_id(txn_vdbe(), *result) != 0)
+	    vdbe_add_new_generated_id(txn_vdbe(), value) != 0)
  		return -1;
  	return 0;
  overflow:
diff --git a/src/box/sequence.h b/src/box/sequence.h
index d38fb2f97..7180ead76 100644
--- a/src/box/sequence.h
+++ b/src/box/sequence.h
@@ -141,7 +141,8 @@ int
  access_check_sequence(struct sequence *seq);
  
  /**
- * Add new generated id in VDBE.
+ * 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.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a3748eae..2571492fc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3015,8 +3015,9 @@ case OP_TransactionBegin: {
   * If there is no active transaction, raise an error.
   */
  case OP_TransactionCommit: {
-	if (box_txn()) {
-		if (txn_commit(in_txn()) != 0) {
+	struct txn *txn = in_txn();
+	if (txn != NULL) {
+		if (txn_commit(txn) != 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 8f877b3a4..ed7fb2f49 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
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 37df5e290..7a7d3dec0 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -367,8 +367,10 @@ struct Vdbe {
  	struct sql_txn *psql_txn;
  	/** The auto-commit flag. */
  	bool auto_commit;
-
-	/** List of id generated in current VDBE. */
+	/**
+	 * List of ids generated in current VDBE. It is returned
+	 * as metadata of SQL response.
+	 */
  	struct stailq generated_ids;
  
  	/* When allocating a new Vdbe object, all of the fields below should be
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 0023dc6cd..ae73f2541 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -57,7 +57,6 @@ sqlite3VdbeCreate(Parse * pParse)
  		return 0;
  	memset(p, 0, sizeof(Vdbe));
  	p->db = db;
-	/* Init list of generated ids. */
  	stailq_create(&p->generated_ids);
  	if (db->pVdbe) {
  		db->pVdbe->pPrev = p;
@@ -77,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);
@@ -86,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;
@@ -105,12 +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;
-			txn->psql_txn->vdbe = vdbe;
+		} else {
+			vdbe->psql_txn = txn->psql_txn;
  		}
-		vdbe->psql_txn = txn->psql_txn;
  	} else {
  		vdbe->psql_txn = NULL;
  	}
@@ -2264,13 +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;
  	}
-	ptxn->psql_txn->vdbe = p;
-	p->psql_txn = ptxn->psql_txn;
  	return 0;
  }
  
diff --git a/src/box/txn.h b/src/box/txn.h
index 2acba9f87..12e9d1013 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -121,9 +121,11 @@ struct sql_txn {
  	struct Vdbe *vdbe;
  };
  
-/** An element of list of generated ids. */
-struct id_entry
-{
+/**
+ * An element of list of autogenerated ids, being returned as SQL
+ * response metadata.
+ */
+struct id_entry {
  	/** A link in a generated id list. */
  	struct stailq_entry link;
  	/** Generated id. */
@@ -349,9 +351,9 @@ void
  txn_on_stop(struct trigger *trigger, void *event);
  
  /**
- * Return VDBE that is currently executed.
+ * Return VDBE that is being currently executed.
   *
- * @retval VDBE that is currently executed.
+ * @retval VDBE that is being currently executed.
   * @retval NULL Either txn or ptxn_sql or vdbe is NULL;
   */
  static inline struct Vdbe *
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 163fb6ed7..af4fc124d 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -637,13 +637,28 @@ cn:execute('select * from test')
    - [125, 1]
    - [126, 1]
  ...
-cn:execute('create table test2 (id integer primary key, a integer)')
+s = box.schema.create_space('test2', {engine = engine})
  ---
-- rowcount: 1
  ...
-cn:execute('drop table test2')
+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)
  ---
-- rowcount: 1
+...
+cn:execute('insert into test values (null, 1)')
+---
+- generated_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
  ...
  cn:close()
  ---
@@ -651,6 +666,12 @@ cn:close()
  box.sql.execute('drop table test')
  ---
  ...
+s:drop()
+---
+...
+sq:drop()
+---
+...
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 6517cdef2..a2caa57b1 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -204,6 +204,7 @@ 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)')
@@ -211,10 +212,18 @@ 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')
-cn:execute('create table test2 (id integer primary key, a integer)')
-cn:execute('drop table test2')
+
+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)')
+
  cn:close()
  box.sql.execute('drop table test')
+s:drop()
+sq:drop()
  
  box.schema.user.revoke('guest', 'read,write,execute', 'universe')
  space = nil