[PATCH] memtx: add yields during index build

[PATCH] memtx: add yields during index build

[Serge Petrenko]

Memtx index build used to stall event loop for all the build period.
Add occasional yields so that the loop is not blocked for too long.

Closes #3976
---
https://github.com/tarantool/tarantool/issues/3976
https://github.com/tarantool/tarantool/tree/sp/gh-3976-background-index-build

 src/box/memtx_space.c                         | 13 +++++
 test/box/memtx_background_index_build.result  | 55 +++++++++++++++++++
 .../box/memtx_background_index_build.test.lua | 32 +++++++++++
 test/box/suite.ini                            |  2 +-
 4 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 test/box/memtx_background_index_build.result
 create mode 100644 test/box/memtx_background_index_build.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 5ddb4f7ee..b90e2707e 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -874,6 +874,15 @@ static int
 memtx_space_build_index(struct space *src_space, struct index *new_index,
 			struct tuple_format *new_format)
 {
+	/*
+	 * Yield every 1K tuples.
+	 * In debug mode yield more often for testing purposes.
+	 */
+#ifdef NDEBUG
+	enum { YIELD_LOOPS = 1000 };
+#else
+	enum { YIELD_LOOPS = 10 };
+#endif
 	/**
 	 * If it's a secondary key, and we're not building them
 	 * yet (i.e. it's snapshot recovery for memtx), do nothing.
@@ -909,6 +918,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	/* Build the new index. */
 	int rc;
 	struct tuple *tuple;
+	size_t count = 0;
 	while ((rc = iterator_next(it, &tuple)) == 0 && tuple != NULL) {
 		/*
 		 * Check that the tuple is OK according to the
@@ -933,6 +943,9 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 		 */
 		if (new_index->def->iid == 0)
 			tuple_ref(tuple);
+		if (++count % YIELD_LOOPS == 0) {
+			fiber_sleep(0);
+		}
 	}
 	iterator_delete(it);
 	return rc;
diff --git a/test/box/memtx_background_index_build.result b/test/box/memtx_background_index_build.result
new file mode 100644
index 000000000..332595bc2
--- /dev/null
+++ b/test/box/memtx_background_index_build.result
@@ -0,0 +1,55 @@
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+fiber = require('fiber')
+---
+...
+num_iters = 0
+---
+...
+test_run:cmd('setopt delimiter ";"')
+---
+- true
+...
+function f()
+    while true do
+        num_iters = num_iters + 1
+        fiber.yield()
+        fiber.testcancel()
+    end
+end;
+---
+...
+test_run:cmd('setopt delimiter ""');
+---
+- true
+...
+_ = box.schema.space.create('test')
+---
+...
+_ = box.space.test:create_index('pk')
+---
+...
+for i = 1,1000 do box.space.test:insert{i} end
+---
+...
+fib = fiber.new(f) _ = box.space.test:create_index('sk') fiber.cancel(fib) fiber.yield()
+---
+...
+fib
+---
+- the fiber is dead
+...
+-- index build in debug mode should yield every 10 iterations.
+-- This means we will have at least 100 event loop iterations
+-- during index build.
+num_iters >= 100 or num_iters
+---
+- true
+...
+box.space.test:drop()
+---
+...
diff --git a/test/box/memtx_background_index_build.test.lua b/test/box/memtx_background_index_build.test.lua
new file mode 100644
index 000000000..7a5711cf6
--- /dev/null
+++ b/test/box/memtx_background_index_build.test.lua
@@ -0,0 +1,32 @@
+env = require('test_run')
+test_run = env.new()
+
+fiber = require('fiber')
+
+num_iters = 0
+
+test_run:cmd('setopt delimiter ";"')
+
+function f()
+    while true do
+        num_iters = num_iters + 1
+        fiber.yield()
+        fiber.testcancel()
+    end
+end;
+test_run:cmd('setopt delimiter ""');
+
+_ = box.schema.space.create('test')
+_ = box.space.test:create_index('pk')
+
+for i = 1,1000 do box.space.test:insert{i} end
+
+fib = fiber.new(f) _ = box.space.test:create_index('sk') fiber.cancel(fib) fiber.yield()
+fib
+
+-- index build in debug mode should yield every 10 iterations.
+-- This means we will have at least 100 event loop iterations
+-- during index build.
+num_iters >= 100 or num_iters
+
+box.space.test:drop()
diff --git a/test/box/suite.ini b/test/box/suite.ini
index c7b75c173..5283d2031 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 description = Database tests
 script = box.lua
 disabled = rtree_errinj.test.lua tuple_bench.test.lua
-release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua
+release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua memtx_background_index_build.test.lua
 lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
 use_unix_sockets = True
 is_parallel = True
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] memtx: add yields during index build

[Vladimir Davydov]

On Wed, May 22, 2019 at 07:11:42PM +0300, Serge Petrenko wrote:
> Memtx index build used to stall event loop for all the build period.
> Add occasional yields so that the loop is not blocked for too long.
> 
> Closes #3976
> ---
> https://github.com/tarantool/tarantool/issues/3976
> https://github.com/tarantool/tarantool/tree/sp/gh-3976-background-index-build
> 
>  src/box/memtx_space.c                         | 13 +++++
>  test/box/memtx_background_index_build.result  | 55 +++++++++++++++++++
>  .../box/memtx_background_index_build.test.lua | 32 +++++++++++
>  test/box/suite.ini                            |  2 +-
>  4 files changed, 101 insertions(+), 1 deletion(-)
>  create mode 100644 test/box/memtx_background_index_build.result
>  create mode 100644 test/box/memtx_background_index_build.test.lua
> 
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 5ddb4f7ee..b90e2707e 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -874,6 +874,15 @@ static int
>  memtx_space_build_index(struct space *src_space, struct index *new_index,
>  			struct tuple_format *new_format)
>  {
> +	/*
> +	 * Yield every 1K tuples.
> +	 * In debug mode yield more often for testing purposes.
> +	 */
> +#ifdef NDEBUG
> +	enum { YIELD_LOOPS = 1000 };
> +#else
> +	enum { YIELD_LOOPS = 10 };
> +#endif
>  	/**
>  	 * If it's a secondary key, and we're not building them
>  	 * yet (i.e. it's snapshot recovery for memtx), do nothing.
> @@ -909,6 +918,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  	/* Build the new index. */
>  	int rc;
>  	struct tuple *tuple;
> +	size_t count = 0;
>  	while ((rc = iterator_next(it, &tuple)) == 0 && tuple != NULL) {
>  		/*
>  		 * Check that the tuple is OK according to the
> @@ -933,6 +943,9 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  		 */
>  		if (new_index->def->iid == 0)
>  			tuple_ref(tuple);
> +		if (++count % YIELD_LOOPS == 0) {
> +			fiber_sleep(0);
> +		}

This isn't enough as tuples may be inserted into / deleted from the
space while DDL is in progress. We need to propagate these changes
to the new index using on_replace trigger. Take a look at how vinyl
handles it.

[tarantool-patches] Re: [PATCH] memtx: add yields during index build

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/22 21:45]:
> This isn't enough as tuples may be inserted into / deleted from the
> space while DDL is in progress. We need to propagate these changes
> to the new index using on_replace trigger. Take a look at how vinyl
> handles it.

+

-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] [PATCH] memtx: add yields during index build

[Serge Petrenko]

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



> 22 мая 2019 г., в 19:28, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Wed, May 22, 2019 at 07:11:42PM +0300, Serge Petrenko wrote:
>> Memtx index build used to stall event loop for all the build period.
>> Add occasional yields so that the loop is not blocked for too long.
>> 
>> Closes #3976
>> ---
>> https://github.com/tarantool/tarantool/issues/3976
>> https://github.com/tarantool/tarantool/tree/sp/gh-3976-background-index-build
>> 
>> src/box/memtx_space.c                         | 13 +++++
>> test/box/memtx_background_index_build.result  | 55 +++++++++++++++++++
>> .../box/memtx_background_index_build.test.lua | 32 +++++++++++
>> test/box/suite.ini                            |  2 +-
>> 4 files changed, 101 insertions(+), 1 deletion(-)
>> create mode 100644 test/box/memtx_background_index_build.result
>> create mode 100644 test/box/memtx_background_index_build.test.lua
>> 
>> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
>> index 5ddb4f7ee..b90e2707e 100644
>> --- a/src/box/memtx_space.c
>> +++ b/src/box/memtx_space.c
>> @@ -874,6 +874,15 @@ static int
>> memtx_space_build_index(struct space *src_space, struct index *new_index,
>> 			struct tuple_format *new_format)
>> {
>> +	/*
>> +	 * Yield every 1K tuples.
>> +	 * In debug mode yield more often for testing purposes.
>> +	 */
>> +#ifdef NDEBUG
>> +	enum { YIELD_LOOPS = 1000 };
>> +#else
>> +	enum { YIELD_LOOPS = 10 };
>> +#endif
>> 	/**
>> 	 * If it's a secondary key, and we're not building them
>> 	 * yet (i.e. it's snapshot recovery for memtx), do nothing.
>> @@ -909,6 +918,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>> 	/* Build the new index. */
>> 	int rc;
>> 	struct tuple *tuple;
>> +	size_t count = 0;
>> 	while ((rc = iterator_next(it, &tuple)) == 0 && tuple != NULL) {
>> 		/*
>> 		 * Check that the tuple is OK according to the
>> @@ -933,6 +943,9 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>> 		 */
>> 		if (new_index->def->iid == 0)
>> 			tuple_ref(tuple);
>> +		if (++count % YIELD_LOOPS == 0) {
>> +			fiber_sleep(0);
>> +		}
> 
> This isn't enough as tuples may be inserted into / deleted from the
> space while DDL is in progress. We need to propagate these changes
> to the new index using on_replace trigger. Take a look at how vinyl
> handles it.

Thank you for pointing this out! Please see v2 in the mailing list.

[-- Attachment #2: Type: text/html, Size: 10187 bytes --]