From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C281F42EF5C for ; Fri, 19 Jun 2020 15:34:26 +0300 (MSK) Date: Fri, 19 Jun 2020 12:34:25 +0000 From: Nikita Pettik Message-ID: <20200619123425.GB19725@tarantool.org> References: <50a25fbae907f1b0d5406fb2e78d40d3e42a8a8d.1591029888.git.korablev@tarantool.org> <4e9d5184-1b3a-d4de-e040-a3edb6ca38cd@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4e9d5184-1b3a-d4de-e040-a3edb6ca38cd@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.