[Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Nikita Pettik]

vy_build_insert_tuple() processes insertion into secondary indexes being
created. It contains yield points during which in-memory level of LSM
tree may change (for example rotate owing to triggered dump). So after
yield point it is required to fetch from LSM struct pointer to mem again
to operate on valid metadata. This patch updates pointer to mem after
mentioned yield point.

Closes #4810
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-4810-dump-during-index-build
Issue: https://github.com/tarantool/tarantool/issues/4810

@ChangeLog:
- Fixed assertion fault due to triggered dump process during secondary
  index build (gh-4810).

 src/box/vinyl.c                               |  10 ++
 .../gh-4810-dump-during-index-build.result    | 150 ++++++++++++++++++
 .../gh-4810-dump-during-index-build.test.lua  |  74 +++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 test/vinyl/gh-4810-dump-during-index-build.result
 create mode 100644 test/vinyl/gh-4810-dump-during-index-build.test.lua

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 252e4e774..55a4c8fb7 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4086,6 +4086,16 @@ vy_build_insert_tuple(struct vy_env *env, struct vy_lsm *lsm,
 		tuple_unref(stmt);
 		return -1;
 	}
+	/*
+	 * Despite the fact that we protected mem from being
+	 * dumped, its generation still may bump due to rotation
+	 * in vy_tx_write_prepare() (insertions during index build
+	 * are still handled by on_replace_trigger). This may happen
+	 * if dump process is triggered (vy_scheduler_trigger_dump()).
+	 * Hence, to get right mem (during mem rotation it becomes
+	 * sealed i.e. read-only) we should fetch it from lsm again.
+	 */
+	mem = lsm->mem;
 
 	/* Insert the new tuple into the in-memory index. */
 	size_t mem_used_before = lsregion_used(&env->mem_env.allocator);
diff --git a/test/vinyl/gh-4810-dump-during-index-build.result b/test/vinyl/gh-4810-dump-during-index-build.result
new file mode 100644
index 000000000..f55c44a1f
--- /dev/null
+++ b/test/vinyl/gh-4810-dump-during-index-build.result
@@ -0,0 +1,150 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd("create server test with script='vinyl/low_quota.lua'")
+ | ---
+ | - true
+ | ...
+test_run:cmd("start server test with args='13421772'")
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch test')
+ | ---
+ | - true
+ | ...
+
+fiber = require 'fiber'
+ | ---
+ | ...
+
+math.randomseed(os.time())
+ | ---
+ | ...
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {parts = {1, 'unsigned'}})
+ | ---
+ | ...
+
+--
+-- Purpose of this test is to trigger dump during secondary index build.
+-- It is worth noting that dump must be triggered by exceeding memory
+-- quota and not by invoking box.snapshot() since checkpoint process
+-- is locked by DDL latch.
+--
+
+MAX_KEY = 1000000
+ | ---
+ | ...
+MAX_VAL = 1000000
+ | ---
+ | ...
+PADDING = string.rep('x', 100)
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+
+function gen_insert()
+    pcall(s.insert, s, {math.random(MAX_KEY), math.random(MAX_VAL),
+                        math.random(MAX_VAL), math.random(MAX_VAL), PADDING})
+end;
+ | ---
+ | ...
+
+function fill_L0_without_dump()
+    local dump_watermark = box.cfg.vinyl_memory / 2
+    while box.stat.vinyl().memory.level0 < dump_watermark do
+        gen_insert()
+    end
+end;
+ | ---
+ | ...
+
+fill_L0_without_dump();
+ | ---
+ | ...
+assert(box.stat.vinyl().scheduler.dump_count == 0);
+ | ---
+ | - true
+ | ...
+
+function insert_loop()
+    while not stop do
+        gen_insert()
+    end
+    ch:put(true)
+end;
+ | ---
+ | ...
+
+function idx_build()
+    _ = s:create_index('i1', {unique = true, parts = {2, 'unsigned', 3, 'unsigned'}})
+    ch:put(true)
+end;
+ | ---
+ | ...
+
+stop = false;
+ | ---
+ | ...
+ch = fiber.channel(2);
+ | ---
+ | ...
+
+_ = fiber.create(idx_build);
+ | ---
+ | ...
+_ = fiber.create(insert_loop);
+ | ---
+ | ...
+
+fiber.sleep(3);
+ | ---
+ | ...
+
+stop = true;
+ | ---
+ | ...
+for i = 1, ch:size() do
+    ch:get()
+end;
+ | ---
+ | ...
+
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+assert(box.stat.vinyl().scheduler.dump_count > 0)
+ | ---
+ | - true
+ | ...
+assert(s.index.i1 ~= nil)
+ | ---
+ | - true
+ | ...
+s:drop()
+ | ---
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd("stop server test")
+ | ---
+ | - true
+ | ...
+test_run:cmd("cleanup server test")
+ | ---
+ | - true
+ | ...
diff --git a/test/vinyl/gh-4810-dump-during-index-build.test.lua b/test/vinyl/gh-4810-dump-during-index-build.test.lua
new file mode 100644
index 000000000..7b7026498
--- /dev/null
+++ b/test/vinyl/gh-4810-dump-during-index-build.test.lua
@@ -0,0 +1,74 @@
+test_run = require('test_run').new()
+
+test_run:cmd("create server test with script='vinyl/low_quota.lua'")
+test_run:cmd("start server test with args='13421772'")
+test_run:cmd('switch test')
+
+fiber = require 'fiber'
+
+math.randomseed(os.time())
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {parts = {1, 'unsigned'}})
+
+--
+-- Purpose of this test is to trigger dump during secondary index build.
+-- It is worth noting that dump must be triggered by exceeding memory
+-- quota and not by invoking box.snapshot() since checkpoint process
+-- is locked by DDL latch.
+--
+
+MAX_KEY = 1000000
+MAX_VAL = 1000000
+PADDING = string.rep('x', 100)
+
+test_run:cmd("setopt delimiter ';'")
+
+function gen_insert()
+    pcall(s.insert, s, {math.random(MAX_KEY), math.random(MAX_VAL),
+                        math.random(MAX_VAL), math.random(MAX_VAL), PADDING})
+end;
+
+function fill_L0_without_dump()
+    local dump_watermark = box.cfg.vinyl_memory / 2
+    while box.stat.vinyl().memory.level0 < dump_watermark do
+        gen_insert()
+    end
+end;
+
+fill_L0_without_dump();
+assert(box.stat.vinyl().scheduler.dump_count == 0);
+
+function insert_loop()
+    while not stop do
+        gen_insert()
+    end
+    ch:put(true)
+end;
+
+function idx_build()
+    _ = s:create_index('i1', {unique = true, parts = {2, 'unsigned', 3, 'unsigned'}})
+    ch:put(true)
+end;
+
+stop = false;
+ch = fiber.channel(2);
+
+_ = fiber.create(idx_build);
+_ = fiber.create(insert_loop);
+
+fiber.sleep(3);
+
+stop = true;
+for i = 1, ch:size() do
+    ch:get()
+end;
+
+test_run:cmd("setopt delimiter ''");
+assert(box.stat.vinyl().scheduler.dump_count > 0)
+assert(s.index.i1 ~= nil)
+s:drop()
+
+test_run:cmd('switch default')
+test_run:cmd("stop server test")
+test_run:cmd("cleanup server test")
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Nikita Pettik]

On 20 Mar 15:39, Nikita Pettik wrote:
> vy_build_insert_tuple() processes insertion into secondary indexes being
> created. It contains yield points during which in-memory level of LSM
> tree may change (for example rotate owing to triggered dump). So after
> yield point it is required to fetch from LSM struct pointer to mem again
> to operate on valid metadata. This patch updates pointer to mem after
> mentioned yield point.
> 
> Closes #4810
> ---

Sorry guys, I had problems with git send-email so accidentally sent patch
twice to tarantool-patches. Please, ignore one of mails.

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/03/20 15:41]:
> vy_build_insert_tuple() processes insertion into secondary indexes being
> created. It contains yield points during which in-memory level of LSM
> tree may change (for example rotate owing to triggered dump). So after
> yield point it is required to fetch from LSM struct pointer to mem again
> to operate on valid metadata. This patch updates pointer to mem after
> mentioned yield point.

The patch is LGTM, how long does the test run? 

Can you add it to an existing low-quota test, to avoid
setup/teardown overhead for such a minor fix?


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Nikita Pettik]

On 20 Mar 16:33, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/03/20 15:41]:
> > vy_build_insert_tuple() processes insertion into secondary indexes being
> > created. It contains yield points during which in-memory level of LSM
> > tree may change (for example rotate owing to triggered dump). So after
> > yield point it is required to fetch from LSM struct pointer to mem again
> > to operate on valid metadata. This patch updates pointer to mem after
> > mentioned yield point.
> 
> The patch is LGTM, how long does the test run? 

Up to ~5 seconds as a rule.
 
> Can you add it to an existing low-quota test, to avoid
> setup/teardown overhead for such a minor fix?

Ok. But I woudn't say it is minor - bug leads to crashes under
highload on customer's servers :)

> 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Kirill Yukhin]

Hello,

On 20 мар 15:06, Nikita Pettik wrote:
> On 20 Mar 16:33, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev@tarantool.org> [20/03/20 15:41]:
> > > vy_build_insert_tuple() processes insertion into secondary indexes being
> > > created. It contains yield points during which in-memory level of LSM
> > > tree may change (for example rotate owing to triggered dump). So after
> > > yield point it is required to fetch from LSM struct pointer to mem again
> > > to operate on valid metadata. This patch updates pointer to mem after
> > > mentioned yield point.
> > 
> > The patch is LGTM, how long does the test run? 
> 
> Up to ~5 seconds as a rule.
>  
> > Can you add it to an existing low-quota test, to avoid
> > setup/teardown overhead for such a minor fix?
> 
> Ok. But I woudn't say it is minor - bug leads to crashes under
> highload on customer's servers :)

It was mentioned multiple times, that we're using separate
files for bug fixes (and will go further in future).

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Kirill Yukhin]

Hello,

On 20 мар 15:32, Nikita Pettik wrote:
> vy_build_insert_tuple() processes insertion into secondary indexes being
> created. It contains yield points during which in-memory level of LSM
> tree may change (for example rotate owing to triggered dump). So after
> yield point it is required to fetch from LSM struct pointer to mem again
> to operate on valid metadata. This patch updates pointer to mem after
> mentioned yield point.
> 
> Closes #4810
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-4810-dump-during-index-build
> Issue: https://github.com/tarantool/tarantool/issues/4810

The patch LGTM.

I've checked it into 1.10, 2.2, 2.3 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/03/20 18:07]:
> > * Nikita Pettik <korablev@tarantool.org> [20/03/20 15:41]:
> > > vy_build_insert_tuple() processes insertion into secondary indexes being
> > > created. It contains yield points during which in-memory level of LSM
> > > tree may change (for example rotate owing to triggered dump). So after
> > > yield point it is required to fetch from LSM struct pointer to mem again
> > > to operate on valid metadata. This patch updates pointer to mem after
> > > mentioned yield point.
> > 
> > The patch is LGTM, how long does the test run? 
> 
> Up to ~5 seconds as a rule.
>  
> > Can you add it to an existing low-quota test, to avoid
> > setup/teardown overhead for such a minor fix?
> 
> Ok. But I woudn't say it is minor - bug leads to crashes under
> highload on customer's servers :)

The problem is pretty minor. The fix is actually not necessarily
the best one, too (but seems adequate for 1.10).

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Konstantin Osipov]

* Kirill Yukhin <kyukhin@tarantool.org> [20/03/20 18:23]:
> It was mentioned multiple times, that we're using separate
> files for bug fixes (and will go further in future).

This works for gcc where each problem is usually a standalone
compilation unit, and once a compiler has run, it leaves no
memory/no state.

It's as terrible idea for a database, when one test case may
impact another, so putting test cases together has a sum effect.

Not to mention the speed of set up and tear down.

To sum up: yet another arrogant and arbitrary decision done
without due process.

In any case, my comment was not just about that. It was about a
yet another push without a proper second code review and
neglecting the previous reviewer's comments.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Nikita Pettik]

On 20 Mar 20:40, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/03/20 18:07]:
> > > * Nikita Pettik <korablev@tarantool.org> [20/03/20 15:41]:
> > > > vy_build_insert_tuple() processes insertion into secondary indexes being
> > > > created. It contains yield points during which in-memory level of LSM
> > > > tree may change (for example rotate owing to triggered dump). So after
> > > > yield point it is required to fetch from LSM struct pointer to mem again
> > > > to operate on valid metadata. This patch updates pointer to mem after
> > > > mentioned yield point.
> > > 
> > > The patch is LGTM, how long does the test run? 
> > 
> > Up to ~5 seconds as a rule.
> >  
> > > Can you add it to an existing low-quota test, to avoid
> > > setup/teardown overhead for such a minor fix?
> > 
> > Ok. But I woudn't say it is minor - bug leads to crashes under
> > highload on customer's servers :)
> 
> The problem is pretty minor. The fix is actually not necessarily
> the best one, too (but seems adequate for 1.10).

Could you please mention other ways to fix it? I'm still new in vinyl,
so it would be helpful to me to investigate all possible solutions and
workarounds. Thanks.
 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] vinyl: update mem ptr in vy_build_insert_tuple() after yield

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/03/20 21:29]:
> > The problem is pretty minor. The fix is actually not necessarily
> > the best one, too (but seems adequate for 1.10).
> 
> Could you please mention other ways to fix it? I'm still new in vinyl,
> so it would be helpful to me to investigate all possible solutions and
> workarounds. Thanks.

1) I'm really am not motivated to do it after the patch is pushed
   like that.
2) There is vy_mem_pin/vy_mem_unpin which is used to pin the mem
   during yields. Generally the mem we're working with should not
   be kicked out of memory inside vy_build_insert_tuple, except for
   when we deliberately wish it to happen in vy_quota_wait().

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