[tarantool-patches] [PATCH v7 0/2] sql: return all generated ids via IPROTO

[tarantool-patches] [PATCH v7 0/2] sql: return all generated ids via IPROTO

[imeevma]

To make implementation of function getGeneratedKeys() of JDBC
driver  we should be able to return information of generated keys
through IPROTO.

Patch 1 implements such functionality.

Patch 2 do some refactoring in VDBE.

Issue: https://github.com/tarantool/tarantool/issues/2618
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids

Mergen Imeev (1):
  sql: return all generated ids via IPROTO

Vladislav Shpilevoy (1):
  sql: remove psql_txn from Vdbe

 src/box/execute.c        | 28 ++++++++++++++-
 src/box/execute.h        |  1 +
 src/box/lua/net_box.c    | 29 ++++++++++++---
 src/box/sequence.c       |  7 ++++
 src/box/sequence.h       | 13 +++++++
 src/box/sql/vdbe.c       | 42 +++++++++++++++++-----
 src/box/sql/vdbe.h       | 12 ++++++-
 src/box/sql/vdbeInt.h    |  7 ++--
 src/box/sql/vdbeaux.c    | 25 ++++++-------
 src/box/txn.h            | 27 ++++++++++++++
 test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++
 12 files changed, 277 insertions(+), 30 deletions(-)

-- 
2.7.4

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

[imeevma]

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.

Closes #2618
---
 src/box/execute.c        | 28 ++++++++++++++-
 src/box/execute.h        |  1 +
 src/box/lua/net_box.c    | 29 ++++++++++++---
 src/box/sequence.c       |  7 ++++
 src/box/sequence.h       | 13 +++++++
 src/box/sql/vdbe.c       | 27 ++++++++++++--
 src/box/sql/vdbe.h       | 12 ++++++-
 src/box/sql/vdbeInt.h    |  5 +++
 src/box/sql/vdbeaux.c    | 21 ++++++-----
 src/box/txn.h            | 27 ++++++++++++++
 test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++
 12 files changed, 268 insertions(+), 18 deletions(-)

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"
 
 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 *generated_ids =
+			vdbe_generated_ids((struct Vdbe *)stmt);
+		uint32_t map_size = stailq_empty(generated_ids) ? 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) {
+				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) +
+				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(generated_ids)) {
+			buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+			buf = mp_encode_array(buf, id_count);
+			struct id_entry *id_entry;
+			stailq_foreach_entry(id_entry, generated_ids, 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..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,
 	sql_info_key_MAX,
 };
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..5a137e0 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_GENERATED_IDS.
+	 */
+	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);
+		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, "generated_ids");
+	}
 }
 
 static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..9a87a56 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_generated_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_generated_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..7180ead 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 {
@@ -140,6 +141,18 @@ 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);
+
+/**
  * Create an iterator over sequence data.
  *
  * The iterator creates a snapshot of sequence data and walks
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..2571492 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,26 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
 	}
 }
 
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe)
+{
+	return &vdbe->generated_ids;
+}
+
+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");
+		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) {
 			rc = SQL_TARANTOOL_ERROR;
 			goto abort_due_to_error;
 		}
@@ -3040,7 +3061,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..ed7fb2f 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_generated_ids(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..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;
 
 	/* 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;
 	}
@@ -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,8 @@ sqlite3VdbeDelete(Vdbe * p)
 	}
 	p->magic = VDBE_MAGIC_DEAD;
 	p->db = 0;
+	if (in_txn() == NULL)
+		fiber_gc();
 	sqlite3DbFree(db, p);
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..12e9d10 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -117,6 +117,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 {
+	/** A link in a generated id list. */
+	struct stailq_entry link;
+	/** Generated id. */
+	int64_t id;
 };
 
 struct txn {
@@ -337,6 +350,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 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.
+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)')
+---
+- generated_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)')
+---
+- generated_ids:
+  - 101
+  - 121
+  rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+---
+- generated_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)')
+---
+- generated_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
+...
+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 220331b..a2caa57 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -201,6 +201,30 @@ 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)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 space = nil
 
-- 
2.7.4

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

[Konstantin Osipov]

* imeevma@tarantool.org <imeevma@tarantool.org> [18/10/29 20:25]:
> 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.

Very nice. 

The trick with fiber_gc() is also good IMHO.
Thank you for working on this!

We're going to do a lot of integration of box and vdbe - triggers,
constraints, etc, so I think this patch is on track from architecture
point of view as well, we will need to access vdbe from box increasingly
more as we progress along with the integration.

> Closes #261
-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] [PATCH v7 2/2] sql: remove psql_txn from Vdbe

[imeevma]

It makes no sense to store it here, and it has never
did. SQL transaction specific things shall be taken
from global txn object, as any transaction specific
things.
---
 src/box/sql/vdbe.c    | 15 +++++++++------
 src/box/sql/vdbeInt.h |  2 --
 src/box/sql/vdbeaux.c | 10 +++-------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2571492..1e732a9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2882,7 +2882,8 @@ case OP_Savepoint: {
 	Savepoint *pNew;
 	Savepoint *pSavepoint;
 	Savepoint *pTmp;
-	struct sql_txn *psql_txn = p->psql_txn;
+	struct txn *txn = in_txn();
+	struct sql_txn *psql_txn = txn != NULL ? txn->psql_txn : NULL;
 
 	if (psql_txn == NULL) {
 		assert(!box_txn());
@@ -2960,7 +2961,7 @@ case OP_Savepoint: {
 				assert(pSavepoint == psql_txn->pSavepoint);
 				psql_txn->pSavepoint = pSavepoint->pNext;
 			} else {
-				p->psql_txn->fk_deferred_count =
+				psql_txn->fk_deferred_count =
 					pSavepoint->tnt_savepoint->fk_deferred_count;
 			}
 		}
@@ -4802,8 +4803,9 @@ case OP_Param: {           /* out2 */
 case OP_FkCounter: {
 	if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1 != 0) &&
 	    !p->auto_commit) {
-		assert(p->psql_txn != NULL);
-		p->psql_txn->fk_deferred_count += pOp->p2;
+		struct txn *txn = in_txn();
+		assert(txn != NULL && txn->psql_txn != NULL);
+		txn->psql_txn->fk_deferred_count += pOp->p2;
 	} else {
 		p->nFkConstraint += pOp->p2;
 	}
@@ -4825,8 +4827,9 @@ case OP_FkCounter: {
 case OP_FkIfZero: {         /* jump */
 	if ((user_session->sql_flags & SQLITE_DeferFKs || pOp->p1) &&
 	    !p->auto_commit) {
-		assert(p->psql_txn != NULL);
-		if (p->psql_txn->fk_deferred_count == 0)
+		struct txn *txn = in_txn();
+		assert(txn != NULL && txn->psql_txn != NULL);
+		if (txn->psql_txn->fk_deferred_count == 0)
 			goto jump_to_p2;
 	} else {
 		if (p->nFkConstraint == 0)
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 7a7d3de..19b35b7 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -363,8 +363,6 @@ struct Vdbe {
 	 * ignoreRaised variable helps to track such situations
 	 */
 	u8 ignoreRaised;	/* Flag for ON CONFLICT IGNORE for triggers */
-	/** Data related to current transaction. */
-	struct sql_txn *psql_txn;
 	/** The auto-commit flag. */
 	bool auto_commit;
 	/**
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index ae73f25..3efe252 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -86,7 +86,6 @@ sql_alloc_txn(struct Vdbe *v)
 		return NULL;
 	}
 	txn->vdbe = v;
-	v->psql_txn = txn;
 	txn->pSavepoint = NULL;
 	txn->fk_deferred_count = 0;
 	return txn;
@@ -109,11 +108,7 @@ sql_vdbe_prepare(struct Vdbe *vdbe)
 			txn->psql_txn = sql_alloc_txn(vdbe);
 			if (txn->psql_txn == NULL)
 				return -1;
-		} else {
-			vdbe->psql_txn = txn->psql_txn;
 		}
-	} else {
-		vdbe->psql_txn = NULL;
 	}
 	return 0;
 }
@@ -2244,8 +2239,9 @@ sqlite3VdbeCloseStatement(Vdbe * p, int eOp)
 int
 sqlite3VdbeCheckFk(Vdbe * p, int deferred)
 {
-	if ((deferred && p->psql_txn != NULL &&
-	     p->psql_txn->fk_deferred_count > 0) ||
+	struct txn *txn = in_txn();
+	if ((deferred && txn != NULL && txn->psql_txn != NULL &&
+	     txn->psql_txn->fk_deferred_count > 0) ||
 	    (!deferred && p->nFkConstraint > 0)) {
 		p->rc = SQLITE_CONSTRAINT_FOREIGNKEY;
 		p->errorAction = ON_CONFLICT_ACTION_ABORT;
-- 
2.7.4

[tarantool-patches] Re: [PATCH v7 0/2] sql: return all generated ids via IPROTO

[Vladislav Shpilevoy]

Hi! Nikita, please, ignore this. It contains a bug.

On 27/10/2018 14:18, imeevma@tarantool.org wrote:
> To make implementation of function getGeneratedKeys() of JDBC
> driver  we should be able to return information of generated keys
> through IPROTO.
> 
> Patch 1 implements such functionality.
> 
> Patch 2 do some refactoring in VDBE.
> 
> Issue: https://github.com/tarantool/tarantool/issues/2618
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2618-return-all-generated-ids
> 
> Mergen Imeev (1):
>    sql: return all generated ids via IPROTO
> 
> Vladislav Shpilevoy (1):
>    sql: remove psql_txn from Vdbe
> 
>   src/box/execute.c        | 28 ++++++++++++++-
>   src/box/execute.h        |  1 +
>   src/box/lua/net_box.c    | 29 ++++++++++++---
>   src/box/sequence.c       |  7 ++++
>   src/box/sequence.h       | 13 +++++++
>   src/box/sql/vdbe.c       | 42 +++++++++++++++++-----
>   src/box/sql/vdbe.h       | 12 ++++++-
>   src/box/sql/vdbeInt.h    |  7 ++--
>   src/box/sql/vdbeaux.c    | 25 ++++++-------
>   src/box/txn.h            | 27 ++++++++++++++
>   test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
>   test/sql/iproto.test.lua | 24 +++++++++++++
>   12 files changed, 277 insertions(+), 30 deletions(-)
> 

[tarantool-patches] Re: [PATCH v7 0/2] sql: return all generated ids via IPROTO

[n.pettik]

> On 29 Oct 2018, at 00:28, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Nikita, please, ignore this. It contains a bug.

Hello.

NP, will be waiting for updated version.

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

[imeevma]

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.

Closes #2618
---
 src/box/execute.c        | 28 ++++++++++++++-
 src/box/execute.h        |  1 +
 src/box/lua/net_box.c    | 29 ++++++++++++---
 src/box/sequence.c       |  7 ++++
 src/box/sequence.h       | 13 +++++++
 src/box/sql/vdbe.c       | 27 ++++++++++++--
 src/box/sql/vdbe.h       | 12 ++++++-
 src/box/sql/vdbeInt.h    |  5 +++
 src/box/sql/vdbeaux.c    | 21 ++++++-----
 src/box/txn.c            | 15 +++++---
 src/box/txn.h            | 27 ++++++++++++++
 src/box/vy_scheduler.c   |  1 +
 test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 24 +++++++++++++
 14 files changed, 279 insertions(+), 23 deletions(-)

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"
 
 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 *generated_ids =
+			vdbe_generated_ids((struct Vdbe *)stmt);
+		uint32_t map_size = stailq_empty(generated_ids) ? 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) {
+				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) +
+				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(generated_ids)) {
+			buf = mp_encode_uint(buf, SQL_INFO_GENERATED_IDS);
+			buf = mp_encode_array(buf, id_count);
+			struct id_entry *id_entry;
+			stailq_foreach_entry(id_entry, generated_ids, 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..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,
 	sql_info_key_MAX,
 };
 
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4c..5a137e0 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_GENERATED_IDS.
+	 */
+	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);
+		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, "generated_ids");
+	}
 }
 
 static int
diff --git a/src/box/sequence.c b/src/box/sequence.c
index 35b7605..9a87a56 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_generated_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_generated_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..7180ead 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 {
@@ -140,6 +141,18 @@ 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);
+
+/**
  * Create an iterator over sequence data.
  *
  * The iterator creates a snapshot of sequence data and walks
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..2571492 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -578,6 +578,26 @@ out2Prerelease(Vdbe *p, VdbeOp *pOp)
 	}
 }
 
+struct stailq *
+vdbe_generated_ids(struct Vdbe *vdbe)
+{
+	return &vdbe->generated_ids;
+}
+
+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");
+		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) {
 			rc = SQL_TARANTOOL_ERROR;
 			goto abort_due_to_error;
 		}
@@ -3040,7 +3061,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..ed7fb2f 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_generated_ids(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..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;
 
 	/* 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;
 	}
@@ -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,8 @@ sqlite3VdbeDelete(Vdbe * p)
 	}
 	p->magic = VDBE_MAGIC_DEAD;
 	p->db = 0;
+	if (in_txn() == NULL)
+		fiber_gc();
 	sqlite3DbFree(db, p);
 }
 
diff --git a/src/box/txn.c b/src/box/txn.c
index 617ceb8..103a8aa 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -250,8 +250,12 @@ txn_commit_stmt(struct txn *txn, struct request *request)
 			goto fail;
 	}
 	--txn->in_sub_stmt;
-	if (txn->is_autocommit && txn->in_sub_stmt == 0)
-		return txn_commit(txn);
+	if (txn->is_autocommit && txn->in_sub_stmt == 0) {
+		int rc = txn_commit(txn);
+		if (rc == 0)
+			fiber_gc();
+		return rc;
+	}
 	return 0;
 fail:
 	txn_rollback_stmt();
@@ -354,8 +358,6 @@ txn_commit(struct txn *txn)
 		txn_stmt_unref_tuples(stmt);
 
 	TRASH(txn);
-	/** Free volatile txn memory. */
-	fiber_gc();
 	fiber_set_txn(fiber(), NULL);
 	return 0;
 fail:
@@ -463,7 +465,10 @@ box_txn_commit()
 		diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
 		return -1;
 	}
-	return txn_commit(txn);
+	int rc = txn_commit(txn);
+	if (rc == 0)
+		fiber_gc();
+	return rc;
 }
 
 int
diff --git a/src/box/txn.h b/src/box/txn.h
index e74c14d..12e9d10 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -117,6 +117,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 {
+	/** A link in a generated id list. */
+	struct stailq_entry link;
+	/** Generated id. */
+	int64_t id;
 };
 
 struct txn {
@@ -337,6 +350,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/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index eab3f6c..6e9cc90 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -886,6 +886,7 @@ vy_deferred_delete_batch_process_f(struct cmsg *cmsg)
 
 	if (txn_commit(txn) != 0)
 		goto fail;
+	fiber_gc();
 
 	return;
 fail:
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.
+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)')
+---
+- generated_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)')
+---
+- generated_ids:
+  - 101
+  - 121
+  rowcount: 4
+...
+cn:execute('insert into test values (null, 1), (null, 1), (null, 1), (null, 1), (null, 1)')
+---
+- generated_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)')
+---
+- generated_ids:
+  - 127
+  - 1
+  - 2
+  rowcount: 1
+...
+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 220331b..a2caa57 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -201,6 +201,30 @@ 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)')
+
+cn:close()
+box.sql.execute('drop table test')
+s:drop()
+sq:drop()
+
 box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 space = nil
 
-- 
2.7.4

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

[Vladislav Shpilevoy]

Hi!

On 27/10/2018 15:43, 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.
> 
> Closes #2618
> ---

Do not omit branch and issue name.

Why did not you send the second commit?

Now I see, that it would be better to split this commit
into 2 parts: factoring fiber_gc() out of txn_commit and
auto-generated ids. I did it already and force pushed on the
branch.

But it appeared that tests fail with this reproduce
file:

========================= rep.yaml ===========================

---
- [sql/select-null.test.lua, memtx]
- [sql/errinj.test.lua, memtx]
- [sql/persistency.test.lua, memtx]
- [sql/errinj.test.lua, vinyl]
- [sql/gh-2929-primary-key.test.lua, vinyl]
- [sql/message-func-indexes.test.lua, vinyl]
- [sql/drop-table.test.lua, memtx]
- [sql/gh2808-inline-unique-persistency-check.test.lua, memtx]
- [sql/transitive-transactions.test.lua, memtx]
- [sql/max-on-index.test.lua, vinyl]
- [sql/iproto.test.lua, vinyl]
- [sql/checks.test.lua, vinyl]
- [sql/triggers.test.lua, memtx]
- [sql/gh-3199-no-mem-leaks.test.lua, memtx]
- [sql/savepoints.test.lua, vinyl]
- [sql/gh2808-inline-unique-persistency-check.test.lua, vinyl]

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

python test-run.py --reproduce rep.yaml

[001] Test failed! Result content mismatch:
[001] --- sql/gh-3199-no-mem-leaks.result	Thu Oct 25 22:58:31 2018
[001] +++ sql/gh-3199-no-mem-leaks.reject	Mon Oct 29 13:12:35 2018
[001] @@ -27,7 +27,7 @@
[001]  ...
[001]  fiber.info()[fiber.self().id()].memory.used
[001]  ---
[001] -- 0
[001] +- 4036
[001]  ...

It repeats both on your branch and on my force
pushed version. Please, fix.

>   src/box/execute.c        | 28 ++++++++++++++-
>   src/box/execute.h        |  1 +
>   src/box/lua/net_box.c    | 29 ++++++++++++---
>   src/box/sequence.c       |  7 ++++
>   src/box/sequence.h       | 13 +++++++
>   src/box/sql/vdbe.c       | 27 ++++++++++++--
>   src/box/sql/vdbe.h       | 12 ++++++-
>   src/box/sql/vdbeInt.h    |  5 +++
>   src/box/sql/vdbeaux.c    | 21 ++++++-----
>   src/box/txn.c            | 15 +++++---
>   src/box/txn.h            | 27 ++++++++++++++
>   src/box/vy_scheduler.c   |  1 +
>   test/sql/iproto.result   | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
>   test/sql/iproto.test.lua | 24 +++++++++++++
>   14 files changed, 279 insertions(+), 23 deletions(-)
> 

Just for record, my new commit:

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

commit bcd32c9ee07f365efe142414f5456e34b7ae7bbb
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Mon Oct 29 12:52:56 2018 +0300

     box: factor fiber_gc out of txn_commit
     
     Now txn_commit is judge, jury and executioner. It both
     commits or rollbacks data, and collects it calling fiber_gc,
     which destroys the region.
     
     But SQL wants to use some transactional data after commit. It is
     autogenerated identifiers - a list of sequence values generated
     for autoincrement columns and explicit sequence:next() calls.
     
     It is possible to store the list on malloced mem inside Vdbe, but
     it complicates deallocation. Much more convenient to store all
     transactional data on the transaction memory region, so it would
     be freed together with fiber_gc.
     
     After this patch applied, Vdbe takes care of txn memory
     deallocation in a finalizer routine. Between commit and
     finalization transactional data can be serialized wherever.
     
     Needed for #2618

diff --git a/src/box/txn.c b/src/box/txn.c
index 617ceb8a2..9bcc6727e 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -250,8 +250,11 @@ txn_commit_stmt(struct txn *txn, struct request *request)
  			goto fail;
  	}
  	--txn->in_sub_stmt;
-	if (txn->is_autocommit && txn->in_sub_stmt == 0)
-		return txn_commit(txn);
+	if (txn->is_autocommit && txn->in_sub_stmt == 0) {
+		int rc = txn_commit(txn);
+		fiber_gc();
+		return rc;
+	}
  	return 0;
  fail:
  	txn_rollback_stmt();
@@ -354,8 +357,6 @@ txn_commit(struct txn *txn)
  		txn_stmt_unref_tuples(stmt);
  
  	TRASH(txn);
-	/** Free volatile txn memory. */
-	fiber_gc();
  	fiber_set_txn(fiber(), NULL);
  	return 0;
  fail:
@@ -463,7 +464,9 @@ box_txn_commit()
  		diag_set(ClientError, ER_COMMIT_IN_SUB_STMT);
  		return -1;
  	}
-	return txn_commit(txn);
+	int rc = txn_commit(txn);
+	fiber_gc();
+	return rc;
  }
  
  int
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index eab3f6c54..4a46243ed 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -886,7 +886,7 @@ vy_deferred_delete_batch_process_f(struct cmsg *cmsg)
  
  	if (txn_commit(txn) != 0)
  		goto fail;
-
+	fiber_gc();
  	return;
  fail:
  	batch->is_failed = true;

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

[Imeev Mergen]

Hi! Thank you for review! My answer below.

On 10/29/18 1:17 PM, Vladislav Shpilevoy wrote:
> Hi!
>
> On 27/10/2018 15:43, 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.
>>
>> Closes #2618
>> ---
>
> Do not omit branch and issue name.
>
> Why did not you send the second commit?
>
> Now I see, that it would be better to split this commit
> into 2 parts: factoring fiber_gc() out of txn_commit and
> auto-generated ids. I did it already and force pushed on the
> branch.
>
> But it appeared that tests fail with this reproduce
> file:
>
> ========================= rep.yaml ===========================
>
> ---
> - [sql/select-null.test.lua, memtx]
> - [sql/errinj.test.lua, memtx]
> - [sql/persistency.test.lua, memtx]
> - [sql/errinj.test.lua, vinyl]
> - [sql/gh-2929-primary-key.test.lua, vinyl]
> - [sql/message-func-indexes.test.lua, vinyl]
> - [sql/drop-table.test.lua, memtx]
> - [sql/gh2808-inline-unique-persistency-check.test.lua, memtx]
> - [sql/transitive-transactions.test.lua, memtx]
> - [sql/max-on-index.test.lua, vinyl]
> - [sql/iproto.test.lua, vinyl]
> - [sql/checks.test.lua, vinyl]
> - [sql/triggers.test.lua, memtx]
> - [sql/gh-3199-no-mem-leaks.test.lua, memtx]
> - [sql/savepoints.test.lua, vinyl]
> - [sql/gh2808-inline-unique-persistency-check.test.lua, vinyl]
>
> ================================================================
>
> python test-run.py --reproduce rep.yaml
>
> [001] Test failed! Result content mismatch:
> [001] --- sql/gh-3199-no-mem-leaks.result    Thu Oct 25 22:58:31 2018
> [001] +++ sql/gh-3199-no-mem-leaks.reject    Mon Oct 29 13:12:35 2018
> [001] @@ -27,7 +27,7 @@
> [001]  ...
> [001]  fiber.info()[fiber.self().id()].memory.used
> [001]  ---
> [001] -- 0
> [001] +- 4036
> [001]  ...
>
> It repeats both on your branch and on my force
> pushed version. Please, fix.
>
This problem is described in issue 3732
https://github.com/tarantool/tarantool/issues/3732

I found that mentioned yaml produces the same error on current 2.1
branch.

Due to this it was decided that issue 2618 shouldn't deal with
the problem.