[PATCH v3 0/2] memtx: add yields during index build

[PATCH v3 0/2] memtx: add yields during index build

[Serge Petrenko]

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

This patchset makes memtx engine yield during index build, which prevents it
from stalling the event loop, and moves appropriate test cases from vinyl suite,
where such a feature was implemented earlier, to engine suite.

The first patch introduces changes in index build mechanism
The second patch adds necessary tests

Changes in v3:
  - split the patch into two,
    the second one moving appropriate
    tests from vinyl to engine suite
  - add a docbot request to the
    first patch
  - ensure index unique constraints
    are checked in on_replace triggers
  - fix tuple comparsion in on_replace
    triggers

Changes in v2:
  - add an on_replace trigger
    to handle concurrent replaces
    while index build yields
  - modify test case slightly,
    test concurrent replaces.

Serge Petrenko (2):
  memtx: add yields during index build
  test: move background index build test to engine suite from vinyl

 src/box/memtx_space.c    | 102 +++++++++++++++++++
 src/box/vinyl.c          |   8 ++
 src/lib/core/errinj.h    |   1 +
 test/box/errinj.result   |   2 +
 test/engine/ddl.result   | 215 +++++++++++++++++++++++++++++++++++++++
 test/engine/ddl.test.lua | 121 ++++++++++++++++++++++
 test/vinyl/ddl.result    | 118 ---------------------
 test/vinyl/ddl.test.lua  |  70 -------------
 8 files changed, 449 insertions(+), 188 deletions(-)

-- 
2.20.1 (Apple Git-117)

[PATCH v3 1/2] 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.
Also make index build set on_replace triggers so that concurrent
replaces are also correctly handled during the build.

Part of #3976

@TarantoolBot document
Title: memtx indices are now built in background

Memtx engine no longer stalls the event loop during an index build.
You may insert tuples into a space while an index build is in progress:
the tuples will be correctly added to the new index. If such tuple
violates the new indexes unique constraint or doesn't match the new
index format, the index build will be aborted.
---
 src/box/memtx_space.c  | 102 +++++++++++++++++++++++++++++++++++++++++
 src/box/vinyl.c        |   8 ++++
 src/lib/core/errinj.h  |   1 +
 test/box/errinj.result |   2 +
 4 files changed, 113 insertions(+)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 5ddb4f7ee..7a3e6b74a 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -870,10 +870,70 @@ memtx_init_ephemeral_space(struct space *space)
 	memtx_space_add_primary_key(space);
 }
 
+/* Ongoing index build state used by memtx_build_on_replace triggers. */
+struct memtx_build_state {
+	/* The index being built. */
+	struct index *index;
+	/* New index format to be enforced. */
+	struct tuple_format *format;
+	/* Index build cursor. Marks the last tuple inserted to date */
+	struct tuple *cursor;
+	/* Primary key key_def to compare inserted tuples with cursor. */
+	struct key_def *cmp_def;
+	struct diag diag;
+	int rc;
+};
+
+static void
+memtx_build_on_replace(struct trigger *trigger, void *event)
+{
+	struct txn *txn = event;
+	struct memtx_build_state *state = trigger->data;
+	struct txn_stmt *stmt = txn_current_stmt(txn);
+
+	struct tuple *cmp_tuple = stmt->new_tuple != NULL ? stmt->new_tuple :
+							    stmt->old_tuple;
+	/*
+	 * Only update the already built part of an index. All the other
+	 * tuples will be inserted when build continues.
+	 */
+	if (tuple_compare(state->cursor, HINT_NONE, cmp_tuple, HINT_NONE,
+			  state->cmp_def) < 0)
+		return;
+
+	if (stmt->new_tuple != NULL &&
+	    tuple_validate(state->format, stmt->new_tuple) != 0) {
+		state->rc = -1;
+		diag_move(diag_get(), &state->diag);
+		return;
+	}
+
+	struct tuple *delete = NULL;
+	enum dup_replace_mode mode =
+		state->index->def->opts.is_unique ? DUP_INSERT :
+						    DUP_REPLACE_OR_INSERT;
+	state->rc = index_replace(state->index, stmt->old_tuple,
+				  stmt->new_tuple, mode, &delete);
+
+	if (state->rc != 0) {
+		diag_move(diag_get(), &state->diag);
+	}
+	return;
+}
+
 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.
@@ -899,6 +959,17 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (it == NULL)
 		return -1;
 
+	struct memtx_build_state state;
+	state.index = new_index;
+	state.format = new_format;
+	state.cmp_def = pk->def->key_def;
+	state.rc = 0;
+	diag_create(&state.diag);
+
+	struct trigger on_replace;
+	trigger_create(&on_replace, memtx_build_on_replace, &state, NULL);
+	trigger_add(&src_space->on_replace, &on_replace);
+
 	/*
 	 * The index has to be built tuple by tuple, since
 	 * there is no guarantee that all tuples satisfy
@@ -909,6 +980,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,8 +1005,38 @@ 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) {
+			/*
+			 * Remember the latest inserted tuple to
+			 * avoid processing yet to be added tuples
+			 * in on_replace triggers.
+			 */
+			state.cursor = tuple;
+			fiber_sleep(0);
+			/*
+			 * The on_replace trigger may have failed
+			 * during the yield.
+			 */
+			if (state.rc != 0) {
+				rc = -1;
+				diag_move(&state.diag, diag_get());
+				break;
+			}
+		}
+		/*
+		 * Sleep after at least 1 tuple is inserted to test
+		 * on_replace triggers for index build.
+		 * Sleep only once.
+		 */
+		struct errinj *inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_DOUBLE);
+		if (inj != NULL && inj->dparam > 0 && count == 1) {
+			state.cursor = tuple;
+			fiber_sleep(inj->dparam);
+		}
 	}
 	iterator_delete(it);
+	diag_destroy(&state.diag);
+	trigger_clear(&on_replace);
 	return rc;
 }
 
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9e731d137..1b649f727 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4414,6 +4414,14 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 			rc = -1;
 			break;
 		}
+		/*
+		 * Sleep after at least 1 tuple is inserted to test
+		 * on_replace triggers for index build.
+		 * Sleep only once.
+		 */
+		struct errinj *inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_DOUBLE);
+		if (inj != NULL && inj->dparam > 0 && loops == 1)
+			fiber_sleep(inj->dparam);
 	}
 	vy_read_iterator_close(&itr);
 
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 462c98464..dabc6ce8e 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -108,6 +108,7 @@ struct errinj {
 	_(ERRINJ_VYRUN_INDEX_GARBAGE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_VYRUN_DATA_READ, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_BUILD_INDEX, ERRINJ_INT, {.iparam = -1}) \
+	_(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \
 	_(ERRINJ_VY_POINT_ITER_WAIT, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_RELAY_EXIT_DELAY, ERRINJ_DOUBLE, {.dparam = 0}) \
 	_(ERRINJ_VY_DELAY_PK_LOOKUP, ERRINJ_BOOL, {.bparam = false}) \
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 061501c92..2216a4dc8 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -38,6 +38,8 @@ errinj.info()
     state: false
   ERRINJ_VYRUN_INDEX_GARBAGE:
     state: false
+  ERRINJ_BUILD_INDEX_DELAY:
+    state: 0
   ERRINJ_COIO_SENDFILE_CHUNK:
     state: -1
   ERRINJ_VY_INDEX_FILE_RENAME:
-- 
2.20.1 (Apple Git-117)

[PATCH v3 2/2] test: move background index build test to engine suite from vinyl

[Serge Petrenko]

Since we have implemented memtx background index build, the
corresponding vinyl test cases are now also suitable for memtx,
so move them to engine suite so that both engines are tested.
Also add some tests to check that an ongoing index build is aborted in
case a tuple violating unique constraing or format of the new index is
inserted.

Closes #3976
---
 test/engine/ddl.result   | 215 +++++++++++++++++++++++++++++++++++++++
 test/engine/ddl.test.lua | 121 ++++++++++++++++++++++
 test/vinyl/ddl.result    | 118 ---------------------
 test/vinyl/ddl.test.lua  |  70 -------------
 4 files changed, 336 insertions(+), 188 deletions(-)

diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index c493bd4ac..2a82399d6 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -2222,3 +2222,218 @@ s.index.pk:drop();
 s:drop();
 ---
 ...
+--
+-- Check that all modifications done to the space during index build
+-- are reflected in the new index.
+--
+math.randomseed(os.time())
+
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+
+inspector:cmd("setopt delimiter ';'")
+
+box.begin()
+for i = 1, 1000 do
+    if (i % 100 == 0) then
+        box.commit()
+        box.begin()
+    end
+    if i % 300 == 0 then
+        box.snapshot()
+    end
+    box.space.test:replace{i, i, i}
+end
+box.commit();
+---
+...
+last_val = 1000;
+---
+...
+function gen_load()
+    local s = box.space.test
+    for i = 1, 200 do
+        local op = math.random(4)
+        local key = math.random(1000)
+        local val1 = math.random(1000)
+        local val2 = last_val + 1
+        last_val = val2
+        if op == 1 then
+            pcall(s.insert, s, {key, val1, val2})
+        elseif op == 2 then
+            pcall(s.replace, s, {key, val1, val2})
+        elseif op == 3 then
+            pcall(s.delete, s, {key})
+        elseif op == 4 then
+            pcall(s.upsert, s, {key, val1, val2}, {{'=', 2, val1}, {'=', 3, val2}})
+        end
+    end
+end;
+---
+...
+inspector:cmd("setopt delimiter ''");
+---
+- true
+...
+fiber = require('fiber')
+---
+...
+ch = fiber.channel(1)
+---
+...
+_ = fiber.create(function() gen_load() ch:put(true) end)
+---
+...
+_ = box.space.test:create_index('sk', {unique = false, parts = {2, 'unsigned'}})
+---
+...
+ch:get()
+---
+- true
+...
+_ = fiber.create(function() gen_load() ch:put(true) end)
+---
+...
+_ = box.space.test:create_index('tk', {unique = true, parts = {3, 'unsigned'}})
+---
+...
+ch:get()
+---
+- true
+...
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+---
+- true
+...
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+---
+- true
+...
+t = box.schema.space.create("space", {engine = engine})
+---
+...
+_ = t:create_index("pk")
+---
+...
+for i = 1,1000 do t:insert{i, i} end
+---
+...
+-- check that index build is aborted in case conflicting tuple is inserted
+-- error injection makes index build yield after 1 tuple is inserted into
+-- new index, check that index build is aborted if unique constraint is
+-- violated, as well as if tuple format doesn't match the new indexes one.
+box.error.injection.set('ERRINJ_BUILD_INDEX_DELAY', 0.001)
+---
+- ok
+...
+inspector:cmd("setopt delimiter ';'")
+---
+- true
+...
+fiber.new(function() t:replace{1, 2} end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+---
+- error: Duplicate key exists in unique index 'sk' in space 'space'
+...
+t:get{1};
+---
+- [1, 2]
+...
+t.index.sk;
+---
+- null
+...
+t:replace{1, 1};
+---
+- [1, 1]
+...
+fiber.new(function() t:replace{2, 3} end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+---
+- error: Duplicate key exists in unique index 'sk' in space 'space'
+...
+t:get{2};
+---
+- [2, 3]
+...
+t.index.sk == nil;
+---
+- true
+...
+t:replace{2, 2};
+---
+- [2, 2]
+...
+fiber.new(function() t:replace{1, 'asdf'} end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+t:get{1};
+---
+- [1, 'asdf']
+...
+t.index.sk == nil;
+---
+- true
+...
+t:replace{1, 1};
+---
+- [1, 1]
+...
+fiber.new(function() t:replace{2, 'asdf'} end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+t:get{2};
+---
+- [2, 'asdf']
+...
+t.index.sk == nil;
+---
+- true
+...
+t:replace{2, 2};
+---
+- [2, 2]
+...
+inspector:cmd("setopt delimiter ''");
+---
+- true
+...
+box.error.injection.set('ERRINJ_BUILD_INDEX_DELAY', 0.0)
+---
+- ok
+...
+inspector:cmd("restart server default")
+box.space.space.index.sk == nil
+---
+- true
+...
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+---
+- true
+...
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+---
+- true
+...
+box.snapshot()
+---
+- ok
+...
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+---
+- true
+...
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+---
+- true
+...
+box.space.space:drop()
+---
+...
+box.space.test:drop()
+---
+...
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 636f6c3b9..c11530c20 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -830,3 +830,124 @@ box.commit();
 -- index and space drop are not currently supported (because of truncate)
 s.index.pk:drop();
 s:drop();
+
+--
+-- Check that all modifications done to the space during index build
+-- are reflected in the new index.
+--
+math.randomseed(os.time())
+
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+
+inspector:cmd("setopt delimiter ';'")
+
+box.begin()
+for i = 1, 1000 do
+    if (i % 100 == 0) then
+        box.commit()
+        box.begin()
+    end
+    if i % 300 == 0 then
+        box.snapshot()
+    end
+    box.space.test:replace{i, i, i}
+end
+box.commit();
+
+last_val = 1000;
+
+function gen_load()
+    local s = box.space.test
+    for i = 1, 200 do
+        local op = math.random(4)
+        local key = math.random(1000)
+        local val1 = math.random(1000)
+        local val2 = last_val + 1
+        last_val = val2
+        if op == 1 then
+            pcall(s.insert, s, {key, val1, val2})
+        elseif op == 2 then
+            pcall(s.replace, s, {key, val1, val2})
+        elseif op == 3 then
+            pcall(s.delete, s, {key})
+        elseif op == 4 then
+            pcall(s.upsert, s, {key, val1, val2}, {{'=', 2, val1}, {'=', 3, val2}})
+        end
+    end
+end;
+
+inspector:cmd("setopt delimiter ''");
+
+fiber = require('fiber')
+ch = fiber.channel(1)
+
+_ = fiber.create(function() gen_load() ch:put(true) end)
+_ = box.space.test:create_index('sk', {unique = false, parts = {2, 'unsigned'}})
+ch:get()
+
+_ = fiber.create(function() gen_load() ch:put(true) end)
+_ = box.space.test:create_index('tk', {unique = true, parts = {3, 'unsigned'}})
+ch:get()
+
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+
+t = box.schema.space.create("space", {engine = engine})
+_ = t:create_index("pk")
+for i = 1,1000 do t:insert{i, i} end
+
+-- check that index build is aborted in case conflicting tuple is inserted
+-- error injection makes index build yield after 1 tuple is inserted into
+-- new index, check that index build is aborted if unique constraint is
+-- violated, as well as if tuple format doesn't match the new indexes one.
+
+box.error.injection.set('ERRINJ_BUILD_INDEX_DELAY', 0.001)
+inspector:cmd("setopt delimiter ';'")
+fiber.new(function() t:replace{1, 2} end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+
+t:get{1};
+t.index.sk;
+
+t:replace{1, 1};
+
+fiber.new(function() t:replace{2, 3} end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+
+t:get{2};
+t.index.sk == nil;
+
+t:replace{2, 2};
+
+fiber.new(function() t:replace{1, 'asdf'} end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+
+t:get{1};
+t.index.sk == nil;
+
+t:replace{1, 1};
+
+fiber.new(function() t:replace{2, 'asdf'} end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+
+t:get{2};
+t.index.sk == nil;
+
+t:replace{2, 2};
+
+inspector:cmd("setopt delimiter ''");
+box.error.injection.set('ERRINJ_BUILD_INDEX_DELAY', 0.0)
+
+inspector:cmd("restart server default")
+
+box.space.space.index.sk == nil
+
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+box.snapshot()
+box.space.test.index.pk:count() == box.space.test.index.sk:count()
+box.space.test.index.pk:count() == box.space.test.index.tk:count()
+
+box.space.space:drop()
+box.space.test:drop()
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 864050b3d..957dbb71d 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -655,124 +655,6 @@ s:drop()
 ---
 ...
 --
--- Check that all modifications done to the space during index build
--- are reflected in the new index.
---
-math.randomseed(os.time())
----
-...
-s = box.schema.space.create('test', {engine = 'vinyl'})
----
-...
-_ = s:create_index('pk')
----
-...
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-box.begin();
----
-...
-for i = 1, 1000 do
-    if (i % 100 == 0) then
-        box.commit()
-        box.begin()
-    end
-    if i % 300 == 0 then
-        box.snapshot()
-    end
-    box.space.test:replace{i, i, i}
-end;
----
-...
-box.commit();
----
-...
-last_val = 1000;
----
-...
-function gen_load()
-    local s = box.space.test
-    for i = 1, 200 do
-        local op = math.random(4)
-        local key = math.random(1000)
-        local val1 = math.random(1000)
-        local val2 = last_val + 1
-        last_val = val2
-        if op == 1 then
-            pcall(s.insert, s, {key, val1, val2})
-        elseif op == 2 then
-            pcall(s.replace, s, {key, val1, val2})
-        elseif op == 3 then
-            pcall(s.delete, s, {key})
-        elseif op == 4 then
-            pcall(s.upsert, s, {key, val1, val2}, {{'=', 2, val1}, {'=', 3, val2}})
-        end
-    end
-end;
----
-...
-test_run:cmd("setopt delimiter ''");
----
-- true
-...
-ch = fiber.channel(1)
----
-...
-_ = fiber.create(function() gen_load() ch:put(true) end)
----
-...
-_ = box.space.test:create_index('sk', {unique = false, parts = {2, 'unsigned'}})
----
-...
-ch:get()
----
-- true
-...
-_ = fiber.create(function() gen_load() ch:put(true) end)
----
-...
-_ = box.space.test:create_index('tk', {unique = true, parts = {3, 'unsigned'}})
----
-...
-ch:get()
----
-- true
-...
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
----
-- true
-...
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
----
-- true
-...
-test_run:cmd("restart server default")
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
----
-- true
-...
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
----
-- true
-...
-box.snapshot()
----
-- ok
-...
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
----
-- true
-...
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
----
-- true
-...
-box.space.test:drop()
----
-...
---
 -- Check that creation of a secondary index triggers dump
 -- if memory quota is exceeded.
 --
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index 46189828b..010ec6d79 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -241,76 +241,6 @@ _ = s:create_index('secondary', {parts = {2, 'unsigned'}})
 s:select()
 s:drop()
 
---
--- Check that all modifications done to the space during index build
--- are reflected in the new index.
---
-math.randomseed(os.time())
-
-s = box.schema.space.create('test', {engine = 'vinyl'})
-_ = s:create_index('pk')
-
-test_run:cmd("setopt delimiter ';'")
-
-box.begin();
-for i = 1, 1000 do
-    if (i % 100 == 0) then
-        box.commit()
-        box.begin()
-    end
-    if i % 300 == 0 then
-        box.snapshot()
-    end
-    box.space.test:replace{i, i, i}
-end;
-box.commit();
-
-last_val = 1000;
-
-function gen_load()
-    local s = box.space.test
-    for i = 1, 200 do
-        local op = math.random(4)
-        local key = math.random(1000)
-        local val1 = math.random(1000)
-        local val2 = last_val + 1
-        last_val = val2
-        if op == 1 then
-            pcall(s.insert, s, {key, val1, val2})
-        elseif op == 2 then
-            pcall(s.replace, s, {key, val1, val2})
-        elseif op == 3 then
-            pcall(s.delete, s, {key})
-        elseif op == 4 then
-            pcall(s.upsert, s, {key, val1, val2}, {{'=', 2, val1}, {'=', 3, val2}})
-        end
-    end
-end;
-
-test_run:cmd("setopt delimiter ''");
-
-ch = fiber.channel(1)
-
-_ = fiber.create(function() gen_load() ch:put(true) end)
-_ = box.space.test:create_index('sk', {unique = false, parts = {2, 'unsigned'}})
-ch:get()
-
-_ = fiber.create(function() gen_load() ch:put(true) end)
-_ = box.space.test:create_index('tk', {unique = true, parts = {3, 'unsigned'}})
-ch:get()
-
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
-
-test_run:cmd("restart server default")
-
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
-box.snapshot()
-box.space.test.index.pk:count() == box.space.test.index.sk:count()
-box.space.test.index.pk:count() == box.space.test.index.tk:count()
-box.space.test:drop()
-
 --
 -- Check that creation of a secondary index triggers dump
 -- if memory quota is exceeded.
-- 
2.20.1 (Apple Git-117)

Re: [tarantool-patches] [PATCH v3 2/2] test: move background index build test to engine suite from vinyl

[Serge Petrenko]

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



> 28 мая 2019 г., в 18:33, Serge Petrenko <sergepetrenko@tarantool.org> написал(а):
> 
> Since we have implemented memtx background index build, the
> corresponding vinyl test cases are now also suitable for memtx,
> so move them to engine suite so that both engines are tested.
> Also add some tests to check that an ongoing index build is aborted in
> case a tuple violating unique constraing or format of the new index is
> inserted.

Typo: constraing -> constraint.

Sorry, fixed on the branch.

> 
> Closes #3976


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

Re: [PATCH v3 0/2] memtx: add yields during index build

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [19/05/28 20:58]:
> https://github.com/tarantool/tarantool/issues/3976
> https://github.com/tarantool/tarantool/tree/sp/gh-3976-background-index-build

LGTM. Vova, please provide a second review.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v3 1/2] memtx: add yields during index build

[Vladimir Davydov]

On Tue, May 28, 2019 at 06:33:24PM +0300, Serge Petrenko wrote:
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 5ddb4f7ee..7a3e6b74a 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -933,8 +1005,38 @@ 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) {
> +			/*
> +			 * Remember the latest inserted tuple to
> +			 * avoid processing yet to be added tuples
> +			 * in on_replace triggers.
> +			 */
> +			state.cursor = tuple;

I think that state.cursor should be referenced counted - otherwise it
might get freed right under us.

> +			fiber_sleep(0);
> +			/*
> +			 * The on_replace trigger may have failed
> +			 * during the yield.
> +			 */
> +			if (state.rc != 0) {
> +				rc = -1;
> +				diag_move(&state.diag, diag_get());
> +				break;
> +			}
> +		}
> +		/*
> +		 * Sleep after at least 1 tuple is inserted to test
> +		 * on_replace triggers for index build.
> +		 * Sleep only once.
> +		 */
> +		struct errinj *inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_DOUBLE);
> +		if (inj != NULL && inj->dparam > 0 && count == 1) {
> +			state.cursor = tuple;
> +			fiber_sleep(inj->dparam);
> +		}

The error injection should be added in the next patch, the one that adds
tests. Please re-split.

Other than that, looks good to me.

Re: [PATCH v3 2/2] test: move background index build test to engine suite from vinyl

[Vladimir Davydov]

On Tue, May 28, 2019 at 06:33:25PM +0300, Serge Petrenko wrote:
> +-- check that index build is aborted in case conflicting tuple is inserted
> +-- error injection makes index build yield after 1 tuple is inserted into
> +-- new index, check that index build is aborted if unique constraint is
> +-- violated, as well as if tuple format doesn't match the new indexes one.
> +box.error.injection.set('ERRINJ_BUILD_INDEX_DELAY', 0.001)

Please use boolean error injection if possible, i.e. loop until it is
cleared. Timeouts are very unreliable in parallel test run.

Also, it looks like this test shouldn'be run in the release mode.
Worth adding engine/errinj_ddl.test.lua?

Also, vinyl has similar tests too, see vinyl/errinj_ddl.test.lua.
It would be great if you could unify them using this new error
injection you introduce.