From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3EA6C2AC89 for ; Mon, 29 Oct 2018 06:17:07 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 1Vzciwjv33Y6 for ; Mon, 29 Oct 2018 06:17:07 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 841B527EF7 for ; Mon, 29 Oct 2018 06:17:06 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v7 1/2] sql: return all generated ids via IPROTO References: From: Vladislav Shpilevoy Message-ID: <3ec6ee67-ed51-9fe6-4165-45c6062389a9@tarantool.org> Date: Mon, 29 Oct 2018 13:17:00 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: imeevma@tarantool.org, tarantool-patches@freelists.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 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;