[Tarantool-patches] [PATCH v3 0/3] vinyl: fix uninitialized memory accesses

[Tarantool-patches] [PATCH v3 0/3] vinyl: fix uninitialized memory accesses

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/commits/np/gh-4864-access-to-uninit-mem
Issue: https://github.com/tarantool/tarantool/issues/4864

Changes in v2:
 - replaced error injection ERRINJ_VY_MAX_TUPLE_SIZE with
ERRINJ_VY_STMT_ALLOC (i.e. now vy_stmt_alloc() fails not due to exceed
max size, but owing to allocation failure);
 - found another one use-after-free bug in case vy_read_view_merge()
fails. Fix is merged into second patch;
 - added ERRINJ_VY_READ_VIEW_MERGE_FAIL to provide test case in
case of vy_read_view_merge() failure;
 - fixed test covering second bug: error injection value accidentally
was set to a wrong value (bad copy-paste).

Changes in v3:
 - instead of nullifing read views (which may lead to tuple leaks), let's
call proper finalizing routine vy_read_view_stmt_destroy();
 - found another one possible crash due to extra tuple format unref
in case of failed compaction; fix to that is introduced in third patch
in series.

@ChangeLog:
* Fixed crash during compaction due to tuples with size exceeding
vinyl_max_tuple_size setting.

Nikita Pettik (3):
  vinyl: init all vars before cleanup in vy_lsm_split_range()
  vinyl: clean-up unprocessed read views in *_build_read_views()
  vinyl: clean-up write iterator if vy_task_write_run() fails

 src/box/vy_lsm.c                              |   4 +-
 src/box/vy_scheduler.c                        |   4 +-
 src/box/vy_stmt.c                             |  10 +
 src/box/vy_write_iterator.c                   |  32 +-
 src/errinj.h                                  |   3 +
 test/box/errinj.result                        |   3 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 333 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 152 ++++++++
 test/vinyl/suite.ini                          |   2 +-
 9 files changed, 536 insertions(+), 7 deletions(-)
 create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.result
 create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua

-- 
2.17.1

[Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Nikita Pettik]

If vy_key_from_msgpack() fails in vy_lsm_split_range(), clean-up
procedure is called. However, at this moment struct vy_range *parts[2]
is not initialized ergo contains garbage and access to this structure
may result in crash, segfault or disk formatting. Let's move
initialization of mentioned variables before call of
vy_lsm_split_range().

Part of #4864
---
 src/box/vy_lsm.c                              |   4 +-
 src/box/vy_stmt.c                             |  10 ++
 src/errinj.h                                  |   1 +
 test/box/errinj.result                        |   1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 123 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  |  55 ++++++++
 test/vinyl/suite.ini                          |   2 +-
 7 files changed, 193 insertions(+), 3 deletions(-)
 create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.result
 create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua

diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 3d3f41b7a..04c9926a8 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -1069,7 +1069,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range)
 
 	/* Split a range in two parts. */
 	const int n_parts = 2;
-
+	struct vy_range *parts[2] = { NULL, NULL };
 	/*
 	 * Determine new ranges' boundaries.
 	 */
@@ -1088,7 +1088,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range)
 	 * the old range's runs for them.
 	 */
 	struct vy_slice *slice, *new_slice;
-	struct vy_range *part, *parts[2] = {NULL, };
+	struct vy_range *part = NULL;
 	for (int i = 0; i < n_parts; i++) {
 		part = vy_range_new(vy_log_next_id(), keys[i], keys[i + 1],
 				    lsm->cmp_def);
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index 9b7c55516..fafe6e56f 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -140,6 +140,16 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
 		error_log(diag_last_error(diag_get()));
 		return NULL;
 	}
+#ifndef NDEBUG
+	struct errinj *inj = errinj(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT);
+	if (inj != NULL && inj->iparam >= 0) {
+		if (inj->iparam-- == 0) {
+			diag_set(OutOfMemory, total_size, "malloc",
+				 "struct vy_stmt");
+			return NULL;
+		}
+	}
+#endif
 	struct tuple *tuple = malloc(total_size);
 	if (unlikely(tuple == NULL)) {
 		diag_set(OutOfMemory, total_size, "malloc", "struct vy_stmt");
diff --git a/src/errinj.h b/src/errinj.h
index 2cb090b68..383dafcb5 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -127,6 +127,7 @@ struct errinj {
 	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
+	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 8090deedc..efbb4e85e 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -75,6 +75,7 @@ evals
   - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT: 0
   - ERRINJ_VY_SCHED_TIMEOUT: 0
   - ERRINJ_VY_SQUASH_TIMEOUT: 0
+  - ERRINJ_VY_STMT_ALLOC: -1
   - ERRINJ_VY_TASK_COMPLETE: false
   - ERRINJ_WAL_BREAK_LSN: -1
   - ERRINJ_WAL_DELAY: false
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
new file mode 100644
index 000000000..1afc02bef
--- /dev/null
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -0,0 +1,123 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+fiber = require('fiber')
+ | ---
+ | ...
+digest = require('digest')
+ | ---
+ | ...
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function dump(big)
+    local step = big and 1 or 5
+    for i = 1, 20, step do
+        s:replace{i, digest.urandom(1000)}
+    end
+    box.snapshot()
+end;
+ | ---
+ | ...
+
+function compact()
+    s.index.pk:compact()
+    repeat
+        fiber.sleep(0.001)
+        local info = s.index.pk:stat()
+    until info.range_count == info.run_count
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+-- The first run should be big enough to prevent major compaction
+-- on the next dump, because run_count_per_level is ignored on the
+-- last level.
+--
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 1)
+ | ---
+ | - true
+ | ...
+
+dump()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
+ | ---
+ | - ok
+ | ...
+-- Should finish successfully despite vy_stmt_alloc() failure.
+-- Still split_range() fails, as a result we get one range
+-- instead two.
+--
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 1)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 1)
+ | ---
+ | - true
+ | ...
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+ | ---
+ | - ok
+ | ...
+
+s:drop()
+ | ---
+ | ...
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
new file mode 100644
index 000000000..bf70bdf75
--- /dev/null
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -0,0 +1,55 @@
+test_run = require('test_run').new()
+fiber = require('fiber')
+digest = require('digest')
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+test_run:cmd("setopt delimiter ';'")
+function dump(big)
+    local step = big and 1 or 5
+    for i = 1, 20, step do
+        s:replace{i, digest.urandom(1000)}
+    end
+    box.snapshot()
+end;
+
+function compact()
+    s.index.pk:compact()
+    repeat
+        fiber.sleep(0.001)
+        local info = s.index.pk:stat()
+    until info.range_count == info.run_count
+end;
+test_run:cmd("setopt delimiter ''");
+
+-- The first run should be big enough to prevent major compaction
+-- on the next dump, because run_count_per_level is ignored on the
+-- last level.
+--
+dump(true)
+dump()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
+
+compact()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 1)
+
+dump()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
+-- Should finish successfully despite vy_stmt_alloc() failure.
+-- Still split_range() fails, as a result we get one range
+-- instead two.
+--
+compact()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 1)
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+
+s:drop()
diff --git a/test/vinyl/suite.ini b/test/vinyl/suite.ini
index 1417d7156..ed602bb64 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
+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
 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

Re: [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Aleksandr Lyapunov]

Nice catch! Jumps over initialization must be prohibited. As C++ prohibits.

On 4/27/20 3:52 AM, Nikita Pettik wrote:
> If vy_key_from_msgpack() fails in vy_lsm_split_range(), clean-up
> procedure is called. However, at this moment struct vy_range *parts[2]
> is not initialized ergo contains garbage and access to this structure
> may result in crash, segfault or disk formatting. Let's move
> initialization of mentioned variables before call of
> vy_lsm_split_range().
>
> Part of #4864
> ---
>   src/box/vy_lsm.c                              |   4 +-
>   src/box/vy_stmt.c                             |  10 ++
>   src/errinj.h                                  |   1 +
>   test/box/errinj.result                        |   1 +
>   .../gh-4864-stmt-alloc-fail-compact.result    | 123 ++++++++++++++++++
>   .../gh-4864-stmt-alloc-fail-compact.test.lua  |  55 ++++++++
>   test/vinyl/suite.ini                          |   2 +-
>   7 files changed, 193 insertions(+), 3 deletions(-)
>   create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.result
>   create mode 100644 test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
>
> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> index 3d3f41b7a..04c9926a8 100644
> --- a/src/box/vy_lsm.c
> +++ b/src/box/vy_lsm.c
> @@ -1069,7 +1069,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range)
>   
>   	/* Split a range in two parts. */
>   	const int n_parts = 2;
> -
> +	struct vy_range *parts[2] = { NULL, NULL };
>   	/*
>   	 * Determine new ranges' boundaries.
>   	 */
> @@ -1088,7 +1088,7 @@ vy_lsm_split_range(struct vy_lsm *lsm, struct vy_range *range)
>   	 * the old range's runs for them.
>   	 */
>   	struct vy_slice *slice, *new_slice;
> -	struct vy_range *part, *parts[2] = {NULL, };
> +	struct vy_range *part = NULL;
>   	for (int i = 0; i < n_parts; i++) {
>   		part = vy_range_new(vy_log_next_id(), keys[i], keys[i + 1],
>   				    lsm->cmp_def);
> diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
> index 9b7c55516..fafe6e56f 100644
> --- a/src/box/vy_stmt.c
> +++ b/src/box/vy_stmt.c
> @@ -140,6 +140,16 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
>   		error_log(diag_last_error(diag_get()));
>   		return NULL;
>   	}
> +#ifndef NDEBUG
> +	struct errinj *inj = errinj(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT);
> +	if (inj != NULL && inj->iparam >= 0) {
> +		if (inj->iparam-- == 0) {
> +			diag_set(OutOfMemory, total_size, "malloc",
> +				 "struct vy_stmt");
> +			return NULL;
> +		}
> +	}
> +#endif
>   	struct tuple *tuple = malloc(total_size);
>   	if (unlikely(tuple == NULL)) {
>   		diag_set(OutOfMemory, total_size, "malloc", "struct vy_stmt");
> diff --git a/src/errinj.h b/src/errinj.h
> index 2cb090b68..383dafcb5 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -127,6 +127,7 @@ struct errinj {
>   	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
>   	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
>   	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
> +	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
>   
>   ENUM0(errinj_id, ERRINJ_LIST);
>   extern struct errinj errinjs[];
> diff --git a/test/box/errinj.result b/test/box/errinj.result
> index 8090deedc..efbb4e85e 100644
> --- a/test/box/errinj.result
> +++ b/test/box/errinj.result
> @@ -75,6 +75,7 @@ evals
>     - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT: 0
>     - ERRINJ_VY_SCHED_TIMEOUT: 0
>     - ERRINJ_VY_SQUASH_TIMEOUT: 0
> +  - ERRINJ_VY_STMT_ALLOC: -1
>     - ERRINJ_VY_TASK_COMPLETE: false
>     - ERRINJ_WAL_BREAK_LSN: -1
>     - ERRINJ_WAL_DELAY: false
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> new file mode 100644
> index 000000000..1afc02bef
> --- /dev/null
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -0,0 +1,123 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +fiber = require('fiber')
> + | ---
> + | ...
> +digest = require('digest')
> + | ---
> + | ...
> +
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> + | ---
> + | ...
> +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
> + | ---
> + | ...
> +
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +function dump(big)
> +    local step = big and 1 or 5
> +    for i = 1, 20, step do
> +        s:replace{i, digest.urandom(1000)}
> +    end
> +    box.snapshot()
> +end;
> + | ---
> + | ...
> +
> +function compact()
> +    s.index.pk:compact()
> +    repeat
> +        fiber.sleep(0.001)
> +        local info = s.index.pk:stat()
> +    until info.range_count == info.run_count
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +-- The first run should be big enough to prevent major compaction
> +-- on the next dump, because run_count_per_level is ignored on the
> +-- last level.
> +--
> +dump(true)
> + | ---
> + | ...
> +dump()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 2)
> + | ---
> + | - true
> + | ...
> +
> +compact()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 1)
> + | ---
> + | - true
> + | ...
> +
> +dump()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 2)
> + | ---
> + | - true
> + | ...
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
> + | ---
> + | - ok
> + | ...
> +-- Should finish successfully despite vy_stmt_alloc() failure.
> +-- Still split_range() fails, as a result we get one range
> +-- instead two.
> +--
> +compact()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> + | ---
> + | - true
> + | ...
> +errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
> + | ---
> + | - ok
> + | ...
> +
> +s:drop()
> + | ---
> + | ...
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> new file mode 100644
> index 000000000..bf70bdf75
> --- /dev/null
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> @@ -0,0 +1,55 @@
> +test_run = require('test_run').new()
> +fiber = require('fiber')
> +digest = require('digest')
> +
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
> +
> +test_run:cmd("setopt delimiter ';'")
> +function dump(big)
> +    local step = big and 1 or 5
> +    for i = 1, 20, step do
> +        s:replace{i, digest.urandom(1000)}
> +    end
> +    box.snapshot()
> +end;
> +
> +function compact()
> +    s.index.pk:compact()
> +    repeat
> +        fiber.sleep(0.001)
> +        local info = s.index.pk:stat()
> +    until info.range_count == info.run_count
> +end;
> +test_run:cmd("setopt delimiter ''");
> +
> +-- The first run should be big enough to prevent major compaction
> +-- on the next dump, because run_count_per_level is ignored on the
> +-- last level.
> +--
> +dump(true)
> +dump()
> +assert(s.index.pk:stat().range_count == 1)
> +assert(s.index.pk:stat().run_count == 2)
> +
> +compact()
> +assert(s.index.pk:stat().range_count == 1)
> +assert(s.index.pk:stat().run_count == 1)
> +
> +dump()
> +assert(s.index.pk:stat().range_count == 1)
> +assert(s.index.pk:stat().run_count == 2)
> +
> +errinj = box.error.injection
> +errinj.set('ERRINJ_VY_STMT_ALLOC', 0)
> +-- Should finish successfully despite vy_stmt_alloc() failure.
> +-- Still split_range() fails, as a result we get one range
> +-- instead two.
> +--
> +compact()
> +assert(s.index.pk:stat().range_count == 1)
> +assert(s.index.pk:stat().run_count == 1)
> +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> +errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
> +
> +s:drop()
> diff --git a/test/vinyl/suite.ini b/test/vinyl/suite.ini
> index 1417d7156..ed602bb64 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
> +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
>   config = suite.cfg
>   lua_libs = suite.lua stress.lua large.lua txn_proxy.lua ../box/lua/utils.lua
>   use_unix_sockets = True

Re: [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Nikita Pettik]

On 06 May 12:04, Aleksandr Lyapunov wrote:
> Nice catch! Jumps over initialization must be prohibited. As C++ prohibits.
>

If everyone is okay with this patch, let's push it out of order.
 

Re: [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Aleksandr Lyapunov]

I agree. This particular commit lgtm.

On 5/6/20 4:12 PM, Nikita Pettik wrote:
> On 06 May 12:04, Aleksandr Lyapunov wrote:
>> Nice catch! Jumps over initialization must be prohibited. As C++ prohibits.
>>
> If everyone is okay with this patch, let's push it out of order.
>   

Re: [Tarantool-patches] [PATCH v3 1/3] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Nikita Pettik]

On 06 May 20:52, Aleksandr Lyapunov wrote:
> I agree. This particular commit lgtm.

Pushed to master; cherry-picked on 2.4, 2.3 and 1.10.
 
> On 5/6/20 4:12 PM, Nikita Pettik wrote:
> > On 06 May 12:04, Aleksandr Lyapunov wrote:
> > > Nice catch! Jumps over initialization must be prohibited. As C++ prohibits.
> > > 
> > If everyone is okay with this patch, let's push it out of order.

[Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Nikita Pettik]

vy_write_iterator->read_views[i].history objects are allocated on
region (see vy_write_iterator_push_rv()) during building history of the
given key. However, in case of fail of vy_write_iterator_build_history()
region is truncated but pointers to vy_write_history objects are not
nullified. As a result, they may be accessed (for instance while
finalizing write_iterator object in  vy_write_iterator_stop) which in
turn may lead to crash, segfaul or disk formatting. The same may happen
if vy_read_view_merge() fails during processing of read view array.
Let's clean-up those objects in case of error takes place.

Part of #4864
---
 src/box/vy_write_iterator.c                   |  23 +++-
 src/errinj.h                                  |   1 +
 test/box/errinj.result                        |   1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 125 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  |  53 ++++++++
 5 files changed, 200 insertions(+), 3 deletions(-)

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 7a6a20627..efb88d1ae 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -790,8 +790,11 @@ next_lsn:
 	 * statement around if this is major compaction, because
 	 * there's no tuple it could overwrite.
 	 */
-	if (rc == 0 && stream->is_last_level &&
-	    stream->deferred_delete_stmt != NULL) {
+	if (rc != 0) {
+		for (int i = 0; i < stream->rv_count; ++i)
+			vy_read_view_stmt_destroy(&stream->read_views[i]);
+	} else if (stream->is_last_level &&
+		   stream->deferred_delete_stmt != NULL) {
 		vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
 		stream->deferred_delete_stmt = NULL;
 	}
@@ -834,6 +837,15 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
 		rv->history = NULL;
 		return 0;
 	}
+#ifndef NDEBUG
+	struct errinj *inj =
+		errinj(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL);
+	if (inj != NULL && inj->bparam) {
+		inj->bparam = false;
+		diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt");
+		return -1;
+	}
+#endif
 	/*
 	 * Two possible hints to remove the current UPSERT.
 	 * 1. If the stream is working on the last level, we
@@ -983,8 +995,13 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 		if (rv->history == NULL)
 			continue;
 		if (vy_read_view_merge(stream, hint, rv,
-				       is_first_insert) != 0)
+				       is_first_insert) != 0) {
+			while (rv >= &stream->read_views[0]) {
+				vy_read_view_stmt_destroy(rv);
+				rv--;
+			}
 			goto error;
+		}
 		assert(rv->history == NULL);
 		if (rv->tuple == NULL)
 			continue;
diff --git a/src/errinj.h b/src/errinj.h
index 383dafcb5..b7550bb5e 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -128,6 +128,7 @@ struct errinj {
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
 	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
+	_(ERRINJ_VY_READ_VIEW_MERGE_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 efbb4e85e..e1b9fbe2a 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -68,6 +68,7 @@ evals
   - ERRINJ_VY_READ_PAGE: false
   - ERRINJ_VY_READ_PAGE_DELAY: false
   - ERRINJ_VY_READ_PAGE_TIMEOUT: 0
+  - ERRINJ_VY_READ_VIEW_MERGE_FAIL: false
   - ERRINJ_VY_RUN_DISCARD: false
   - ERRINJ_VY_RUN_FILE_RENAME: false
   - ERRINJ_VY_RUN_WRITE: false
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
index 1afc02bef..af116a4b4 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -121,3 +121,128 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
 s:drop()
  | ---
  | ...
+
+-- All the same except for delayed vy_stmt_alloc() fail.
+-- Re-create space for the sake of test purity.
+--
+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()
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
+ | ---
+ | - ok
+ | ...
+-- Compaction of first range fails, so it is re-scheduled and
+-- then successfully finishes at the second attempt.
+--
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 2)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+ | ---
+ | - ok
+ | ...
+-- Unthrottle scheduler to allow next dump.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
+ | ---
+ | - ok
+ | ...
+
+s:drop()
+ | ---
+ | ...
+
+-- Once again but test that clean-up is made in case
+-- vy_read_view_merge() fails.
+--
+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()
+ | ---
+ | ...
+
+dump()
+ | ---
+ | ...
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
+ | ---
+ | - ok
+ | ...
+compact()
+ | ---
+ | ...
+assert(s.index.pk:stat().range_count == 2)
+ | ---
+ | - true
+ | ...
+assert(s.index.pk:stat().run_count == 2)
+ | ---
+ | - true
+ | ...
+assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
+ | ---
+ | - true
+ | ...
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
+ | ---
+ | - ok
+ | ...
+s:drop()
+ | ---
+ | ...
+
+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 bf70bdf75..a68c73d32 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -53,3 +53,56 @@ assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
 errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
 
 s:drop()
+
+-- All the same except for delayed vy_stmt_alloc() fail.
+-- Re-create space for the sake of test purity.
+--
+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()
+
+dump()
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
+-- Compaction of first range fails, so it is re-scheduled and
+-- then successfully finishes at the second attempt.
+--
+compact()
+assert(s.index.pk:stat().range_count == 2)
+assert(s.index.pk:stat().run_count == 2)
+assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
+errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
+-- Unthrottle scheduler to allow next dump.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
+
+s:drop()
+
+-- Once again but test that clean-up is made in case
+-- vy_read_view_merge() fails.
+--
+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()
+
+dump()
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
+compact()
+assert(s.index.pk:stat().range_count == 2)
+assert(s.index.pk:stat().run_count == 2)
+assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
+errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
+s:drop()
+
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Aleksandr Lyapunov]

This will work, but I think we have to make the patch better.

The cause of the issue is that the state of an iterator is not obvious. 
Is the history
collected or not? Is it merged already? Must it be cleaned up? Even the 
cleanup
is split to two stages - tuple unrefs and region truncate.

Now you propose to call  vy_read_view_stmt_destroy from several places 
that also
unrefs tuples from read view (not only from its history). This makes  
the state
of an iterator even more unpredictable: when vy_write_iterator_stop is 
called
sometimes read views are already cleaned up, sometimes are not.

Let's fix this mess.
1. History is collected (rallocated and refed) only in 
vy_write_iterator_build_history.
2. The history must be destructed after call to 
vy_write_iterator_build_history in any case,
whether an error occurred or not (comment about it?).
3. There must be a method for history destruction with two steps: unref 
all and rtruncate.
4. Only vy_write_iterator_build_read_views must call this method. 
vy_write_iterator_stop
or anybody else shouldn't be concerned about history.
5. vy_write_iterator_build_read_views must not unref resulting tuples 
(entry.stmt) in case of error,
vy_write_iterator_stop must do it.


On 4/27/20 3:52 AM, Nikita Pettik wrote:
> vy_write_iterator->read_views[i].history objects are allocated on
> region (see vy_write_iterator_push_rv()) during building history of the
> given key. However, in case of fail of vy_write_iterator_build_history()
> region is truncated but pointers to vy_write_history objects are not
> nullified. As a result, they may be accessed (for instance while
> finalizing write_iterator object in  vy_write_iterator_stop) which in
> turn may lead to crash, segfaul or disk formatting. The same may happen
> if vy_read_view_merge() fails during processing of read view array.
> Let's clean-up those objects in case of error takes place.
>
> Part of #4864
> ---
>   src/box/vy_write_iterator.c                   |  23 +++-
>   src/errinj.h                                  |   1 +
>   test/box/errinj.result                        |   1 +
>   .../gh-4864-stmt-alloc-fail-compact.result    | 125 ++++++++++++++++++
>   .../gh-4864-stmt-alloc-fail-compact.test.lua  |  53 ++++++++
>   5 files changed, 200 insertions(+), 3 deletions(-)
>
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index 7a6a20627..efb88d1ae 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -790,8 +790,11 @@ next_lsn:
>   	 * statement around if this is major compaction, because
>   	 * there's no tuple it could overwrite.
>   	 */
> -	if (rc == 0 && stream->is_last_level &&
> -	    stream->deferred_delete_stmt != NULL) {
> +	if (rc != 0) {
> +		for (int i = 0; i < stream->rv_count; ++i)
> +			vy_read_view_stmt_destroy(&stream->read_views[i]);
> +	} else if (stream->is_last_level &&
> +		   stream->deferred_delete_stmt != NULL) {
>   		vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
>   		stream->deferred_delete_stmt = NULL;
>   	}
> @@ -834,6 +837,15 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
>   		rv->history = NULL;
>   		return 0;
>   	}
> +#ifndef NDEBUG
> +	struct errinj *inj =
> +		errinj(ERRINJ_VY_READ_VIEW_MERGE_FAIL, ERRINJ_BOOL);
> +	if (inj != NULL && inj->bparam) {
> +		inj->bparam = false;
> +		diag_set(OutOfMemory, 666, "malloc", "struct vy_stmt");
> +		return -1;
> +	}
> +#endif
>   	/*
>   	 * Two possible hints to remove the current UPSERT.
>   	 * 1. If the stream is working on the last level, we
> @@ -983,8 +995,13 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>   		if (rv->history == NULL)
>   			continue;
>   		if (vy_read_view_merge(stream, hint, rv,
> -				       is_first_insert) != 0)
> +				       is_first_insert) != 0) {
> +			while (rv >= &stream->read_views[0]) {
> +				vy_read_view_stmt_destroy(rv);
> +				rv--;
> +			}
>   			goto error;
> +		}
>   		assert(rv->history == NULL);
>   		if (rv->tuple == NULL)
>   			continue;
> diff --git a/src/errinj.h b/src/errinj.h
> index 383dafcb5..b7550bb5e 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -128,6 +128,7 @@ struct errinj {
>   	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
>   	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
>   	_(ERRINJ_VY_STMT_ALLOC, ERRINJ_INT, {.iparam = -1})\
> +	_(ERRINJ_VY_READ_VIEW_MERGE_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 efbb4e85e..e1b9fbe2a 100644
> --- a/test/box/errinj.result
> +++ b/test/box/errinj.result
> @@ -68,6 +68,7 @@ evals
>     - ERRINJ_VY_READ_PAGE: false
>     - ERRINJ_VY_READ_PAGE_DELAY: false
>     - ERRINJ_VY_READ_PAGE_TIMEOUT: 0
> +  - ERRINJ_VY_READ_VIEW_MERGE_FAIL: false
>     - ERRINJ_VY_RUN_DISCARD: false
>     - ERRINJ_VY_RUN_FILE_RENAME: false
>     - ERRINJ_VY_RUN_WRITE: false
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> index 1afc02bef..af116a4b4 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -121,3 +121,128 @@ errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
>   s:drop()
>    | ---
>    | ...
> +
> +-- All the same except for delayed vy_stmt_alloc() fail.
> +-- Re-create space for the sake of test purity.
> +--
> +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()
> + | ---
> + | ...
> +
> +dump()
> + | ---
> + | ...
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
> + | ---
> + | - ok
> + | ...
> +-- Compaction of first range fails, so it is re-scheduled and
> +-- then successfully finishes at the second attempt.
> +--
> +compact()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 2)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 2)
> + | ---
> + | - true
> + | ...
> +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> + | ---
> + | - true
> + | ...
> +errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
> + | ---
> + | - ok
> + | ...
> +-- Unthrottle scheduler to allow next dump.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
> + | ---
> + | - ok
> + | ...
> +
> +s:drop()
> + | ---
> + | ...
> +
> +-- Once again but test that clean-up is made in case
> +-- vy_read_view_merge() fails.
> +--
> +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()
> + | ---
> + | ...
> +
> +dump()
> + | ---
> + | ...
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
> + | ---
> + | - ok
> + | ...
> +compact()
> + | ---
> + | ...
> +assert(s.index.pk:stat().range_count == 2)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 2)
> + | ---
> + | - true
> + | ...
> +assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
> + | ---
> + | - true
> + | ...
> +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
> + | ---
> + | - ok
> + | ...
> +s:drop()
> + | ---
> + | ...
> +
> +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 bf70bdf75..a68c73d32 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> @@ -53,3 +53,56 @@ assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
>   errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
>   
>   s:drop()
> +
> +-- All the same except for delayed vy_stmt_alloc() fail.
> +-- Re-create space for the sake of test purity.
> +--
> +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()
> +
> +dump()
> +
> +errinj = box.error.injection
> +errinj.set('ERRINJ_VY_STMT_ALLOC', 5)
> +-- Compaction of first range fails, so it is re-scheduled and
> +-- then successfully finishes at the second attempt.
> +--
> +compact()
> +assert(s.index.pk:stat().range_count == 2)
> +assert(s.index.pk:stat().run_count == 2)
> +assert(errinj.get('ERRINJ_VY_STMT_ALLOC') == -1)
> +errinj.set('ERRINJ_VY_STMT_ALLOC', -1)
> +-- Unthrottle scheduler to allow next dump.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.0001)
> +
> +s:drop()
> +
> +-- Once again but test that clean-up is made in case
> +-- vy_read_view_merge() fails.
> +--
> +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()
> +
> +dump()
> +
> +errinj = box.error.injection
> +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', true)
> +compact()
> +assert(s.index.pk:stat().range_count == 2)
> +assert(s.index.pk:stat().run_count == 2)
> +assert(errinj.get('ERRINJ_VY_READ_VIEW_MERGE_FAIL') == false)
> +errinj.set('ERRINJ_VY_READ_VIEW_MERGE_FAIL', false)
> +s:drop()
> +
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0)

Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Nikita Pettik]

On 06 May 12:56, Aleksandr Lyapunov wrote:
> This will work, but I think we have to make the patch better.
> 
> The cause of the issue is that the state of an iterator is not obvious. Is
> the history
> collected or not? Is it merged already? Must it be cleaned up? Even the
> cleanup
> is split to two stages - tuple unrefs and region truncate.
> 
> Now you propose to call  vy_read_view_stmt_destroy from several places that
> also
> unrefs tuples from read view (not only from its history). This makes  the
> state
> of an iterator even more unpredictable: when vy_write_iterator_stop is
> called
> sometimes read views are already cleaned up, sometimes are not.

On the other hand clean-up provided by vy_read_view_stmt_destroy()
is safe: it nullifies rv->tuple and rv->history so that next call
of vy_read_view_stmt_destroy() will be no-op in fact.
Anyway, I've refactored this code according to your plan, so
vy_read_view_stmt_destroy() now unrefs only resulting tuple.

> Let's fix this mess.
> 1. History is collected (rallocated and refed) only in
> vy_write_iterator_build_history.

Check.

> 2. The history must be destructed after call to
> vy_write_iterator_build_history in any case,
> whether an error occurred or not (comment about it?).

But histories are used in vy_read_view_merge(). Do you mean destruct all
histories after they are processed (i.e. before region_truncate())?

> 3. There must be a method for history destruction with two steps: unref all
> and rtruncate.
> 4. Only vy_write_iterator_build_read_views must call this method.
> vy_write_iterator_stop
> or anybody else shouldn't be concerned about history.
> 5. vy_write_iterator_build_read_views must not unref resulting tuples
> (entry.stmt) in case of error,
> vy_write_iterator_stop must do it.

Ok, I agree. I've taken these points into consideration while preparing
next version. Please, check it out. Also here's diff between versions:

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index efb88d1ae..7784dd13a 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -151,9 +151,11 @@ vy_read_view_stmt_destroy(struct vy_read_view_stmt *rv)
        if (rv->tuple != NULL)
                vy_stmt_unref_if_possible(rv->tuple);
        rv->tuple = NULL;
-       if (rv->history != NULL)
-               vy_write_history_destroy(rv->history);
-       rv->history = NULL;
+       /*
+        * History must be already cleaned up in
+        * vy_write_iterator_build_read_views().
+        */
+       assert(rv->history == NULL);
 }
 
 /* @sa vy_write_iterator.h */
@@ -790,11 +792,7 @@ next_lsn:
         * statement around if this is major compaction, because
         * there's no tuple it could overwrite.
         */
-       if (rc != 0) {
-               for (int i = 0; i < stream->rv_count; ++i)
-                       vy_read_view_stmt_destroy(&stream->read_views[i]);
-       } else if (stream->is_last_level &&
-                  stream->deferred_delete_stmt != NULL) {
+       if (stream->is_last_level && stream->deferred_delete_stmt != NULL) {
                vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
                stream->deferred_delete_stmt = NULL;
        }
@@ -952,6 +950,25 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
        return 0;
 }
 
+/**
+ * Clean up all histories related to given write iterator.
+ * Particular history is allocated using region, so single
+ * region truncation is enough to release all memory at once.
+ * Before that we should also unref tuples stored in those
+ * histories (which is done in vy_write_history_destroy()).
+ */
+static void
+vy_write_iterator_history_destroy(struct vy_write_iterator *stream,
+                                 struct region *region, size_t used)
+{
+       for (int i = 0; i < stream->rv_count; ++i) {
+               if (stream->read_views[i].history != NULL)
+                       vy_write_history_destroy(stream->read_views[i].history);
+               stream->read_views[i].history = NULL;
+       }
+       region_truncate(region, used);
+}
+
 /**
  * Split the current key into a sequence of read view
  * statements. @sa struct vy_write_iterator comment for details
@@ -972,9 +989,12 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
        struct region *region = &fiber()->gc;
        size_t used = region_used(region);
        stream->rv_used_count = 0;
+       int rc = 0;
        if (vy_write_iterator_build_history(stream, &raw_count,
-                                           &is_first_insert) != 0)
-               goto error;
+                                           &is_first_insert) != 0) {
+               rc = -1;
+               goto cleanup;
+       }
        if (raw_count == 0) {
                /* A key is fully optimized. */
                region_truncate(region, used);
@@ -996,11 +1016,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
                        continue;
                if (vy_read_view_merge(stream, hint, rv,
                                       is_first_insert) != 0) {
-                       while (rv >= &stream->read_views[0]) {
-                               vy_read_view_stmt_destroy(rv);
-                               rv--;
-                       }
-                       goto error;
+                       rc = -1;
+                       goto cleanup;
                }
                assert(rv->history == NULL);
                if (rv->tuple == NULL)
@@ -1009,11 +1026,10 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
                ++*count;
                hint = rv->tuple;
        }
-       region_truncate(region, used);
-       return 0;
-error:
-       region_truncate(region, used);
-       return -1;
+
+cleanup:
+       vy_write_iterator_history_destroy(stream, region, used);
+       return rc;
 }

Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Aleksandr Lyapunov]

Thanks for the patch! Almost perfect, except some looking like a 
misprint, see my comments below.

On 5/7/20 3:29 AM, Nikita Pettik wrote:
>
> On the other hand clean-up provided by vy_read_view_stmt_destroy()
> is safe: it nullifies rv->tuple and rv->history so that next call
> of vy_read_view_stmt_destroy() will be no-op in fact.
> Anyway, I've refactored this code according to your plan, so
> vy_read_view_stmt_destroy() now unrefs only resulting tuple.
Yes, as I said your code was correct, but I'm talking about good structure
of the code that is easier to support and (I hope) leave less chances
to make a mistake or forget something like cleanup. Divide responsibility
between parts of code and conquer!
>
>> 2. The history must be destructed after call to
>> vy_write_iterator_build_history in any case,
>> whether an error occurred or not (comment about it?).
> But histories are used in vy_read_view_merge(). Do you mean destruct all
> histories after they are processed (i.e. before region_truncate())?
Exactly, in the end of vy_write_iterator_build_read_views.
>
> -       if (rc != 0) {
> -               for (int i = 0; i < stream->rv_count; ++i)
> -                       vy_read_view_stmt_destroy(&stream->read_views[i]);
> -       } else if (stream->is_last_level &&
> -                  stream->deferred_delete_stmt != NULL) {
> +       if (stream->is_last_level && stream->deferred_delete_stmt != NULL) {
>                  vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
>                  stream->deferred_delete_stmt = NULL;
>          }
Originally it was:

if (rc == 0 && stream->is_last_level && stream->deferred_delete_stmt != NULL)

Are you sure you want to change it, or it was a misprint?.

Re: [Tarantool-patches] [PATCH v3 2/3] vinyl: clean-up unprocessed read views in *_build_read_views()

[Nikita Pettik]

On 07 May 11:44, Aleksandr Lyapunov wrote:
> Thanks for the patch! Almost perfect, except some looking like a misprint,
> see my comments below.
> 
> On 5/7/20 3:29 AM, Nikita Pettik wrote:
> > 
> > On the other hand clean-up provided by vy_read_view_stmt_destroy()
> > is safe: it nullifies rv->tuple and rv->history so that next call
> > of vy_read_view_stmt_destroy() will be no-op in fact.
> > Anyway, I've refactored this code according to your plan, so
> > vy_read_view_stmt_destroy() now unrefs only resulting tuple.
> Yes, as I said your code was correct, but I'm talking about good structure
> of the code that is easier to support and (I hope) leave less chances
> to make a mistake or forget something like cleanup. Divide responsibility
> between parts of code and conquer!

Agree.
 
> > > 2. The history must be destructed after call to
> > > vy_write_iterator_build_history in any case,
> > > whether an error occurred or not (comment about it?).
> > But histories are used in vy_read_view_merge(). Do you mean destruct all
> > histories after they are processed (i.e. before region_truncate())?
> Exactly, in the end of vy_write_iterator_build_read_views.
> > 
> > -       if (rc != 0) {
> > -               for (int i = 0; i < stream->rv_count; ++i)
> > -                       vy_read_view_stmt_destroy(&stream->read_views[i]);
> > -       } else if (stream->is_last_level &&
> > -                  stream->deferred_delete_stmt != NULL) {
> > +       if (stream->is_last_level && stream->deferred_delete_stmt != NULL) {
> >                  vy_stmt_unref_if_possible(stream->deferred_delete_stmt);
> >                  stream->deferred_delete_stmt = NULL;
> >          }
> Originally it was:
> 
> if (rc == 0 && stream->is_last_level && stream->deferred_delete_stmt != NULL)
> 
> Are you sure you want to change it, or it was a misprint?.

Oh, it's misprint for sure. Fixed.

[Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Nikita Pettik]

In vy_task_write_run if write_iterator->start() fails, there's no call
of corresponding stop() method. vy_task_write_run() is executed in
auxiliary thread (dump or compaction). 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 vy_task_write_run() after
fail, this procedure takes place in vy_task_compaction_abort() and
vy_task_dump_abort(). These *_abort() functions in turn are executed in
the main thread. Taking this into consideration, tuple may be allocated
in aux. thread and deleted in the main thread. As a result, format
reference counter decreases, whereas it shouldn't change (otherwise
tuple format will be destroyed before all tuples of this format are gone).

Real example of the bug described above can be achieved in the following
way:
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. since there's no clean-up of tuples in slice streams, they are
destroyed in vy_task_compaction_abort() in the main thread exit;
5. now format reference counter is less than it was before compaction.

Closes #4864
---
 src/box/vy_scheduler.c                        |  4 +-
 src/box/vy_write_iterator.c                   |  9 ++
 src/errinj.h                                  |  1 +
 test/box/errinj.result                        |  1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 85 +++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 44 ++++++++++
 6 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 9dba93d34..387f58723 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
 				 no_compression) != 0)
 		goto fail;
 
-	if (wi->iface->start(wi) != 0)
+	if (wi->iface->start(wi) != 0) {
+		wi->iface->stop(wi);
 		goto fail_abort_writer;
+	}
 	int rc;
 	int loops = 0;
 	struct tuple *stmt = NULL;
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index efb88d1ae..0b741b3dc 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -400,6 +400,15 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
 	rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
 		if (vy_write_iterator_add_src(stream, src) != 0)
 			return -1;
+#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");
+			return -1;
+		}
+#endif
 	}
 	return 0;
 }
diff --git a/src/errinj.h b/src/errinj.h
index b7550bb5e..8562aab1c 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -129,6 +129,7 @@ struct errinj {
 	_(ERRINJ_INDEX_RESERVE, 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 e1b9fbe2a..2a87b5f33 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -78,6 +78,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 af116a4b4..ea8dce0ba 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -242,6 +242,91 @@ 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()
+ | ---
+ | ...
+
+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
+ | ...
+-- Prevent next attempt to compact in a row.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
+ | ---
+ | - ok
+ | ...
+
+s.index.pk:compact()
+ | ---
+ | ...
+-- Leave a time gap between compaction and index drop just in case
+-- (to make sure that compaction is already finished (re-scheduled)
+--  when at the moment of index drop).
+--
+fiber.sleep(0.5)
+ | ---
+ | ...
+
+-- 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")
+ | ---
+ | - 0
+ | ...
+-- Give GC some time to operate on.
+--
+fiber.sleep(1)
+ | ---
+ | ...
+
+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 a68c73d32..3c2b38160 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -105,4 +105,48 @@ 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()
+
+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)
+-- Prevent next attempt to compact in a row.
+--
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
+
+s.index.pk:compact()
+-- Leave a time gap between compaction and index drop just in case
+-- (to make sure that compaction is already finished (re-scheduled)
+--  when at the moment of index drop).
+--
+fiber.sleep(0.5)
+
+-- 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")
+-- Give GC some time to operate on.
+--
+fiber.sleep(1)
+
+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)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
> index 9dba93d34..387f58723 100644
> --- a/src/box/vy_scheduler.c
> +++ b/src/box/vy_scheduler.c
> @@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
>  				 no_compression) != 0)
>  		goto fail;
>  
> -	if (wi->iface->start(wi) != 0)
> +	if (wi->iface->start(wi) != 0) {
> +		wi->iface->stop(wi);

1. I would better make start() more self-sufficient. Otherwise it
failed to start, and yet you somewhy need to stop it. Looks confusing.

>  		goto fail_abort_writer;
> +	}
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> index af116a4b4..ea8dce0ba 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -242,6 +242,91 @@ s:drop()
> +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
> + | ...
> +-- Prevent next attempt to compact in a row.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
> + | ---
> + | - ok
> + | ...
> +
> +s.index.pk:compact()
> + | ---
> + | ...
> +-- Leave a time gap between compaction and index drop just in case
> +-- (to make sure that compaction is already finished (re-scheduled)
> +--  when at the moment of index drop).
> +--
> +fiber.sleep(0.5)

2. Can't you wait for compaction actively on some condition? Such as
smaller run count. Half of second is quite a big timeout for a regular
test.

> + | ---
> + | ...
> +
> +-- 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")
> + | ---
> + | - 0
> + | ...
> +-- Give GC some time to operate on.
> +--
> +fiber.sleep(1)

3. GC is synchronous. So if collectgarbage() has returned, GC is done.

> + | ---
> + | ...
> +

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/05/01 22:16]:


> > +-- Give GC some time to operate on.
> > +--
> > +fiber.sleep(1)
> 
> 3. GC is synchronous. So if collectgarbage() has returned, GC is done.

Unchecked sleeps should be banned from functional tests regardless.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Nikita Pettik]

On 01 May 02:55, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
> > diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
> > index 9dba93d34..387f58723 100644
> > --- a/src/box/vy_scheduler.c
> > +++ b/src/box/vy_scheduler.c
> > @@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
> >  				 no_compression) != 0)
> >  		goto fail;
> >  
> > -	if (wi->iface->start(wi) != 0)
> > +	if (wi->iface->start(wi) != 0) {
> > +		wi->iface->stop(wi);
> 
> 1. I would better make start() more self-sufficient. Otherwise it
> failed to start, and yet you somewhy need to stop it. Looks confusing.

Ok, here's diff:

diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 387f58723..9dba93d34 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1065,10 +1065,8 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
                                 no_compression) != 0)
                goto fail;
 
-       if (wi->iface->start(wi) != 0) {
-               wi->iface->stop(wi);
+       if (wi->iface->start(wi) != 0)
                goto fail_abort_writer;
-       }
        int rc;
        int loops = 0;
        struct tuple *stmt = NULL;
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 21c18d3dc..33ad5ed51 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -401,18 +401,23 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
        struct vy_write_src *src, *tmp;
        rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
                if (vy_write_iterator_add_src(stream, src) != 0)
-                       return -1;
+                       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");
-                       return -1;
+                       goto fail;
                }
 #endif
        }
        return 0;
+fail:
+       /* Clean-up all previously added sources. */
+       rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp)
+               vy_write_iterator_delete_src(stream, src);
+       return -1;
 }

 
> > diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> > index af116a4b4..ea8dce0ba 100644
> > --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> > +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> > @@ -242,6 +242,91 @@ s:drop()
> > +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
> > + | ...
> > +-- Prevent next attempt to compact in a row.
> > +--
> > +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
> > + | ---
> > + | - ok
> > + | ...
> > +
> > +s.index.pk:compact()
> > + | ---
> > + | ...
> > +-- Leave a time gap between compaction and index drop just in case
> > +-- (to make sure that compaction is already finished (re-scheduled)
> > +--  when at the moment of index drop).
> > +--
> > +fiber.sleep(0.5)
> 
> 2. Can't you wait for compaction actively on some condition? Such as
> smaller run count. Half of second is quite a big timeout for a regular
> test.

Ok, but here we can't rely on run/range count, but can use
explicit scheduler.tasks_completed statistics. Diff:

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 3c2b38160..547ab628e 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -124,16 +124,15 @@ 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)
--- Prevent next attempt to compact in a row.
---
-errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
-
+errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 0.1)
+tasks_completed = box.stat.vinyl().scheduler.tasks_completed
 s.index.pk:compact()
--- Leave a time gap between compaction and index drop just in case
--- (to make sure that compaction is already finished (re-scheduled)
---  when at the moment of index drop).
+-- Tuple clean-up takes place after compaction is completed.
+-- Meanwhile range count is updated during compaction process.
+-- So instead of relying on range/run match, let's check explicitly
+-- number of completed tasks.
 --
-fiber.sleep(0.5)
+repeat fiber.sleep(0.001) until box.stat.vinyl().scheduler.tasks_completed >= tasks_completed + 1
 
 -- Drop is required to unref all tuples.
 --
@@ -142,10 +141,7 @@ s:drop()
 -- they may be still referenced (while being pushed) in Lua. So
 -- invoke GC explicitly.
 --
-collectgarbage("collect")
--- Give GC some time to operate on.
---
-fiber.sleep(1)
+_ = collectgarbage("collect")
 
 assert(errinj.get('ERRINJ_VY_WRITE_ITERATOR_START_FAIL') == false)
 errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', false)

> > +-- 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")
> > + | ---
> > + | - 0
> > + | ...
> > +-- Give GC some time to operate on.
> > +--
> > +fiber.sleep(1)
> 
> 3. GC is synchronous. So if collectgarbage() has returned, GC is done.

Indeed, removed this sleep.

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Aleksandr Lyapunov]

It's very strange, as I see start() method already makes its own cleanup
in case of failure and does not require 'stop' call in this case.
If it does not do it correctly - it must be fixed (or removed completely,
but it seems to be very serious change).

On 4/27/20 3:52 AM, Nikita Pettik wrote:
> In vy_task_write_run if write_iterator->start() fails, there's no call
> of corresponding stop() method. vy_task_write_run() is executed in
> auxiliary thread (dump or compaction). 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 vy_task_write_run() after
> fail, this procedure takes place in vy_task_compaction_abort() and
> vy_task_dump_abort(). These *_abort() functions in turn are executed in
> the main thread. Taking this into consideration, tuple may be allocated
> in aux. thread and deleted in the main thread. As a result, format
> reference counter decreases, whereas it shouldn't change (otherwise
> tuple format will be destroyed before all tuples of this format are gone).
>
> Real example of the bug described above can be achieved in the following
> way:
> 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. since there's no clean-up of tuples in slice streams, they are
> destroyed in vy_task_compaction_abort() in the main thread exit;
> 5. now format reference counter is less than it was before compaction.
>
> Closes #4864
> ---
>   src/box/vy_scheduler.c                        |  4 +-
>   src/box/vy_write_iterator.c                   |  9 ++
>   src/errinj.h                                  |  1 +
>   test/box/errinj.result                        |  1 +
>   .../gh-4864-stmt-alloc-fail-compact.result    | 85 +++++++++++++++++++
>   .../gh-4864-stmt-alloc-fail-compact.test.lua  | 44 ++++++++++
>   6 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
> index 9dba93d34..387f58723 100644
> --- a/src/box/vy_scheduler.c
> +++ b/src/box/vy_scheduler.c
> @@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
>   				 no_compression) != 0)
>   		goto fail;
>   
> -	if (wi->iface->start(wi) != 0)
> +	if (wi->iface->start(wi) != 0) {
> +		wi->iface->stop(wi);
>   		goto fail_abort_writer;
> +	}
>   	int rc;
>   	int loops = 0;
>   	struct tuple *stmt = NULL;
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index efb88d1ae..0b741b3dc 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -400,6 +400,15 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
>   	rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
>   		if (vy_write_iterator_add_src(stream, src) != 0)
>   			return -1;
> +#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");
> +			return -1;
> +		}
> +#endif
>   	}
>   	return 0;
>   }
> diff --git a/src/errinj.h b/src/errinj.h
> index b7550bb5e..8562aab1c 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -129,6 +129,7 @@ struct errinj {
>   	_(ERRINJ_INDEX_RESERVE, 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 e1b9fbe2a..2a87b5f33 100644
> --- a/test/box/errinj.result
> +++ b/test/box/errinj.result
> @@ -78,6 +78,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 af116a4b4..ea8dce0ba 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -242,6 +242,91 @@ 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()
> + | ---
> + | ...
> +
> +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
> + | ...
> +-- Prevent next attempt to compact in a row.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
> + | ---
> + | - ok
> + | ...
> +
> +s.index.pk:compact()
> + | ---
> + | ...
> +-- Leave a time gap between compaction and index drop just in case
> +-- (to make sure that compaction is already finished (re-scheduled)
> +--  when at the moment of index drop).
> +--
> +fiber.sleep(0.5)
> + | ---
> + | ...
> +
> +-- 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")
> + | ---
> + | - 0
> + | ...
> +-- Give GC some time to operate on.
> +--
> +fiber.sleep(1)
> + | ---
> + | ...
> +
> +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 a68c73d32..3c2b38160 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
> @@ -105,4 +105,48 @@ 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()
> +
> +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)
> +-- Prevent next attempt to compact in a row.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
> +
> +s.index.pk:compact()
> +-- Leave a time gap between compaction and index drop just in case
> +-- (to make sure that compaction is already finished (re-scheduled)
> +--  when at the moment of index drop).
> +--
> +fiber.sleep(0.5)
> +
> +-- 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")
> +-- Give GC some time to operate on.
> +--
> +fiber.sleep(1)
> +
> +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)

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Nikita Pettik]

On 06 May 13:37, Aleksandr Lyapunov wrote:
> It's very strange, as I see start() method already makes its own cleanup
> in case of failure and does not require 'stop' call in this case.
> If it does not do it correctly - it must be fixed (or removed completely,
> but it seems to be very serious change).

Turns out start() doesn't provide proper clean-up: in case
vy_write_iterator_add_src() fails, only last source is cleaned up,
meanwhile the rest will be destroyed in vy_write_iterator_stop().
I've moved clean-up part from stop() method right to start(). Here's diff:

diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 387f58723..9dba93d34 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -1065,10 +1065,8 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
                                 no_compression) != 0)
                goto fail;
 
-       if (wi->iface->start(wi) != 0) {
-               wi->iface->stop(wi);
+       if (wi->iface->start(wi) != 0)
                goto fail_abort_writer;
-       }
        int rc;
        int loops = 0;
        struct tuple *stmt = NULL;
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 21c18d3dc..33ad5ed51 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -401,18 +401,23 @@ vy_write_iterator_start(struct vy_stmt_stream *vstream)
        struct vy_write_src *src, *tmp;
        rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp) {
                if (vy_write_iterator_add_src(stream, src) != 0)
-                       return -1;
+                       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");
-                       return -1;
+                       goto fail;
                }
 #endif
        }
        return 0;
+fail:
+       /* Clean-up all previously added sources. */
+       rlist_foreach_entry_safe(src, &stream->src_list, in_src_list, tmp)
+               vy_write_iterator_delete_src(stream, src);
+       return -1;
 }
 

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Aleksandr Lyapunov]

On 5/7/20 3:36 AM, Nikita Pettik wrote:
> On 06 May 13:37, Aleksandr Lyapunov wrote:
>> It's very strange, as I see start() method already makes its own cleanup
>> in case of failure and does not require 'stop' call in this case.
>> If it does not do it correctly - it must be fixed (or removed completely,
>> but it seems to be very serious change).
> Turns out start() doesn't provide proper clean-up: in case
> vy_write_iterator_add_src() fails, only last source is cleaned up,
> meanwhile the rest will be destroyed in vy_write_iterator_stop().
> I've moved clean-up part from stop() method right to start(). Here's diff:

Could you see commit 2f17c929? I see it in master.
It does exactly the same and some other good looking related changes.

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Nikita Pettik]

On 07 May 10:53, Aleksandr Lyapunov wrote:
> On 5/7/20 3:36 AM, Nikita Pettik wrote:
> > On 06 May 13:37, Aleksandr Lyapunov wrote:
> > > It's very strange, as I see start() method already makes its own cleanup
> > > in case of failure and does not require 'stop' call in this case.
> > > If it does not do it correctly - it must be fixed (or removed completely,
> > > but it seems to be very serious change).
> > Turns out start() doesn't provide proper clean-up: in case
> > vy_write_iterator_add_src() fails, only last source is cleaned up,
> > meanwhile the rest will be destroyed in vy_write_iterator_stop().
> > I've moved clean-up part from stop() method right to start(). Here's diff:
> 
> Could you see commit 2f17c929? I see it in master.
> It does exactly the same and some other good looking related changes.
>

I explored that patch. It is not backported to 1.10 (obviously) since
other changes except for additional clean-up in vy_write_iterator_start()
look like refactoring (we push to 1.10 only bug fixes). It can't be
cherry-picked now since code is bit different and 1.10 lacks some
features (like heap helpers to determine whether heap node is linked or
not). So I suggest to leave my patch as is for 1.10 branch and backport
test case to master branch.
 

Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Aleksandr Lyapunov]

On 5/8/20 1:16 AM, Nikita Pettik wrote:
>
> I explored that patch. It is not backported to 1.10 (obviously) since
> other changes except for additional clean-up in vy_write_iterator_start()
> look like refactoring (we push to 1.10 only bug fixes). It can't be
> cherry-picked now since code is bit different and 1.10 lacks some
> features (like heap helpers to determine whether heap node is linked or
> not). So I suggest to leave my patch as is for 1.10 branch and backport
> test case to master branch.
>   
I think it's OK.