[Tarantool-patches] [PATCH 00/14] rework async and sync transactions

[Tarantool-patches] [PATCH 00/14] rework async and sync transactions

[Cyrill Gorcunov]

Kostya, could you please take a look once time permit.
While there are a bunch of patches the base idea is the
following:

 - make journal_write to be synchronous
 - journal_write_async in turn is working in async mode
 - txn_write becomes txn_commit_async
 - the journal helpers do operate with fiber storage by
   self so now it should be possible to run rollbacks
   without problems.

The series is not for merge yet I just wanna share it
asap, I need tests and etc, I compile tested it only
just to draw the picture.

Cyrill Gorcunov (14):
  box/txn: fix void args mess
  box/journal: use plain int for return value
  box/journal: sanitize completion naming
  box/txn: rename txn_entry_done_cb to txn_entry_complete_cb
  box/txn: rename txn_write_to_wal to txn_write_to_wal_async
  box/journal: supersede journal_write with journal_write_async
  box/txn: rename txn_write to txn_commit_async
  box/txn: move setup of transaction start time to txn_prepare
  box/txn: make txn nop processing a separate routine
  box/txn: move journal entry allocation into separate routine
  box/txn: merge txn_write_to_wal_async to txn_commit_async
  box/txn: do not use journal_write_async under the hood
  box/journal: introduce journal_write
  box/txn: use journal_write in txn_commit

 src/box/applier.cc     |   2 +-
 src/box/box.cc         |   2 +-
 src/box/journal.c      |  56 +++++++++++++++--
 src/box/journal.h      |  35 ++++++-----
 src/box/txn.c          | 136 +++++++++++++++++++++++------------------
 src/box/txn.h          |  15 ++++-
 src/box/vy_scheduler.c |   2 +-
 src/box/wal.c          |   8 +--
 8 files changed, 167 insertions(+), 89 deletions(-)

-- 
2.20.1

[Tarantool-patches] [PATCH 01/14] box/txn: fix void args mess

[Cyrill Gorcunov]

Using void explicitly in functions which take
no arguments allows to optimize code a bit and
don't assume if there might be variable args.

Moreover in commit e070cc4d9 we dropped arguments
from txn_begin but didn't update vy_scheduler.c.
The compiler didn't complain because it assumed
there are vargs.

Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c          | 14 +++++++-------
 src/box/txn.h          |  4 ++--
 src/box/vy_scheduler.c |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 30ec586ba..5dadf4985 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -173,7 +173,7 @@ txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp)
  * Return a txn from cache or create a new one if cache is empty.
  */
 inline static struct txn *
-txn_new()
+txn_new(void)
 {
 	if (!stailq_empty(&txn_cache))
 		return stailq_shift_entry(&txn_cache, struct txn, in_txn_cache);
@@ -209,7 +209,7 @@ txn_free(struct txn *txn)
 }
 
 struct txn *
-txn_begin()
+txn_begin(void)
 {
 	static int64_t tsn = 0;
 	assert(! in_txn());
@@ -670,13 +670,13 @@ box_txn_id(void)
 }
 
 bool
-box_txn()
+box_txn(void)
 {
 	return in_txn() != NULL;
 }
 
 int
-box_txn_begin()
+box_txn_begin(void)
 {
 	if (in_txn()) {
 		diag_set(ClientError, ER_ACTIVE_TRANSACTION);
@@ -688,7 +688,7 @@ box_txn_begin()
 }
 
 int
-box_txn_commit()
+box_txn_commit(void)
 {
 	struct txn *txn = in_txn();
 	/**
@@ -709,7 +709,7 @@ box_txn_commit()
 }
 
 int
-box_txn_rollback()
+box_txn_rollback(void)
 {
 	struct txn *txn = in_txn();
 	if (txn == NULL)
@@ -788,7 +788,7 @@ txn_savepoint_by_name(struct txn *txn, const char *name)
 }
 
 box_txn_savepoint_t *
-box_txn_savepoint()
+box_txn_savepoint(void)
 {
 	struct txn *txn = in_txn();
 	if (txn == NULL) {
diff --git a/src/box/txn.h b/src/box/txn.h
index 97d7b5de5..ae2c3a58f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -251,7 +251,7 @@ txn_clear_flag(struct txn *txn, enum txn_flag flag)
 
 /* Pointer to the current transaction (if any) */
 static inline struct txn *
-in_txn()
+in_txn(void)
 {
 	return fiber()->storage.txn;
 }
@@ -261,7 +261,7 @@ in_txn()
  * @pre no transaction is active
  */
 struct txn *
-txn_begin();
+txn_begin(void);
 
 /**
  * Commit a transaction.
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 86bed8013..acc909d09 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -895,7 +895,7 @@ vy_deferred_delete_batch_process_f(struct cmsg *cmsg)
 	deferred_delete_space = space_by_id(BOX_VINYL_DEFERRED_DELETE_ID);
 	assert(deferred_delete_space != NULL);
 
-	struct txn *txn = txn_begin(false);
+	struct txn *txn = txn_begin();
 	if (txn == NULL)
 		goto fail;
 
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 01/14] box/txn: fix void args mess

[Nikita Pettik]

On 19 Feb 21:37, Cyrill Gorcunov wrote:
> Using void explicitly in functions which take
> no arguments allows to optimize code a bit and
> don't assume if there might be variable args.
> 
> Moreover in commit e070cc4d9 we dropped arguments
> from txn_begin but didn't update vy_scheduler.c.
> The compiler didn't complain because it assumed
> there are vargs.
> 
> Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

LGTM. I assume you can push such patches aimed solely at refactoring
(taking into account second ack).

Re: [Tarantool-patches] [PATCH 01/14] box/txn: fix void args mess

[Cyrill Gorcunov]

On Thu, Feb 20, 2020 at 05:10:48PM +0300, Nikita Pettik wrote:
> On 19 Feb 21:37, Cyrill Gorcunov wrote:
> > Using void explicitly in functions which take
> > no arguments allows to optimize code a bit and
> > don't assume if there might be variable args.
> > 
> > Moreover in commit e070cc4d9 we dropped arguments
> > from txn_begin but didn't update vy_scheduler.c.
> > The compiler didn't complain because it assumed
> > there are vargs.
> > 
> > Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> 
> LGTM. I assume you can push such patches aimed solely at refactoring
> (taking into account second ack).

Thanks, the first 4 patches are just namings and ect, no
functional changes. Actually I've prepared the branch
for this and gave it to Kirill. Letme talk to him,
I think I'll push myself and add your Acks as well.

Re: [Tarantool-patches] [PATCH 01/14] box/txn: fix void args mess

[Nikita Pettik]

On 20 Feb 17:16, Cyrill Gorcunov wrote:
> On Thu, Feb 20, 2020 at 05:10:48PM +0300, Nikita Pettik wrote:
> > On 19 Feb 21:37, Cyrill Gorcunov wrote:
> > > Using void explicitly in functions which take
> > > no arguments allows to optimize code a bit and
> > > don't assume if there might be variable args.
> > > 
> > > Moreover in commit e070cc4d9 we dropped arguments
> > > from txn_begin but didn't update vy_scheduler.c.
> > > The compiler didn't complain because it assumed
> > > there are vargs.
> > > 
> > > Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> > > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > 
> > LGTM. I assume you can push such patches aimed solely at refactoring
> > (taking into account second ack).
> 
> Thanks, the first 4 patches are just namings and ect, no
> functional changes. Actually I've prepared the branch
> for this and gave it to Kirill. Letme talk to him,
> I think I'll push myself and add your Acks as well.

Pushed to master (first 4 patches).

[Tarantool-patches] [PATCH 02/14] box/journal: use plain int for return value

[Cyrill Gorcunov]

We're returning int64_t with values 0 or -1 by now,
there is no need for such big return type, plain
integer is enough.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/box.cc    | 2 +-
 src/box/journal.c | 2 +-
 src/box/journal.h | 8 ++++----
 src/box/wal.c     | 8 ++++----
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 1b2b27d61..9e8311d1e 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -310,7 +310,7 @@ struct recovery_journal {
  * exact same signature during local recovery to properly mark
  * min/max LSN of created LSM levels.
  */
-static int64_t
+static int
 recovery_journal_write(struct journal *base,
 		       struct journal_entry *entry)
 {
diff --git a/src/box/journal.c b/src/box/journal.c
index d762613dd..cf6485642 100644
--- a/src/box/journal.c
+++ b/src/box/journal.c
@@ -36,7 +36,7 @@
  * Used to load from a memtx snapshot. LSN is not used,
  * but txn_commit() must work.
  */
-static int64_t
+static int
 dummy_journal_write(struct journal *journal, struct journal_entry *entry)
 {
 	(void) journal;
diff --git a/src/box/journal.h b/src/box/journal.h
index 236058bb1..55900d8e5 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -112,8 +112,8 @@ journal_entry_complete(struct journal_entry *entry)
  * synchronous replication.
  */
 struct journal {
-	int64_t (*write)(struct journal *journal,
-			 struct journal_entry *req);
+	int (*write)(struct journal *journal,
+		     struct journal_entry *req);
 	void (*destroy)(struct journal *journal);
 };
 
@@ -128,7 +128,7 @@ extern struct journal *current_journal;
  *
  * @return 0 if write was scheduled or -1 in case of an error.
  */
-static inline int64_t
+static inline int
 journal_write(struct journal_entry *entry)
 {
 	return current_journal->write(current_journal, entry);
@@ -165,7 +165,7 @@ journal_set(struct journal *new_journal)
 
 static inline void
 journal_create(struct journal *journal,
-	       int64_t (*write)(struct journal *, struct journal_entry *),
+	       int (*write)(struct journal *, struct journal_entry *),
 	       void (*destroy)(struct journal *))
 {
 	journal->write = write;
diff --git a/src/box/wal.c b/src/box/wal.c
index 0ae66ff32..ac977c16e 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -60,10 +60,10 @@ const char *wal_mode_STRS[] = { "none", "write", "fsync", NULL };
 
 int wal_dir_lock = -1;
 
-static int64_t
+static int
 wal_write(struct journal *, struct journal_entry *);
 
-static int64_t
+static int
 wal_write_in_wal_mode_none(struct journal *, struct journal_entry *);
 
 /*
@@ -1157,7 +1157,7 @@ wal_writer_f(va_list ap)
  * WAL writer main entry point: queue a single request
  * to be written to disk.
  */
-static int64_t
+static int
 wal_write(struct journal *journal, struct journal_entry *entry)
 {
 	struct wal_writer *writer = (struct wal_writer *) journal;
@@ -1213,7 +1213,7 @@ wal_write(struct journal *journal, struct journal_entry *entry)
 	return -1;
 }
 
-static int64_t
+static int
 wal_write_in_wal_mode_none(struct journal *journal,
 			   struct journal_entry *entry)
 {
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 02/14] box/journal: use plain int for return value

[Nikita Pettik]

On 19 Feb 21:37, Cyrill Gorcunov wrote:
> We're returning int64_t with values 0 or -1 by now,
> there is no need for such big return type, plain
> integer is enough.
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

LGTM

[Tarantool-patches] [PATCH 03/14] box/journal: sanitize completion naming

[Cyrill Gorcunov]

Instead of on_done use on_complete prefix
since done it too general while we're trying
to complete write procedue. Also it is more
consistent with txn_complete name.

Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/journal.c |  8 ++++----
 src/box/journal.h | 12 ++++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/box/journal.c b/src/box/journal.c
index cf6485642..11e78990d 100644
--- a/src/box/journal.c
+++ b/src/box/journal.c
@@ -54,8 +54,8 @@ struct journal *current_journal = &dummy_journal;
 
 struct journal_entry *
 journal_entry_new(size_t n_rows, struct region *region,
-		  journal_entry_done_cb on_done_cb,
-		  void *on_done_cb_data)
+		  journal_entry_complete_cb on_complete_cb,
+		  void *on_complete_cb_data)
 {
 	struct journal_entry *entry;
 
@@ -71,8 +71,8 @@ journal_entry_new(size_t n_rows, struct region *region,
 	entry->approx_len = 0;
 	entry->n_rows = n_rows;
 	entry->res = -1;
-	entry->on_done_cb = on_done_cb;
-	entry->on_done_cb_data = on_done_cb_data;
+	entry->on_complete_cb = on_complete_cb;
+	entry->on_complete_cb_data = on_complete_cb_data;
 	return entry;
 }
 
diff --git a/src/box/journal.h b/src/box/journal.h
index 55900d8e5..64f167c6f 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -43,7 +43,7 @@ struct xrow_header;
 struct journal_entry;
 
 /** Journal entry finalization callback typedef. */
-typedef void (*journal_entry_done_cb)(struct journal_entry *entry, void *data);
+typedef void (*journal_entry_complete_cb)(struct journal_entry *entry, void *data);
 
 /**
  * An entry for an abstract journal.
@@ -66,11 +66,11 @@ struct journal_entry {
 	 * or fail. Entry->res is set to a result value before the callback
 	 * is fired.
 	 */
-	journal_entry_done_cb on_done_cb;
+	journal_entry_complete_cb on_complete_cb;
 	/**
 	 * A journal entry completion callback argument.
 	 */
-	void *on_done_cb_data;
+	void *on_complete_cb_data;
 	/**
 	 * Approximate size of this request when encoded.
 	 */
@@ -94,8 +94,8 @@ struct region;
  */
 struct journal_entry *
 journal_entry_new(size_t n_rows, struct region *region,
-		  journal_entry_done_cb on_done_cb,
-		  void *on_done_cb_data);
+		  journal_entry_complete_cb on_complete_cb,
+		  void *on_complete_cb_data);
 
 /**
  * Finalize a single entry.
@@ -103,7 +103,7 @@ journal_entry_new(size_t n_rows, struct region *region,
 static inline void
 journal_entry_complete(struct journal_entry *entry)
 {
-	entry->on_done_cb(entry, entry->on_done_cb_data);
+	entry->on_complete_cb(entry, entry->on_complete_cb_data);
 }
 
 /**
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 03/14] box/journal: sanitize completion naming

[Nikita Pettik]

On 19 Feb 21:37, Cyrill Gorcunov wrote:
> Instead of on_done use on_complete prefix
> since done it too general while we're trying
> to complete write procedue. Also it is more
> consistent with txn_complete name.
> 
> Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/journal.c |  8 ++++----
>  src/box/journal.h | 12 ++++++------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>

LGTM

Btw why not to do the same with txn_entry_done_cb ?
 

Re: [Tarantool-patches] [PATCH 03/14] box/journal: sanitize completion naming

[Nikita Pettik]

On 20 Feb 17:12, Nikita Pettik wrote:
> On 19 Feb 21:37, Cyrill Gorcunov wrote:
> > Instead of on_done use on_complete prefix
> > since done it too general while we're trying
> > to complete write procedue. Also it is more
> > consistent with txn_complete name.
> > 
> > Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> > Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> > ---
> >  src/box/journal.c |  8 ++++----
> >  src/box/journal.h | 12 ++++++------
> >  2 files changed, 10 insertions(+), 10 deletions(-)
> >
> 
> LGTM
> 
> Btw why not to do the same with txn_entry_done_cb ?
>

Nevermind, it is done in the next patch (I've accidentally skipped it).
  

[Tarantool-patches] [PATCH 04/14] box/txn: rename txn_entry_done_cb to txn_entry_complete_cb

[Cyrill Gorcunov]

To look similar to txn_complete.

Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 5dadf4985..a4ca48224 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -469,7 +469,7 @@ txn_complete(struct txn *txn)
 }
 
 static void
-txn_entry_done_cb(struct journal_entry *entry, void *data)
+txn_entry_complete_cb(struct journal_entry *entry, void *data)
 {
 	struct txn *txn = data;
 	txn->signature = entry->res;
@@ -493,7 +493,7 @@ txn_write_to_wal(struct txn *txn)
 	struct journal_entry *req = journal_entry_new(txn->n_new_rows +
 						      txn->n_applier_rows,
 						      &txn->region,
-						      txn_entry_done_cb,
+						      txn_entry_complete_cb,
 						      txn);
 	if (req == NULL) {
 		txn_rollback(txn);
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 04/14] box/txn: rename txn_entry_done_cb to txn_entry_complete_cb

[Nikita Pettik]

On 19 Feb 21:37, Cyrill Gorcunov wrote:
> To look similar to txn_complete.
> 
> Acked-by: Konstantin Osipov <kostja.osipov@gmail.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

LGTM as well

[Tarantool-patches] [PATCH 05/14] box/txn: rename txn_write_to_wal to txn_write_to_wal_async

[Cyrill Gorcunov]

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index a4ca48224..682480c16 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -485,7 +485,7 @@ txn_entry_complete_cb(struct journal_entry *entry, void *data)
 }
 
 static int64_t
-txn_write_to_wal(struct txn *txn)
+txn_write_to_wal_async(struct txn *txn)
 {
 	assert(txn->n_new_rows + txn->n_applier_rows > 0);
 
@@ -584,7 +584,7 @@ txn_write(struct txn *txn)
 		return 0;
 	}
 	fiber_set_txn(fiber(), NULL);
-	return txn_write_to_wal(txn);
+	return txn_write_to_wal_async(txn);
 }
 
 int
-- 
2.20.1

[Tarantool-patches] [PATCH 06/14] box/journal: supersede journal_write with journal_write_async

[Cyrill Gorcunov]

1) name it journal_write_async, since it operates in async
   mode where completion handlers are deferred upon journal
   engine does a real write;

2) require a caller to ship a fiber with active transaction
   bound so we could restore it back if write operation failed
   thus rollback will obtain consistent active transaction;

3) move fiber_set_txn to the header file for reuse in
   journal_write_async.

Suggested-by: Konstantin Osipov <kostja.osipov@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/journal.c | 22 ++++++++++++++++++++++
 src/box/journal.h |  9 +++------
 src/box/txn.c     |  9 +--------
 src/box/txn.h     |  9 +++++++++
 4 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/src/box/journal.c b/src/box/journal.c
index 11e78990d..238860165 100644
--- a/src/box/journal.c
+++ b/src/box/journal.c
@@ -29,6 +29,8 @@
  * SUCH DAMAGE.
  */
 #include "journal.h"
+#include "fiber.h"
+#include "txn.h"
 #include <small/region.h>
 #include <diag.h>
 
@@ -76,3 +78,23 @@ journal_entry_new(size_t n_rows, struct region *region,
 	return entry;
 }
 
+int
+journal_write_async(struct journal_entry *entry)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL);
+
+	/*
+	 * The transaction should be unbound
+	 * from current fiber, once operation
+	 * is complete the journal engine will
+	 * bound it back. For rollback sake we
+	 * bind it by self on error path.
+	 */
+	fiber_set_txn(fiber(), NULL);
+	int ret = current_journal->write(current_journal, entry);
+	if (ret != 0)
+		fiber_set_txn(fiber(), txn);
+
+	return ret;
+}
diff --git a/src/box/journal.h b/src/box/journal.h
index 64f167c6f..efb6ac8e1 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -124,15 +124,12 @@ struct journal {
 extern struct journal *current_journal;
 
 /**
- * Send a single entry to write.
+ * Queue a single entry to write.
  *
  * @return 0 if write was scheduled or -1 in case of an error.
  */
-static inline int
-journal_write(struct journal_entry *entry)
-{
-	return current_journal->write(current_journal, entry);
-}
+extern int
+journal_write_async(struct journal_entry *entry);
 
 /**
  * Change the current implementation of the journaling API.
diff --git a/src/box/txn.c b/src/box/txn.c
index 682480c16..05321e97d 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -49,12 +49,6 @@ txn_on_yield(struct trigger *trigger, void *event);
 static void
 txn_run_rollback_triggers(struct txn *txn, struct rlist *triggers);
 
-static inline void
-fiber_set_txn(struct fiber *fiber, struct txn *txn)
-{
-	fiber->storage.txn = txn;
-}
-
 static int
 txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
 {
@@ -520,7 +514,7 @@ txn_write_to_wal_async(struct txn *txn)
 	assert(local_row == remote_row + txn->n_new_rows);
 
 	/* Send the entry to the journal. */
-	if (journal_write(req) < 0) {
+	if (journal_write_async(req) < 0) {
 		diag_set(ClientError, ER_WAL_IO);
 		diag_log();
 		return -1;
@@ -583,7 +577,6 @@ txn_write(struct txn *txn)
 		fiber_set_txn(fiber(), NULL);
 		return 0;
 	}
-	fiber_set_txn(fiber(), NULL);
 	return txn_write_to_wal_async(txn);
 }
 
diff --git a/src/box/txn.h b/src/box/txn.h
index ae2c3a58f..fe13ef5f8 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -256,6 +256,15 @@ in_txn(void)
 	return fiber()->storage.txn;
 }
 
+/**
+ * Set pointer to the current transaction (if any).
+ */
+static inline void
+fiber_set_txn(struct fiber *fiber, struct txn *txn)
+{
+	fiber->storage.txn = txn;
+}
+
 /**
  * Start a transaction explicitly.
  * @pre no transaction is active
-- 
2.20.1

[Tarantool-patches] [PATCH 07/14] box/txn: rename txn_write to txn_commit_async

[Cyrill Gorcunov]

Plain txn_write is too general.

Suggested-by: Konstantin Osipov <kostja.osipov@gmail.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/applier.cc | 2 +-
 src/box/txn.c      | 4 ++--
 src/box/txn.h      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index ae3d281a5..10859a835 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -805,7 +805,7 @@ applier_apply_tx(struct stailq *rows)
 	trigger_create(on_commit, applier_txn_commit_cb, NULL, NULL);
 	txn_on_commit(txn, on_commit);
 
-	if (txn_write(txn) < 0)
+	if (txn_commit_async(txn) < 0)
 		goto fail;
 
 	/* Transaction was sent to journal so promote vclock. */
diff --git a/src/box/txn.c b/src/box/txn.c
index 05321e97d..0ad596c1e 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -558,7 +558,7 @@ txn_prepare(struct txn *txn)
 }
 
 int
-txn_write(struct txn *txn)
+txn_commit_async(struct txn *txn)
 {
 	if (txn_prepare(txn) != 0) {
 		txn_rollback(txn);
@@ -585,7 +585,7 @@ txn_commit(struct txn *txn)
 {
 	txn->fiber = fiber();
 
-	if (txn_write(txn) != 0)
+	if (txn_commit_async(txn) != 0)
 		return -1;
 	/*
 	 * In case of non-yielding journal the transaction could already
diff --git a/src/box/txn.h b/src/box/txn.h
index fe13ef5f8..f2e0fa9e7 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -302,7 +302,7 @@ txn_rollback(struct txn *txn);
  * freed.
  */
 int
-txn_write(struct txn *txn);
+txn_commit_async(struct txn *txn);
 
 /**
  * Most txns don't have triggers, and txn objects
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 07/14] box/txn: rename txn_write to txn_commit_async

[Georgy Kirichenko]

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

Hi!

Well done but I have a minor remark about some renames you suggest.
So if we would like to talk about synchronous replication then renaming
txn_write to txn_commit seems quite incorrect as there would be two stages
of transaction commit - transaction was written to journal and then 
transaction was completed(with commit or rollback). And the stages should be 
distinguishable.
Obviously it depends on further implementation so I let the final resolution up 
to you.

On Wednesday, February 19, 2020 9:37:06 PM MSK Cyrill Gorcunov wrote:
> Plain txn_write is too general.
> 
> Suggested-by: Konstantin Osipov <kostja.osipov@gmail.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/applier.cc | 2 +-
>  src/box/txn.c      | 4 ++--
>  src/box/txn.h      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index ae3d281a5..10859a835 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -805,7 +805,7 @@ applier_apply_tx(struct stailq *rows)
>  	trigger_create(on_commit, applier_txn_commit_cb, NULL, NULL);
>  	txn_on_commit(txn, on_commit);
> 
> -	if (txn_write(txn) < 0)
> +	if (txn_commit_async(txn) < 0)
>  		goto fail;
> 
>  	/* Transaction was sent to journal so promote vclock. */
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 05321e97d..0ad596c1e 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -558,7 +558,7 @@ txn_prepare(struct txn *txn)
>  }
> 
>  int
> -txn_write(struct txn *txn)
> +txn_commit_async(struct txn *txn)
>  {
>  	if (txn_prepare(txn) != 0) {
>  		txn_rollback(txn);
> @@ -585,7 +585,7 @@ txn_commit(struct txn *txn)
>  {
>  	txn->fiber = fiber();
> 
> -	if (txn_write(txn) != 0)
> +	if (txn_commit_async(txn) != 0)
>  		return -1;
>  	/*
>  	 * In case of non-yielding journal the transaction could already
> diff --git a/src/box/txn.h b/src/box/txn.h
> index fe13ef5f8..f2e0fa9e7 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -302,7 +302,7 @@ txn_rollback(struct txn *txn);
>   * freed.
>   */
>  int
> -txn_write(struct txn *txn);
> +txn_commit_async(struct txn *txn);
> 
>  /**
>   * Most txns don't have triggers, and txn objects


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Tarantool-patches] [PATCH 07/14] box/txn: rename txn_write to txn_commit_async

[Cyrill Gorcunov]

On Sat, Feb 22, 2020 at 11:28:41PM +0300, Georgy Kirichenko wrote:
> Hi!
> 
> Well done but I have a minor remark about some renames you suggest.
> So if we would like to talk about synchronous replication then renaming
> txn_write to txn_commit seems quite incorrect as there would be two stages
> of transaction commit - transaction was written to journal and then 
> transaction was completed(with commit or rollback). And the stages should be 
> distinguishable.
> Obviously it depends on further implementation so I let the final resolution up 
> to you.

Thanks! You know at first I need to finish the series and have
everything working and passing tests, so I'll resend the updated
series and then we will discuss the better naming, sounds good?

[Tarantool-patches] [PATCH 08/14] box/txn: move setup of transaction start time to txn_prepare

[Cyrill Gorcunov]

It is more logical to put this timestamp in prepare
state. Also this will allow us to reuse the txn_prepare
code.

Note the former code did the same -- this member get
set right after txn_prepare call so nothing changed.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 0ad596c1e..3a784b18b 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -554,6 +554,8 @@ txn_prepare(struct txn *txn)
 	trigger_clear(&txn->fiber_on_stop);
 	if (!txn_has_flag(txn, TXN_CAN_YIELD))
 		trigger_clear(&txn->fiber_on_yield);
+
+	txn->start_tm = ev_monotonic_now(loop());
 	return 0;
 }
 
@@ -569,7 +571,6 @@ txn_commit_async(struct txn *txn)
 	 * After this point the transaction must not be used
 	 * so reset the corresponding key in the fiber storage.
 	 */
-	txn->start_tm = ev_monotonic_now(loop());
 	if (txn->n_new_rows + txn->n_applier_rows == 0) {
 		/* Nothing to do. */
 		txn->signature = 0;
-- 
2.20.1

[Tarantool-patches] [PATCH 09/14] box/txn: make txn nop processing a separate routine

[Cyrill Gorcunov]

To reuse in sync commits in next patches.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 3a784b18b..a6a0704c8 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -462,6 +462,22 @@ txn_complete(struct txn *txn)
 	}
 }
 
+/**
+ * Try to complete transaction early if there is
+ * nothing to be sent to the journal engine.
+ */
+static bool
+txn_complete_nop(struct txn *txn)
+{
+	if (txn->n_new_rows + txn->n_applier_rows != 0)
+		return false;
+
+	txn->signature = 0;
+	txn_complete(txn);
+	fiber_set_txn(fiber(), NULL);
+	return 0;
+}
+
 static void
 txn_entry_complete_cb(struct journal_entry *entry, void *data)
 {
@@ -567,17 +583,9 @@ txn_commit_async(struct txn *txn)
 		return -1;
 	}
 
-	/*
-	 * After this point the transaction must not be used
-	 * so reset the corresponding key in the fiber storage.
-	 */
-	if (txn->n_new_rows + txn->n_applier_rows == 0) {
-		/* Nothing to do. */
-		txn->signature = 0;
-		txn_complete(txn);
-		fiber_set_txn(fiber(), NULL);
+	if (txn_complete_nop(txn))
 		return 0;
-	}
+
 	return txn_write_to_wal_async(txn);
 }
 
-- 
2.20.1

[Tarantool-patches] [PATCH 10/14] box/txn: move journal entry allocation into separate routine

[Cyrill Gorcunov]

For reuse in next patches.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index a6a0704c8..4dbaf5b91 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -494,41 +494,57 @@ txn_entry_complete_cb(struct journal_entry *entry, void *data)
 	fiber_set_txn(fiber(), NULL);
 }
 
-static int64_t
-txn_write_to_wal_async(struct txn *txn)
+/**
+ * Allocate and prepare new journal entry.
+ */
+static struct journal_entry *
+txn_entry_new(struct txn *txn)
 {
 	assert(txn->n_new_rows + txn->n_applier_rows > 0);
+	struct journal_entry *req;
+	struct txn_stmt *stmt;
 
-	/* Prepare a journal entry. */
-	struct journal_entry *req = journal_entry_new(txn->n_new_rows +
-						      txn->n_applier_rows,
-						      &txn->region,
-						      txn_entry_complete_cb,
-						      txn);
-	if (req == NULL) {
-		txn_rollback(txn);
-		return -1;
-	}
+	req = journal_entry_new(txn->n_new_rows + txn->n_applier_rows,
+				&txn->region, txn_entry_complete_cb, txn);
+	if (req == NULL)
+		return NULL;
 
-	struct txn_stmt *stmt;
 	struct xrow_header **remote_row = req->rows;
 	struct xrow_header **local_row = req->rows + txn->n_applier_rows;
+
 	stailq_foreach_entry(stmt, &txn->stmts, next) {
 		if (stmt->has_triggers) {
 			txn_init_triggers(txn);
 			rlist_splice(&txn->on_commit, &stmt->on_commit);
 		}
+
+		/* A read (e.g. select) request */
 		if (stmt->row == NULL)
-			continue; /* A read (e.g. select) request */
+			continue;
+
 		if (stmt->row->replica_id == 0)
 			*local_row++ = stmt->row;
 		else
 			*remote_row++ = stmt->row;
+
 		req->approx_len += xrow_approx_len(stmt->row);
 	}
+
 	assert(remote_row == req->rows + txn->n_applier_rows);
 	assert(local_row == remote_row + txn->n_new_rows);
 
+	return req;
+}
+
+static int64_t
+txn_write_to_wal_async(struct txn *txn)
+{
+	struct journal_entry *req = txn_entry_new(txn);
+	if (req == NULL) {
+		txn_rollback(txn);
+		return -1;
+	}
+
 	/* Send the entry to the journal. */
 	if (journal_write_async(req) < 0) {
 		diag_set(ClientError, ER_WAL_IO);
-- 
2.20.1

[Tarantool-patches] [PATCH 11/14] box/txn: merge txn_write_to_wal_async to txn_commit_async

[Cyrill Gorcunov]

No need for separate routine, everything is ready
to be used inside txn_commit_async itself.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 4dbaf5b91..a95c28431 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -536,24 +536,6 @@ txn_entry_new(struct txn *txn)
 	return req;
 }
 
-static int64_t
-txn_write_to_wal_async(struct txn *txn)
-{
-	struct journal_entry *req = txn_entry_new(txn);
-	if (req == NULL) {
-		txn_rollback(txn);
-		return -1;
-	}
-
-	/* Send the entry to the journal. */
-	if (journal_write_async(req) < 0) {
-		diag_set(ClientError, ER_WAL_IO);
-		diag_log();
-		return -1;
-	}
-	return 0;
-}
-
 /*
  * Prepare a transaction using engines.
  */
@@ -602,7 +584,20 @@ txn_commit_async(struct txn *txn)
 	if (txn_complete_nop(txn))
 		return 0;
 
-	return txn_write_to_wal_async(txn);
+	struct journal_entry *req = txn_entry_new(txn);
+	if (req == NULL) {
+		txn_rollback(txn);
+		return -1;
+	}
+
+	/* Send the entry to the journal. */
+	if (journal_write_async(req) < 0) {
+		diag_set(ClientError, ER_WAL_IO);
+		diag_log();
+		return -1;
+	}
+
+	return 0;
 }
 
 int
-- 
2.20.1

[Tarantool-patches] [PATCH 12/14] box/txn: do not use journal_write_async under the hood

[Cyrill Gorcunov]

While there is some code duplication (maybe we should
make merge this into txn_prepare?) the idea is to
unweave journal_write_async from txn_commit
which is synchronous.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index a95c28431..38241c066 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -605,8 +605,27 @@ txn_commit(struct txn *txn)
 {
 	txn->fiber = fiber();
 
-	if (txn_commit_async(txn) != 0)
+	if (txn_prepare(txn) != 0) {
+		txn_rollback(txn);
+		return -1;
+	}
+
+	if (txn_complete_nop(txn))
+		return 0;
+
+	struct journal_entry *req = txn_entry_new(txn);
+	if (req == NULL) {
+		txn_rollback(txn);
+		return -1;
+	}
+
+	/* Send the entry to the journal. */
+	if (journal_write_async(req) < 0) {
+		diag_set(ClientError, ER_WAL_IO);
+		diag_log();
 		return -1;
+	}
+
 	/*
 	 * In case of non-yielding journal the transaction could already
 	 * be done and there is nothing to wait in such cases.
-- 
2.20.1

[Tarantool-patches] [PATCH 13/14] box/journal: introduce journal_write

[Cyrill Gorcunov]

To write entries in synchronous way. It
uses journal_write_async under the hood
together with fiber_wait.

Note both journal_write and journal_write_async
require current fiber to come in with txn
bound to the storage for context unification
sake.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/journal.c | 24 ++++++++++++++++++++++++
 src/box/journal.h |  8 ++++++++
 2 files changed, 32 insertions(+)

diff --git a/src/box/journal.c b/src/box/journal.c
index 238860165..d0860e883 100644
--- a/src/box/journal.c
+++ b/src/box/journal.c
@@ -98,3 +98,27 @@ journal_write_async(struct journal_entry *entry)
 
 	return ret;
 }
+
+int
+journal_write(struct journal_entry *entry)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL);
+
+	if (journal_write_async(entry))
+		return -1;
+
+	/*
+	 * In case of non-yielding journal the transaction could
+	 * already be done and there is nothing to wait in such
+	 * case, otherwise wait for wakeup upon transaction
+	 * is complete.
+	 */
+	if (!txn_has_flag(txn, TXN_IS_DONE)) {
+		bool cancellable = fiber_set_cancellable(false);
+		fiber_yield();
+		fiber_set_cancellable(cancellable);
+	}
+
+	return txn->signature >= 0 ? 0 : -1;
+}
diff --git a/src/box/journal.h b/src/box/journal.h
index efb6ac8e1..e73d684e5 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -131,6 +131,14 @@ extern struct journal *current_journal;
 extern int
 journal_write_async(struct journal_entry *entry);
 
+/**
+ * Write a single entry to the journal.
+ *
+ * @return 0 if write was written or -1 in case of an error.
+ */
+extern int
+journal_write(struct journal_entry *entry);
+
 /**
  * Change the current implementation of the journaling API.
  * Happens during life cycle of an instance:
-- 
2.20.1

[Tarantool-patches] [PATCH 14/14] box/txn: use journal_write in txn_commit

[Cyrill Gorcunov]

This allows us to keep wait for transaction
complete in one place -- inside journal_write.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/box/txn.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 38241c066..8f9be3269 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -619,25 +619,11 @@ txn_commit(struct txn *txn)
 		return -1;
 	}
 
-	/* Send the entry to the journal. */
-	if (journal_write_async(req) < 0) {
+	int res = journal_write(req);
+	if (res != 0) {
 		diag_set(ClientError, ER_WAL_IO);
 		diag_log();
-		return -1;
-	}
-
-	/*
-	 * In case of non-yielding journal the transaction could already
-	 * be done and there is nothing to wait in such cases.
-	 */
-	if (!txn_has_flag(txn, TXN_IS_DONE)) {
-		bool cancellable = fiber_set_cancellable(false);
-		fiber_yield();
-		fiber_set_cancellable(cancellable);
 	}
-	int res = txn->signature >= 0 ? 0 : -1;
-	if (res != 0)
-		diag_set(ClientError, ER_WAL_IO);
 
 	/* Synchronous transactions are freed by the calling fiber. */
 	txn_free(txn);
-- 
2.20.1

Re: [Tarantool-patches] [PATCH 14/14] box/txn: use journal_write in txn_commit

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [20/02/19 21:44]:
> This allows us to keep wait for transaction
> complete in one place -- inside journal_write.
> 

I thinks this patch set is on track. I would go further and would
add patches which push journal_write() implementation down to
wal.cc, since only wal.cc's journal_write is different from
journal_write_async().


-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [Tarantool-patches] [PATCH 14/14] box/txn: use journal_write in txn_commit

[Cyrill Gorcunov]

On Wed, Feb 19, 2020 at 10:09:35PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [20/02/19 21:44]:
> > This allows us to keep wait for transaction
> > complete in one place -- inside journal_write.
> > 
> 
> I thinks this patch set is on track. I would go further and would
> add patches which push journal_write() implementation down to
> wal.cc, since only wal.cc's journal_write is different from
> journal_write_async().

Thanks, will do.