[Tarantool-patches] [PATCH 0/2] Fix transaction manager

[Tarantool-patches] [PATCH 0/2] Fix transaction manager

[Aleksandr Lyapunov]

There was two issues:
 * https://github.com/tarantool/tarantool/issues/5423
 * tx_man test faild on OSX builds

Both problems are fixed now.

Branch: alyapunov/gh-5423-dirty-tuple-unref

Aleksandr Lyapunov (2):
  memtx: fix a bug in unlinking story lists
  memtx: fix a bug in TX that caused deletion of a durty tuple

 src/box/memtx_space.c    |  1 +
 src/box/memtx_tx.c       | 67 +++++++++++++++++++++++++++++++++++++++---------
 test/box/tx_man.result   | 43 +++++++++++++++++++++++++++++++
 test/box/tx_man.test.lua | 16 ++++++++++++
 4 files changed, 115 insertions(+), 12 deletions(-)

-- 
2.7.4

[Tarantool-patches] [PATCH 1/2] memtx: fix a bug in unlinking story lists

[Aleksandr Lyapunov]

Tx stories must be linked into correct double-linked list.
Preserve it.

Part of #5423
---
 src/box/memtx_tx.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index 55748ad..e378054 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -433,9 +433,20 @@ memtx_tx_story_gc_step()
 				}
 				assert(story->tuple == unused);
 			}
-			memtx_tx_story_unlink(story, i);
+
+			if (link->older.is_story) {
+				struct memtx_story *older_story = link->older.story;
+				memtx_tx_story_unlink(story, i);
+				older_story->link[i].newer_story = older_story;
+			} else {
+				memtx_tx_story_unlink(story, i);
+			}
 		} else {
+			/* Just unlink from list */
 			link->newer_story->link[i].older = link->older;
+			if (link->older.is_story)
+				link->older.story->link[i].newer_story =
+					link->newer_story;
 			link->older.is_story = false;
 			link->older.story = NULL;
 			link->newer_story = NULL;
-- 
2.7.4

[Tarantool-patches] [PATCH 2/2] memtx: fix a bug in TX that caused deletion of a durty tuple

[Aleksandr Lyapunov]

There was a mess in tuple refernce in TX history.
Now it was remade in the following asumptions:
 * a clean tuple belongs to space, and the space implicitly holds
a reference to the tuple.
 * a dirty tuple belongs to TX manager and a reference is held
in the corresponding story.

Closes #5423
---
 src/box/memtx_space.c    |  1 +
 src/box/memtx_tx.c       | 54 ++++++++++++++++++++++++++++++++++++++----------
 test/box/tx_man.result   | 43 ++++++++++++++++++++++++++++++++++++++
 test/box/tx_man.test.lua | 16 ++++++++++++++
 4 files changed, 103 insertions(+), 11 deletions(-)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index d4b18d9..2a43e64 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -78,6 +78,7 @@ void
 memtx_space_update_bsize(struct space *space, struct tuple *old_tuple,
 			 struct tuple *new_tuple)
 {
+	assert(space->vtab->destroy == &memtx_space_destroy);
 	struct memtx_space *memtx_space = (struct memtx_space *)space;
 	ssize_t old_bsize = old_tuple ? box_tuple_bsize(old_tuple) : 0;
 	ssize_t new_bsize = new_tuple ? box_tuple_bsize(new_tuple) : 0;
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index e378054..bcd4a37 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -418,8 +418,8 @@ memtx_tx_story_gc_step()
 		if (link->newer_story == NULL) {
 			/*
 			 * We are at the top of the chain. That means
-			 * that story->tuple is in index. If the story is
-			 * actually delete the tuple, it must be deleted from
+			 * that story->tuple is in index. If the story
+			 * actually deletes the tuple, it must be deleted from
 			 * index.
 			 */
 			if (story->del_psn > 0 && story->space != NULL) {
@@ -432,6 +432,12 @@ memtx_tx_story_gc_step()
 					panic("failed to rollback change");
 				}
 				assert(story->tuple == unused);
+			} else if (i == 0) {
+				/*
+				 * The tuple is left clean in the space.
+				 * It now belongs to the space and must be referenced.
+				 */
+				tuple_ref(story->tuple);
 			}
 
 			if (link->older.is_story) {
@@ -655,6 +661,14 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple,
 					  &replaced) != 0)
 				goto fail;
 			memtx_tx_story_link_tuple(add_story, replaced, i);
+			if (i == 0 && replaced != NULL && !replaced->is_dirty) {
+				/*
+				 * The tuple was clean and thus belonged to
+				 * the space. Now tx manager takes ownership
+				 * of it.
+				 */
+				tuple_unref(replaced);
+			}
 			add_story_linked++;
 
 			struct tuple *visible_replaced = NULL;
@@ -729,15 +743,16 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple,
 		collected_conflicts = collected_conflicts->next;
 	}
 
-	/*
-	 * We now reference both new and old tuple because the stmt holds
-	 * pointers to them.
-	 */
-	if (stmt->new_tuple != NULL)
-		tuple_ref(stmt->new_tuple);
 	*result = old_tuple;
-	if (*result != NULL)
+	if (*result != NULL) {
+		/*
+		 * The result must be a referenced pointer. The caller must
+		 * unreference it by itself.
+		 * Actually now it goes only to stmt->old_tuple, and
+		 * stmt->old_tuple is unreferenced when stmt is destroyed.
+		 */
 		tuple_ref(*result);
+	}
 	return 0;
 
 	fail:
@@ -756,6 +771,10 @@ memtx_tx_history_add_stmt(struct txn_stmt *stmt, struct tuple *old_tuple,
 				unreachable();
 				panic("failed to rollback change");
 			}
+			if (i == 0 && was != NULL && !was->is_dirty) {
+				/* Just rollback previous tuple_unref. */
+				tuple_ref(was);
+			}
 
 			memtx_tx_story_unlink(stmt->add_story, i);
 
@@ -787,6 +806,10 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt)
 		for (uint32_t i = 0; i < story->index_count; i++) {
 			struct memtx_story_link *link = &story->link[i];
 			if (link->newer_story == NULL) {
+				/*
+				 * We are at top of story list and thus
+				 * story->tuple is in index directly.
+				 */
 				struct tuple *unused;
 				struct index *index = stmt->space->index[i];
 				struct tuple *was = memtx_tx_story_older_tuple(link);
@@ -796,6 +819,17 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt)
 					unreachable();
 					panic("failed to rollback change");
 				}
+				if (i == 0 && was != NULL &&
+				    !link->older.is_story) {
+					/*
+					 * That was the last story in history.
+					 * The last tuple now belongs to space
+					 * and the space must hold a reference
+					 * to it.
+					 */
+					tuple_ref(was);
+				}
+
 			} else {
 				struct memtx_story *newer = link->newer_story;
 				assert(newer->link[i].older.is_story);
@@ -813,7 +847,6 @@ memtx_tx_history_rollback_stmt(struct txn_stmt *stmt)
 		}
 		stmt->add_story->add_stmt = NULL;
 		stmt->add_story = NULL;
-		tuple_unref(stmt->new_tuple);
 	}
 
 	if (stmt->del_story != NULL) {
@@ -987,7 +1020,6 @@ memtx_tx_history_commit_stmt(struct txn_stmt *stmt)
 		assert(stmt->del_story->del_stmt == stmt);
 		assert(stmt->next_in_del_list == NULL);
 		res -= stmt->del_story->tuple->bsize;
-		tuple_unref(stmt->del_story->tuple);
 		stmt->del_story->del_stmt = NULL;
 		stmt->del_story = NULL;
 	}
diff --git a/test/box/tx_man.result b/test/box/tx_man.result
index 3a6375d..a294165 100644
--- a/test/box/tx_man.result
+++ b/test/box/tx_man.result
@@ -826,6 +826,49 @@ s:select{}
  | - - [1, 3]
  | ...
 
+s:drop()
+ | ---
+ | ...
+s2:drop()
+ | ---
+ | ...
+
+-- https://github.com/tarantool/tarantool/issues/5423
+s = box.schema.space.create('test')
+ | ---
+ | ...
+i1 = s:create_index('pk', {parts={{1, 'uint'}}})
+ | ---
+ | ...
+i2 = s:create_index('sec', {parts={{2, 'uint'}}})
+ | ---
+ | ...
+
+s:replace{1, 0}
+ | ---
+ | - [1, 0]
+ | ...
+s:delete{1}
+ | ---
+ | - [1, 0]
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+s:replace{1, 1}
+ | ---
+ | - [1, 1]
+ | ...
+s:replace{1, 2 }
+ | ---
+ | - [1, 2]
+ | ...
+
+s:drop()
+ | ---
+ | ...
+
 test_run:cmd("switch default")
  | ---
  | - true
diff --git a/test/box/tx_man.test.lua b/test/box/tx_man.test.lua
index 60fa52b..53b0773 100644
--- a/test/box/tx_man.test.lua
+++ b/test/box/tx_man.test.lua
@@ -236,6 +236,22 @@ s:select{}
 tx3:commit()
 s:select{}
 
+s:drop()
+s2:drop()
+
+-- https://github.com/tarantool/tarantool/issues/5423
+s = box.schema.space.create('test')
+i1 = s:create_index('pk', {parts={{1, 'uint'}}})
+i2 = s:create_index('sec', {parts={{2, 'uint'}}})
+
+s:replace{1, 0}
+s:delete{1}
+collectgarbage()
+s:replace{1, 1}
+s:replace{1, 2 }
+
+s:drop()
+
 test_run:cmd("switch default")
 test_run:cmd("stop server tx_man")
 test_run:cmd("cleanup server tx_man")
-- 
2.7.4

Re: [Tarantool-patches] [PATCH 0/2] Fix transaction manager

[Alexander V. Tikhonov]

Hi Aleksandr, thanks for the patch. As I see it really fixed the issue
[1], and no new degradations occurred. Patchset LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/206407310

On Thu, Oct 22, 2020 at 07:02:31PM +0300, Aleksandr Lyapunov wrote:
> There was two issues:
>  * https://github.com/tarantool/tarantool/issues/5423
>  * tx_man test faild on OSX builds
> 
> Both problems are fixed now.
> 
> Branch: alyapunov/gh-5423-dirty-tuple-unref
> 
> Aleksandr Lyapunov (2):
>   memtx: fix a bug in unlinking story lists
>   memtx: fix a bug in TX that caused deletion of a durty tuple
> 
>  src/box/memtx_space.c    |  1 +
>  src/box/memtx_tx.c       | 67 +++++++++++++++++++++++++++++++++++++++---------
>  test/box/tx_man.result   | 43 +++++++++++++++++++++++++++++++
>  test/box/tx_man.test.lua | 16 ++++++++++++
>  4 files changed, 115 insertions(+), 12 deletions(-)
> 
> -- 
> 2.7.4
>