[PATCH 0/2] memtx: make space format check yield occasionally

[PATCH 0/2] memtx: make space format check yield occasionally

[Serge Petrenko]

https://github.com/tarantool/tarantool/issues/3976
https://github.com/tarantool/tarantool/tree/sp/background-space-format

After we've made memtx index build non-blocking, it seems reasonable to do the
same thing for format checks.

The first patch adds yields to memtx_space_check_format, and an on_replace
trigger to enforce new format for concurrently inserted tuples.

The second patch moves an appropriate test case from vinyl suite to engine
suite and adds a handy error injection for the matter.

Serge Petrenko (2):
  memtx: add yields during space format check
  test: move vinyl space format test case to engine suite

 src/box/memtx_space.c           | 125 +++++++++++++++++++++++++-------
 src/box/vinyl.c                 |   6 ++
 src/lib/core/errinj.h           |   1 +
 test/box/errinj.result          |  36 ++++-----
 test/engine/errinj_ddl.result   |  68 +++++++++++++++++
 test/engine/errinj_ddl.test.lua |  33 +++++++++
 test/vinyl/errinj_ddl.result    |  75 -------------------
 test/vinyl/errinj_ddl.test.lua  |  35 ---------
 8 files changed, 226 insertions(+), 153 deletions(-)

-- 
2.20.1 (Apple Git-117)

[PATCH 1/2] memtx: add yields during space format check

[Serge Petrenko]

Just like index build, space check in memtx stalls event loop for the
whole check time. Add occasional yields, and an on_replace trigger,
which checks format for tuples inserted while space format check is in
progress.

Follow-up #3976

@TarantoolBol document
Title: memtx now checks space format in background

There is no event loop stall when memtx engine checks space format
anymore. You may insert tuples into a space, while its new format is
being checked. If the tuples don't match the new format, format change
will be aborted.
---
 src/box/memtx_space.c | 110 ++++++++++++++++++++++++++++++++----------
 1 file changed, 84 insertions(+), 26 deletions(-)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 1d209033c..a370c038a 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -43,6 +43,16 @@
 #include "column_mask.h"
 #include "sequence.h"
 
+/*
+ * Yield every 1K tuples.
+ * In debug mode yield more often for testing purposes.
+ */
+#ifdef NDEBUG
+enum { MEMTX_YIELD_LOOPS = 1000 };
+#else
+enum { MEMTX_YIELD_LOOPS = 10 };
+#endif
+
 static void
 memtx_space_destroy(struct space *space)
 {
@@ -814,6 +824,52 @@ memtx_space_add_primary_key(struct space *space)
 	return 0;
 }
 
+/*
+ * Ongoing index build or format check state used by
+ * corrseponding on_replace triggers.
+ */
+struct memtx_op_state {
+	/* The index being built. */
+	struct index *index;
+	/* New format to be enforced. */
+	struct tuple_format *format;
+	/* Operation cursor. Marks the last processed tuple to date */
+	struct tuple *cursor;
+	/* Primary key key_def to compare new tuples with cursor. */
+	struct key_def *cmp_def;
+	struct diag diag;
+	int rc;
+};
+
+static void
+memtx_check_on_replace(struct trigger *trigger, void *event)
+{
+	struct txn *txn = event;
+	struct memtx_op_state *state = trigger->data;
+	struct txn_stmt *stmt = txn_current_stmt(txn);
+
+	/* Nothing to check on DELETE. */
+	if (stmt->new_tuple == NULL)
+		return;
+
+	/* We have already failed. */
+	if (state->rc != 0)
+		return;
+
+	/*
+	 * Only check format for already processed part of the space,
+	 * all the tuples inserted below cursor will be checked by the
+	 * main routine later.
+	 */
+	if (tuple_compare(state->cursor, HINT_NONE, stmt->new_tuple, HINT_NONE,
+			  state->cmp_def) < 0)
+		return;
+
+	state->rc = tuple_validate(state->format, stmt->new_tuple);
+	if (state->rc != 0)
+		diag_move(diag_get(), &state->diag);
+}
+
 static int
 memtx_space_check_format(struct space *space, struct tuple_format *format)
 {
@@ -827,8 +883,19 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	if (it == NULL)
 		return -1;
 
+	struct memtx_op_state state;
+	state.format = format;
+	state.cmp_def = pk->def->key_def;
+	state.rc = 0;
+	diag_create(&state.diag);
+
+	struct trigger on_replace;
+	trigger_create(&on_replace, memtx_check_on_replace, &state, NULL);
+	trigger_add(&space->on_replace, &on_replace);
+
 	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
@@ -837,8 +904,22 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 		rc = tuple_validate(format, tuple);
 		if (rc != 0)
 			break;
+		if (++count % MEMTX_YIELD_LOOPS == 0) {
+			state.cursor = tuple;
+			tuple_ref(state.cursor);
+			fiber_sleep(0);
+			tuple_unref(state.cursor);
+
+			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;
 }
 
@@ -870,25 +951,11 @@ 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 memtx_op_state *state = trigger->data;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 
 	struct tuple *cmp_tuple = stmt->new_tuple != NULL ? stmt->new_tuple :
@@ -925,15 +992,6 @@ 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.
@@ -959,7 +1017,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (it == NULL)
 		return -1;
 
-	struct memtx_build_state state;
+	struct memtx_op_state state;
 	state.index = new_index;
 	state.format = new_format;
 	state.cmp_def = pk->def->key_def;
@@ -1005,7 +1063,7 @@ 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) {
+		if (++count % MEMTX_YIELD_LOOPS == 0) {
 			/*
 			 * Remember the latest inserted tuple to
 			 * avoid processing yet to be added tuples
-- 
2.20.1 (Apple Git-117)

[PATCH 2/2] test: move vinyl space format test case to engine suite

[Serge Petrenko]

After making memtx space format check non-blocking, move the appropriate
vinyl test case to engine suite. Introduce a new errinj,
ERRINJ_CHECK_FORMAT_DELAY, to unify the test case for both engines.

Follow-up #3976
---
 src/box/memtx_space.c           | 15 +++++++
 src/box/vinyl.c                 |  6 +++
 src/lib/core/errinj.h           |  1 +
 test/box/errinj.result          | 36 ++++++++--------
 test/engine/errinj_ddl.result   | 68 ++++++++++++++++++++++++++++++
 test/engine/errinj_ddl.test.lua | 33 +++++++++++++++
 test/vinyl/errinj_ddl.result    | 75 ---------------------------------
 test/vinyl/errinj_ddl.test.lua  | 35 ---------------
 8 files changed, 142 insertions(+), 127 deletions(-)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index a370c038a..54d08b531 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -916,6 +916,21 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 				break;
 			}
 		}
+
+		struct errinj *inj = errinj(ERRINJ_CHECK_FORMAT_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);
+			if (state.rc != 0) {
+				rc = -1;
+				diag_move(&state.diag, diag_get());
+				break;
+			}
+		}
 	}
 	iterator_delete(it);
 	diag_destroy(&state.diag);
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 9b4bc53bf..0c80496d9 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1156,6 +1156,12 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 		 */
 		if (++loops % VY_YIELD_LOOPS == 0)
 			fiber_sleep(0);
+		struct errinj *inj = errinj(ERRINJ_CHECK_FORMAT_DELAY, ERRINJ_BOOL);
+		if (inj != NULL && inj->bparam && loops == 1) {
+			do {
+				fiber_sleep(0);
+			} while (inj->bparam);
+		}
 		if (ctx.is_failed) {
 			diag_move(&ctx.diag, diag_get());
 			rc = -1;
diff --git a/src/lib/core/errinj.h b/src/lib/core/errinj.h
index 796d3a194..787836514 100644
--- a/src/lib/core/errinj.h
+++ b/src/lib/core/errinj.h
@@ -107,6 +107,7 @@ struct errinj {
 	_(ERRINJ_XLOG_READ, ERRINJ_INT, {.iparam = -1}) \
 	_(ERRINJ_VYRUN_INDEX_GARBAGE, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_VYRUN_DATA_READ, ERRINJ_BOOL, {.bparam = false}) \
+	_(ERRINJ_CHECK_FORMAT_DELAY, 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}) \
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 5759e7494..f24bf7813 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -36,32 +36,34 @@ errinj.info()
     state: -1
   ERRINJ_WAL_WRITE_EOF:
     state: false
+  ERRINJ_COIO_SENDFILE_CHUNK:
+    state: -1
   ERRINJ_VYRUN_INDEX_GARBAGE:
     state: false
   ERRINJ_BUILD_INDEX_DELAY:
     state: false
-  ERRINJ_COIO_SENDFILE_CHUNK:
-    state: -1
-  ERRINJ_VY_INDEX_FILE_RENAME:
-    state: false
-  ERRINJ_VY_POINT_ITER_WAIT:
+  ERRINJ_VY_RUN_FILE_RENAME:
     state: false
+  ERRINJ_BUILD_INDEX:
+    state: -1
+  ERRINJ_RELAY_BREAK_LSN:
+    state: -1
   ERRINJ_VY_DELAY_PK_LOOKUP:
     state: false
   ERRINJ_VY_TASK_COMPLETE:
     state: false
   ERRINJ_PORT_DUMP:
     state: false
-  ERRINJ_WAL_BREAK_LSN:
-    state: -1
+  ERRINJ_CHECK_FORMAT_DELAY:
+    state: false
   ERRINJ_WAL_IO:
     state: false
   ERRINJ_WAL_FALLOCATE:
     state: 0
-  ERRINJ_LOG_ROTATE:
-    state: false
   ERRINJ_VY_DUMP_DELAY:
     state: false
+  ERRINJ_WAL_BREAK_LSN:
+    state: -1
   ERRINJ_TUPLE_FORMAT_COUNT:
     state: -1
   ERRINJ_TUPLE_ALLOC:
@@ -72,25 +74,25 @@ errinj.info()
     state: false
   ERRINJ_RELAY_REPORT_INTERVAL:
     state: 0
-  ERRINJ_RELAY_BREAK_LSN:
-    state: -1
+  ERRINJ_VY_INDEX_FILE_RENAME:
+    state: false
   ERRINJ_VY_READ_PAGE_TIMEOUT:
     state: 0
   ERRINJ_XLOG_META:
     state: false
   ERRINJ_SIO_READ_MAX:
     state: -1
-  ERRINJ_VY_RUN_FILE_RENAME:
+  ERRINJ_VY_LOG_FILE_RENAME:
     state: false
   ERRINJ_WAL_WRITE_DISK:
     state: false
-  ERRINJ_VY_LOG_FILE_RENAME:
-    state: false
   ERRINJ_HTTP_RESPONSE_ADD_WAIT:
     state: false
+  ERRINJ_SNAP_COMMIT_DELAY:
+    state: false
   ERRINJ_VY_RUN_WRITE:
     state: false
-  ERRINJ_SNAP_COMMIT_DELAY:
+  ERRINJ_LOG_ROTATE:
     state: false
   ERRINJ_VY_LOG_FLUSH_DELAY:
     state: false
@@ -104,10 +106,10 @@ errinj.info()
     state: false
   ERRINJ_WAL_ROTATE:
     state: false
-  ERRINJ_BUILD_INDEX:
-    state: -1
   ERRINJ_RELAY_EXIT_DELAY:
     state: 0
+  ERRINJ_VY_POINT_ITER_WAIT:
+    state: false
   ERRINJ_MEMTX_DELAY_GC:
     state: false
   ERRINJ_IPROTO_TX_DELAY:
diff --git a/test/engine/errinj_ddl.result b/test/engine/errinj_ddl.result
index 7396de7c6..5f4043005 100644
--- a/test/engine/errinj_ddl.result
+++ b/test/engine/errinj_ddl.result
@@ -10,6 +10,74 @@ engine = test_run:get_cfg('engine')
 errinj = box.error.injection
 ---
 ...
+--
+-- Check that ALTER is abroted if a tuple inserted during space
+-- format change does not conform to the new format.
+--
+format = {}
+---
+...
+format[1] = {name = 'field1', type = 'unsigned'}
+---
+...
+format[2] = {name = 'field2', type = 'string', is_nullable = true}
+---
+...
+s = box.schema.space.create('test', {engine = engine, format = format})
+---
+...
+_ = s:create_index('pk')
+---
+...
+pad = string.rep('x', 16)
+---
+...
+for i = 101, 200 do s:replace{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, box.NULL}
+    end
+    errinj.set("ERRINJ_CHECK_FORMAT_DELAY", false)
+    ch:put(true)
+end);
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+format[2].is_nullable = false
+---
+...
+errinj.set("ERRINJ_CHECK_FORMAT_DELAY", true)
+---
+- ok
+...
+s:format(format) -- must fail
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected string'
+...
+ch:get()
+---
+- true
+...
+s:count() -- 200
+---
+- 200
+...
+s:drop()
+---
+...
 t = box.schema.space.create("space", {engine = engine})
 ---
 ...
diff --git a/test/engine/errinj_ddl.test.lua b/test/engine/errinj_ddl.test.lua
index f67ca5055..a382daa44 100644
--- a/test/engine/errinj_ddl.test.lua
+++ b/test/engine/errinj_ddl.test.lua
@@ -3,6 +3,39 @@ fiber = require('fiber')
 engine = test_run:get_cfg('engine')
 errinj = box.error.injection
 
+--
+-- Check that ALTER is abroted if a tuple inserted during space
+-- format change does not conform to the new format.
+--
+format = {}
+format[1] = {name = 'field1', type = 'unsigned'}
+format[2] = {name = 'field2', type = 'string', is_nullable = true}
+s = box.schema.space.create('test', {engine = engine, format = format})
+_ = s:create_index('pk')
+
+pad = string.rep('x', 16)
+for i = 101, 200 do s:replace{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, box.NULL}
+    end
+    errinj.set("ERRINJ_CHECK_FORMAT_DELAY", false)
+    ch:put(true)
+end);
+test_run:cmd("setopt delimiter ''");
+
+format[2].is_nullable = false
+errinj.set("ERRINJ_CHECK_FORMAT_DELAY", true)
+s:format(format) -- must fail
+ch:get()
+
+s:count() -- 200
+s:drop()
+
 t = box.schema.space.create("space", {engine = engine})
 _ = t:create_index("pk")
 for i = 1,1000 do t:insert{i, i} end
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 4c3df3acd..8fe9064b8 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -8,81 +8,6 @@ errinj = box.error.injection
 ---
 ...
 --
--- Check that ALTER is abroted if a tuple inserted during space
--- format change does not conform to the new format.
---
-format = {}
----
-...
-format[1] = {name = 'field1', type = 'unsigned'}
----
-...
-format[2] = {name = 'field2', type = 'string', is_nullable = true}
----
-...
-s = box.schema.space.create('test', {engine = 'vinyl', format = format})
----
-...
-_ = s:create_index('pk', {page_size = 16})
----
-...
-pad = string.rep('x', 16)
----
-...
-for i = 101, 200 do s:replace{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, box.NULL}
-    end
-    ch:put(true)
-end);
----
-...
-test_run:cmd("setopt delimiter ''");
----
-- true
-...
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
----
-- ok
-...
-format[2].is_nullable = false
----
-...
-s:format(format) -- must fail
----
-- error: 'Tuple field 2 type does not match one required by operation: expected string'
-...
-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 830d1d1be..3bc8bed80 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -2,41 +2,6 @@ test_run = require('test_run').new()
 fiber = require('fiber')
 errinj = box.error.injection
 
---
--- Check that ALTER is abroted if a tuple inserted during space
--- format change does not conform to the new format.
---
-format = {}
-format[1] = {name = 'field1', type = 'unsigned'}
-format[2] = {name = 'field2', type = 'string', is_nullable = true}
-s = box.schema.space.create('test', {engine = 'vinyl', format = format})
-_ = s:create_index('pk', {page_size = 16})
-
-pad = string.rep('x', 16)
-for i = 101, 200 do s:replace{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, box.NULL}
-    end
-    ch:put(true)
-end);
-test_run:cmd("setopt delimiter ''");
-
-errinj.set("ERRINJ_VY_READ_PAGE_TIMEOUT", 0.001)
-format[2].is_nullable = false
-s:format(format) -- 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: [tarantool-patches] [PATCH 1/2] memtx: add yields during space format check

[Serge Petrenko]

> 3 июня 2019 г., в 18:51, Serge Petrenko <sergepetrenko@tarantool.org> написал(а):
> 
> Just like index build, space check in memtx stalls event loop for the
> whole check time. Add occasional yields, and an on_replace trigger,
> which checks format for tuples inserted while space format check is in
> progress.
> 
> Follow-up #3976
> 
> @TarantoolBol document

«TarantoolBol» -> «TarantoolBot». Sorry, fixed.

> Title: memtx now checks space format in background
> 
> There is no event loop stall when memtx engine checks space format
> anymore. You may insert tuples into a space, while its new format is
> being checked. If the tuples don't match the new format, format change
> will be aborted.
> ---
> src/box/memtx_space.c | 110 ++++++++++++++++++++++++++++++++----------
> 1 file changed, 84 insertions(+), 26 deletions(-)
> 
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 1d209033c..a370c038a 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -43,6 +43,16 @@
> #include "column_mask.h"
> #include "sequence.h"
> 
> +/*
> + * Yield every 1K tuples.
> + * In debug mode yield more often for testing purposes.
> + */
> +#ifdef NDEBUG
> +enum { MEMTX_YIELD_LOOPS = 1000 };
> +#else
> +enum { MEMTX_YIELD_LOOPS = 10 };
> +#endif
> +
> static void
> memtx_space_destroy(struct space *space)
> {
> @@ -814,6 +824,52 @@ memtx_space_add_primary_key(struct space *space)
> 	return 0;
> }
> 
> +/*
> + * Ongoing index build or format check state used by
> + * corrseponding on_replace triggers.
> + */
> +struct memtx_op_state {
> +	/* The index being built. */
> +	struct index *index;
> +	/* New format to be enforced. */
> +	struct tuple_format *format;
> +	/* Operation cursor. Marks the last processed tuple to date */
> +	struct tuple *cursor;
> +	/* Primary key key_def to compare new tuples with cursor. */
> +	struct key_def *cmp_def;
> +	struct diag diag;
> +	int rc;
> +};
> +
> +static void
> +memtx_check_on_replace(struct trigger *trigger, void *event)
> +{
> +	struct txn *txn = event;
> +	struct memtx_op_state *state = trigger->data;
> +	struct txn_stmt *stmt = txn_current_stmt(txn);
> +
> +	/* Nothing to check on DELETE. */
> +	if (stmt->new_tuple == NULL)
> +		return;
> +
> +	/* We have already failed. */
> +	if (state->rc != 0)
> +		return;
> +
> +	/*
> +	 * Only check format for already processed part of the space,
> +	 * all the tuples inserted below cursor will be checked by the
> +	 * main routine later.
> +	 */
> +	if (tuple_compare(state->cursor, HINT_NONE, stmt->new_tuple, HINT_NONE,
> +			  state->cmp_def) < 0)
> +		return;
> +
> +	state->rc = tuple_validate(state->format, stmt->new_tuple);
> +	if (state->rc != 0)
> +		diag_move(diag_get(), &state->diag);
> +}
> +
> static int
> memtx_space_check_format(struct space *space, struct tuple_format *format)
> {
> @@ -827,8 +883,19 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
> 	if (it == NULL)
> 		return -1;
> 
> +	struct memtx_op_state state;
> +	state.format = format;
> +	state.cmp_def = pk->def->key_def;
> +	state.rc = 0;
> +	diag_create(&state.diag);
> +
> +	struct trigger on_replace;
> +	trigger_create(&on_replace, memtx_check_on_replace, &state, NULL);
> +	trigger_add(&space->on_replace, &on_replace);
> +
> 	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
> @@ -837,8 +904,22 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
> 		rc = tuple_validate(format, tuple);
> 		if (rc != 0)
> 			break;
> +		if (++count % MEMTX_YIELD_LOOPS == 0) {
> +			state.cursor = tuple;
> +			tuple_ref(state.cursor);
> +			fiber_sleep(0);
> +			tuple_unref(state.cursor);
> +
> +			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;
> }
> 
> @@ -870,25 +951,11 @@ 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 memtx_op_state *state = trigger->data;
> 	struct txn_stmt *stmt = txn_current_stmt(txn);
> 
> 	struct tuple *cmp_tuple = stmt->new_tuple != NULL ? stmt->new_tuple :
> @@ -925,15 +992,6 @@ 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.
> @@ -959,7 +1017,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
> 	if (it == NULL)
> 		return -1;
> 
> -	struct memtx_build_state state;
> +	struct memtx_op_state state;
> 	state.index = new_index;
> 	state.format = new_format;
> 	state.cmp_def = pk->def->key_def;
> @@ -1005,7 +1063,7 @@ 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) {
> +		if (++count % MEMTX_YIELD_LOOPS == 0) {
> 			/*
> 			 * Remember the latest inserted tuple to
> 			 * avoid processing yet to be added tuples
> -- 
> 2.20.1 (Apple Git-117)
> 
> 

Re: [PATCH 1/2] memtx: add yields during space format check

[Vladimir Davydov]

On Mon, Jun 03, 2019 at 06:51:05PM +0300, Serge Petrenko wrote:
> Just like index build, space check in memtx stalls event loop for the
> whole check time. Add occasional yields, and an on_replace trigger,
> which checks format for tuples inserted while space format check is in
> progress.
> 
> Follow-up #3976
> 
> @TarantoolBol document
> Title: memtx now checks space format in background
> 
> There is no event loop stall when memtx engine checks space format
> anymore. You may insert tuples into a space, while its new format is
> being checked. If the tuples don't match the new format, format change
> will be aborted.
> ---
>  src/box/memtx_space.c | 110 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 84 insertions(+), 26 deletions(-)
> 
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 1d209033c..a370c038a 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -43,6 +43,16 @@
>  #include "column_mask.h"
>  #include "sequence.h"
>  
> +/*
> + * Yield every 1K tuples.
> + * In debug mode yield more often for testing purposes.
> + */
> +#ifdef NDEBUG
> +enum { MEMTX_YIELD_LOOPS = 1000 };
> +#else
> +enum { MEMTX_YIELD_LOOPS = 10 };
> +#endif
> +
>  static void
>  memtx_space_destroy(struct space *space)
>  {
> @@ -814,6 +824,52 @@ memtx_space_add_primary_key(struct space *space)
>  	return 0;
>  }
>  
> +/*
> + * Ongoing index build or format check state used by
> + * corrseponding on_replace triggers.
> + */
> +struct memtx_op_state {

What's 'op'? The name's too generic. I renamed it to memtx_ddl_state.
I also renamed MEMTX_YIELD_LOOPS to MEMTX_DDL_YIELD_LOOPS to emphasize
that this is about DDL.

I pushed both patches to master with this minor changes. Thanks!