Re: [Tarantool-patches] [PATCH] vinyl: restart read iterator in case of roll backed WAL

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

I would suggest adding Alexander L. to the review. He participated in
implementation of iterator restoration a lot.

See 2 comments below.

1. I see the test fails on the branch in one of the jobs:
https://travis-ci.org/github/tarantool/tarantool/jobs/688791208

I also was able to reproduce it locally. The test hangs, or prints
mismatching results like this:

[001] vinyl/gh-3395-read-prepared-uncommitted.test.lua                [ fail ]
[001] 
[001] Test failed! Result content mismatch:
[001] --- vinyl/gh-3395-read-prepared-uncommitted.result	Wed May 20 23:42:38 2020
[001] +++ vinyl/gh-3395-read-prepared-uncommitted.reject	Wed May 20 23:51:18 2020
[001] @@ -82,7 +82,8 @@
[001]  
[001]  c:get()
[001]   | ---
[001] - | - - [2, 2]
[001] + | - - [1, 2]
[001] + |   - [2, 2]
[001]   |   - [3, 2]
[001]   | ...
[001]  
[001]

The mismatch happens almost on every run.

> diff --git a/test/vinyl/gh-3395-read-prepared-uncommitted.result b/test/vinyl/gh-3395-read-prepared-uncommitted.result
> new file mode 100644
> index 000000000..a4ec023fd
> --- /dev/null
> +++ b/test/vinyl/gh-3395-read-prepared-uncommitted.result
> @@ -0,0 +1,95 @@
> +-- test-run result file version 2
> +-- Disk scan in vinyl is known to yield during read. So it opens
> +-- a window for modifications of in-memory level. Let's test
> +-- following case: right before data selection tuple is inserted
> +-- into space. It passes first stage of commit procedure, i.e. it
> +-- is prepared to be committed but still not yet reached WAL.
> +-- Meanwhile we are starting read of the same key. At this moment
> +-- prepared statement is already inserted to in-memory tree. So,
> +-- read iterator fetches this statement and proceeds to disk scan.
> +-- In turn, disk scan yields and in this moment WAL fails to write
> +-- statement on disk, so it is roll-backed. Read iterator must
> +-- recognize this situation and must not include roll-backed tuple
> +-- into result set.
> +--
> +fiber = require('fiber')
> + | ---
> + | ...
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +s = box.schema.create_space('test', {engine = 'vinyl'})
> + | ---
> + | ...
> +pk = s:create_index('pk')
> + | ---
> + | ...
> +sk = s:create_index('sk', {parts = {{2, 'unsigned'}}, unique = false})
> + | ---
> + | ...
> +s:replace{3, 2}
> + | ---
> + | - [3, 2]
> + | ...
> +s:replace{2, 2}
> + | ---
> + | - [2, 2]
> + | ...
> +box.snapshot()
> + | ---
> + | - ok
> + | ...
> +-- Tuple {1, 2} is not going to be committed, so it should not
> +-- appear in the result set.
> +--
> +c = fiber.channel(1)
> + | ---
> + | ...
> +function do_write() s:replace{1, 2} end
> + | ---
> + | ...
> +errinj.set("ERRINJ_WAL_DELAY", true)
> + | ---
> + | - ok
> + | ...
> +writer = fiber.create(do_write)
> + | ---
> + | ...
> +
> +function do_read() local ret = sk:select{2} c:put(ret) end

2. What if it will read not the first tuple? What if it will
read a second or a third? I didn't check, just want to ensure,
that the iterator restart in this case won't literally restart
the iteration, and won't return the same tuples twice.