Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL

[Nikita Pettik <korablev@tarantool.org>]

On 01 Jun 19:40, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 01/06/2020 18:46, Nikita Pettik wrote:
> > Data read in vinyl is known to yield in case of disc access. So it opens
> > a window for modifications of in-memory level. Imagine following scenario:
> > 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
> > is not yet reached WAL.  Meanwhile iterator is starting to read the same key.
> > At this moment prepared statement is already inserted to in-memory tree
> > ergo visible to read iterator. 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. Next, two cases are possible:
> > 1. WAL thread has enough time to complete rollback procedure.
> > 2. WAL thread fails to finish rollback in this time gap.
> Seems like the second test case is not really different from no rollback
> at all. Because basically the WAL thread execution is still in the same
> function, wal_write_to_disk(). Both ERRINJ_WAL_DELAY and ERRINJ_RELAY_FASTER_THAN_TX
> give the same effect, if the transaction is rolled back. If it is committed,
> they give also the same effect from TX<->WAL interaction point of view. The
> difference only appears in TX<->WAL<->Relay threads.
> 
> So probably you could drop this case from the patch. It would simplify
> things. But up to you and Alexander L.
> 
> Talking of the tests, I tried running multiple instances of the new
> test:
> 
>     python test-run.py gh-3395-read-prepared-un gh-3395-read-prepared-un gh-3395-read-prepared-un gh-3395-read-prepared-un gh-3395-read-prepared-un gh-3395-read-prepared-un
> 
> And it fails sometimes. So it is flaky. Better fix it before push.

I've added a tiny fix:

diff --git a/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua b/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua
index 10b70dd85..70136ddff 100644
--- a/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua
+++ b/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua
@@ -55,9 +55,7 @@ function read_prepared_with_delay(is_tx_faster_than_wal)
         errinj.set("ERRINJ_RELAY_FASTER_THAN_TX", true)
     end
     errinj.set("ERRINJ_WAL_DELAY", false)
-    if not is_tx_faster_than_wal then
-        fiber.sleep(0.1)
-    end
+    fiber.sleep(0.1)
     errinj.set("ERRINJ_VY_READ_PAGE_DELAY", false)
     local res = c:get()
     errinj.set("ERRINJ_WAL_WRITE", false)


Could you please check if it still fails on your machine?
 
> > diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c
> > index dcc9a0873..62a8722d9 100644
> > --- a/src/box/vy_read_iterator.c
> > +++ b/src/box/vy_read_iterator.c
> > @@ -382,6 +382,9 @@ vy_read_iterator_scan_disk(struct vy_read_iterator *itr, uint32_t disk_src,
> >   * Restore the position of the active in-memory tree iterator
> >   * after a yield caused by a disk read and update 'next'
> >   * if necessary.
> > + * @retval -1 In case of error (e.g. OOM);
> > + * @retval 0 Successful execution;
> > + * @retval 1 Restart of advance_iterator is required.
> 
> What is advance_iterator?

It is supposed to be vy_read_iterator_advance(). Nevermind, in the
new version I've gotten rid of this function at all.