From ce5752ce235324fcefd5a3d0503fd3f8a1800d38 Mon Sep 17 00:00:00 2001
From: Nikita Pettik <korablev@tarantool.org>
Date: Tue, 31 Aug 2021 18:11:39 +0300
Subject: [PATCH] txm: rollback all statements related to space on alter
There was a bug that led to dirty read after space alter. For the
simplicity sake imagine following setup:
-- space 's' is empty
tx1:begin()
tx1('s:replace{2}')
s:alter({format = format})
s:select{}
Last select returns tuple {2}, however transaction tx1 hasn't been
committed. This happens due to the fact that during alter operation we
create new space, swap all unchanged parts of old space and then delete
old space. During removal of old space we also clean-up all stories
related to it. In turn story destruction may make dirty tuple clean in
case it remains in the index. In the previous implementation there was
no removal of uncommitted tuples from corresponding indexes. So let's
rollback all changes happened to the space right in time of alter. It is
legal since DDL operation anyway aborts ALL other transactions.
Closes #6318
Closes #6263
---
.../gh-6263-fix-dirty-read-after-alter.md | 3 +
src/box/memtx_tx.c | 33 +++---
test/box/tx_man.result | 111 ++++++++++++++++++
test/box/tx_man.test.lua | 45 +++++++
4 files changed, 178 insertions(+), 14 deletions(-)
create mode 100644 changelogs/unreleased/gh-6263-fix-dirty-read-after-alter.md
diff --git a/changelogs/unreleased/gh-6263-fix-dirty-read-after-alter.md b/changelogs/unreleased/gh-6263-fix-dirty-read-after-alter.md
new file mode 100644
index 0000000000..46961c9e93
--- /dev/null
+++ b/changelogs/unreleased/gh-6263-fix-dirty-read-after-alter.md
@@ -0,0 +1,3 @@
+## bugfix/core
+
+* Fixed dirty read in MVCC after space alter (gh-6263, gh-6318).
diff --git a/src/box/memtx_tx.c b/src/box/memtx_tx.c
index a550514425..0642d21376 100644
--- a/src/box/memtx_tx.c
+++ b/src/box/memtx_tx.c
@@ -687,10 +687,16 @@ memtx_tx_story_unlink_top(struct memtx_story *story, uint32_t idx)
struct memtx_story_link *link = &story->link[idx];
assert(link->newer_story == NULL);
- struct index *index = story->space->index[idx];
+ /*
+ * Note that link[idx].in_index may not be the same as
+ * story->space->index[idx] in case space is going to be deleted
+ * in memtx_tx_on_space_delete(): during space alter operation we
+ * swap all indexes to the new space object and instead use dummy
+ * structs.
+ */
+ struct index *index = story->link[idx].in_index;
struct memtx_story *old_story = link->older_story;
- assert(story->link[idx].in_index == index);
assert(old_story == NULL || old_story->link[idx].in_index == NULL);
struct tuple *old_tuple = old_story == NULL ? NULL : old_story->tuple;
struct tuple *removed, *unused;
@@ -2014,20 +2020,19 @@ memtx_tx_on_space_delete(struct space *space)
= rlist_first_entry(&space->memtx_stories,
struct memtx_story,
in_space_stories);
+ /*
+ * Space is to be altered (not necessarily dropped). Since
+ * this operation is considered to be DDL, all other
+ * transactions will be aborted anyway. We can't postpone
+ * rollback till actual call of commit/rollback since stories
+ * should be destroyed immediately.
+ */
+ if (story->add_stmt != NULL)
+ memtx_tx_history_rollback_stmt(story->add_stmt);
+ if (story->del_stmt != NULL)
+ memtx_tx_history_rollback_stmt(story->del_stmt);
rlist_del(&story->in_space_stories);
memtx_tx_story_full_unlink(story);
- if (story->add_stmt != NULL) {
- story->add_stmt->add_story = NULL;
- story->add_stmt->space = NULL;
- story->add_stmt = NULL;
- }
- while (story->del_stmt != NULL) {
- struct txn_stmt *stmt = story->del_stmt;
- stmt->del_story = NULL;
- story->del_stmt = stmt->next_in_del_list;
- stmt->next_in_del_list = NULL;
- stmt->space = NULL;
- }
memtx_tx_story_delete(story);
}
}
diff --git a/test/box/tx_man.result b/test/box/tx_man.result
index 2ef1030265..1ed4eb403b 100644
--- a/test/box/tx_man.result
+++ b/test/box/tx_man.result
@@ -3673,6 +3673,117 @@ box.execute([[DROP TABLE k1;]])
| - row_count: 1
| ...
+-- gh-6318: make sure that space alter does not result in dirty read.
+--
+s3 = box.schema.space.create('test', { engine = 'memtx' })
+ | ---
+ | ...
+_ = s3:create_index('primary')
+ | ---
+ | ...
+
+format = {{name = 'field1', type = 'unsigned'}}
+ | ---
+ | ...
+tx1:begin()
+ | ---
+ | -
+ | ...
+tx1('s3:replace{2}')
+ | ---
+ | - - [2]
+ | ...
+s3:select()
+ | ---
+ | - []
+ | ...
+s3:alter({format = format})
+ | ---
+ | ...
+s3:select{}
+ | ---
+ | - []
+ | ...
+-- Alter operation aborts transaction, so results of tx1 should be rolled back.
+--
+tx1:commit()
+ | ---
+ | - - {'error': 'Transaction has been aborted by conflict'}
+ | ...
+s3:select()
+ | ---
+ | - []
+ | ...
+s3:drop()
+ | ---
+ | ...
+
+--gh-6263: basically the same as previous version but involves index creation.
+--
+s = box.schema.space.create('test')
+ | ---
+ | ...
+_ = s:create_index('pk')
+ | ---
+ | ...
+
+ch1 = fiber.channel()
+ | ---
+ | ...
+ch2 = fiber.channel()
+ | ---
+ | ...
+
+_ = test_run:cmd("setopt delimiter ';'")
+ | ---
+ | ...
+fiber.create(function()
+ box.begin()
+ s:insert{1, 1}
+ ch1:get()
+ _ = pcall(box.commit)
+ ch2:put(true)
+end)
+_ = test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+s:create_index('sk', {parts = {{2, 'unsigned'}}})
+ | ---
+ | - unique: true
+ | parts:
+ | - type: unsigned
+ | is_nullable: false
+ | fieldno: 2
+ | hint: true
+ | id: 1
+ | type: TREE
+ | space_id: 668
+ | name: sk
+ | ...
+
+ch1:put(true)
+ | ---
+ | - true
+ | ...
+ch2:get()
+ | ---
+ | - true
+ | ...
+
+s.index.pk:select()
+ | ---
+ | - []
+ | ...
+s.index.sk:select()
+ | ---
+ | - []
+ | ...
+
+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 19bc14a676..a2b2061f32 100644
--- a/test/box/tx_man.test.lua
+++ b/test/box/tx_man.test.lua
@@ -1182,6 +1182,51 @@ box.execute([[DROP TABLE k3;]])
box.execute([[DROP TABLE k2;]])
box.execute([[DROP TABLE k1;]])
+-- gh-6318: make sure that space alter does not result in dirty read.
+--
+s3 = box.schema.space.create('test', { engine = 'memtx' })
+_ = s3:create_index('primary')
+
+format = {{name = 'field1', type = 'unsigned'}}
+tx1:begin()
+tx1('s3:replace{2}')
+s3:select()
+s3:alter({format = format})
+s3:select{}
+-- Alter operation aborts transaction, so results of tx1 should be rolled back.
+--
+tx1:commit()
+s3:select()
+s3:drop()
+
+--gh-6263: basically the same as previous version but involves index creation.
+--
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+
+ch1 = fiber.channel()
+ch2 = fiber.channel()
+
+_ = test_run:cmd("setopt delimiter ';'")
+fiber.create(function()
+ box.begin()
+ s:insert{1, 1}
+ ch1:get()
+ _ = pcall(box.commit)
+ ch2:put(true)
+end)
+_ = test_run:cmd("setopt delimiter ''");
+
+s:create_index('sk', {parts = {{2, 'unsigned'}}})
+
+ch1:put(true)
+ch2:get()
+
+s.index.pk:select()
+s.index.sk:select()
+
+s:drop()
+
test_run:cmd("switch default")
test_run:cmd("stop server tx_man")
test_run:cmd("cleanup server tx_man")
--
GitLab