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

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

[Serge Petrenko]

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 v4:
  - move errinj introduction to the
    second patch
  - move more tests from vinyl/errinj_ddl
    to engine/errinj_ddl
  - unify errinjs in memtx and vinyl index
    build for easier testing
  - ref count cursor before yield in index
    build

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           | 107 ++++++++++++
 src/box/vinyl.c                 |  16 ++
 src/lib/core/errinj.h           |   1 +
 test/box/errinj.result          |   2 +
 test/engine/ddl.result          | 111 +++++++++++++
 test/engine/ddl.test.lua        |  72 ++++++++
 test/engine/errinj_ddl.result   | 284 ++++++++++++++++++++++++++++++++
 test/engine/errinj_ddl.test.lua | 133 +++++++++++++++
 test/engine/suite.ini           |   2 +-
 test/vinyl/ddl.result           | 118 -------------
 test/vinyl/ddl.test.lua         |  70 --------
 test/vinyl/errinj_ddl.result    | 182 --------------------
 test/vinyl/errinj_ddl.test.lua  |  81 ---------
 13 files changed, 727 insertions(+), 452 deletions(-)
 create mode 100644 test/engine/errinj_ddl.result
 create mode 100644 test/engine/errinj_ddl.test.lua

-- 
2.20.1 (Apple Git-117)

[PATCH v4 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 | 94 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 5ddb4f7ee..f13a9456c 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,30 @@ 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;
+			tuple_ref(state.cursor);
+			fiber_sleep(0);
+			tuple_unref(state.cursor);
+			/*
+			 * The on_replace trigger may have failed
+			 * during the yield.
+			 */
+			if (state.rc != 0) {
+				rc = -1;
+				diag_move(&state.diag, diag_get());
+				break;
+			}
+		}
 	}
 	iterator_delete(it);
+	diag_destroy(&state.diag);
+	trigger_clear(&on_replace);
 	return rc;
 }
 
-- 
2.20.1 (Apple Git-117)

[PATCH v4 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 constraint or format of the new index is
inserted.
Add some error injections to unify appropriate memtx/vinyl tests.

Closes #3976
---
 src/box/memtx_space.c           |  13 ++
 src/box/vinyl.c                 |  16 ++
 src/lib/core/errinj.h           |   1 +
 test/box/errinj.result          |   2 +
 test/engine/ddl.result          | 111 +++++++++++++
 test/engine/ddl.test.lua        |  72 ++++++++
 test/engine/errinj_ddl.result   | 284 ++++++++++++++++++++++++++++++++
 test/engine/errinj_ddl.test.lua | 133 +++++++++++++++
 test/engine/suite.ini           |   2 +-
 test/vinyl/ddl.result           | 118 -------------
 test/vinyl/ddl.test.lua         |  70 --------
 test/vinyl/errinj_ddl.result    | 182 --------------------
 test/vinyl/errinj_ddl.test.lua  |  81 ---------
 13 files changed, 633 insertions(+), 452 deletions(-)
 create mode 100644 test/engine/errinj_ddl.result
 create mode 100644 test/engine/errinj_ddl.test.lua

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f13a9456c..1d209033c 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -1025,6 +1025,19 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 				break;
 			}
 		}
+		/*
+		 * Sleep after at least one tuple is inserted to test
+		 * on_replace triggers for index build.
+		 */
+		struct errinj *inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_BOOL);
+		if (inj != NULL && inj->bparam && count == 1) {
+			state.cursor = tuple;
+			tuple_ref(state.cursor);
+			do {
+				fiber_sleep(0);
+			} while (inj->bparam);
+			tuple_unref(state.cursor);
+		}
 	}
 	iterator_delete(it);
 	diag_destroy(&state.diag);
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9e731d137..8286fed7c 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -4339,6 +4339,12 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 		return -1;
 	}
 
+	struct errinj *inj = errinj(ERRINJ_BUILD_INDEX, ERRINJ_INT);
+	if (inj != NULL && inj->iparam == (int)new_index->def->iid) {
+		diag_set(ClientError, ER_INJECTION, "build index");
+		return -1;
+	}
+
 	if (vinyl_index_open(new_index) != 0)
 		return -1;
 
@@ -4414,6 +4420,16 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 			rc = -1;
 			break;
 		}
+		/*
+		 * Sleep after one tuple is inserted to test
+		 * on_replace triggers for index build.
+		 */
+		inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_BOOL);
+		if (inj != NULL && inj->bparam && loops == 1) {
+			do {
+				fiber_sleep(0);
+			} while (inj->bparam);
+		}
 	}
 	vy_read_iterator_close(&itr);
 
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 462c98464..796d3a194 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_BOOL, {.bparam = false}) \
 	_(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..5759e7494 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: false
   ERRINJ_COIO_SENDFILE_CHUNK:
     state: -1
   ERRINJ_VY_INDEX_FILE_RENAME:
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index c493bd4ac..30f305e9e 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -2222,3 +2222,114 @@ 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
+...
+inspector: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()
+---
+...
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 636f6c3b9..25ce8ee6b 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -830,3 +830,75 @@ 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()
+
+inspector: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()
diff --git a/test/engine/errinj_ddl.result b/test/engine/errinj_ddl.result
new file mode 100644
index 000000000..7396de7c6
--- /dev/null
+++ b/test/engine/errinj_ddl.result
@@ -0,0 +1,284 @@
+test_run = require('test_run').new()
+---
+...
+fiber = require('fiber')
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+errinj = box.error.injection
+---
+...
+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.
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+---
+- ok
+...
+fiber.new(function() t:replace{1, 2} errinj.set('ERRINJ_BUILD_INDEX_DELAY',false) 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]
+...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+---
+- ok
+...
+fiber.new(function() t:replace{2, 3} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) 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]
+...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+---
+- ok
+...
+fiber.new(function() t:replace{1, 'asdf'} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) 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]
+...
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+---
+- ok
+...
+fiber.new(function() t:replace{2, 'asdf'} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) 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]
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+t:drop()
+---
+...
+--
+-- gh-2449: change 'unique' index property from true to false
+-- is done without index rebuild.
+--
+s = box.schema.space.create('test', { engine = engine })
+---
+...
+_ = s:create_index('primary')
+---
+...
+_ = s:create_index('secondary', {unique = true, parts = {2, 'unsigned'}})
+---
+...
+s:insert{1, 10}
+---
+- [1, 10]
+...
+errinj.set("ERRINJ_BUILD_INDEX", 1);
+---
+- ok
+...
+s.index.secondary:alter{unique = false} -- ok
+---
+...
+s.index.secondary.unique
+---
+- false
+...
+s.index.secondary:alter{unique = true} -- error
+---
+- error: Error injection 'build index'
+...
+s.index.secondary.unique
+---
+- false
+...
+errinj.set("ERRINJ_BUILD_INDEX", -1);
+---
+- ok
+...
+s:insert{2, 10}
+---
+- [2, 10]
+...
+s.index.secondary:select(10)
+---
+- - [1, 10]
+  - [2, 10]
+...
+s:drop()
+---
+...
+--
+-- Check that ALTER is aborted if a tuple inserted during index build
+-- doesn't conform to the new format.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+pad = string.rep('x', 16)
+---
+...
+for i = 101, 200 do s:replace{i, i, pad} end
+---
+...
+ch = fiber.channel(1)
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+_ = fiber.create(function()
+    fiber.sleep(0.01)
+    for i = 1, 100 do
+        s:replace{i}
+    end
+    errinj.set("ERRINJ_BUILD_INDEX_DELAY", false)
+    ch:put(true)
+end);
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+errinj.set("ERRINJ_BUILD_INDEX_DELAY", true)
+---
+- ok
+...
+s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
+---
+- error: Tuple field 2 required by space format is missing
+...
+ch:get()
+---
+- true
+...
+s:count() -- 200
+---
+- 200
+...
+s:drop()
+---
+...
+--
+-- Check that ALTER is aborted if a tuple inserted during index build
+-- violates unique constraint.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+pad = string.rep('x', 16)
+---
+...
+for i = 101, 200 do s:replace{i, i, pad} end
+---
+...
+ch = fiber.channel(1)
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+_ = fiber.create(function()
+    fiber.sleep(0.01)
+    for i = 1, 100 do
+        s:replace{i, i + 1}
+    end
+    errinj.set("ERRINJ_BUILD_INDEX_DELAY", false)
+    ch:put(true)
+end);
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+errinj.set("ERRINJ_BUILD_INDEX_DELAY", true)
+---
+- ok
+...
+s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
+---
+- error: Duplicate key exists in unique index 'sk' in space 'test'
+...
+ch:get()
+---
+- true
+...
+s:count() -- 200
+---
+- 200
+...
+s:drop()
+---
+...
diff --git a/test/engine/errinj_ddl.test.lua b/test/engine/errinj_ddl.test.lua
new file mode 100644
index 000000000..f67ca5055
--- /dev/null
+++ b/test/engine/errinj_ddl.test.lua
@@ -0,0 +1,133 @@
+test_run = require('test_run').new()
+fiber = require('fiber')
+engine = test_run:get_cfg('engine')
+errinj = box.error.injection
+
+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.
+
+test_run:cmd("setopt delimiter ';'")
+
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+fiber.new(function() t:replace{1, 2} errinj.set('ERRINJ_BUILD_INDEX_DELAY',false) end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+
+t:get{1};
+t.index.sk;
+
+t:replace{1, 1};
+
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+fiber.new(function() t:replace{2, 3} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) end)
+t:create_index('sk', {parts = {2, 'unsigned', unique=true}});
+
+t:get{2};
+t.index.sk == nil;
+
+t:replace{2, 2};
+
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+fiber.new(function() t:replace{1, 'asdf'} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+
+t:get{1};
+t.index.sk == nil;
+
+t:replace{1, 1};
+
+errinj.set('ERRINJ_BUILD_INDEX_DELAY', true);
+fiber.new(function() t:replace{2, 'asdf'} errinj.set('ERRINJ_BUILD_INDEX_DELAY', false) end)
+t:create_index('sk', {parts = {2, 'unsigned'}});
+
+t:get{2};
+t.index.sk == nil;
+
+t:replace{2, 2};
+
+test_run:cmd("setopt delimiter ''");
+
+t:drop()
+
+--
+-- gh-2449: change 'unique' index property from true to false
+-- is done without index rebuild.
+--
+s = box.schema.space.create('test', { engine = engine })
+_ = s:create_index('primary')
+_ = s:create_index('secondary', {unique = true, parts = {2, 'unsigned'}})
+s:insert{1, 10}
+errinj.set("ERRINJ_BUILD_INDEX", 1);
+s.index.secondary:alter{unique = false} -- ok
+s.index.secondary.unique
+s.index.secondary:alter{unique = true} -- error
+s.index.secondary.unique
+errinj.set("ERRINJ_BUILD_INDEX", -1);
+s:insert{2, 10}
+s.index.secondary:select(10)
+s:drop()
+
+--
+-- Check that ALTER is aborted if a tuple inserted during index build
+-- doesn't conform to the new format.
+--
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+
+pad = string.rep('x', 16)
+for i = 101, 200 do s:replace{i, i, pad} end
+
+ch = fiber.channel(1)
+test_run:cmd("setopt delimiter ';'")
+_ = fiber.create(function()
+    fiber.sleep(0.01)
+    for i = 1, 100 do
+        s:replace{i}
+    end
+    errinj.set("ERRINJ_BUILD_INDEX_DELAY", false)
+    ch:put(true)
+end);
+test_run:cmd("setopt delimiter ''");
+
+errinj.set("ERRINJ_BUILD_INDEX_DELAY", true)
+s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
+
+ch:get()
+
+s:count() -- 200
+s:drop()
+
+--
+-- Check that ALTER is aborted if a tuple inserted during index build
+-- violates unique constraint.
+--
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+
+pad = string.rep('x', 16)
+for i = 101, 200 do s:replace{i, i, pad} end
+
+ch = fiber.channel(1)
+test_run:cmd("setopt delimiter ';'")
+_ = fiber.create(function()
+    fiber.sleep(0.01)
+    for i = 1, 100 do
+        s:replace{i, i + 1}
+    end
+    errinj.set("ERRINJ_BUILD_INDEX_DELAY", false)
+    ch:put(true)
+end);
+test_run:cmd("setopt delimiter ''");
+
+errinj.set("ERRINJ_BUILD_INDEX_DELAY", true)
+s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
+
+ch:get()
+
+s:count() -- 200
+s:drop()
diff --git a/test/engine/suite.ini b/test/engine/suite.ini
index d79efa659..64ddafa81 100644
--- a/test/engine/suite.ini
+++ b/test/engine/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
 description = tarantool multiengine tests
 script = box.lua
 use_unix_sockets = True
-release_disabled = errinj.test.lua
+release_disabled = errinj.test.lua errinj_ddl.test.lua
 config = engine.cfg
 #disabled = replica_join.test.lua
 lua_libs = conflict.lua ../box/lua/utils.lua ../box/lua/push.lua
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index d584708b9..9c453a007 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.
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index dcbf90f78..4c3df3acd 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -83,188 +83,6 @@ s:drop()
 ---
 ...
 --
--- gh-2449: change 'unique' index property from true to false
--- is done without index rebuild.
---
-s = box.schema.space.create('test', { engine = 'vinyl' })
----
-...
-_ = s:create_index('primary')
----
-...
-_ = s:create_index('secondary', {unique = true, parts = {2, 'unsigned'}})
----
-...
-s:insert{1, 10}
----
-- [1, 10]
-...
-box.snapshot()
----
-- ok
-...
-errinj.set("ERRINJ_VY_READ_PAGE", true);
----
-- ok
-...
-s.index.secondary:alter{unique = false} -- ok
----
-...
-s.index.secondary.unique
----
-- false
-...
-s.index.secondary:alter{unique = true} -- error
----
-- error: Error injection 'vinyl page read'
-...
-s.index.secondary.unique
----
-- false
-...
-errinj.set("ERRINJ_VY_READ_PAGE", false);
----
-- ok
-...
-s:insert{2, 10}
----
-- [2, 10]
-...
-s.index.secondary:select(10)
----
-- - [1, 10]
-  - [2, 10]
-...
-s:drop()
----
-...
---
--- Check that ALTER is aborted if a tuple inserted during index build
--- doesn't conform to the new format.
---
-s = box.schema.space.create('test', {engine = 'vinyl'})
----
-...
-_ = s:create_index('pk', {page_size = 16})
----
-...
-pad = string.rep('x', 16)
----
-...
-for i = 101, 200 do s:replace{i, i, pad} end
----
-...
-box.snapshot()
----
-- ok
-...
-ch = fiber.channel(1)
----
-...
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-_ = fiber.create(function()
-    fiber.sleep(0.01)
-    for i = 1, 100 do
-        s:replace{i}
-    end
-    ch:put(true)
-end);
----
-...
-test_run:cmd("setopt delimiter ''");
----
-- true
-...
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
----
-- ok
-...
-s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
----
-- error: Tuple field 2 required by space format is missing
-...
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0)
----
-- ok
-...
-ch:get()
----
-- true
-...
-s:count() -- 200
----
-- 200
-...
-s:drop()
----
-...
---
--- Check that ALTER is aborted if a tuple inserted during index build
--- violates unique constraint.
---
-s = box.schema.space.create('test', {engine = 'vinyl'})
----
-...
-_ = s:create_index('pk', {page_size = 16})
----
-...
-pad = string.rep('x', 16)
----
-...
-for i = 101, 200 do s:replace{i, i, pad} end
----
-...
-box.snapshot()
----
-- ok
-...
-ch = fiber.channel(1)
----
-...
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-_ = fiber.create(function()
-    fiber.sleep(0.01)
-    for i = 1, 100 do
-        s:replace{i, i + 1}
-    end
-    ch:put(true)
-end);
----
-...
-test_run:cmd("setopt delimiter ''");
----
-- true
-...
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
----
-- ok
-...
-s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
----
-- error: Duplicate key exists in unique index 'sk' in space 'test'
-...
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0)
----
-- ok
-...
-ch:get()
----
-- true
-...
-s:count() -- 200
----
-- 200
-...
-s:drop()
----
-...
---
 -- Check that modifications done to the space during the final dump
 -- of a newly built index are recovered properly.
 --
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 6413c352e..830d1d1be 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -37,87 +37,6 @@ ch:get()
 s:count() -- 200
 s:drop()
 
---
--- gh-2449: change 'unique' index property from true to false
--- is done without index rebuild.
---
-s = box.schema.space.create('test', { engine = 'vinyl' })
-_ = s:create_index('primary')
-_ = s:create_index('secondary', {unique = true, parts = {2, 'unsigned'}})
-s:insert{1, 10}
-box.snapshot()
-errinj.set("ERRINJ_VY_READ_PAGE", true);
-s.index.secondary:alter{unique = false} -- ok
-s.index.secondary.unique
-s.index.secondary:alter{unique = true} -- error
-s.index.secondary.unique
-errinj.set("ERRINJ_VY_READ_PAGE", false);
-s:insert{2, 10}
-s.index.secondary:select(10)
-s:drop()
-
---
--- Check that ALTER is aborted if a tuple inserted during index build
--- doesn't conform to the new format.
---
-s = box.schema.space.create('test', {engine = 'vinyl'})
-_ = s:create_index('pk', {page_size = 16})
-
-pad = string.rep('x', 16)
-for i = 101, 200 do s:replace{i, i, pad} end
-box.snapshot()
-
-ch = fiber.channel(1)
-test_run:cmd("setopt delimiter ';'")
-_ = fiber.create(function()
-    fiber.sleep(0.01)
-    for i = 1, 100 do
-        s:replace{i}
-    end
-    ch:put(true)
-end);
-test_run:cmd("setopt delimiter ''");
-
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
-s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0)
-
-ch:get()
-
-s:count() -- 200
-s:drop()
-
---
--- Check that ALTER is aborted if a tuple inserted during index build
--- violates unique constraint.
---
-s = box.schema.space.create('test', {engine = 'vinyl'})
-_ = s:create_index('pk', {page_size = 16})
-
-pad = string.rep('x', 16)
-for i = 101, 200 do s:replace{i, i, pad} end
-box.snapshot()
-
-ch = fiber.channel(1)
-test_run:cmd("setopt delimiter ';'")
-_ = fiber.create(function()
-    fiber.sleep(0.01)
-    for i = 1, 100 do
-        s:replace{i, i + 1}
-    end
-    ch:put(true)
-end);
-test_run:cmd("setopt delimiter ''");
-
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
-s:create_index('sk', {parts = {2, 'unsigned'}}) -- must fail
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0)
-
-ch:get()
-
-s:count() -- 200
-s:drop()
-
 --
 -- Check that modifications done to the space during the final dump
 -- of a newly built index are recovered properly.
-- 
2.20.1 (Apple Git-117)

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

[Vladimir Davydov]

On Thu, May 30, 2019 at 03:13:26PM +0300, Serge Petrenko wrote:
> 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 v4:
>   - move errinj introduction to the
>     second patch
>   - move more tests from vinyl/errinj_ddl
>     to engine/errinj_ddl
>   - unify errinjs in memtx and vinyl index
>     build for easier testing
>   - ref count cursor before yield in index
>     build
> 
> 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

Pushed to master. Thanks for addressing all my comments.

We should probably make format check yielding, too. This should be
very similar to what you've just done. Please consider doing this in
a follow-up patch.

Re: [tarantool-patches] Re: [PATCH v4 0/2] memtx: add yields during index build

[Serge Petrenko]

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



> 30 мая 2019 г., в 16:38, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Thu, May 30, 2019 at 03:13:26PM +0300, Serge Petrenko wrote:
>> 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 v4:
>>  - move errinj introduction to the
>>    second patch
>>  - move more tests from vinyl/errinj_ddl
>>    to engine/errinj_ddl
>>  - unify errinjs in memtx and vinyl index
>>    build for easier testing
>>  - ref count cursor before yield in index
>>    build
>> 
>> 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
> 
> Pushed to master. Thanks for addressing all my comments.
> 
> We should probably make format check yielding, too. This should be
> very similar to what you've just done. Please consider doing this in
> a follow-up patch.

Ok, will do. Thank you


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