[tarantool-patches] Re: [PATCH v2 2/8] Get rid of autocommit from a txn structure

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [19/05/23 12:14]:
> Move transaction auto start and auto commit behavior to the box level.
> >From now a transaction won't start and commit automatically without
> txn_begin/txn_commit invocations. This is a part of a bigger transaction
> refactoring in order to implement detachable transactions and a parallel
> applier.

I started looking at this patch and I don't fully understand it.

- why do you sometimes call fiber_gc() after txn_commit() and
  sometimes do not?

It used to be called before, and not called in some cases after
your change.

Here's the diff (it doesn't pass tests, which baffles me, could
you please explain?


---
 src/box/applier.cc |  4 +++-
 src/box/box.cc     | 52 ++++++++++++++++++++++++++--------------------
 src/box/journal.h  |  1 -
 3 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index 9d2989094..75698fea7 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -178,8 +178,10 @@ apply_initial_join_row(struct xrow_header *row)
 	struct request request;
 	xrow_decode_dml(row, &request, dml_request_key_map(row->type));
 	struct space *space = space_cache_find(request.space_id);
-	if (space == NULL)
+	if (space == NULL) {
+		txn_rollback();
 		return -1;
+	}
 	/* no access checks here - applier always works with admin privs */
 	if (space_apply_initial_join_row(space, &request)) {
 		txn_rollback();
diff --git a/src/box/box.cc b/src/box/box.cc
index 2a738fa36..a371af813 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -184,30 +184,27 @@ box_process_rw(struct request *request, struct space *space,
 		txn_rollback_stmt(txn);
 		goto fail;
 	}
-	/*
-	 * Pin the tuple locally before the commit,
-	 * otherwise it may go away during yield in
-	 * when WAL is written in autocommit mode.
-	 */
 	if (result != NULL)
 		*result = tuple;
 
 	if (result == NULL || tuple == NULL) {
-		if (!is_autocommit)
-			return txn_commit_stmt(txn, request);
-		/* Autocommit mode. */
-		if (txn_commit_stmt(txn, request) != 0) {
-			txn_rollback();
-			return -1;
+		if (txn_commit_stmt(txn, request) != 0)
+			goto fail;
+		if (is_autocommit) {
+			if (txn_commit(txn) != 0)
+				return -1;
+			fiber_gc();
 		}
-		if (txn_commit(txn) != 0)
-			return -1;
-		fiber_gc();
 		return 0;
 	}
+	/*
+	 * Pin the tuple locally before the commit,
+	 * otherwise it may go away during yield in
+	 * when WAL is written in autocommit mode.
+	 */
 	tuple_ref(tuple);
 
-	if (txn_commit_stmt(txn, request)) {
+	if (txn_commit_stmt(txn, request) != 0) {
 		/* Unref tuple and rollback if autocommit. */
 		tuple_unref(tuple);
 		goto fail;
@@ -328,25 +325,32 @@ apply_wal_row(struct xstream *stream, struct xrow_header *row)
 {
 	struct request request;
 	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
+	struct wal_stream *xstream =
+		container_of(stream, struct wal_stream, base);
+
 	if (request.type != IPROTO_NOP) {
 		struct space *space = space_cache_find_xc(request.space_id);
 		struct txn *txn = txn_begin();
-		if (txn == NULL || box_process_rw(&request, space, NULL) != 0 ||
-		    txn_commit(txn) != 0) {
-			say_error("error applying row: %s", request_str(&request));
-			if (txn != NULL)
-				txn_rollback();
-			diag_raise();
+		if (txn == NULL)
+			goto error;
+		if (box_process_rw(&request, space, NULL) != 0) {
+			txn_rollback();
+			goto error;
 		}
+		if (txn_commit(txn) != 0)
+			goto error;
+		fiber_gc();
 	}
-	struct wal_stream *xstream =
-		container_of(stream, struct wal_stream, base);
 	/**
 	 * Yield once in a while, but not too often,
 	 * mostly to allow signal handling to take place.
 	 */
 	if (++xstream->rows % xstream->yield == 0)
 		fiber_sleep(0);
+error:
+	say_error("error applying row: %s", request_str(&request));
+	diag_raise();
+
 }
 
 static void
@@ -1358,6 +1362,7 @@ box_register_replica(uint32_t id, const struct tt_uuid *uuid)
 	}
 	if (txn_commit(txn) != 0)
 		diag_raise();
+	fiber_gc();
 	assert(replica_by_uuid(uuid)->id == id);
 }
 
@@ -1689,6 +1694,7 @@ box_set_replicaset_uuid(const struct tt_uuid *replicaset_uuid)
 	}
 	if (txn_commit(txn) != 0)
 		diag_raise();
+	fiber_gc();
 }
 
 void
diff --git a/src/box/journal.h b/src/box/journal.h
index 77d3073c8..8ac32ee5e 100644
--- a/src/box/journal.h
+++ b/src/box/journal.h
@@ -32,7 +32,6 @@
  */
 #include <stdint.h>
 #include <stdbool.h>
-#include "trigger.h"
 #include "salad/stailq.h"
 
 #if defined(__cplusplus)

-- 
Konstantin Osipov, Moscow, Russia