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

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

[Nikita Pettik]

It is found that if vy_stmt_alloc() fails (due to OOM or most likely
owing to exceeding max tuple size) during compaction process, it may
result in instance crashes. This patch-set contains two fixes for
problems connected with wrong handling of vy_stmt_alloc() failure.

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

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

Nikita Pettik (2):
  vinyl: init all vars before cleanup in vy_lsm_split_range()
  vinyl: clean-up read views if *_build_history() fails

 src/box/vy_lsm.c                              |   4 +-
 src/box/vy_stmt.c                             |   5 +
 src/box/vy_write_iterator.c                   |   5 +-
 src/errinj.h                                  |   1 +
 test/box/errinj.result                        |   1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 144 ++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  |  73 +++++++++
 test/vinyl/suite.ini                          |   2 +-
 8 files changed, 231 insertions(+), 4 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 1/2] 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                             |  5 +
 src/errinj.h                                  |  1 +
 test/box/errinj.result                        |  1 +
 .../gh-4864-stmt-alloc-fail-compact.result    | 93 +++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 49 ++++++++++
 test/vinyl/suite.ini                          |  2 +-
 7 files changed, 152 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..d2469961f 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
 {
 	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
 		bsize;
+	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
+	if (inj != NULL && inj->iparam >= 0) {
+		if (inj->iparam-- == 0)
+			total_size = vy_max_tuple_size + 1;
+	}
 	if (unlikely(total_size > vy_max_tuple_size)) {
 		diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
 			 (unsigned) total_size);
diff --git a/src/errinj.h b/src/errinj.h
index 2cb090b68..dd81938e8 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_MAX_TUPLE_SIZE, 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..d107329f9 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -64,6 +64,7 @@ evals
   - ERRINJ_VY_LOG_FILE_RENAME: false
   - ERRINJ_VY_LOG_FLUSH: false
   - ERRINJ_VY_LOG_FLUSH_DELAY: false
+  - ERRINJ_VY_MAX_TUPLE_SIZE: -1
   - ERRINJ_VY_POINT_ITER_WAIT: false
   - ERRINJ_VY_READ_PAGE: false
   - ERRINJ_VY_READ_PAGE_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..2c03697f6
--- /dev/null
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -0,0 +1,93 @@
+-- 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()
+ | ---
+ | ...
+-- 1 range, 2 runs
+
+compact()
+ | ---
+ | ...
+-- 1 range, 1 run after compaction
+
+dump()
+ | ---
+ | ...
+-- 1 range, 2 runs
+
+errinj = box.error.injection
+ | ---
+ | ...
+errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
+ | ---
+ | - ok
+ | ...
+-- Should finish successfully despite vy_stmt_alloc() fail.
+--
+compact()
+ | ---
+ | ...
+-- 1 range, 1 run
+s.index.pk:stat().range_count
+ | ---
+ | - 1
+ | ...
+s.index.pk:stat().run_count
+ | ---
+ | - 1
+ | ...
+
+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..53a050c9b
--- /dev/null
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -0,0 +1,49 @@
+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()
+-- 1 range, 2 runs
+
+compact()
+-- 1 range, 1 run after compaction
+
+dump()
+-- 1 range, 2 runs
+
+errinj = box.error.injection
+errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
+-- Should finish successfully despite vy_stmt_alloc() fail.
+--
+compact()
+-- 1 range, 1 run
+s.index.pk:stat().range_count
+s.index.pk:stat().run_count
+
+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 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range()

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:

> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> +	if (inj != NULL && inj->iparam >= 0) {
> +		if (inj->iparam-- == 0)
> +			total_size = vy_max_tuple_size + 1;
> +	}

shouldn't this be under #ifdef

The patch is LGTM, please solicit another review.

 

-- 
Konstantin Osipov, Moscow, Russia

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

[Nikita Pettik]

On 09 Apr 11:18, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:
> 
> > +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> > +	if (inj != NULL && inj->iparam >= 0) {
> > +		if (inj->iparam-- == 0)
> > +			total_size = vy_max_tuple_size + 1;
> > +	}
> 
> shouldn't this be under #ifdef

Looks like not: most of other errinj() calls are not wrapped in
#ifdef NDEBUG macro (in case of debug build errinj() always
returns NULL).
 
> The patch is LGTM, please solicit another review.
> 
>  
> 
> -- 
> Konstantin Osipov, Moscow, Russia

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

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/09 14:00]:
> On 09 Apr 11:18, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:
> > 
> > > +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> > > +	if (inj != NULL && inj->iparam >= 0) {
> > > +		if (inj->iparam-- == 0)
> > > +			total_size = vy_max_tuple_size + 1;
> > > +	}
> > 
> > shouldn't this be under #ifdef
> 
> Looks like not: most of other errinj() calls are not wrapped in
> #ifdef NDEBUG macro (in case of debug build errinj() always
> returns NULL).

In most cases the compiler can optimize these away, but the rules
are complicated.
Regarding other uses of errinj() one should take into
consideration whether it's a hot path or not, whether the code
in which injection is used is small/simple or not, etc.

I'd say as a rule of thumb anything using errinj and is multiline
is better off using #ifdef.

> > The patch is LGTM, please solicit another review.

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

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

[Nikita Pettik]

On 09 Apr 14:07, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/04/09 14:00]:
> > On 09 Apr 11:18, Konstantin Osipov wrote:
> > > * Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:
> > > 
> > > > +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> > > > +	if (inj != NULL && inj->iparam >= 0) {
> > > > +		if (inj->iparam-- == 0)
> > > > +			total_size = vy_max_tuple_size + 1;
> > > > +	}
> > > 
> > > shouldn't this be under #ifdef
> > 
> > Looks like not: most of other errinj() calls are not wrapped in
> > #ifdef NDEBUG macro (in case of debug build errinj() always
> > returns NULL).
> 
> In most cases the compiler can optimize these away, but the rules
> are complicated.
> Regarding other uses of errinj() one should take into
> consideration whether it's a hot path or not, whether the code
> in which injection is used is small/simple or not, etc.
> 
> I'd say as a rule of thumb anything using errinj and is multiline
> is better off using #ifdef.

Ok, as you wish, I don't mind this change. Diff:

diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index d2469961f..e822137ec 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -134,11 +134,13 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
 {
        uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
                bsize;
+#ifndef NDEBUG
        struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
        if (inj != NULL && inj->iparam >= 0) {
                if (inj->iparam-- == 0)
                        total_size = vy_max_tuple_size + 1;
        }
+#endif
        if (unlikely(total_size > vy_max_tuple_size)) {
                diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
                         (unsigned) total_size);

 
> > > The patch is LGTM, please solicit another review.
> 
> -- 
> Konstantin Osipov, Moscow, Russia
> https://scylladb.com

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 3 comments below.

> 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
> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
>  {
>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
>  		bsize;
> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> +	if (inj != NULL && inj->iparam >= 0) {
> +		if (inj->iparam-- == 0)

1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
which would set it to false instead of decrement? That would make it
clear the injection works only once.

Also it looks too artificial. The injection basically simulates a tuple
with too big size which was inserted bypassing max_tuple_size check,
and suddenly it was checked here, already after insertion.

Better add an OOM injection for malloc a few lines below, would be more
correct.

> +			total_size = vy_max_tuple_size + 1;
> +	}
>  	if (unlikely(total_size > vy_max_tuple_size)) {
>  		diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
>  			 (unsigned) total_size);
> 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..2c03697f6
> --- /dev/null
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -0,0 +1,93 @@
> +-- 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()
> + | ---
> + | ...
> +-- 1 range, 2 runs

2. Can you add an assertion, that it is really 1 range and 2 runs?
To be sure that the test won't become useless in future if suddenly
something will change there.

> +
> +compact()
> + | ---
> + | ...
> +-- 1 range, 1 run after compaction
> +
> +dump()
> + | ---
> + | ...
> +-- 1 range, 2 runs
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
> + | ---
> + | - ok
> + | ...
> +-- Should finish successfully despite vy_stmt_alloc() fail.
> +--
> +compact()
> + | ---
> + | ...
> +-- 1 range, 1 run
> +s.index.pk:stat().range_count
> + | ---
> + | - 1
> + | ...
> +s.index.pk:stat().run_count

3. I would add an assertion/print, that ERRINJ_VY_MAX_TUPLE_SIZE
(or whatever will be used after the review) is changed back to
off, and then change it to off again anyway. Because if the test
won't hit the errinj because of something in future, the errinj
will be left set, and we won't notice that here. Also it will
make all next vinyl tests running in the same job fail in very
surprising ways.

The same for the next commit.

> + | ---
> + | - 1
> + | ...
> +
> +s:drop()
> + | ---
> + | ...

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

[Nikita Pettik]

On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 3 comments below.
> 
> > 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
> > @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
> >  {
> >  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
> >  		bsize;
> > +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> > +	if (inj != NULL && inj->iparam >= 0) {
> > +		if (inj->iparam-- == 0)
> 
> 1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
> which would set it to false instead of decrement? That would make it
> clear the injection works only once.

Cause integer allows setting delay of vy_stmt_alloc() failure.
For instance, I don't want first invocation to vy_stmt_alloc()
fail, but the second, third or tenth one - it may turn out to be
vital. This patch fixes bug when first call of vy_stmt_alloc()
during compaction fails; the next patch - if tenth call of
vy_stmt_alloc() fails.
 
> Also it looks too artificial. The injection basically simulates a tuple
> with too big size which was inserted bypassing max_tuple_size check,
> and suddenly it was checked here, already after insertion.

Konstantint said, that squashing two upserts of size 'x' may result
in new vy_stmt with size > 'x'. Despite the fact that I did not
attempt at reproducing this statement, I saw these errors appearing
on production machine during compaction. I do not know the exact reason
why they revealed, but it is a fact.
 
> Better add an OOM injection for malloc a few lines below, would be more
> correct.
> 
> > +			total_size = vy_max_tuple_size + 1;
> > +	}
> >  	if (unlikely(total_size > vy_max_tuple_size)) {
> >  		diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
> >  			 (unsigned) total_size);

It makes sense on the one hand, but on another hand - we know that
emulating malloc OOM is likely to be artificial as well.

> > 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..2c03697f6
> > --- /dev/null
> > +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> > @@ -0,0 +1,93 @@
> > +-- 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()
> > + | ---
> > + | ...
> > +-- 1 range, 2 runs
> 
> 2. Can you add an assertion, that it is really 1 range and 2 runs?
> To be sure that the test won't become useless in future if suddenly
> something will change there.

Ok, will add.
 
> > +
> > +compact()
> > + | ---
> > + | ...
> > +-- 1 range, 1 run after compaction
> > +
> > +dump()
> > + | ---
> > + | ...
> > +-- 1 range, 2 runs
> > +
> > +errinj = box.error.injection
> > + | ---
> > + | ...
> > +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
> > + | ---
> > + | - ok
> > + | ...
> > +-- Should finish successfully despite vy_stmt_alloc() fail.
> > +--
> > +compact()
> > + | ---
> > + | ...
> > +-- 1 range, 1 run
> > +s.index.pk:stat().range_count
> > + | ---
> > + | - 1
> > + | ...
> > +s.index.pk:stat().run_count
> 
> 3. I would add an assertion/print, that ERRINJ_VY_MAX_TUPLE_SIZE
> (or whatever will be used after the review) is changed back to
> off, and then change it to off again anyway. Because if the test
> won't hit the errinj because of something in future, the errinj
> will be left set, and we won't notice that here. Also it will
> make all next vinyl tests running in the same job fail in very
> surprising ways.
> 
> The same for the next commit.

Ok, will fix.

> > + | ---
> > + | - 1
> > + | ...
> > +
> > +s:drop()
> > + | ---
> > + | ...

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

[Nikita Pettik]

On 10 Apr 15:40, Nikita Pettik wrote:
> On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> > > + | ---
> > > + | ...
> > > +dump()
> > > + | ---
> > > + | ...
> > > +-- 1 range, 2 runs
> > 
> > 2. Can you add an assertion, that it is really 1 range and 2 runs?
> > To be sure that the test won't become useless in future if suddenly
> > something will change there.
> 
> Ok, will add.
>  
> > > +
> > > +compact()
> > > + | ---
> > > + | ...
> > > +-- 1 range, 1 run after compaction
> > > +
> > > +dump()
> > > + | ---
> > > + | ...
> > > +-- 1 range, 2 runs
> > > +
> > > +errinj = box.error.injection
> > > + | ---
> > > + | ...
> > > +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
> > > + | ---
> > > + | - ok
> > > + | ...
> > > +-- Should finish successfully despite vy_stmt_alloc() fail.
> > > +--
> > > +compact()
> > > + | ---
> > > + | ...
> > > +-- 1 range, 1 run
> > > +s.index.pk:stat().range_count
> > > + | ---
> > > + | - 1
> > > + | ...
> > > +s.index.pk:stat().run_count
> > 
> > 3. I would add an assertion/print, that ERRINJ_VY_MAX_TUPLE_SIZE
> > (or whatever will be used after the review) is changed back to
> > off, and then change it to off again anyway. Because if the test
> > won't hit the errinj because of something in future, the errinj
> > will be left set, and we won't notice that here. Also it will
> > make all next vinyl tests running in the same job fail in very
> > surprising ways.
> > 
> > The same for the next commit.
> 
> Ok, will fix.
>

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 53a050c9b..693a4ff22 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -29,13 +29,17 @@ test_run:cmd("setopt delimiter ''");
 --
 dump(true)
 dump()
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
 -- 1 range, 2 runs
 
 compact()
--- 1 range, 1 run after compaction
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 1)
 
 dump()
--- 1 range, 2 runs
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 2)
 
 errinj = box.error.injection
 errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
@@ -43,7 +47,9 @@ errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
 --
 compact()
 -- 1 range, 1 run
-s.index.pk:stat().range_count
-s.index.pk:stat().run_count
+assert(s.index.pk:stat().range_count == 1)
+assert(s.index.pk:stat().run_count == 1)
+assert(errinj.get('ERRINJ_VY_MAX_TUPLE_SIZE') == -1)
+errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', -1)
 
 s:drop()
 

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

[Vladislav Shpilevoy]

>>> 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
>>> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
>>>  {
>>>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
>>>  		bsize;
>>> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
>>> +	if (inj != NULL && inj->iparam >= 0) {
>>> +		if (inj->iparam-- == 0)
>>
>> 1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
>> which would set it to false instead of decrement? That would make it
>> clear the injection works only once.
> 
> Cause integer allows setting delay of vy_stmt_alloc() failure.
> For instance, I don't want first invocation to vy_stmt_alloc()
> fail, but the second, third or tenth one - it may turn out to be
> vital. This patch fixes bug when first call of vy_stmt_alloc()
> during compaction fails; the next patch - if tenth call of
> vy_stmt_alloc() fails.

Nope, in the next patch you use 0 too. Moreover, when I changed it
to 10, I got the test hanging in 100% CPU. Regardless of with the
fix or without.

>> Also it looks too artificial. The injection basically simulates a tuple
>> with too big size which was inserted bypassing max_tuple_size check,
>> and suddenly it was checked here, already after insertion.
> 
> Konstantint said, that squashing two upserts of size 'x' may result
> in new vy_stmt with size > 'x'. Despite the fact that I did not
> attempt at reproducing this statement, I saw these errors appearing
> on production machine during compaction. I do not know the exact reason
> why they revealed, but it is a fact.

And still this particular test does not use any upserts. So OOM here
is more likely to happen than max tuple size violation.

>> Better add an OOM injection for malloc a few lines below, would be more
>> correct.

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

[Nikita Pettik]

On 11 Apr 19:39, Vladislav Shpilevoy wrote:
> >>> 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
> >>> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
> >>>  {
> >>>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
> >>>  		bsize;
> >>> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> >>> +	if (inj != NULL && inj->iparam >= 0) {
> >>> +		if (inj->iparam-- == 0)
> >>
> >> 1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
> >> which would set it to false instead of decrement? That would make it
> >> clear the injection works only once.
> > 
> > Cause integer allows setting delay of vy_stmt_alloc() failure.
> > For instance, I don't want first invocation to vy_stmt_alloc()
> > fail, but the second, third or tenth one - it may turn out to be
> > vital. This patch fixes bug when first call of vy_stmt_alloc()
> > during compaction fails; the next patch - if tenth call of
> > vy_stmt_alloc() fails.
> 
> Nope, in the next patch you use 0 too. Moreover, when I changed it
> to 10, I got the test hanging in 100% CPU. Regardless of with the
> fix or without.

It should have been 10. Kind of strange since it is exactly this
value that helped me to reveal this bug. Mb it is still unpredictable
consequences of invalid memory access. I will investigate and test on
my mac before next updates. Thx.
 
> >> Also it looks too artificial. The injection basically simulates a tuple
> >> with too big size which was inserted bypassing max_tuple_size check,
> >> and suddenly it was checked here, already after insertion.
> > 
> > Konstantint said, that squashing two upserts of size 'x' may result
> > in new vy_stmt with size > 'x'. Despite the fact that I did not
> > attempt at reproducing this statement, I saw these errors appearing
> > on production machine during compaction. I do not know the exact reason
> > why they revealed, but it is a fact.
> 
> And still this particular test does not use any upserts. So OOM here
> is more likely to happen than max tuple size violation.
> 
> >> Better add an OOM injection for malloc a few lines below, would be more
> >> correct.

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

[Nikita Pettik]

On 13 Apr 22:29, Nikita Pettik wrote:
> On 11 Apr 19:39, Vladislav Shpilevoy wrote:
> > >>> 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
> > >>> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
> > >>>  {
> > >>>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
> > >>>  		bsize;
> > >>> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> > >>> +	if (inj != NULL && inj->iparam >= 0) {
> > >>> +		if (inj->iparam-- == 0)
> > >>
> > >> 1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
> > >> which would set it to false instead of decrement? That would make it
> > >> clear the injection works only once.
> > > 
> > > Cause integer allows setting delay of vy_stmt_alloc() failure.
> > > For instance, I don't want first invocation to vy_stmt_alloc()
> > > fail, but the second, third or tenth one - it may turn out to be
> > > vital. This patch fixes bug when first call of vy_stmt_alloc()
> > > during compaction fails; the next patch - if tenth call of
> > > vy_stmt_alloc() fails.
> > 
> > Nope, in the next patch you use 0 too. Moreover, when I changed it
> > to 10, I got the test hanging in 100% CPU. Regardless of with the
> > fix or without.
> 
> It should have been 10. Kind of strange since it is exactly this
> value that helped me to reveal this bug. Mb it is still unpredictable
> consequences of invalid memory access. I will investigate and test on
> my mac before next updates. Thx.
>  
> > >> Also it looks too artificial. The injection basically simulates a tuple
> > >> with too big size which was inserted bypassing max_tuple_size check,
> > >> and suddenly it was checked here, already after insertion.

I've tested on my mac: without fix Tarantool really gets stuck.
But when I run process in gdb/lldb it always crashes (in the same
place as on my linux machine - when accessing invalid memory). So
I assume accessing invalid memory may result in any behaviour
whether it is infinite loop or crash. With applied fix everything
works OK on both linux and mac.
 
> > > Konstantint said, that squashing two upserts of size 'x' may result
> > > in new vy_stmt with size > 'x'. Despite the fact that I did not
> > > attempt at reproducing this statement, I saw these errors appearing
> > > on production machine during compaction. I do not know the exact reason
> > > why they revealed, but it is a fact.
> > 
> > And still this particular test does not use any upserts. So OOM here
> > is more likely to happen than max tuple size violation.
> > 
> > >> Better add an OOM injection for malloc a few lines below, would be more
> > >> correct.

[Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[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. Let's nullify those
objects right after function returns with rc != 0.

Closes #4864
---
 src/box/vy_write_iterator.c                   |  5 +-
 .../gh-4864-stmt-alloc-fail-compact.result    | 51 +++++++++++++++++++
 .../gh-4864-stmt-alloc-fail-compact.test.lua  | 24 +++++++++
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 7a6a20627..f6e6ed4d2 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -961,8 +961,11 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 	size_t used = region_used(region);
 	stream->rv_used_count = 0;
 	if (vy_write_iterator_build_history(stream, &raw_count,
-					    &is_first_insert) != 0)
+					    &is_first_insert) != 0) {
+		for (int i = 0; i < stream->rv_count; ++i)
+			stream->read_views[i].history = NULL;
 		goto error;
+	}
 	if (raw_count == 0) {
 		/* A key is fully optimized. */
 		region_truncate(region, used);
diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
index 2c03697f6..770efcca8 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
@@ -91,3 +91,54 @@ s.index.pk:stat().run_count
 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_MAX_TUPLE_SIZE', 0)
+ | ---
+ | - ok
+ | ...
+-- Should finish successfully despite vy_stmt_alloc() fail.
+--
+compact()
+ | ---
+ | ...
+-- 1 range, 1 run
+s.index.pk:stat().range_count
+ | ---
+ | - 1
+ | ...
+s.index.pk:stat().run_count
+ | ---
+ | - 1
+ | ...
+
+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
index 53a050c9b..8b5c79025 100644
--- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
+++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.test.lua
@@ -47,3 +47,27 @@ s.index.pk:stat().range_count
 s.index.pk:stat().run_count
 
 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_MAX_TUPLE_SIZE', 0)
+-- Should finish successfully despite vy_stmt_alloc() fail.
+--
+compact()
+-- 1 range, 1 run
+s.index.pk:stat().range_count
+s.index.pk:stat().run_count
+
+s:drop()
-- 
2.17.1

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0)
> +					    &is_first_insert) != 0) {
> +		for (int i = 0; i < stream->rv_count; ++i)
> +			stream->read_views[i].history = NULL;

This violates stream encapsulation and should be a method of the
stream. 

Otherwise lgtm.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik]

On 09 Apr 11:19, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/04/09 00:39]:
> >  	if (vy_write_iterator_build_history(stream, &raw_count,
> > -					    &is_first_insert) != 0)
> > +					    &is_first_insert) != 0) {
> > +		for (int i = 0; i < stream->rv_count; ++i)
> > +			stream->read_views[i].history = NULL;
> 
> This violates stream encapsulation and should be a method of the
> stream. 

read_views is a member of write_iterator, not stream. Stream has
only for methods: start, next, stop, close.
vy_write_iterator_build_history() is a part of next method. So it
is likely to be already encapsulated, isn't it? Access to truncated
region memory occurs in stop() method (i.e. write_iterator_stop()).
 
> Otherwise lgtm.
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Vladislav Shpilevoy]

[-- Attachment #1: Type: text/plain, Size: 2387 bytes --]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index 7a6a20627..f6e6ed4d2 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -961,8 +961,11 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>  	size_t used = region_used(region);
>  	stream->rv_used_count = 0;
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0)
> +					    &is_first_insert) != 0) {
> +		for (int i = 0; i < stream->rv_count; ++i)
> +			stream->read_views[i].history = NULL;
>  		goto error;

1. Kostja probably meant encapsulation of function
vy_write_iterator_build_history(). Currently that function
leaves garbage in case of a fail. Therefore it becomes not
self sufficient. Better cleanup the views in the same place
where they are created - inside build_history().

Imagine, if build_history() would be called not in a single
place. We would need to cleanup its garbage in each one.

Diff below and attached as a file.
====================
diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index f6e6ed4d2..7d8c60d73 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)
+			stream->read_views[i].history = NULL;
+	} 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;
 	}
@@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 	size_t used = region_used(region);
 	stream->rv_used_count = 0;
 	if (vy_write_iterator_build_history(stream, &raw_count,
-					    &is_first_insert) != 0) {
-		for (int i = 0; i < stream->rv_count; ++i)
-			stream->read_views[i].history = NULL;
+					    &is_first_insert) != 0)
 		goto error;
-	}
 	if (raw_count == 0) {
 		/* A key is fully optimized. */
 		region_truncate(region, used);
====================

2. I rolled back the patch and run the test - it passed. What can be a reason?

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 1188 bytes --]

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index f6e6ed4d2..7d8c60d73 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)
+			stream->read_views[i].history = NULL;
+	} 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;
 	}
@@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
 	size_t used = region_used(region);
 	stream->rv_used_count = 0;
 	if (vy_write_iterator_build_history(stream, &raw_count,
-					    &is_first_insert) != 0) {
-		for (int i = 0; i < stream->rv_count; ++i)
-			stream->read_views[i].history = NULL;
+					    &is_first_insert) != 0)
 		goto error;
-	}
 	if (raw_count == 0) {
 		/* A key is fully optimized. */
 		region_truncate(region, used);

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik]

On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> > index 7a6a20627..f6e6ed4d2 100644
> > --- a/src/box/vy_write_iterator.c
> > +++ b/src/box/vy_write_iterator.c
> > @@ -961,8 +961,11 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
> >  	size_t used = region_used(region);
> >  	stream->rv_used_count = 0;
> >  	if (vy_write_iterator_build_history(stream, &raw_count,
> > -					    &is_first_insert) != 0)
> > +					    &is_first_insert) != 0) {
> > +		for (int i = 0; i < stream->rv_count; ++i)
> > +			stream->read_views[i].history = NULL;
> >  		goto error;
> 
> 1. Kostja probably meant encapsulation of function
> vy_write_iterator_build_history(). Currently that function
> leaves garbage in case of a fail. Therefore it becomes not
> self sufficient. Better cleanup the views in the same place
> where they are created - inside build_history().
> 
> Imagine, if build_history() would be called not in a single
> place. We would need to cleanup its garbage in each one.

Ah, of course I agree. Applied.
 
> Diff below and attached as a file.
> ====================
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 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)
> +			stream->read_views[i].history = NULL;
> +	} 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;
>  	}
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>  	size_t used = region_used(region);
>  	stream->rv_used_count = 0;
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0) {
> -		for (int i = 0; i < stream->rv_count; ++i)
> -			stream->read_views[i].history = NULL;
> +					    &is_first_insert) != 0)
>  		goto error;
> -	}
>  	if (raw_count == 0) {
>  		/* A key is fully optimized. */
>  		region_truncate(region, used);
> ====================
> 
> 2. I rolled back the patch and run the test - it passed. What can be a reason?

Hm, it fails not each run, but like every second/third execution.
I guess it depends on state of memory which has been truncated
(position of particular slabs in area etc). Not sure if we can
affect it.

> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index f6e6ed4d2..7d8c60d73 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)
> +			stream->read_views[i].history = NULL;
> +	} 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;
>  	}
> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>  	size_t used = region_used(region);
>  	stream->rv_used_count = 0;
>  	if (vy_write_iterator_build_history(stream, &raw_count,
> -					    &is_first_insert) != 0) {
> -		for (int i = 0; i < stream->rv_count; ++i)
> -			stream->read_views[i].history = NULL;
> +					    &is_first_insert) != 0)
>  		goto error;
> -	}
>  	if (raw_count == 0) {
>  		/* A key is fully optimized. */
>  		region_truncate(region, used);

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik]

On 10 Apr 15:47, Nikita Pettik wrote:

I've also found that vy_read_view_merge() can fail as well during
processing array of read views. Thus, we should clean-up unprocessed
read views as well. Diff:

diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
index 7d8c60d73..165b422fd 100644
--- a/src/box/vy_write_iterator.c
+++ b/src/box/vy_write_iterator.c
@@ -986,8 +986,11 @@ 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]) {
+                               rv->history = NULL;
+                               rv--;
+                       }
                        goto error;
+               }
                assert(rv->history == NULL);
                if (rv->tuple == NULL)
                        continue;

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Vladislav Shpilevoy]

On 10/04/2020 23:45, Nikita Pettik wrote:
> On 10 Apr 15:47, Nikita Pettik wrote:
> 
> I've also found that vy_read_view_merge() can fail as well during
> processing array of read views. Thus, we should clean-up unprocessed
> read views as well. Diff:
> 
> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index 7d8c60d73..165b422fd 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -986,8 +986,11 @@ 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]) {
> +                               rv->history = NULL;
> +                               rv--;
> +                       }
>                         goto error;

The same question as to the previous fix - shouldn't that be
cleaned by the function, which created the mess, by
vy_read_view_merge() internally?

> +               }
>                 assert(rv->history == NULL);
>                 if (rv->tuple == NULL)
>                         continue;
> 

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/04/11 10:25]:
> > @@ -986,8 +986,11 @@ 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]) {
> > +                               rv->history = NULL;
> > +                               rv--;
> > +                       }
> >                         goto error;
> 
> The same question as to the previous fix - shouldn't that be
> cleaned by the function, which created the mess, by
> vy_read_view_merge() internally?

and shouldn't this have an own test case? 

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik]

On 11 Apr 20:30, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/04/11 10:25]:
> > > @@ -986,8 +986,11 @@ 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]) {
> > > +                               rv->history = NULL;
> > > +                               rv--;
> > > +                       }
> > >                         goto error;
> > 
> > The same question as to the previous fix - shouldn't that be
> > cleaned by the function, which created the mess, by
> > vy_read_view_merge() internally?
> 
> and shouldn't this have an own test case? 

I will try to come up with test, but I found this accidentally (due
to another bug, so original reproducer is not really suitable..).
As a last resort, I can use another one error injection.
 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/14 01:32]:
> As a last resort, I can use another one error injection.

Why last resort? Let's just use it.

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Nikita Pettik]

On 11 Apr 00:32, Vladislav Shpilevoy wrote:
> 
> 
> On 10/04/2020 23:45, Nikita Pettik wrote:
> > On 10 Apr 15:47, Nikita Pettik wrote:
> > 
> > I've also found that vy_read_view_merge() can fail as well during
> > processing array of read views. Thus, we should clean-up unprocessed
> > read views as well. Diff:
> > 
> > diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> > index 7d8c60d73..165b422fd 100644
> > --- a/src/box/vy_write_iterator.c
> > +++ b/src/box/vy_write_iterator.c
> > @@ -986,8 +986,11 @@ 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]) {
> > +                               rv->history = NULL;
> > +                               rv--;
> > +                       }
> >                         goto error;
> 
> The same question as to the previous fix - shouldn't that be
> cleaned by the function, which created the mess, by
> vy_read_view_merge() internally?

Iteration over read views takes place in _build_read_views() so
to me it seems to be sensible to clean-up it there..
In fact, view_merge() does not create mess. Moreover, it doesn't
even have explicit access to array of read views (only via
stream->read_views[]) - read view to be processed is passed as a
separate parameter. Purpose of view_merge() is to merge statements,
so why it should clean-up read-views which belong to stream object?
All in all, I do not see much sense to move this clean-up to
read_view_merge(), only if you insist on this change.

> > +               }
> >                 assert(rv->history == NULL);
> >                 if (rv->tuple == NULL)
> >                         continue;
> > 

Re: [Tarantool-patches] [PATCH 2/2] vinyl: clean-up read views if *_build_history() fails

[Vladislav Shpilevoy]

>> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
>> index f6e6ed4d2..7d8c60d73 100644
>> --- a/src/box/vy_write_iterator.c
>> +++ b/src/box/vy_write_iterator.c
>> @@ -961,11 +964,8 @@ vy_write_iterator_build_read_views(struct vy_write_iterator *stream, int *count)
>>  	size_t used = region_used(region);
>>  	stream->rv_used_count = 0;
>>  	if (vy_write_iterator_build_history(stream, &raw_count,
>> -					    &is_first_insert) != 0) {
>> -		for (int i = 0; i < stream->rv_count; ++i)
>> -			stream->read_views[i].history = NULL;
>> +					    &is_first_insert) != 0)
>>  		goto error;
>> -	}
>>  	if (raw_count == 0) {
>>  		/* A key is fully optimized. */
>>  		region_truncate(region, used);
>> ====================
>>
>> 2. I rolled back the patch and run the test - it passed. What can be a reason?
> 
> Hm, it fails not each run, but like every second/third execution.
> I guess it depends on state of memory which has been truncated
> (position of particular slabs in area etc). Not sure if we can
> affect it.

Did you check it locally? I run it tens times and still nothing.