Skip to content

Commit

Permalink
vinyl: fix cache iterator skipping tuples in read view
Browse files Browse the repository at this point in the history
The tuple cache doesn't store older tuple versions so if a reader is
in a read view, it must skip tuples that are newer than the read view,
see `vy_cache_iterator_stmt_is_visible()`. A reader must also ignore
cached intervals if any of the tuples used as a boundary is invisible
from the read view, see `vy_cache_iterator_skip_to_read_view()`.
There's a bug in `vy_cache_iterator_restore()` because of which such
an interval may be returned to the reader: when we step backwards
from the last returned tuple we consider only one of the boundaries.
As a result, if the other boundary is invisible from the read view,
the reader will assume there's nothing in the index between the
boundaries and skip reading older sources (memory, disk). Fix this by
always checking if the other boundary is visible.

Closes #10109

NO_DOC=bug fix
  • Loading branch information
locker committed Jun 13, 2024
1 parent 72763f9 commit 7b72080
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 2 deletions.
4 changes: 4 additions & 0 deletions changelogs/unreleased/gh-10109-vy-read-iterator-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## bugfix/vinyl

* Fixed a bug when a tuple was not returned by range `select`. The bug could
also trigger a crash in the read iterator (gh-10109).
7 changes: 5 additions & 2 deletions src/box/vy_cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -826,10 +826,13 @@ vy_cache_iterator_restore(struct vy_cache_iterator *itr, struct vy_entry last,
struct vy_cache_node *node =
*vy_cache_tree_iterator_get_elem(tree, &pos);
int cmp = dir * vy_entry_compare(node->entry, key, def);
bool is_visible = vy_cache_iterator_stmt_is_visible(
itr, node->entry.stmt);
if (!is_visible)
*stop = false;
if (cmp < 0 || (cmp == 0 && !key_belongs))
break;
if (vy_cache_iterator_stmt_is_visible(
itr, node->entry.stmt)) {
if (is_visible) {
itr->curr_pos = pos;
if (itr->curr.stmt != NULL)
tuple_unref(itr->curr.stmt);
Expand Down
41 changes: 41 additions & 0 deletions test/vinyl-luatest/gh_10109_read_iterator_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,44 @@ g.test_read_upsert = function(cg)
{{400}, {300}, {200}, {100}})
end)
end

g.test_read_cache = function(cg)
cg.server:exec(function()
local fiber = require('fiber')
local timeout = 30
box.error.injection.set('ERRINJ_VY_COMPACTION_DELAY', true)
local s = box.schema.create_space('test', {engine = 'vinyl'})
s:create_index('pk')
s:replace({500})
box.snapshot()
s:upsert({200}, {})
box.snapshot()
s:replace({100})
s:replace({300})
s:replace({400})
box.snapshot()
local c = fiber.channel()
fiber.new(function()
local _
local result = {}
local gen, param, state = s:pairs({400}, {iterator = 'lt'})
_, result[#result + 1] = gen(param, state) -- {300}
c:put(true)
c:get()
_, result[#result + 1] = gen(param, state) -- {200}
_, result[#result + 1] = gen(param, state) -- {100}
_, result[#result + 1] = gen(param, state) -- eof
c:put(result)
end)
t.assert(c:get(timeout))
s:replace({350, 'x'}) -- send the iterator to a read view
s:replace({150, 'x'}) -- must be invisible to the iterator
-- Add the interval connecting the tuple {100} visible from the read
-- view with the tuple {150, 'x'} invisible from the read view to
-- the cache.
t.assert_equals(s:select({100}, {iterator = 'ge', limit = 2}),
{{100}, {150, 'x'}})
c:put(true, timeout)
t.assert_equals(c:get(timeout), {{300}, {200}, {100}})
end)
end

0 comments on commit 7b72080

Please sign in to comment.