From 9db816b14b9ff9cf11eeed11336022ae6a43c877 Mon Sep 17 00:00:00 2001
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
Date: Fri, 30 Jul 2021 18:31:54 +0300
Subject: [PATCH] txm: track read more carefully
The previous commit fixed a bug that caused dirty read but also
introduced a much less significat problem - excess conflict in
some cases.
Usually if a reader reads a tuple - in its story aspecial record
is stored. Any write that replaces or deletes that tuple can now
cause conflict of current transaction.
The problem happened when a reader tries to execute select from
some index, but only deleted story is found there. The record is
stored and that is good - we must know when somebody will insert
a tuple to this place in index. But actually we need to know it
only for the index from which the reader executed select.
This patch introduces a special index mask in read tracker that is
used in the case above to be more precise in conflict detection.
Closes #6206
---
.../gh-6206-repeat-read-violation.md | 3 +
src/box/memtx_tx.c | 32 +++++--
src/box/memtx_tx.h | 2 +
test/box/tx_man.result | 84 +++++++++++++++++++
test/box/tx_man.test.lua | 26 ++++++
5 files changed, 139 insertions(+), 8 deletions(-)
create mode 100644 changelogs/unreleased/gh-6206-repeat-read-violation.md
diff --git a/changelogs/unreleased/gh-6206-repeat-read-violation.md b/changelogs/unreleased/gh-6206-repeat-read-violation.md
new file mode 100644
index 0000000000..b73b9908e1
--- /dev/null
+++ b/changelogs/unreleased/gh-6206-repeat-read-violation.md
@@ -0,0 +1,3 @@
+## bugfix/core
+
+* fix a repeatable read violation after delete (gh-6206)
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index 669640d344..14203f2c59 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -31,6 +31,7 @@
#include "memtx_tx.h"
#include <assert.h>
+#include <limits.h>
#include <stddef.h>
#include <stdint.h>
@@ -1707,6 +1708,9 @@ memtx_tx_history_prepare_insert_stmt(struct txn_stmt *stmt)
in_reader_list) {
if (tracker->reader == stmt->txn)
continue;
+ uint64_t index_mask = 1ull << (i & 63);
+ if ((tracker->index_mask & index_mask) == 0)
+ continue;
memtx_tx_handle_conflict(stmt->txn, tracker->reader);
}
}
@@ -1789,7 +1793,7 @@ memtx_tx_history_commit_stmt(struct txn_stmt *stmt)
static int
memtx_tx_track_read_story(struct txn *txn, struct space *space,
- struct memtx_story *story);
+ struct memtx_story *story, uint64_t index_max);
struct tuple *
memtx_tx_tuple_clarify_slow(struct txn *txn, struct space *space,
@@ -1816,8 +1820,17 @@ memtx_tx_tuple_clarify_slow(struct txn *txn, struct space *space,
if (story == NULL)
break;
}
- if (!own_change && story != NULL)
- memtx_tx_track_read_story(txn, space, story);
+ if (!own_change && story != NULL) {
+ /*
+ * If the result tuple exists (is visible) - it is visible in
+ * every index. But if we found a story of deleted tuple - we
+ * should record that only in the given index this transaction
+ * have found nothing by this key.
+ */
+ int shift = index->dense_id & 63;
+ uint64_t mask = result == NULL ? 1ull << shift : UINT64_MAX;
+ memtx_tx_track_read_story(txn, space, story, mask);
+ }
if (mk_index != 0) {
assert(false); /* TODO: multiindex */
panic("multikey indexes are not supported int TX manager");
@@ -1911,7 +1924,8 @@ memtx_tx_on_space_delete(struct space *space)
* (diag is set). Links in lists are not initialized though.
*/
static struct tx_read_tracker *
-tx_read_tracker_new(struct txn *reader, struct memtx_story *story)
+tx_read_tracker_new(struct txn *reader, struct memtx_story *story,
+ uint64_t index_mask)
{
size_t sz;
struct tx_read_tracker *tracker;
@@ -1923,12 +1937,13 @@ tx_read_tracker_new(struct txn *reader, struct memtx_story *story)
}
tracker->reader = reader;
tracker->story = story;
+ tracker->index_mask = index_mask;
return tracker;
}
static int
memtx_tx_track_read_story(struct txn *txn, struct space *space,
- struct memtx_story *story)
+ struct memtx_story *story, uint64_t index_mask)
{
if (txn == NULL)
return 0;
@@ -1960,8 +1975,9 @@ memtx_tx_track_read_story(struct txn *txn, struct space *space,
/* Move to the beginning of a list for faster further lookups.*/
rlist_del(&tracker->in_reader_list);
rlist_del(&tracker->in_read_set);
+ tracker->index_mask |= index_mask;
} else {
- tracker = tx_read_tracker_new(txn, story);
+ tracker = tx_read_tracker_new(txn, story, index_mask);
if (tracker == NULL)
return -1;
}
@@ -1985,13 +2001,13 @@ memtx_tx_track_read(struct txn *txn, struct space *space, struct tuple *tuple)
if (tuple->is_dirty) {
struct memtx_story *story = memtx_tx_story_get(tuple);
- return memtx_tx_track_read_story(txn, space, story);
+ return memtx_tx_track_read_story(txn, space, story, UINT64_MAX);
} else {
struct memtx_story *story = memtx_tx_story_new(space, tuple);
if (story == NULL)
return -1;
struct tx_read_tracker *tracker;
- tracker = tx_read_tracker_new(txn, story);
+ tracker = tx_read_tracker_new(txn, story, UINT64_MAX);
if (tracker == NULL) {
memtx_tx_story_delete(story);
return -1;
diff --git a/src/box/memtx_tx.h b/src/box/memtx_tx.h
index 4c56617694..a9677ea65d 100644
--- a/src/box/memtx_tx.h
+++ b/src/box/memtx_tx.h
@@ -75,6 +75,8 @@ struct tx_read_tracker {
struct rlist in_reader_list;
/** Link in reader->read_set. */
struct rlist in_read_set;
+ /** Bit field of indexes in which the data was tread by reader. */
+ uint64_t index_mask;
};
/**
diff --git a/test/box/tx_man.result b/test/box/tx_man.result
index e6a83ada27..ffe96097ef 100644
--- a/test/box/tx_man.result
+++ b/test/box/tx_man.result
@@ -3159,6 +3159,90 @@ spc:drop()
| ---
| ...
+-- Check possible excess conflict: reference test
+s = box.schema.create_space('test')
+ | ---
+ | ...
+primary = s:create_index('pk', {parts={1, 'uint'}})
+ | ---
+ | ...
+secondary = s:create_index('sk', {parts={2, 'uint'}})
+ | ---
+ | ...
+tx1:begin()
+ | ---
+ | -
+ | ...
+tx1('secondary:select{1}')
+ | ---
+ | - - []
+ | ...
+s:replace{1, 2}
+ | ---
+ | - [1, 2]
+ | ...
+tx1('primary:select{1}')
+ | ---
+ | - - [[1, 2]]
+ | ...
+tx1('s:replace{3, 3}')
+ | ---
+ | - - [3, 3]
+ | ...
+tx1:commit()
+ | ---
+ | -
+ | ...
+s:drop()
+ | ---
+ | ...
+
+-- Check possible excess conflict: test with deleted story
+s = box.schema.create_space('test')
+ | ---
+ | ...
+primary = s:create_index('pk', {parts={1, 'uint'}})
+ | ---
+ | ...
+secondary = s:create_index('sk', {parts={2, 'uint'}})
+ | ---
+ | ...
+s:replace{1, 1} -- The difference from the test above is that the tuple
+ | ---
+ | - [1, 1]
+ | ...
+s:delete{1} -- is inserted and immediately deleted.
+ | ---
+ | - [1, 1]
+ | ...
+tx1:begin()
+ | ---
+ | -
+ | ...
+tx1('secondary:select{1}')
+ | ---
+ | - - []
+ | ...
+s:replace{1, 2}
+ | ---
+ | - [1, 2]
+ | ...
+tx1('primary:select{1}')
+ | ---
+ | - - [[1, 2]]
+ | ...
+tx1('s:replace{3, 3}')
+ | ---
+ | - - [3, 3]
+ | ...
+tx1:commit()
+ | ---
+ | -
+ | ...
+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 9e1827884b..a216bd8263 100644
--- a/test/box/tx_man.test.lua
+++ b/test/box/tx_man.test.lua
@@ -1004,6 +1004,32 @@ tx2:commit()
spc:drop()
+-- Check possible excess conflict: reference test
+s = box.schema.create_space('test')
+primary = s:create_index('pk', {parts={1, 'uint'}})
+secondary = s:create_index('sk', {parts={2, 'uint'}})
+tx1:begin()
+tx1('secondary:select{1}')
+s:replace{1, 2}
+tx1('primary:select{1}')
+tx1('s:replace{3, 3}')
+tx1:commit()
+s:drop()
+
+-- Check possible excess conflict: test with deleted story
+s = box.schema.create_space('test')
+primary = s:create_index('pk', {parts={1, 'uint'}})
+secondary = s:create_index('sk', {parts={2, 'uint'}})
+s:replace{1, 1} -- The difference from the test above is that the tuple
+s:delete{1} -- is inserted and immediately deleted.
+tx1:begin()
+tx1('secondary:select{1}')
+s:replace{1, 2}
+tx1('primary:select{1}')
+tx1('s:replace{3, 3}')
+tx1:commit()
+s:drop()
+
test_run:cmd("switch default")
test_run:cmd("stop server tx_man")
test_run:cmd("cleanup server tx_man")
--
GitLab