From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 E7FC4469710 for ; Tue, 19 May 2020 15:50:42 +0300 (MSK) From: Nikita Pettik Date: Tue, 19 May 2020 15:50:40 +0300 Message-Id: <48001497bf915ff9358b5d50a713a7c458255bae.1589892568.git.korablev@tarantool.org> Subject: [Tarantool-patches] [PATCH] vinyl: restart read iterator in case of roll backed WAL List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org Disk scan in vinyl is known to yield during read. 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 we are 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, so it is roll-backed and removed from in-memory tree. Read iterator must recognize this situation and should not include roll-backed tuple into result set. To achieve this let's patch vy_read_iterator_restore_mem() so that it returns return code > 0 in case read iterator should be restarted. Close #3395 --- Branch: https://github.com/tarantool/tarantool/tree/np/gh-3395-vy-read-prepared Issue: https://github.com/tarantool/tarantool/issues/3395 src/box/vy_read_iterator.c | 18 +++- .../gh-3395-read-prepared-uncommitted.result | 95 +++++++++++++++++++ ...gh-3395-read-prepared-uncommitted.test.lua | 41 ++++++++ test/vinyl/suite.ini | 2 +- 4 files changed, 153 insertions(+), 3 deletions(-) create mode 100644 test/vinyl/gh-3395-read-prepared-uncommitted.result create mode 100644 test/vinyl/gh-3395-read-prepared-uncommitted.test.lua diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c index dcc9a0873..d83d5f81d 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. */ static NODISCARD int vy_read_iterator_restore_mem(struct vy_read_iterator *itr, @@ -405,8 +408,14 @@ vy_read_iterator_restore_mem(struct vy_read_iterator *itr, * Memory trees are append-only so if the * source is not on top of the heap after * restoration, it was not before. + * It may turn out that we read statement + * which was prepared but not committed to + * WAL. In such a rare case, we should restart + * iterator_advance() to get the right key. */ - assert(src->front_id < itr->front_id); + assert(src->front_id <= itr->front_id); + if (src->front_id == itr->front_id) + return 1; return 0; } if (cmp < 0) { @@ -529,8 +538,13 @@ rescan_disk: * as it is owned exclusively by the current fiber so the only * source to check is the active in-memory tree. */ - if (vy_read_iterator_restore_mem(itr, &next) != 0) + int rc = vy_read_iterator_restore_mem(itr, &next); + if (rc < 0) return -1; + if (rc > 0) { + vy_read_iterator_restore(itr); + goto restart; + } /* * Scan the next range in case we transgressed the current * range's boundaries. 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 + | --- + | ... +errinj.set("ERRINJ_VY_READ_PAGE_DELAY", true) + | --- + | - ok + | ... +f = fiber.create(do_read) + | --- + | ... +errinj.set("ERRINJ_WAL_WRITE", true) + | --- + | - ok + | ... +errinj.set("ERRINJ_WAL_DELAY", false) + | --- + | - ok + | ... +errinj.set("ERRINJ_VY_READ_PAGE_DELAY", false) + | --- + | - ok + | ... + +c:get() + | --- + | - - [2, 2] + | - [3, 2] + | ... + +errinj.set("ERRINJ_WAL_WRITE", false) + | --- + | - ok + | ... +s:drop() + | --- + | ... diff --git a/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua b/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua new file mode 100644 index 000000000..d75b36d1a --- /dev/null +++ b/test/vinyl/gh-3395-read-prepared-uncommitted.test.lua @@ -0,0 +1,41 @@ +-- 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} +s:replace{2, 2} +box.snapshot() +-- 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) +writer = fiber.create(do_write) + +function do_read() local ret = sk:select{2} c:put(ret) end +errinj.set("ERRINJ_VY_READ_PAGE_DELAY", true) +f = fiber.create(do_read) +errinj.set("ERRINJ_WAL_WRITE", true) +errinj.set("ERRINJ_WAL_DELAY", false) +errinj.set("ERRINJ_VY_READ_PAGE_DELAY", false) + +c:get() + +errinj.set("ERRINJ_WAL_WRITE", false) +s:drop() diff --git a/test/vinyl/suite.ini b/test/vinyl/suite.ini index 8b7e87955..09829b975 100644 --- a/test/vinyl/suite.ini +++ b/test/vinyl/suite.ini @@ -2,7 +2,7 @@ core = tarantool description = vinyl integration tests script = vinyl.lua -release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua gh-4864-stmt-alloc-fail-compact.test.lua gh-4805-open-run-err-recovery.test.lua +release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua gh-4864-stmt-alloc-fail-compact.test.lua gh-4805-open-run-err-recovery.test.lua gh-3395-read-prepared-uncommitted.test.lua config = suite.cfg lua_libs = suite.lua stress.lua large.lua txn_proxy.lua ../box/lua/utils.lua use_unix_sockets = True -- 2.17.1