[PATCH v2 0/2] vinyl: in a task use copy of index key defs to protect from alter

[PATCH v2 0/2] vinyl: in a task use copy of index key defs to protect from alter

[Vladislav Shpilevoy]

Branch: http://github.com/tarantool/tarantool/tree/gh-3229-space-format-enables-optional-v2
Issue: https://github.com/tarantool/tarantool/issues/3229

A vinyl space can be altered in such a way, that key definitions
of indexes are not changed, but comparators do. It is because
space format reset can make some indexed fields optional.

To be able update key definitions in place, they must not be used
in a worker thread. So lets copy key_defs for a worker, and
update index key definitions in place.

An alternative is key_defs reference counting, but there is open
questions what to do in key_defs in mems, ranges, iterators,
runs, slices. Now lets do a hotfix of a crash, and then
refactoring.

Vladislav Shpilevoy (2):
  Do not mix box_key_def_delete and free to delete key_def
  vinyl: in a task use copy of index key defs to protect from alter

 src/box/alter.cc                |  3 ++-
 src/box/key_def.cc              | 10 ++++++++--
 src/box/key_def.h               |  7 +++++++
 src/box/schema.cc               |  4 ++--
 src/box/vinyl.c                 | 10 ++++++++--
 src/box/vy_index.c              |  8 ++++----
 src/box/vy_scheduler.c          | 25 ++++++++++++++++++++++---
 test/engine/ddl.result          |  2 +-
 test/engine/ddl.test.lua        |  2 +-
 test/unit/vy_iterators_helper.c |  2 +-
 test/unit/vy_mem.c              |  4 ++--
 test/unit/vy_point_lookup.c     |  2 +-
 test/unit/vy_write_iterator.c   |  2 +-
 13 files changed, 60 insertions(+), 21 deletions(-)

-- 
2.14.3 (Apple Git-98)

[PATCH v2 1/2] Do not mix box_key_def_delete and free to delete key_def

[Vladislav Shpilevoy]

Use key_def_delete instead. It is more safe to use one
destructor everywhere for a case, if in a future key_def will not
be deleted by simple free().

Signed-off-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
---
 src/box/alter.cc                |  3 ++-
 src/box/key_def.cc              | 10 ++++++++--
 src/box/key_def.h               |  7 +++++++
 src/box/schema.cc               |  4 ++--
 src/box/vinyl.c                 |  4 ++--
 src/box/vy_index.c              |  8 ++++----
 test/unit/vy_iterators_helper.c |  2 +-
 test/unit/vy_mem.c              |  4 ++--
 test/unit/vy_point_lookup.c     |  2 +-
 test/unit/vy_write_iterator.c   |  2 +-
 10 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 8455373b2..c89fe8814 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -328,7 +328,8 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 	}
 	auto key_def_guard = make_scoped_guard([&] {
 		free(part_def);
-		free(key_def);
+		if (key_def != NULL)
+			key_def_delete(key_def);
 	});
 	if (is_166plus) {
 		/* 1.6.6+ */
diff --git a/src/box/key_def.cc b/src/box/key_def.cc
index 955349cf3..2f1574195 100644
--- a/src/box/key_def.cc
+++ b/src/box/key_def.cc
@@ -106,6 +106,12 @@ key_def_dup(const struct key_def *src)
 	return res;
 }
 
+void
+key_def_delete(struct key_def *def)
+{
+	free(def);
+}
+
 static void
 key_def_set_cmp(struct key_def *def)
 {
@@ -145,7 +151,7 @@ key_def_new_with_parts(struct key_part_def *parts, uint32_t part_count)
 			if (coll == NULL) {
 				diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
 					 i + 1, "collation was not found by ID");
-				free(def);
+				key_def_delete(def);
 				return NULL;
 			}
 		}
@@ -187,7 +193,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
 void
 box_key_def_delete(box_key_def_t *key_def)
 {
-	free(key_def);
+	key_def_delete(key_def);
 }
 
 int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 4726c38a2..fe9ee56f7 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -149,6 +149,13 @@ struct key_def {
 struct key_def *
 key_def_dup(const struct key_def *src);
 
+/**
+ * Delete @a key_def.
+ * @param def Key_def to delete.
+ */
+void
+key_def_delete(struct key_def *def);
+
 /** \cond public */
 
 typedef struct key_def box_key_def_t;
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 2e8533b94..1b96f978c 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -278,7 +278,7 @@ schema_init()
 	struct key_def *key_def = key_def_new(1); /* part count */
 	if (key_def == NULL)
 		diag_raise();
-	auto key_def_guard = make_scoped_guard([&] { box_key_def_delete(key_def); });
+	auto key_def_guard = make_scoped_guard([&] { key_def_delete(key_def); });
 
 	key_def_set_part(key_def, 0 /* part no */, 0 /* field no */,
 			 FIELD_TYPE_STRING, false, NULL);
@@ -329,7 +329,7 @@ schema_init()
 	sc_space_new(BOX_CLUSTER_ID, "_cluster", key_def, &on_replace_cluster,
 		     NULL);
 
-	free(key_def);
+	key_def_delete(key_def);
 	key_def = key_def_new(2); /* part count */
 	if (key_def == NULL)
 		diag_raise();
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 3467281a7..225698eb0 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3097,7 +3097,7 @@ vy_join_cb(const struct vy_log_record *record, void *arg)
 		ctx->space_id = record->space_id;
 		ctx->index_id = record->index_id;
 		if (ctx->key_def != NULL)
-			free(ctx->key_def);
+			key_def_delete(ctx->key_def);
 		ctx->key_def = key_def_new_with_parts(record->key_parts,
 						      record->key_part_count);
 		if (ctx->key_def == NULL)
@@ -3234,7 +3234,7 @@ vinyl_engine_join(struct engine *engine, struct vclock *vclock,
 
 	/* Cleanup. */
 	if (ctx->key_def != NULL)
-		free(ctx->key_def);
+		key_def_delete(ctx->key_def);
 	if (ctx->format != NULL)
 		tuple_format_unref(ctx->format);
 	if (ctx->upsert_format != NULL)
diff --git a/src/box/vy_index.c b/src/box/vy_index.c
index 68fccab52..d9160041b 100644
--- a/src/box/vy_index.c
+++ b/src/box/vy_index.c
@@ -257,9 +257,9 @@ fail_mem_format_with_colmask:
 fail_upsert_format:
 	tuple_format_unref(index->disk_format);
 fail_format:
-	free(cmp_def);
+	key_def_delete(cmp_def);
 fail_cmp_def:
-	free(key_def);
+	key_def_delete(key_def);
 fail_key_def:
 	free(index->tree);
 fail_tree:
@@ -308,8 +308,8 @@ vy_index_delete(struct vy_index *index)
 	tuple_format_unref(index->disk_format);
 	tuple_format_unref(index->mem_format_with_colmask);
 	tuple_format_unref(index->upsert_format);
-	free(index->cmp_def);
-	free(index->key_def);
+	key_def_delete(index->cmp_def);
+	key_def_delete(index->key_def);
 	histogram_delete(index->run_hist);
 	vy_index_stat_destroy(&index->stat);
 	vy_cache_destroy(&index->cache);
diff --git a/test/unit/vy_iterators_helper.c b/test/unit/vy_iterators_helper.c
index 0e41de4b1..9eab13d34 100644
--- a/test/unit/vy_iterators_helper.c
+++ b/test/unit/vy_iterators_helper.c
@@ -252,7 +252,7 @@ destroy_test_cache(struct vy_cache *cache, struct key_def *def,
 {
 	tuple_format_unref(format);
 	vy_cache_destroy(cache);
-	box_key_def_delete(def);
+	key_def_delete(def);
 }
 
 bool
diff --git a/test/unit/vy_mem.c b/test/unit/vy_mem.c
index 5d1608771..60fcf0849 100644
--- a/test/unit/vy_mem.c
+++ b/test/unit/vy_mem.c
@@ -56,7 +56,7 @@ test_basic(void)
 
 	/* Clean up */
 	vy_mem_delete(mem);
-	box_key_def_delete(key_def);
+	key_def_delete(key_def);
 
 	fiber_gc();
 	footer();
@@ -302,7 +302,7 @@ test_iterator_restore_after_insertion()
 
 	tuple_format_unref(format);
 	lsregion_destroy(&lsregion);
-	box_key_def_delete(key_def);
+	key_def_delete(key_def);
 
 	fiber_gc();
 
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index 52f4427e9..963329c9f 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -312,7 +312,7 @@ test_basic()
 	index_def_delete(index_def);
 	tuple_format_unref(format);
 	vy_cache_destroy(&cache);
-	box_key_def_delete(key_def);
+	key_def_delete(key_def);
 	vy_cache_env_destroy(&cache_env);
 	vy_run_env_destroy(&run_env);
 	vy_index_env_destroy(&index_env);
diff --git a/test/unit/vy_write_iterator.c b/test/unit/vy_write_iterator.c
index 9058298cb..bf8ef39e8 100644
--- a/test/unit/vy_write_iterator.c
+++ b/test/unit/vy_write_iterator.c
@@ -501,7 +501,7 @@ test_basic(void)
 				       expected, expected_count,
 				       vlsns, vlsns_count, true, false);
 }
-	box_key_def_delete(key_def);
+	key_def_delete(key_def);
 	fiber_gc();
 	footer();
 	check_plan();
-- 
2.14.3 (Apple Git-98)

[PATCH v2 2/2] vinyl: in a task use copy of index key defs to protect from alter

[Vladislav Shpilevoy]

A vinyl space can be altered in such a way, that key definitions
of indexes are not changed, but comparators do. It is because
space format reset can make some indexed fields optional.

To be able update key definitions in place, they must not be used
in a worker thread. So lets copy key_defs for a worker, and
update index key definitions in place.

An alternative is key_defs reference counting, but there is open
questions what to do in key_defs in mems, ranges, iterators,
runs, slices. Now lets do a hotfix of a crash, and then
refactoring.

Closes #3229

Signed-off-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
---
 src/box/vinyl.c          |  6 ++++++
 src/box/vy_scheduler.c   | 25 ++++++++++++++++++++++---
 test/engine/ddl.result   |  2 +-
 test/engine/ddl.test.lua |  2 +-
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 225698eb0..229232060 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1134,6 +1134,8 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 	pk->mem_format = new_format;
 	tuple_format_ref(new_format);
 	vy_index_validate_formats(pk);
+	key_def_update_optionality(pk->key_def, new_format->min_field_count);
+	key_def_update_optionality(pk->cmp_def, new_format->min_field_count);
 
 	for (uint32_t i = 1; i < new_space->index_count; ++i) {
 		struct vy_index *index = vy_index(new_space->index[i]);
@@ -1153,6 +1155,10 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 		tuple_format_ref(index->mem_format_with_colmask);
 		tuple_format_ref(index->mem_format);
 		tuple_format_ref(index->upsert_format);
+		key_def_update_optionality(index->key_def,
+					   new_format->min_field_count);
+		key_def_update_optionality(index->cmp_def,
+					   new_format->min_field_count);
 		vy_index_validate_formats(index);
 	}
 
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 382cf0718..b6225ace6 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -110,6 +110,12 @@ struct vy_task {
 	struct diag diag;
 	/** Index this task is for. */
 	struct vy_index *index;
+	/**
+	 * Copies of index->key/cmp_def to protect from
+	 * multithread read/write on alter.
+	 */
+	struct key_def *cmp_def;
+	struct key_def *key_def;
 	/** Range to compact. */
 	struct vy_range *range;
 	/** Run written by this task. */
@@ -164,6 +170,17 @@ vy_task_new(struct mempool *pool, struct vy_index *index,
 	memset(task, 0, sizeof(*task));
 	task->ops = ops;
 	task->index = index;
+	task->cmp_def = key_def_dup(index->cmp_def);
+	if (task->cmp_def == NULL) {
+		mempool_free(pool, task);
+		return NULL;
+	}
+	task->key_def = key_def_dup(index->key_def);
+	if (task->key_def == NULL) {
+		key_def_delete(task->cmp_def);
+		mempool_free(pool, task);
+		return NULL;
+	}
 	vy_index_ref(index);
 	diag_create(&task->diag);
 	return task;
@@ -173,6 +190,8 @@ vy_task_new(struct mempool *pool, struct vy_index *index,
 static void
 vy_task_delete(struct mempool *pool, struct vy_task *task)
 {
+	key_def_delete(task->cmp_def);
+	key_def_delete(task->key_def);
 	vy_index_unref(task->index);
 	diag_destroy(&task->diag);
 	TRASH(task);
@@ -638,7 +657,7 @@ vy_task_write_run(struct vy_scheduler *scheduler, struct vy_task *task)
 	struct vy_run_writer writer;
 	if (vy_run_writer_create(&writer, task->new_run, index->env->path,
 				 index->space_id, index->id,
-				 index->cmp_def, index->key_def,
+				 task->cmp_def, task->key_def,
 				 task->page_size, task->bloom_fpr,
 				 task->max_output_count) != 0)
 		goto fail;
@@ -989,7 +1008,7 @@ vy_task_dump_new(struct vy_scheduler *scheduler, struct vy_index *index,
 
 	struct vy_stmt_stream *wi;
 	bool is_last_level = (index->run_count == 0);
-	wi = vy_write_iterator_new(index->cmp_def, index->disk_format,
+	wi = vy_write_iterator_new(task->cmp_def, index->disk_format,
 				   index->upsert_format, index->id == 0,
 				   is_last_level, scheduler->read_views);
 	if (wi == NULL)
@@ -1264,7 +1283,7 @@ vy_task_compact_new(struct vy_scheduler *scheduler, struct vy_index *index,
 
 	struct vy_stmt_stream *wi;
 	bool is_last_level = (range->compact_priority == range->slice_count);
-	wi = vy_write_iterator_new(index->cmp_def, index->disk_format,
+	wi = vy_write_iterator_new(task->cmp_def, index->disk_format,
 				   index->upsert_format, index->id == 0,
 				   is_last_level, scheduler->read_views);
 	if (wi == NULL)
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index 906c9d11c..c3cb1efc2 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -394,7 +394,7 @@ s:drop()
 box.cfg{}
 ---
 ...
-s = box.schema.create_space('test', {engine = 'memtx'})
+s = box.schema.create_space('test', {engine = engine})
 ---
 ...
 format = {}
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 3b4cd1e5e..25381a3c8 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -142,7 +142,7 @@ s:drop()
 -- not only when indexes are updated.
 --
 box.cfg{}
-s = box.schema.create_space('test', {engine = 'memtx'})
+s = box.schema.create_space('test', {engine = engine})
 format = {}
 format[1] = {'field1', 'unsigned'}
 format[2] = {'field2', 'unsigned', is_nullable = true}
-- 
2.14.3 (Apple Git-98)

Re: [PATCH v2 0/2] vinyl: in a task use copy of index key defs to protect from alter

[Vladimir Davydov]

Looks good to me.

On Thu, Mar 15, 2018 at 03:19:14PM +0300, Vladislav Shpilevoy wrote:
> Branch: http://github.com/tarantool/tarantool/tree/gh-3229-space-format-enables-optional-v2
> Issue: https://github.com/tarantool/tarantool/issues/3229
> 
> A vinyl space can be altered in such a way, that key definitions
> of indexes are not changed, but comparators do. It is because
> space format reset can make some indexed fields optional.
> 
> To be able update key definitions in place, they must not be used
> in a worker thread. So lets copy key_defs for a worker, and
> update index key definitions in place.
> 
> An alternative is key_defs reference counting, but there is open
> questions what to do in key_defs in mems, ranges, iterators,
> runs, slices. Now lets do a hotfix of a crash, and then
> refactoring.
> 
> Vladislav Shpilevoy (2):
>   Do not mix box_key_def_delete and free to delete key_def
>   vinyl: in a task use copy of index key defs to protect from alter
> 
>  src/box/alter.cc                |  3 ++-
>  src/box/key_def.cc              | 10 ++++++++--
>  src/box/key_def.h               |  7 +++++++
>  src/box/schema.cc               |  4 ++--
>  src/box/vinyl.c                 | 10 ++++++++--
>  src/box/vy_index.c              |  8 ++++----
>  src/box/vy_scheduler.c          | 25 ++++++++++++++++++++++---
>  test/engine/ddl.result          |  2 +-
>  test/engine/ddl.test.lua        |  2 +-
>  test/unit/vy_iterators_helper.c |  2 +-
>  test/unit/vy_mem.c              |  4 ++--
>  test/unit/vy_point_lookup.c     |  2 +-
>  test/unit/vy_write_iterator.c   |  2 +-
>  13 files changed, 60 insertions(+), 21 deletions(-)

Re: [PATCH v2 0/2] vinyl: in a task use copy of index key defs to protect from alter

[Vladimir Davydov]

Looks good to me.

On Thu, Mar 15, 2018 at 03:19:14PM +0300, Vladislav Shpilevoy wrote:
> Branch: http://github.com/tarantool/tarantool/tree/gh-3229-space-format-enables-optional-v2
> Issue: https://github.com/tarantool/tarantool/issues/3229
> 
> A vinyl space can be altered in such a way, that key definitions
> of indexes are not changed, but comparators do. It is because
> space format reset can make some indexed fields optional.
> 
> To be able update key definitions in place, they must not be used
> in a worker thread. So lets copy key_defs for a worker, and
> update index key definitions in place.
> 
> An alternative is key_defs reference counting, but there is open
> questions what to do in key_defs in mems, ranges, iterators,
> runs, slices. Now lets do a hotfix of a crash, and then
> refactoring.
> 
> Vladislav Shpilevoy (2):
>   Do not mix box_key_def_delete and free to delete key_def
>   vinyl: in a task use copy of index key defs to protect from alter
> 
>  src/box/alter.cc                |  3 ++-
>  src/box/key_def.cc              | 10 ++++++++--
>  src/box/key_def.h               |  7 +++++++
>  src/box/schema.cc               |  4 ++--
>  src/box/vinyl.c                 | 10 ++++++++--
>  src/box/vy_index.c              |  8 ++++----
>  src/box/vy_scheduler.c          | 25 ++++++++++++++++++++++---
>  test/engine/ddl.result          |  2 +-
>  test/engine/ddl.test.lua        |  2 +-
>  test/unit/vy_iterators_helper.c |  2 +-
>  test/unit/vy_mem.c              |  4 ++--
>  test/unit/vy_point_lookup.c     |  2 +-
>  test/unit/vy_write_iterator.c   |  2 +-
>  13 files changed, 60 insertions(+), 21 deletions(-)

Re: [PATCH v2 0/2] vinyl: in a task use copy of index key defs to protect from alter

[v.shpilevoy]

The patch will be auto-pushed in 4 days, if Kostja would neither ack nor reject nor review it.

> 20 марта 2018 г., в 16:13, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> Looks good to me.
> 
> On Thu, Mar 15, 2018 at 03:19:14PM +0300, Vladislav Shpilevoy wrote:
>> Branch: http://github.com/tarantool/tarantool/tree/gh-3229-space-format-enables-optional-v2
>> Issue: https://github.com/tarantool/tarantool/issues/3229
>> 
>> A vinyl space can be altered in such a way, that key definitions
>> of indexes are not changed, but comparators do. It is because
>> space format reset can make some indexed fields optional.
>> 
>> To be able update key definitions in place, they must not be used
>> in a worker thread. So lets copy key_defs for a worker, and
>> update index key definitions in place.
>> 
>> An alternative is key_defs reference counting, but there is open
>> questions what to do in key_defs in mems, ranges, iterators,
>> runs, slices. Now lets do a hotfix of a crash, and then
>> refactoring.
>> 
>> Vladislav Shpilevoy (2):
>>  Do not mix box_key_def_delete and free to delete key_def
>>  vinyl: in a task use copy of index key defs to protect from alter
>> 
>> src/box/alter.cc                |  3 ++-
>> src/box/key_def.cc              | 10 ++++++++--
>> src/box/key_def.h               |  7 +++++++
>> src/box/schema.cc               |  4 ++--
>> src/box/vinyl.c                 | 10 ++++++++--
>> src/box/vy_index.c              |  8 ++++----
>> src/box/vy_scheduler.c          | 25 ++++++++++++++++++++++---
>> test/engine/ddl.result          |  2 +-
>> test/engine/ddl.test.lua        |  2 +-
>> test/unit/vy_iterators_helper.c |  2 +-
>> test/unit/vy_mem.c              |  4 ++--
>> test/unit/vy_point_lookup.c     |  2 +-
>> test/unit/vy_write_iterator.c   |  2 +-
>> 13 files changed, 60 insertions(+), 21 deletions(-)