commit 7b72080ddf926d271a4a4cad6f6ffe6e6c00e332
from: Vladimir Davydov <vdavydov@tarantool.org>
via: Vladimir Davydov <vdavydov.dev@gmail.com>
date: Thu Jun 13 07:25:34 2024 UTC

vinyl: fix cache iterator skipping tuples in read view

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

commit - 72763f947b94a99ccc8bb5e0fff2cce943b38497
commit + 7b72080ddf926d271a4a4cad6f6ffe6e6c00e332
blob - /dev/null
blob + c52eea7512d727676173ff13282f197cc58c502c (mode 644)
--- /dev/null
+++ changelogs/unreleased/gh-10109-vy-read-iterator-fix.md
@@ -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).
blob - 0604d6ab96ed50bf0952dae33ddced19f6480a10
blob + 0ebb49229f1f9dcc7efebcd276f01a7d6b02afcc
--- src/box/vy_cache.c
+++ src/box/vy_cache.c
@@ -826,10 +826,13 @@ vy_cache_iterator_restore(struct vy_cache_iterator *it
 			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);
blob - 6e0296e5c037cd8089ae59ad07bdafe9ae9c034a
blob + 3a99e69361e48cfbd630776241bcacdb650773d9
--- test/vinyl-luatest/gh_10109_read_iterator_test.lua
+++ test/vinyl-luatest/gh_10109_read_iterator_test.lua
@@ -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