From 8b05755f9230b83af3f96786add175be91296706 Mon Sep 17 00:00:00 2001
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
Date: Tue, 13 Dec 2022 10:04:00 +0300
Subject: [PATCH] txm: carefully handle conflict
When a transaction is in read-confirmed state it must ignore all
prepared changes, and if it actually ignores something - it must
fall to read-view state.
By a mistake the check relied not on actual skipping of a prepared
statement, but on the fact that there is a deleting statement. That
leads to excess conflicts for transactions with read-committed
isolation level.
Fix it by raising a conflict only if a deleting statement is skipped.
Closes #8122
Needed for #7202
NO_DOC=bugfix
(cherry picked from commit 91d6d70fe87ea6dc69c85f85cd93d15847f21f10)
---
.../gh-8122-excess-conflict-in-mvcc.md | 3 +
src/box/memtx_tx.c | 27 ++--
...gh_8122_excess_conflict_secondary_test.lua | 142 ++++++++++++++++++
3 files changed, 161 insertions(+), 11 deletions(-)
create mode 100644 changelogs/unreleased/gh-8122-excess-conflict-in-mvcc.md
create mode 100644 test/box-luatest/gh_8122_excess_conflict_secondary_test.lua
diff --git a/changelogs/unreleased/gh-8122-excess-conflict-in-mvcc.md b/changelogs/unreleased/gh-8122-excess-conflict-in-mvcc.md
new file mode 100644
index 0000000000..fb24fadcac
--- /dev/null
+++ b/changelogs/unreleased/gh-8122-excess-conflict-in-mvcc.md
@@ -0,0 +1,3 @@
+## bugfix/core
+
+* Fixed an excess conflict of a transaction in case of double delete of the same key when MVCC engine is used (gh-8122).
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index 87546479c3..5091095ea8 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -2673,6 +2673,18 @@ memtx_tx_tuple_clarify_impl(struct txn *txn, struct space *space,
result = NULL;
break;
}
+ if (story->del_psn != 0 && story->del_stmt != NULL &&
+ txn != NULL) {
+ assert(story->del_psn == story->del_stmt->txn->psn);
+ /*
+ * If we skip deletion of a tuple by prepared
+ * transaction then the transaction must be before
+ * prepared in serialization order.
+ * That can be a read view or conflict already.
+ */
+ memtx_tx_handle_conflict(story->del_stmt->txn, txn);
+ }
+
if (memtx_tx_story_insert_is_visible(story, txn, is_prepared_ok,
&own_change)) {
result = story->tuple;
@@ -2682,25 +2694,18 @@ memtx_tx_tuple_clarify_impl(struct txn *txn, struct space *space,
txn != NULL) {
assert(story->add_psn == story->add_stmt->txn->psn);
/*
- * If we skip prepared story then the transaction
- * must be before prepared in serialization order.
+ * If we skip addition of a tuple by prepared
+ * transaction then the transaction must be before
+ * prepared in serialization order.
* That can be a read view or conflict already.
*/
memtx_tx_handle_conflict(story->add_stmt->txn, txn);
}
+
if (story->link[index->dense_id].older_story == NULL)
break;
story = story->link[index->dense_id].older_story;
}
- if (story->del_psn != 0 && story->del_stmt != NULL && txn != NULL) {
- assert(story->del_psn == story->del_stmt->txn->psn);
- /*
- * If we see a tuple that is deleted by prepared transaction
- * then the transaction must be before prepared in serialization
- * order. That can be a read view or conflict already.
- */
- memtx_tx_handle_conflict(story->del_stmt->txn, txn);
- }
if (!own_change) {
/*
* If the result tuple exists (is visible) - it is visible in
diff --git a/test/box-luatest/gh_8122_excess_conflict_secondary_test.lua b/test/box-luatest/gh_8122_excess_conflict_secondary_test.lua
new file mode 100644
index 0000000000..0c6e55eebc
--- /dev/null
+++ b/test/box-luatest/gh_8122_excess_conflict_secondary_test.lua
@@ -0,0 +1,142 @@
+local server = require('luatest.server')
+local t = require('luatest')
+
+local g = t.group()
+
+g.before_all = function()
+ g.server = server:new{
+ alias = 'default',
+ box_cfg = {
+ memtx_use_mvcc_engine = true,
+ }
+ }
+ g.server:start()
+end
+
+g.after_all = function()
+ g.server:drop()
+end
+
+g.after_each(function()
+ g.server:exec(function()
+ if box.space.test then
+ box.space.test:drop()
+ end
+ end)
+end)
+
+-- Test right from the https://github.com/tarantool/tarantool/issues/8122.
+g.test_simple_conflict = function()
+ g.server:exec(function()
+ box.cfg{txn_isolation = 'best-effort'}
+ box.schema.space.create('test'):create_index('pk')
+ box.space.test:replace{1}
+ require('fiber').create(function() box.space.test:delete{1} end)
+ box.space.test:delete{1} -- must not fail.
+ end)
+end
+
+-- Test right from the https://github.com/tarantool/tarantool/issues/7202,
+-- a bit shortened but very close to the original.
+g.test_excess_conflict_secondary_original = function()
+ g.server:exec(function()
+ box.cfg{txn_isolation = 'read-committed'}
+ local test = box.schema.create_space("test", {
+ format = {{"id", type = "number"},
+ {"indexed", type = "string"}}
+ })
+ test:create_index("id", {unique = true, parts = {{1, "number"}}})
+ test:create_index("indexed", {unique = false, parts = {{2, "string"}}})
+
+ local t = require('luatest')
+ local fiber = require('fiber')
+
+ local id = 0
+ local nextId = function()
+ id = id + 1;
+ return id;
+ end
+
+ local fibers = {}
+ local results = {}
+
+ local put = function(fib_no)
+ table.insert(fibers, fiber.self())
+ fiber.self():set_joinable(true)
+ box.begin()
+ test:put({nextId(), "queue", "READY", "test" })
+ results[fib_no] = pcall(box.commit)
+ end
+
+ local firstUpdate = function()
+ table.insert(fibers, fiber.self())
+ fiber.self():set_joinable(true)
+ box.begin()
+ for _, task in test.index.indexed:pairs() do
+ test:update(task.id, { { '=', "indexed", "test + test" } })
+ end
+ results[3] = pcall(box.commit)
+ end
+
+ local secondUpdate = function()
+ table.insert(fibers, fiber.self())
+ fiber.self():set_joinable(true)
+ results[4] = false
+ box.begin()
+ for _, task in test.index.indexed:pairs() do
+ test:update(task.id, { { '=', "indexed", "test - test" } })
+ end
+ results[4] = pcall(box.commit)
+ end
+
+ for fib_no = 1, 2 do
+ fiber.create(put, fib_no)
+ end
+ fiber.create(firstUpdate)
+ fiber.create(secondUpdate)
+
+ t.assert_equals(#fibers, 4)
+ for _,f in pairs(fibers) do
+ f:join()
+ end
+
+ t.assert_equals(results, {true, true, true, true})
+ t.assert_equals(test:select{},
+ {{1, 'test - test', 'READY', 'test'},
+ {2, 'test - test', 'READY', 'test'}})
+ end)
+end
+
+-- Checks that in case we find a deleted story during tuple clarification,
+-- and this deleted story is prepared, we do not conflict the current
+-- transaction, since we do not actually skip the story.
+g.test_conflict_of_visible_deleted_secondary_index_story = function(cg)
+ cg.server:exec(function()
+ local fiber = require('fiber')
+ local t = require('luatest')
+
+ local test = box.schema.space.create('test')
+ test:create_index('pk')
+ test:create_index('sk', {parts = {{2}}})
+
+ test:replace{0, 0}
+
+ local first_replace = fiber.new(function()
+ box.atomic(function()
+ test:replace{0, 1}
+ end)
+ end)
+ local second_replace = fiber.new(function()
+ box.atomic(function()
+ test.index[1]:get{0}
+ test:replace {0, 0}
+ end)
+ end)
+
+ first_replace:set_joinable(true)
+ second_replace:set_joinable(true)
+
+ t.assert(first_replace:join())
+ t.assert(second_replace:join())
+ end)
+end
--
GitLab