Skip to content

Commit 0751619

Browse files
CuriousGeorgiyalyapunov
CuriousGeorgiy
and
alyapunov
committed
mvcc: fix reverse tree iterators gap tracking
Apparently, the current implementation of `tree_iterator_start_raw` is buggy for reverse iterators: instead of tracking gaps for successors of keys, it tracks gaps for tuples shifted by one to the left of the successor: reorder the code of `tree_iterator_start_raw` to get the successor tuple prior to shifting done for reverse iterators and simplify the implementation to make it more straightforward and thus comprehensible. Closes tarantool#7073 Closes tarantool#7113 NO_DOC=bugfix Co-authored-by: Alexander Lyapunov <[email protected]>
1 parent 88c9567 commit 0751619

File tree

3 files changed

+171
-41
lines changed

3 files changed

+171
-41
lines changed

‎changelogs/unreleased/gh-7113-rev-tree-iters-gap-tracking.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
## bugfix/core
2+
3+
* Fixed reversed iterators gap tracking: instead of tracking gaps for
4+
successors of keys, gaps for tuples shifted by one to the left of
5+
the successor were tracked (gh-7113).

‎src/box/memtx_tree.cc

+30-41
Original file line numberDiff line numberDiff line change
@@ -670,6 +670,13 @@ tree_iterator_start_raw(struct iterator *iterator, struct tuple **ret)
670670
}
671671
}
672672

673+
/*
674+
* `it->tree_iterator` could potentially be positioned on successor of
675+
* key: we need to track gap based on it.
676+
*/
677+
struct memtx_tree_data<USE_HINT> *res =
678+
memtx_tree_iterator_get_elem(tree, &it->tree_iterator);
679+
struct tuple *successor = res == NULL ? NULL : res->tuple;
673680
if (iterator_type_is_reverse(type)) {
674681
/*
675682
* Because of limitations of tree search API we use use
@@ -683,57 +690,39 @@ tree_iterator_start_raw(struct iterator *iterator, struct tuple **ret)
683690
* last position in the tree, that's what we need.
684691
*/
685692
memtx_tree_iterator_prev(tree, &it->tree_iterator);
693+
res = memtx_tree_iterator_get_elem(tree, &it->tree_iterator);
686694
}
687-
688-
if (!equals && (type == ITER_EQ || type == ITER_REQ)) {
689-
/*
690-
* Found nothing, iteration will be stopped now. That is the
691-
* last chance to record that the transaction have read the key.
692-
*/
693-
if (key_is_full) {
694-
memtx_tx_track_point(txn, space, idx, it->key_data.key);
695-
return 0;
696-
}
697-
/* it->tree_iterator is positioned on successor of a key! */
698-
struct memtx_tree_data<USE_HINT> *res =
699-
memtx_tree_iterator_get_elem(tree, &it->tree_iterator);
700-
struct tuple *successor = res == NULL ? NULL : res->tuple;
701-
memtx_tx_track_gap(txn, space, idx, successor, type,
702-
it->key_data.key, it->key_data.part_count);
703-
return 0;
704-
}
705-
706-
struct memtx_tree_data<USE_HINT> *res =
707-
memtx_tree_iterator_get_elem(tree, &it->tree_iterator);
708-
uint32_t mk_index = 0;
709-
if (res != NULL) {
710-
*ret = res->tuple;
695+
/*
696+
* Equality iterators requires exact key match: if the result does not
697+
* equal to the key, iteration ends.
698+
*/
699+
bool eq_match = equals || (type != ITER_EQ && type != ITER_REQ);
700+
if (res != NULL && eq_match) {
711701
tree_iterator_set_current(it, res);
712702
tree_iterator_set_next_method(it);
713703
bool is_multikey = iterator->index->def->key_def->is_multikey;
714-
mk_index = is_multikey ? (uint32_t)res->hint : 0;
704+
uint32_t mk_index = is_multikey ? (uint32_t)res->hint : 0;
705+
/*
706+
* We need to clarify the result tuple before story garbage
707+
* collection, otherwise it could get cleaned there.
708+
*/
709+
*ret = memtx_tx_tuple_clarify(txn, space, res->tuple, idx,
710+
mk_index);
715711
}
716-
717-
if ((!key_is_full || (type != ITER_EQ && type != ITER_REQ)) &&
718-
memtx_tx_manager_use_mvcc_engine) {
719-
/* it->tree_iterator is positioned on successor of a key! */
720-
struct tuple *successor = res == NULL ? NULL : res->tuple;
721-
712+
if (key_is_full && !eq_match)
713+
memtx_tx_track_point(txn, space, idx, it->key_data.key);
714+
if (!key_is_full ||
715+
((type == ITER_ALL || type == ITER_GE || type == ITER_LE) &&
716+
!equals) || (type == ITER_GT || type == ITER_LT))
722717
/********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND START*********/
723-
memtx_tx_track_gap(in_txn(), space, idx, successor, type,
718+
memtx_tx_track_gap(txn, space, idx, successor, type,
724719
it->key_data.key, it->key_data.part_count);
725720
/*********MVCC TRANSACTION MANAGER STORY GARBAGE COLLECTION BOUND END**********/
726-
}
727-
728-
if (!res)
721+
if (res == NULL || !eq_match)
729722
return 0;
730-
731-
*ret = memtx_tx_tuple_clarify(txn, space, *ret, idx, mk_index);
732-
if (*ret == NULL) {
723+
if (*ret == NULL)
733724
return iterator->next_raw(iterator, ret);
734-
} else {
735-
tree_iterator_set_current_tuple(it, *ret);
736-
}
725+
tree_iterator_set_current_tuple(it, *ret);
737726
return 0;
738727
}
739728

‎test/box-luatest/gh_7113_rev_tree_iters_gap_tracking_test.lua

+136
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
local server = require('test.luatest_helpers.server')
2+
local t = require('luatest')
3+
4+
local g = t.group()
5+
6+
g.before_all(function()
7+
g.server = server:new{
8+
alias = 'dflt',
9+
box_cfg = {memtx_use_mvcc_engine = true}
10+
}
11+
g.server:start()
12+
end)
13+
14+
g.after_all(function()
15+
g.server:drop()
16+
end)
17+
18+
g.before_each(function()
19+
g.server:exec(function()
20+
local s = box.schema.space.create('s')
21+
s:create_index('pk', {parts = {{1, 'unsigned'},
22+
{2, 'unsigned'}}})
23+
s:insert{0, 0}
24+
s:insert{1, 0}
25+
end)
26+
end)
27+
28+
g.after_each(function()
29+
g.server:eval('box.space.s:drop()')
30+
end)
31+
32+
g['test_reverse_iter_gap_tracking'] = function()
33+
g.server:exec(function()
34+
local t = require('luatest')
35+
local txn_proxy = require('test.box.lua.txn_proxy')
36+
37+
local tx = txn_proxy:new()
38+
39+
local conflict_err = 'Transaction has been aborted by conflict'
40+
41+
tx:begin()
42+
tx('box.space.s:select({1, 0}, {iterator = "LT"})')
43+
box.space.s:insert{0, 1}
44+
tx('box.space.s:insert{2, 0}')
45+
t.assert_equals(tx:commit(), {{error = conflict_err}})
46+
box.space.s:delete{0, 1}
47+
48+
tx:begin()
49+
tx('box.space.s:select({1, 0}, {iterator = "LE"})')
50+
box.space.s:insert{0, 1}
51+
tx('box.space.s:insert{2, 0}')
52+
t.assert_equals(tx:commit(), {{error = conflict_err}})
53+
box.space.s:delete{0, 1}
54+
55+
tx:begin()
56+
tx('box.space.s:select({1}, {iterator = "LT"})')
57+
box.space.s:insert{0, 1}
58+
tx('box.space.s:insert{2, 0}')
59+
t.assert_equals(tx:commit(), {{error = conflict_err}})
60+
box.space.s:delete{0, 1}
61+
62+
tx:begin()
63+
tx('box.space.s:select({1}, {iterator = "LE"})')
64+
box.space.s:insert{0, 1}
65+
tx('box.space.s:insert{2, 0}')
66+
t.assert_equals(tx:commit(), {{error = conflict_err}})
67+
box.space.s:delete{0, 1}
68+
69+
tx:begin()
70+
tx('box.space.s:select({0}, {iterator = "REQ"})')
71+
box.space.s:insert{0, 1}
72+
tx('box.space.s:insert{2, 0}')
73+
t.assert_equals(tx:commit(), {{error = conflict_err}})
74+
box.space.s:delete{0, 1}
75+
76+
tx:begin()
77+
tx('box.space.s:select({1}, {iterator = "REQ"})')
78+
box.space.s:insert{1, 1}
79+
tx('box.space.s:insert{2, 0}')
80+
t.assert_equals(tx:commit(), {{error = conflict_err}})
81+
box.space.s:delete{1, 1}
82+
83+
tx:begin()
84+
end)
85+
end
86+
87+
g['test_reverse_iter_clarify_before_gap_tracking'] = function()
88+
g.server:exec(function()
89+
local t = require('luatest')
90+
local txn_proxy = require('test.box.lua.txn_proxy')
91+
92+
local tx = txn_proxy:new()
93+
94+
--[[
95+
The following tests are a safety net for catching the buggy case
96+
when tuple clarification could be done after gap tracking
97+
(gh-7073).
98+
--]]
99+
box.internal.memtx_tx_gc(128)
100+
101+
tx:begin()
102+
box.space.s:delete{0, 0}
103+
t.assert_equals(tx("box.space.s:select({1, 0}, {iterator = 'LT'})"),
104+
{{}})
105+
tx:commit()
106+
box.space.s:insert{0, 0}
107+
108+
tx:begin()
109+
box.space.s:delete{0, 0}
110+
t.assert_equals(tx("box.space.s:select({0, 0}, {iterator = 'LE'})"),
111+
{{}})
112+
tx:commit()
113+
box.space.s:insert{0, 0}
114+
115+
tx:begin()
116+
box.space.s:delete{0, 0}
117+
t.assert_equals(tx("box.space.s:select({1}, {iterator = 'LT'})"),
118+
{{}})
119+
tx:commit()
120+
box.space.s:insert{0, 0}
121+
122+
tx:begin()
123+
box.space.s:delete{0, 0}
124+
t.assert_equals(tx("box.space.s:select({0}, {iterator = 'LE'})"),
125+
{{}})
126+
tx:commit()
127+
box.space.s:insert{0, 0}
128+
129+
tx:begin()
130+
box.space.s:delete{0, 0}
131+
t.assert_equals(tx("box.space.s:select({0}, {iterator = 'REQ'})"),
132+
{{}})
133+
tx:commit()
134+
box.space.s:insert{0, 0}
135+
end)
136+
end

0 commit comments

Comments
 (0)