From cb06201765911142e0a6a9afefa6941d55faec9d Mon Sep 17 00:00:00 2001
From: Nikita Pettik <korablev@tarantool.org>
Date: Fri, 24 Apr 2020 21:10:08 +0300
Subject: [PATCH] vinyl: add test on failed write iterator during compaction
vy_task_write_run() is executed in auxiliary thread (dump or
compaction). Write iterator is created and used inside this function.
Meanwhile, creating/destroying tuples in these threads does not change
reference counter of corresponding tuple formats (see vy_tuple_delete()
and vy_stmt_alloc()). Without cleaning up write iterator right in
write_iterator_start() after fail, this procedure takes place in
vy_task_compaction_abort() or vy_task_dump_abort(). These *_abort()
functions in turn are executed in the main thread. Taking this into
consideration, tuple might be allocated in aux. thread and deleted in
the main thread. As a result, format reference counter might decrease,
whereas it shouldn't change (otherwise tuple format will be destroyed
before all tuples of this format are gone).
Fortunately, clean-up of write iterator in another thread was found only
on 1.10 branch, master branch already contains fix but lacks test
(2f17c92). So let's introduce test with following scenario:
1. run compaction process;
2. add one or more slice sources in vy_write_iterator_start():
corresponding slice_stream structures obtain newly created tuples
in vy_slice_stream_next();
3. the next call of vy_write_iterator_add_src() fails due to OOM,
invalid run file or whatever;
4. if write_iterator_start() didn't provide clean-up of sources, it
would take place in vy_task_dump_abort() which would be executed in
the main thread;
5. now format reference counter would be less than it was before
compaction.
Closes #4864
---
src/box/vy_write_iterator.c | 9 +++
src/lib/core/errinj.h | 1 +
test/box/errinj.result | 1 +
.../gh-4864-stmt-alloc-fail-compact.result | 69 +++++++++++++++++++
.../gh-4864-stmt-alloc-fail-compact.test.lua | 33 +++++++++
5 files changed, 113 insertions(+)
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 70064c66e5..01962faa3e 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -405,6 +405,15 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
rlist_foreach_entry(src, &stream->src_list, in_src_list) {
if (vy_write_iterator_add_src(stream, src) != 0)
goto fail;
+#ifndef NDEBUG
+ struct errinj *inj =
+ errinj(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL);
+ if (inj != NULL && inj->bparam) {
+ inj->bparam = false;
+ diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt");
+ goto fail;
+ }
+#endif
}
return 0;
fail:
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index f3d42d72a8..27562b8b50 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -142,6 +142,7 @@ struct errinj {
_(ERRINJ_TXN_COMMIT_ASYNC, ERRINJ_BOOL, {.bparam = false})\
_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
_(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL, {.bparam = false})\
+ _(ERRINJ_VY_WRITE_ITERATOR_START_FAIL, ERRINJ_BOOL, {.bparam = false})\
ENUM0(errinj_id, ERRINJ_LIST);
extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index a1c13b5899..12c7e2c085 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -101,6 +101,7 @@ evals
- ERRINJ_VY_SQUASH_TIMEOUT: 0
- ERRINJ_VY_STMT_ALLOC: -1
- ERRINJ_VY_TASK_COMPLETE: false
+ - ERRINJ_VY_WRITE_ITERATOR_START_FAIL: false
- ERRINJ_WAL_BREAK_LSN: -1
- ERRINJ_WAL_DELAY: false
- ERRINJ_WAL_FALLOCATE: 0
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
index 7de141e362..6a1f49626b 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -248,6 +248,75 @@ s:drop()
| ---
| ...
+-- Make sure that there's no extra format unref due to tuple
+-- clean-up in the main thread. To achieve this let's sabotage
+-- compaction process and delete all tuples: in case ref/unref
+-- is the same, format will be deleted alongside with the last
+-- tuple.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+
+compact(1)
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
+ | ---
+ | - ok
+ | ...
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
+ | ---
+ | - ok
+ | ...
+compact(2)
+ | ---
+ | ...
+
+-- Drop is required to unref all tuples.
+--
+s:drop()
+ | ---
+ | ...
+-- After index is dropped, not all tuples are deallocated at once:
+-- they may be still referenced (while being pushed) in Lua. So
+-- invoke GC explicitly.
+--
+_ = collectgarbage("collect")
+ | ---
+ | ...
+
+assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false)
+ | ---
+ | - ok
+ | ...
errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
| ---
| - ok
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
index eebd168392..4b3c55505d 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -111,4 +111,37 @@ assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
s:drop()
+-- Make sure that there's no extra format unref due to tuple
+-- clean-up in the main thread. To achieve this let's sabotage
+-- compaction process and delete all tuples: in case ref/unref
+-- is the same, format will be deleted alongside with the last
+-- tuple.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+dump(true)
+dump()
+
+compact(1)
+
+dump()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
+
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
+compact(2)
+
+-- Drop is required to unref all tuples.
+--
+s:drop()
+-- After index is dropped, not all tuples are deallocated at once:
+-- they may be still referenced (while being pushed) in Lua. So
+-- invoke GC explicitly.
+--
+_ = collectgarbage("collect")
+
+assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false)
+errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false)
errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
--
GitLab