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

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

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;