[PATCH 00/12] vinyl: allow to extend key def of non-empty index

[PATCH 00/12] vinyl: allow to extend key def of non-empty index

[Vladimir Davydov]

The goal of this patch set is to allow altering non-empty vinyl
indexes in case the new key definition is fully compatible with the
old one (i.e. has the same key parts, but more generic field types).
This is an important step toward closing #1653 [1].

The code is available at GitHub, see [2]. Note this patch set is based
on top of the patch that removes vinyl upsert_format, see [3, 4].

[1] https://github.com/tarantool/tarantool/issues/1653
[2] https://github.com/tarantool/tarantool/commits/vy-allow-to-extend-key-def
[3] https://github.com/tarantool/tarantool/commits/vy-zap-upsert-format
[4] https://www.freelists.org/post/tarantool-patches/PATCH-02-vinyl-zap-upsert-format

Vladimir Davydov (12):
  index: add commit_modify virtual method
  alter: require rebuild of all secondary vinyl indexes if pk changes
  alter: do not rebuild secondary indexes on compatible pk changes
  space: make space_swap_index method virtual
  vinyl: do not reallocate tuple formats on alter
  vinyl: zap vy_lsm_validate_formats
  vinyl: zap vy_mem_update_formats
  vinyl: remove pointless is_nullable initialization for disk_format
  vinyl: use source tuple format when copying field map
  vinyl: rename vy_log_record::commit_lsn to create_lsn
  vinyl: do not write VY_LOG_DUMP_LSM record to snapshot
  vinyl: allow to modify key definition if it does not require rebuild

 src/box/alter.cc                  |  28 +++++---
 src/box/index.cc                  |  10 +++
 src/box/index.h                   |  28 ++++++++
 src/box/key_def.cc                |   9 +++
 src/box/key_def.h                 |   7 ++
 src/box/memtx_bitset.c            |   2 +
 src/box/memtx_hash.c              |   2 +
 src/box/memtx_rtree.c             |   2 +
 src/box/memtx_space.c             |   1 +
 src/box/memtx_tree.c              |  10 +++
 src/box/space.c                   |   9 ++-
 src/box/space.h                   |  33 ++++++---
 src/box/sysview_engine.c          |   1 +
 src/box/sysview_index.c           |   2 +
 src/box/tuple_format.c            |  18 -----
 src/box/tuple_format.h            |   8 ---
 src/box/vinyl.c                   | 144 ++++++++++++++++++++++----------------
 src/box/vy_log.c                  | 124 ++++++++++++++++++++++++--------
 src/box/vy_log.h                  |  36 ++++++++--
 src/box/vy_lsm.c                  |  49 +++----------
 src/box/vy_lsm.h                  |  17 ++---
 src/box/vy_mem.c                  |  15 +---
 src/box/vy_point_lookup.c         |   3 +-
 src/box/vy_stmt.c                 |  18 ++---
 src/box/vy_stmt.h                 |   6 +-
 src/box/vy_upsert.c               |   4 +-
 test/engine/ddl.result            | 113 +++++++++++++++++++++++++++++-
 test/engine/ddl.test.lua          |  35 ++++++++-
 test/engine/replica_join.result   |  26 ++++++-
 test/engine/replica_join.test.lua |   2 +
 test/engine/truncate.result       |   2 +-
 test/engine/truncate.test.lua     |   2 +-
 test/vinyl/ddl.result             |  78 ++++++---------------
 test/vinyl/ddl.test.lua           |  30 ++------
 test/vinyl/gh.result              |   2 +-
 test/vinyl/layout.result          |  75 +++++++++-----------
 test/vinyl/layout.test.lua        |   4 +-
 37 files changed, 595 insertions(+), 360 deletions(-)

-- 
2.11.0

[PATCH 01/12] index: add commit_modify virtual method

[Vladimir Davydov]

The new method is called after successful update of index definition.
It is passed the signature of the WAL record that committed the
operation. It will be used by Vinyl to update key definition in vylog.
---
 src/box/alter.cc        |  9 +++++++++
 src/box/index.cc        |  5 +++++
 src/box/index.h         | 15 +++++++++++++++
 src/box/memtx_bitset.c  |  1 +
 src/box/memtx_hash.c    |  1 +
 src/box/memtx_rtree.c   |  1 +
 src/box/memtx_tree.c    |  1 +
 src/box/sysview_index.c |  1 +
 src/box/vinyl.c         |  1 +
 9 files changed, 35 insertions(+)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 6e5d6e52..eb548e0b 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1109,6 +1109,7 @@ public:
 	struct index_def *old_index_def;
 	virtual void alter_def(struct alter_space *alter);
 	virtual void alter(struct alter_space *alter);
+	virtual void commit(struct alter_space *alter, int64_t lsn);
 	virtual void rollback(struct alter_space *alter);
 	virtual ~ModifyIndex();
 };
@@ -1142,6 +1143,14 @@ ModifyIndex::alter(struct alter_space *alter)
 }
 
 void
+ModifyIndex::commit(struct alter_space *alter, int64_t signature)
+{
+	struct index *new_index = space_index(alter->new_space,
+					      new_index_def->iid);
+	index_commit_modify(new_index, signature);
+}
+
+void
 ModifyIndex::rollback(struct alter_space *alter)
 {
 	assert(old_index_def->iid == new_index_def->iid);
diff --git a/src/box/index.cc b/src/box/index.cc
index d02cbbc5..11a6683d 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -546,6 +546,11 @@ generic_index_abort_create(struct index *)
 }
 
 void
+generic_index_commit_modify(struct index *, int64_t)
+{
+}
+
+void
 generic_index_commit_drop(struct index *)
 {
 }
diff --git a/src/box/index.h b/src/box/index.h
index b8b0feb2..2e43d999 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -356,6 +356,14 @@ struct index_vtab {
 	 */
 	void (*abort_create)(struct index *index);
 	/**
+	 * Called after WAL write to comit index definition update.
+	 * Must not fail.
+	 *
+	 * @signature is the LSN that was assigned to the row
+	 * that modified the index.
+	 */
+	void (*commit_modify)(struct index *index, int64_t signature);
+	/**
 	 * Called after WAL write to commit index drop.
 	 * Must not fail.
 	 */
@@ -480,6 +488,12 @@ index_abort_create(struct index *index)
 }
 
 static inline void
+index_commit_modify(struct index *index, int64_t signature)
+{
+	index->vtab->commit_modify(index, signature);
+}
+
+static inline void
 index_commit_drop(struct index *index)
 {
 	index->vtab->commit_drop(index);
@@ -599,6 +613,7 @@ index_end_build(struct index *index)
  */
 void generic_index_commit_create(struct index *, int64_t);
 void generic_index_abort_create(struct index *);
+void generic_index_commit_modify(struct index *, int64_t);
 void generic_index_commit_drop(struct index *);
 void generic_index_update_def(struct index *);
 ssize_t generic_index_size(struct index *);
diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
index c2685a65..0e546217 100644
--- a/src/box/memtx_bitset.c
+++ b/src/box/memtx_bitset.c
@@ -458,6 +458,7 @@ static const struct index_vtab memtx_bitset_index_vtab = {
 	/* .destroy = */ memtx_bitset_index_destroy,
 	/* .commit_create = */ generic_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .size = */ memtx_bitset_index_size,
diff --git a/src/box/memtx_hash.c b/src/box/memtx_hash.c
index 4dab23cd..9c72af5d 100644
--- a/src/box/memtx_hash.c
+++ b/src/box/memtx_hash.c
@@ -382,6 +382,7 @@ static const struct index_vtab memtx_hash_index_vtab = {
 	/* .destroy = */ memtx_hash_index_destroy,
 	/* .commit_create = */ generic_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ memtx_hash_index_update_def,
 	/* .size = */ memtx_hash_index_size,
diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index dea68897..373d658e 100644
--- a/src/box/memtx_rtree.c
+++ b/src/box/memtx_rtree.c
@@ -296,6 +296,7 @@ static const struct index_vtab memtx_rtree_index_vtab = {
 	/* .destroy = */ memtx_rtree_index_destroy,
 	/* .commit_create = */ generic_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .size = */ memtx_rtree_index_size,
diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c
index 06b74e11..7178a4bc 100644
--- a/src/box/memtx_tree.c
+++ b/src/box/memtx_tree.c
@@ -583,6 +583,7 @@ static const struct index_vtab memtx_tree_index_vtab = {
 	/* .destroy = */ memtx_tree_index_destroy,
 	/* .commit_create = */ generic_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ memtx_tree_index_update_def,
 	/* .size = */ memtx_tree_index_size,
diff --git a/src/box/sysview_index.c b/src/box/sysview_index.c
index be17fd32..e2b2fb96 100644
--- a/src/box/sysview_index.c
+++ b/src/box/sysview_index.c
@@ -162,6 +162,7 @@ static const struct index_vtab sysview_index_vtab = {
 	/* .destroy = */ sysview_index_destroy,
 	/* .commit_create = */ generic_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .size = */ generic_index_size,
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index ce51fdd0..cebe1615 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3935,6 +3935,7 @@ static const struct index_vtab vinyl_index_vtab = {
 	/* .destroy = */ vinyl_index_destroy,
 	/* .commit_create = */ vinyl_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
+	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ vinyl_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .size = */ vinyl_index_size,
-- 
2.11.0

[PATCH 02/12] alter: require rebuild of all secondary vinyl indexes if pk changes

[Vladimir Davydov]

If the primary key is modified, we schedule rebuild of all non-unique
(including nullable) secondary TREE indexes. This is valid for memtx,
but is not quite right for vinyl. For vinyl we have to rebuild all
secondary indexes, because they are all non-clustered (i.e. point to
tuples via primary key parts). This doesn't result in any bugs for now,
because rebuild of vinyl indexes is not supported, but hopefully this is
going to change soon. So let's introduce a new virtual index method,
index_vtab::depends_on_pk, which returns true iff the index needs to be
updated if the primary key changes, and define this new method for vinyl
and memtx TREE indexes.
---
 src/box/alter.cc        | 11 ++++-------
 src/box/index.cc        |  5 +++++
 src/box/index.h         | 13 +++++++++++++
 src/box/memtx_bitset.c  |  1 +
 src/box/memtx_hash.c    |  1 +
 src/box/memtx_rtree.c   |  1 +
 src/box/memtx_tree.c    |  9 +++++++++
 src/box/sysview_index.c |  1 +
 src/box/vinyl.c         | 12 ++++++++++++
 9 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index eb548e0b..7f130297 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1100,7 +1100,7 @@ public:
 	                         old_index_def->key_def->part_count) != 0) {
 	                /*
 	                 * Primary parts have been changed -
-	                 * update non-unique secondary indexes.
+	                 * update secondary indexes.
 	                 */
 	                alter->pk_def = new_index_def->key_def;
 	        }
@@ -1411,9 +1411,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 		struct index_def *old_def = old_index->def;
 		struct index_def *new_def;
 		uint32_t min_field_count = alter->new_min_field_count;
-		if ((old_def->opts.is_unique &&
-		     !old_def->key_def->is_nullable) ||
-		    old_def->type != TREE || alter->pk_def == NULL) {
+		if (alter->pk_def == NULL || !index_depends_on_pk(old_index)) {
 			if (is_min_field_count_changed) {
 				new_def = index_def_dup(old_def);
 				index_def_update_optionality(new_def,
@@ -1425,9 +1423,8 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 			continue;
 		}
 		/*
-		 * Rebuild non-unique secondary keys along with
-		 * the primary, since primary key parts have
-		 * changed.
+		 * Rebuild secondary indexes that depend on the
+		 * primary key since primary key parts have changed.
 		 */
 		new_def = index_def_new(old_def->space_id, old_def->iid,
 					old_def->name, strlen(old_def->name),
diff --git a/src/box/index.cc b/src/box/index.cc
index 11a6683d..6cd04833 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -560,6 +560,11 @@ generic_index_update_def(struct index *)
 {
 }
 
+bool generic_index_depends_on_pk(struct index *)
+{
+	return false;
+}
+
 ssize_t
 generic_index_size(struct index *index)
 {
diff --git a/src/box/index.h b/src/box/index.h
index 2e43d999..f19fbd16 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -373,6 +373,12 @@ struct index_vtab {
 	 * require index rebuild.
 	 */
 	void (*update_def)(struct index *);
+	/**
+	 * Return true if the index depends on the primary
+	 * key definition and hence needs to be updated if
+	 * the primary key is modified.
+	 */
+	bool (*depends_on_pk)(struct index *);
 
 	ssize_t (*size)(struct index *);
 	ssize_t (*bsize)(struct index *);
@@ -505,6 +511,12 @@ index_update_def(struct index *index)
 	index->vtab->update_def(index);
 }
 
+static inline bool
+index_depends_on_pk(struct index *index)
+{
+	return index->vtab->depends_on_pk(index);
+}
+
 static inline ssize_t
 index_size(struct index *index)
 {
@@ -616,6 +628,7 @@ void generic_index_abort_create(struct index *);
 void generic_index_commit_modify(struct index *, int64_t);
 void generic_index_commit_drop(struct index *);
 void generic_index_update_def(struct index *);
+bool generic_index_depends_on_pk(struct index *);
 ssize_t generic_index_size(struct index *);
 int generic_index_min(struct index *, const char *, uint32_t, struct tuple **);
 int generic_index_max(struct index *, const char *, uint32_t, struct tuple **);
diff --git a/src/box/memtx_bitset.c b/src/box/memtx_bitset.c
index 0e546217..e66247ee 100644
--- a/src/box/memtx_bitset.c
+++ b/src/box/memtx_bitset.c
@@ -461,6 +461,7 @@ static const struct index_vtab memtx_bitset_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
+	/* .depends_on_pk = */ generic_index_depends_on_pk,
 	/* .size = */ memtx_bitset_index_size,
 	/* .bsize = */ memtx_bitset_index_bsize,
 	/* .min = */ generic_index_min,
diff --git a/src/box/memtx_hash.c b/src/box/memtx_hash.c
index 9c72af5d..c4081edc 100644
--- a/src/box/memtx_hash.c
+++ b/src/box/memtx_hash.c
@@ -385,6 +385,7 @@ static const struct index_vtab memtx_hash_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ memtx_hash_index_update_def,
+	/* .depends_on_pk = */ generic_index_depends_on_pk,
 	/* .size = */ memtx_hash_index_size,
 	/* .bsize = */ memtx_hash_index_bsize,
 	/* .min = */ generic_index_min,
diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c
index 373d658e..7cd3ac30 100644
--- a/src/box/memtx_rtree.c
+++ b/src/box/memtx_rtree.c
@@ -299,6 +299,7 @@ static const struct index_vtab memtx_rtree_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
+	/* .depends_on_pk = */ generic_index_depends_on_pk,
 	/* .size = */ memtx_rtree_index_size,
 	/* .bsize = */ memtx_rtree_index_bsize,
 	/* .min = */ generic_index_min,
diff --git a/src/box/memtx_tree.c b/src/box/memtx_tree.c
index 7178a4bc..a06b590d 100644
--- a/src/box/memtx_tree.c
+++ b/src/box/memtx_tree.c
@@ -316,6 +316,14 @@ memtx_tree_index_update_def(struct index *base)
 	index->tree.arg = memtx_tree_index_cmp_def(index);
 }
 
+static bool
+memtx_tree_index_depends_on_pk(struct index *base)
+{
+	struct index_def *def = base->def;
+	/* See comment to memtx_tree_index_cmp_def(). */
+	return !def->opts.is_unique || def->key_def->is_nullable;
+}
+
 static ssize_t
 memtx_tree_index_size(struct index *base)
 {
@@ -586,6 +594,7 @@ static const struct index_vtab memtx_tree_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ memtx_tree_index_update_def,
+	/* .depends_on_pk = */ memtx_tree_index_depends_on_pk,
 	/* .size = */ memtx_tree_index_size,
 	/* .bsize = */ memtx_tree_index_bsize,
 	/* .min = */ generic_index_min,
diff --git a/src/box/sysview_index.c b/src/box/sysview_index.c
index e2b2fb96..72961401 100644
--- a/src/box/sysview_index.c
+++ b/src/box/sysview_index.c
@@ -165,6 +165,7 @@ static const struct index_vtab sysview_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ generic_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
+	/* .depends_on_pk = */ generic_index_depends_on_pk,
 	/* .size = */ generic_index_size,
 	/* .bsize = */ sysview_index_bsize,
 	/* .min = */ generic_index_min,
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index cebe1615..1afc6664 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -906,6 +906,17 @@ vinyl_index_commit_drop(struct index *index)
 	vy_log_tx_try_commit();
 }
 
+static bool
+vinyl_index_depends_on_pk(struct index *index)
+{
+	(void)index;
+	/*
+	 * All secondary Vinyl indexes are non-clustered and hence
+	 * have to be updated if the primary key is modified.
+	 */
+	return true;
+}
+
 static void
 vinyl_init_system_space(struct space *space)
 {
@@ -3938,6 +3949,7 @@ static const struct index_vtab vinyl_index_vtab = {
 	/* .commit_modify = */ generic_index_commit_modify,
 	/* .commit_drop = */ vinyl_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
+	/* .depends_on_pk = */ vinyl_index_depends_on_pk,
 	/* .size = */ vinyl_index_size,
 	/* .bsize = */ vinyl_index_bsize,
 	/* .min = */ generic_index_min,
-- 
2.11.0

[PATCH 03/12] alter: do not rebuild secondary indexes on compatible pk changes

[Vladimir Davydov]

If the new cmp_def of a secondary index is compatible with the old one
after the primary key parts have changed, we don't need to rebuild it,
we just need to update its definition.
---
 src/box/alter.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 7f130297..174d53fa 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1432,7 +1432,13 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 					old_def->key_def, alter->pk_def);
 		index_def_update_optionality(new_def, min_field_count);
 		auto guard = make_scoped_guard([=] { index_def_delete(new_def); });
-		(void) new RebuildIndex(alter, new_def, old_def);
+		if (key_part_check_compatibility(old_def->cmp_def->parts,
+						 old_def->cmp_def->part_count,
+						 new_def->cmp_def->parts,
+						 new_def->cmp_def->part_count))
+			(void) new ModifyIndex(alter, new_def, old_def);
+		else
+			(void) new RebuildIndex(alter, new_def, old_def);
 		guard.is_active = false;
 	}
 }
-- 
2.11.0

[PATCH 04/12] space: make space_swap_index method virtual

[Vladimir Davydov]

This function is called by MoveIndex and ModifyIndex ALTER operations,
i.e. when the index definition is not changed at all or is extended.
Making this method virtual will allow to avoid reallocation of vinyl
formats in vinyl_space_commit_alter().
---
 src/box/memtx_space.c    |  1 +
 src/box/space.c          |  9 ++++-----
 src/box/space.h          | 33 ++++++++++++++++++++++++---------
 src/box/sysview_engine.c |  1 +
 src/box/vinyl.c          |  1 +
 5 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index c7e58946..e0fa3e2c 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -895,6 +895,7 @@ static const struct space_vtab memtx_space_vtab = {
 	/* .drop_primary_key = */ memtx_space_drop_primary_key,
 	/* .check_format  = */ memtx_space_check_format,
 	/* .build_secondary_key = */ memtx_space_build_secondary_key,
+	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ memtx_space_prepare_alter,
 	/* .commit_alter = */ memtx_space_commit_alter,
 };
diff --git a/src/box/space.c b/src/box/space.c
index cc6cbeda..02a97926 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -227,12 +227,11 @@ space_index_key_def(struct space *space, uint32_t id)
 }
 
 void
-space_swap_index(struct space *lhs, struct space *rhs,
-		 uint32_t lhs_id, uint32_t rhs_id)
+generic_space_swap_index(struct space *old_space, struct space *new_space,
+			 uint32_t old_index_id, uint32_t new_index_id)
 {
-	struct index *tmp = lhs->index_map[lhs_id];
-	lhs->index_map[lhs_id] = rhs->index_map[rhs_id];
-	rhs->index_map[rhs_id] = tmp;
+	SWAP(old_space->index_map[old_index_id],
+	     new_space->index_map[new_index_id]);
 }
 
 void
diff --git a/src/box/space.h b/src/box/space.h
index 65f1531d..cf1be1e9 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -104,6 +104,13 @@ struct space_vtab {
 				   struct space *new_space,
 				   struct index *new_index);
 	/**
+	 * Exchange two index objects in two spaces. Used
+	 * to update a space with a newly built index, while
+	 * making sure the old index doesn't leak.
+	 */
+	void (*swap_index)(struct space *old_space, struct space *new_space,
+			   uint32_t old_index_id, uint32_t new_index_id);
+	/**
 	 * Notify the engine about the changed space,
 	 * before it's done, to prepare 'new_space' object.
 	 */
@@ -284,6 +291,14 @@ int
 space_execute_dml(struct space *space, struct txn *txn,
 		  struct request *request, struct tuple **result);
 
+/**
+ * Generic implementation of space_vtab::swap_index
+ * that simply swaps the two indexes in index maps.
+ */
+void
+generic_space_swap_index(struct space *old_space, struct space *new_space,
+			 uint32_t old_index_id, uint32_t new_index_id);
+
 static inline void
 init_system_space(struct space *space)
 {
@@ -330,6 +345,15 @@ space_build_secondary_key(struct space *old_space,
 						    new_space, new_index);
 }
 
+static inline void
+space_swap_index(struct space *old_space, struct space *new_space,
+		 uint32_t old_index_id, uint32_t new_index_id)
+{
+	assert(old_space->vtab == new_space->vtab);
+	return new_space->vtab->swap_index(old_space, new_space,
+					   old_index_id, new_index_id);
+}
+
 static inline int
 space_prepare_alter(struct space *old_space, struct space *new_space)
 {
@@ -374,15 +398,6 @@ space_delete(struct space *space);
 void
 space_dump_def(const struct space *space, struct rlist *key_list);
 
-/**
- * Exchange two index objects in two spaces. Used
- * to update a space with a newly built index, while
- * making sure the old index doesn't leak.
- */
-void
-space_swap_index(struct space *lhs, struct space *rhs,
-		 uint32_t lhs_id, uint32_t rhs_id);
-
 /** Rebuild index map in a space after a series of swap index. */
 void
 space_fill_index_map(struct space *space);
diff --git a/src/box/sysview_engine.c b/src/box/sysview_engine.c
index f6122645..fd4ebe5b 100644
--- a/src/box/sysview_engine.c
+++ b/src/box/sysview_engine.c
@@ -185,6 +185,7 @@ static const struct space_vtab sysview_space_vtab = {
 	/* .drop_primary_key = */ sysview_space_drop_primary_key,
 	/* .check_format = */ sysview_space_check_format,
 	/* .build_secondary_key = */ sysview_space_build_secondary_key,
+	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ sysview_space_prepare_alter,
 	/* .commit_alter = */ sysview_space_commit_alter,
 };
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 1afc6664..fc37283a 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -3938,6 +3938,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .drop_primary_key = */ vinyl_space_drop_primary_key,
 	/* .check_format = */ vinyl_space_check_format,
 	/* .build_secondary_key = */ vinyl_space_build_secondary_key,
+	/* .swap_index = */ generic_space_swap_index,
 	/* .prepare_alter = */ vinyl_space_prepare_alter,
 	/* .commit_alter = */ vinyl_space_commit_alter,
 };
-- 
2.11.0

[PATCH 05/12] vinyl: do not reallocate tuple formats on alter

[Vladimir Davydov]

We create new formats for all indexes of the new space in
vinyl_space_commit_alter() while we don't actually need to
do this, because the new formats have already been created
by vy_lsm_new() - all we need to do is reuse them somehow.

This patch does the trick: it implements the swap_index()
space virtual method for vinyl so that it swaps tuple formats
between the old and new spaces.
---
 src/box/vinyl.c | 58 ++++++++++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 36 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index fc37283a..f3cef52f 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1024,30 +1024,9 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 
 	struct tuple_format *new_format = new_space->format;
 	struct vy_lsm *pk = vy_lsm(new_space->index[0]);
-	struct index_def *new_index_def = space_index_def(new_space, 0);
-
 	assert(pk->pk == NULL);
 
-	/* Update the format with column mask. */
-	struct tuple_format *format =
-		vy_tuple_format_new_with_colmask(new_format);
-	if (format == NULL)
-		goto fail;
-
-	/* Set possibly changed opts. */
-	pk->opts = new_index_def->opts;
 	pk->check_is_unique = true;
-
-	/* Set new formats. */
-	tuple_format_unref(pk->disk_format);
-	tuple_format_unref(pk->mem_format);
-	tuple_format_unref(pk->mem_format_with_colmask);
-	pk->disk_format = new_format;
-	tuple_format_ref(new_format);
-	pk->mem_format_with_colmask = format;
-	tuple_format_ref(format);
-	pk->mem_format = new_format;
-	tuple_format_ref(new_format);
 	vy_lsm_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);
@@ -1057,15 +1036,7 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 		vy_lsm_unref(lsm->pk);
 		vy_lsm_ref(pk);
 		lsm->pk = pk;
-		new_index_def = space_index_def(new_space, i);
-		lsm->opts = new_index_def->opts;
 		lsm->check_is_unique = lsm->opts.is_unique;
-		tuple_format_unref(lsm->mem_format_with_colmask);
-		tuple_format_unref(lsm->mem_format);
-		lsm->mem_format_with_colmask = pk->mem_format_with_colmask;
-		lsm->mem_format = pk->mem_format;
-		tuple_format_ref(lsm->mem_format_with_colmask);
-		tuple_format_ref(lsm->mem_format);
 		key_def_update_optionality(lsm->key_def,
 					   new_format->min_field_count);
 		key_def_update_optionality(lsm->cmp_def,
@@ -1093,12 +1064,27 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 			}
 		}
 	}
-	return;
-fail:
-	/* FIXME: space_vtab::commit_alter() must not fail. */
-	diag_log();
-	unreachable();
-	panic("failed to alter space");
+}
+
+static void
+vinyl_space_swap_index(struct space *old_space, struct space *new_space,
+		       uint32_t old_index_id, uint32_t new_index_id)
+{
+	struct vy_lsm *old_lsm = vy_lsm(old_space->index_map[old_index_id]);
+	struct vy_lsm *new_lsm = vy_lsm(new_space->index_map[new_index_id]);
+
+	/*
+	 * Swap the two indexes between the two spaces,
+	 * but leave tuple formats.
+	 */
+	generic_space_swap_index(old_space, new_space,
+				 old_index_id, new_index_id);
+
+	SWAP(old_lsm->mem_format, new_lsm->mem_format);
+	SWAP(old_lsm->mem_format_with_colmask,
+	     new_lsm->mem_format_with_colmask);
+	SWAP(old_lsm->disk_format, new_lsm->disk_format);
+	SWAP(old_lsm->opts, new_lsm->opts);
 }
 
 static int
@@ -3938,7 +3924,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .drop_primary_key = */ vinyl_space_drop_primary_key,
 	/* .check_format = */ vinyl_space_check_format,
 	/* .build_secondary_key = */ vinyl_space_build_secondary_key,
-	/* .swap_index = */ generic_space_swap_index,
+	/* .swap_index = */ vinyl_space_swap_index,
 	/* .prepare_alter = */ vinyl_space_prepare_alter,
 	/* .commit_alter = */ vinyl_space_commit_alter,
 };
-- 
2.11.0

[PATCH 06/12] vinyl: zap vy_lsm_validate_formats

[Vladimir Davydov]

We allocate index formats in the only place, vy_lsm_new, so there's no
point in this debug-only check anymore.
---
 src/box/vinyl.c  |  2 --
 src/box/vy_lsm.c | 25 -------------------------
 src/box/vy_lsm.h |  7 -------
 3 files changed, 34 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index f3cef52f..145fde70 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1027,7 +1027,6 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 	assert(pk->pk == NULL);
 
 	pk->check_is_unique = true;
-	vy_lsm_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);
 
@@ -1041,7 +1040,6 @@ vinyl_space_commit_alter(struct space *old_space, struct space *new_space)
 					   new_format->min_field_count);
 		key_def_update_optionality(lsm->cmp_def,
 					   new_format->min_field_count);
-		vy_lsm_validate_formats(lsm);
 	}
 
 	/*
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 4b2c3ef3..6cb722f4 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -52,30 +52,6 @@
 #include "vy_upsert.h"
 #include "vy_read_set.h"
 
-void
-vy_lsm_validate_formats(const struct vy_lsm *lsm)
-{
-	(void) lsm;
-	assert(lsm->disk_format != NULL);
-	assert(lsm->mem_format != NULL);
-	assert(lsm->mem_format_with_colmask != NULL);
-	uint32_t index_field_count = lsm->mem_format->index_field_count;
-	(void) index_field_count;
-	if (lsm->index_id == 0) {
-		assert(lsm->disk_format == lsm->mem_format);
-		assert(lsm->disk_format->index_field_count ==
-		       index_field_count);
-		assert(lsm->mem_format_with_colmask->index_field_count ==
-		       index_field_count);
-	} else {
-		assert(lsm->disk_format != lsm->mem_format);
-		assert(lsm->disk_format->index_field_count <=
-		       index_field_count);
-	}
-	assert(lsm->mem_format_with_colmask->index_field_count ==
-	       index_field_count);
-}
-
 int
 vy_lsm_env_create(struct vy_lsm_env *env, const char *path,
 		  int64_t *p_generation,
@@ -232,7 +208,6 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 	vy_lsm_read_set_new(&lsm->read_set);
 
 	lsm_env->lsm_count++;
-	vy_lsm_validate_formats(lsm);
 	return lsm;
 
 fail_mem:
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index ab54da75..f5862a52 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -286,13 +286,6 @@ struct vy_lsm {
 	vy_lsm_read_set_t read_set;
 };
 
-/**
- * Assert if an LSM tree formats are inconsistent.
- * @param lsm LSM tree to validate.
- */
-void
-vy_lsm_validate_formats(const struct vy_lsm *lsm);
-
 /** Return LSM tree name. Used for logging. */
 const char *
 vy_lsm_name(struct vy_lsm *lsm);
-- 
2.11.0

[PATCH 07/12] vinyl: zap vy_mem_update_formats

[Vladimir Davydov]

A piece of code left from the inglorious past, which doesn't even have
a forward declaration, let alone used anywhere. Remove it.
---
 src/box/vy_mem.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 68abf5bc..9aaabf0b 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -124,19 +124,6 @@ vy_mem_new(struct vy_mem_env *env, int64_t generation,
 }
 
 void
-vy_mem_update_formats(struct vy_mem *mem, struct tuple_format *new_format,
-		      struct tuple_format *new_format_with_colmask)
-{
-	assert(mem->count.rows == 0);
-	tuple_format_unref(mem->format);
-	tuple_format_unref(mem->format_with_colmask);
-	mem->format = new_format;
-	mem->format_with_colmask = new_format_with_colmask;
-	tuple_format_ref(mem->format);
-	tuple_format_ref(mem->format_with_colmask);
-}
-
-void
 vy_mem_delete(struct vy_mem *index)
 {
 	index->env->tree_extent_size -= index->tree_extent_size;
-- 
2.11.0

[PATCH 08/12] vinyl: remove pointless is_nullable initialization for disk_format

[Vladimir Davydov]

space->format and cmp_def must be compatible, i.e. space->format has
is_nullable flag set for a field iff it is set for all key parts
indexing this field. Therefore there's no point to set is_nullable for
disk_format as it must have been initialized by tuple_format_create().
Remove the pointless loop.

Also, while we are at it, fix the minor memory leak - disk_format is
referenced twice for the primary key.
---
 src/box/vy_lsm.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 6cb722f4..88de1e61 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -147,18 +147,12 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 		 * definitions as well as space->format tuples.
 		 */
 		lsm->disk_format = format;
-		tuple_format_ref(format);
 	} else {
 		lsm->disk_format = tuple_format_new(&vy_tuple_format_vtab,
 						    &cmp_def, 1, 0, NULL, 0,
 						    NULL);
 		if (lsm->disk_format == NULL)
 			goto fail_format;
-		for (uint32_t i = 0; i < cmp_def->part_count; ++i) {
-			uint32_t fieldno = cmp_def->parts[i].fieldno;
-			lsm->disk_format->fields[fieldno].is_nullable =
-				format->fields[fieldno].is_nullable;
-		}
 	}
 	tuple_format_ref(lsm->disk_format);
 
@@ -167,11 +161,10 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 			vy_tuple_format_new_with_colmask(format);
 		if (lsm->mem_format_with_colmask == NULL)
 			goto fail_mem_format_with_colmask;
-		tuple_format_ref(lsm->mem_format_with_colmask);
 	} else {
 		lsm->mem_format_with_colmask = pk->mem_format_with_colmask;
-		tuple_format_ref(lsm->mem_format_with_colmask);
 	}
+	tuple_format_ref(lsm->mem_format_with_colmask);
 
 	if (vy_lsm_stat_create(&lsm->stat) != 0)
 		goto fail_stat;
-- 
2.11.0

[PATCH 09/12] vinyl: use source tuple format when copying field map

[Vladimir Davydov]

There are two functions in vy_stmt.c that blindly copy tuple field map,
vy_stmt_dup() and vy_stmt_replace_from_upsert(). Both these functions
take a tuple format to use for the new statement and require this format
to be the same as the source tuple format in terms of fields definition,
otherwise they'll just crash. The only reason why we did that is that
back when these functions were written we used a separate format for
UPSERT statements so we needed this extra argument for creating a
REPLACE from UPSERT. Now it's not needed, and we can use the source
tuple format instead. Moreover, passing the current tuple format to any
of those functions is even harmful, because tuple format can be extended
by ALTER, in which case these functions will crash if called on a
statement created before ALTER. That being said, let's drop the tuple
format argument.
---
 src/box/tuple_format.c    | 18 ------------------
 src/box/tuple_format.h    |  8 --------
 src/box/vy_lsm.c          |  2 +-
 src/box/vy_mem.c          |  2 +-
 src/box/vy_point_lookup.c |  3 +--
 src/box/vy_stmt.c         | 18 +++++-------------
 src/box/vy_stmt.h         |  6 ++----
 src/box/vy_upsert.c       |  4 ++--
 8 files changed, 12 insertions(+), 49 deletions(-)

diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index e458f49a..d3a7702d 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -324,24 +324,6 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
 	return true;
 }
 
-bool
-tuple_format_eq(const struct tuple_format *a, const struct tuple_format *b)
-{
-	if (a->field_map_size != b->field_map_size ||
-	    a->field_count != b->field_count)
-		return false;
-	for (uint32_t i = 0; i < a->field_count; ++i) {
-		if (a->fields[i].type != b->fields[i].type ||
-		    a->fields[i].offset_slot != b->fields[i].offset_slot)
-			return false;
-		if (a->fields[i].is_key_part != b->fields[i].is_key_part)
-			return false;
-		if (a->fields[i].is_nullable != b->fields[i].is_nullable)
-			return false;
-	}
-	return true;
-}
-
 struct tuple_format *
 tuple_format_dup(struct tuple_format *src)
 {
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index d35182df..7678b6a1 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -215,14 +215,6 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
 				       const struct tuple_format *format2);
 
 /**
- * Check that two tuple formats are identical.
- * @param a format a
- * @param b format b
- */
-bool
-tuple_format_eq(const struct tuple_format *a, const struct tuple_format *b);
-
-/**
  * Register the duplicate of the specified format.
  * @param src Original format.
  *
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 88de1e61..915ffaeb 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -822,7 +822,7 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem,
 			return;
 		}
 
-		struct tuple *dup = vy_stmt_dup(stmt, lsm->mem_format);
+		struct tuple *dup = vy_stmt_dup(stmt);
 		if (dup != NULL) {
 			lsm->env->upsert_thresh_cb(lsm, dup,
 					lsm->env->upsert_thresh_arg);
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 9aaabf0b..22cd33fa 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -279,7 +279,7 @@ vy_mem_iterator_copy_to(struct vy_mem_iterator *itr, struct tuple **ret)
 	assert(itr->curr_stmt != NULL);
 	if (itr->last_stmt)
 		tuple_unref(itr->last_stmt);
-	itr->last_stmt = vy_stmt_dup(itr->curr_stmt, tuple_format(itr->curr_stmt));
+	itr->last_stmt = vy_stmt_dup(itr->curr_stmt);
 	*ret = itr->last_stmt;
 	if (itr->last_stmt != NULL) {
 		vy_stmt_counter_acct_tuple(&itr->stat->get, *ret);
diff --git a/src/box/vy_point_lookup.c b/src/box/vy_point_lookup.c
index 0f4d75f7..0618590a 100644
--- a/src/box/vy_point_lookup.c
+++ b/src/box/vy_point_lookup.c
@@ -347,8 +347,7 @@ vy_point_lookup_apply_history(struct vy_lsm *lsm,
 		if (vy_stmt_type(node->stmt) == IPROTO_DELETE) {
 			/* Ignore terminal delete */
 		} else if (node->src_type == ITER_SRC_MEM) {
-			curr_stmt = vy_stmt_dup(node->stmt,
-						tuple_format(node->stmt));
+			curr_stmt = vy_stmt_dup(node->stmt);
 		} else {
 			curr_stmt = node->stmt;
 			tuple_ref(curr_stmt);
diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
index f65b63c5..2229eb2e 100644
--- a/src/box/vy_stmt.c
+++ b/src/box/vy_stmt.c
@@ -109,22 +109,20 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
 }
 
 struct tuple *
-vy_stmt_dup(const struct tuple *stmt, struct tuple_format *format)
+vy_stmt_dup(const struct tuple *stmt)
 {
 	/*
 	 * We don't use tuple_new() to avoid the initializing of
 	 * tuple field map. This map can be simple memcopied from
 	 * the original tuple.
 	 */
-	struct tuple *res = vy_stmt_alloc(format, stmt->bsize);
+	struct tuple *res = vy_stmt_alloc(tuple_format(stmt), stmt->bsize);
 	if (res == NULL)
 		return NULL;
 	assert(tuple_size(res) == tuple_size(stmt));
 	assert(res->data_offset == stmt->data_offset);
 	memcpy(res, stmt, tuple_size(stmt));
 	res->refs = 1;
-	res->format_id = tuple_format_id(format);
-	assert(tuple_size(res) == tuple_size(stmt));
 	return res;
 }
 
@@ -294,8 +292,7 @@ vy_stmt_new_insert(struct tuple_format *format, const char *tuple_begin,
 }
 
 struct tuple *
-vy_stmt_replace_from_upsert(struct tuple_format *replace_format,
-			    const struct tuple *upsert)
+vy_stmt_replace_from_upsert(const struct tuple *upsert)
 {
 	assert(vy_stmt_type(upsert) == IPROTO_UPSERT);
 	/* Get statement size without UPSERT operations */
@@ -304,13 +301,8 @@ vy_stmt_replace_from_upsert(struct tuple_format *replace_format,
 	assert(bsize <= upsert->bsize);
 
 	/* Copy statement data excluding UPSERT operations */
-	struct tuple_format *format = tuple_format_by_id(upsert->format_id);
-	/*
-	 * In other fields the REPLACE tuple format must equal to
-	 * the UPSERT tuple format.
-	 */
-	assert(tuple_format_eq(format, replace_format));
-	struct tuple *replace = vy_stmt_alloc(replace_format, bsize);
+	struct tuple_format *format = tuple_format(upsert);
+	struct tuple *replace = vy_stmt_alloc(format, bsize);
 	if (replace == NULL)
 		return NULL;
 	/* Copy both data and field_map. */
diff --git a/src/box/vy_stmt.h b/src/box/vy_stmt.h
index 9e73b2ca..e53f98ce 100644
--- a/src/box/vy_stmt.h
+++ b/src/box/vy_stmt.h
@@ -209,7 +209,7 @@ vy_tuple_delete(struct tuple_format *format, struct tuple *tuple);
  * @return new statement of the same type with the same data.
  */
 struct tuple *
-vy_stmt_dup(const struct tuple *stmt, struct tuple_format *format);
+vy_stmt_dup(const struct tuple *stmt);
 
 struct lsregion;
 
@@ -504,14 +504,12 @@ vy_stmt_new_upsert(struct tuple_format *format,
 /**
  * Create REPLACE statement from UPSERT statement.
  *
- * @param replace_format Format for new REPLACE statement.
  * @param upsert         Upsert statement.
  * @retval not NULL Success.
  * @retval     NULL Memory error.
  */
 struct tuple *
-vy_stmt_replace_from_upsert(struct tuple_format *replace_format,
-			    const struct tuple *upsert);
+vy_stmt_replace_from_upsert(const struct tuple *upsert);
 
 /**
  * Extract MessagePack data from the REPLACE/UPSERT statement.
diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index 2b38139a..67f9ef09 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -140,7 +140,7 @@ vy_apply_upsert(const struct tuple *new_stmt, const struct tuple *old_stmt,
 		/*
 		 * INSERT case: return new stmt.
 		 */
-		return vy_stmt_replace_from_upsert(format, new_stmt);
+		return vy_stmt_replace_from_upsert(new_stmt);
 	}
 
 	/*
@@ -244,7 +244,7 @@ check_key:
 		 * @retval the old stmt.
 		 */
 		tuple_unref(result_stmt);
-		result_stmt = vy_stmt_dup(old_stmt, format);
+		result_stmt = vy_stmt_dup(old_stmt);
 	}
 	return result_stmt;
 }
-- 
2.11.0

[PATCH 10/12] vinyl: rename vy_log_record::commit_lsn to create_lsn

[Vladimir Davydov]

So as to draw the line between LSN of index creation and LSN of last
index modification, which is introduced by later in the series.
---
 src/box/vy_log.c | 40 ++++++++++++++++++++--------------------
 src/box/vy_log.h | 14 +++++++-------
 src/box/vy_lsm.c |  8 ++++----
 3 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index aeb31781..7fa395d5 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -80,7 +80,7 @@ enum vy_log_key {
 	VY_LOG_KEY_DUMP_LSN		= 9,
 	VY_LOG_KEY_GC_LSN		= 10,
 	VY_LOG_KEY_TRUNCATE_COUNT	= 11,
-	VY_LOG_KEY_COMMIT_LSN		= 12,
+	VY_LOG_KEY_CREATE_LSN		= 12,
 };
 
 /** vy_log_key -> human readable name. */
@@ -97,7 +97,7 @@ static const char *vy_log_key_name[] = {
 	[VY_LOG_KEY_DUMP_LSN]		= "dump_lsn",
 	[VY_LOG_KEY_GC_LSN]		= "gc_lsn",
 	[VY_LOG_KEY_TRUNCATE_COUNT]	= "truncate_count",
-	[VY_LOG_KEY_COMMIT_LSN]		= "commit_lsn",
+	[VY_LOG_KEY_CREATE_LSN]		= "create_lsn",
 };
 
 /** vy_log_type -> human readable name. */
@@ -247,10 +247,10 @@ vy_log_record_snprint(char *buf, int size, const struct vy_log_record *record)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_SLICE_ID],
 			record->slice_id);
-	if (record->commit_lsn > 0)
+	if (record->create_lsn > 0)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
-			vy_log_key_name[VY_LOG_KEY_COMMIT_LSN],
-			record->commit_lsn);
+			vy_log_key_name[VY_LOG_KEY_CREATE_LSN],
+			record->create_lsn);
 	if (record->dump_lsn > 0)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_DUMP_LSN],
@@ -355,9 +355,9 @@ vy_log_record_encode(const struct vy_log_record *record,
 		size += mp_sizeof_uint(record->slice_id);
 		n_keys++;
 	}
-	if (record->commit_lsn > 0) {
-		size += mp_sizeof_uint(VY_LOG_KEY_COMMIT_LSN);
-		size += mp_sizeof_uint(record->commit_lsn);
+	if (record->create_lsn > 0) {
+		size += mp_sizeof_uint(VY_LOG_KEY_CREATE_LSN);
+		size += mp_sizeof_uint(record->create_lsn);
 		n_keys++;
 	}
 	if (record->dump_lsn > 0) {
@@ -428,9 +428,9 @@ vy_log_record_encode(const struct vy_log_record *record,
 		pos = mp_encode_uint(pos, VY_LOG_KEY_SLICE_ID);
 		pos = mp_encode_uint(pos, record->slice_id);
 	}
-	if (record->commit_lsn > 0) {
-		pos = mp_encode_uint(pos, VY_LOG_KEY_COMMIT_LSN);
-		pos = mp_encode_uint(pos, record->commit_lsn);
+	if (record->create_lsn > 0) {
+		pos = mp_encode_uint(pos, VY_LOG_KEY_CREATE_LSN);
+		pos = mp_encode_uint(pos, record->create_lsn);
 	}
 	if (record->dump_lsn > 0) {
 		pos = mp_encode_uint(pos, VY_LOG_KEY_DUMP_LSN);
@@ -549,8 +549,8 @@ vy_log_record_decode(struct vy_log_record *record,
 		case VY_LOG_KEY_SLICE_ID:
 			record->slice_id = mp_decode_uint(&pos);
 			break;
-		case VY_LOG_KEY_COMMIT_LSN:
-			record->commit_lsn = mp_decode_uint(&pos);
+		case VY_LOG_KEY_CREATE_LSN:
+			record->create_lsn = mp_decode_uint(&pos);
 			break;
 		case VY_LOG_KEY_DUMP_LSN:
 			record->dump_lsn = mp_decode_uint(&pos);
@@ -568,16 +568,16 @@ vy_log_record_decode(struct vy_log_record *record,
 			goto fail;
 		}
 	}
-	if (record->type == VY_LOG_CREATE_LSM && record->commit_lsn == 0) {
+	if (record->type == VY_LOG_CREATE_LSM && record->create_lsn == 0) {
 		/*
 		 * We used to use LSN as unique LSM tree identifier
 		 * and didn't store LSN separately so if there's
-		 * no 'commit_lsn' field in the record, we are
+		 * no 'create_lsn' field in the record, we are
 		 * recovering from an old vylog and 'id' is in
 		 * fact the LSN of the WAL record that committed
 		 * the LSM tree.
 		 */
-		record->commit_lsn = record->lsm_id;
+		record->create_lsn = record->lsm_id;
 	}
 	return 0;
 fail:
@@ -1171,7 +1171,7 @@ static int
 vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 		       uint32_t space_id, uint32_t index_id,
 		       const struct key_part_def *key_parts,
-		       uint32_t key_part_count, int64_t commit_lsn)
+		       uint32_t key_part_count, int64_t create_lsn)
 {
 	struct vy_lsm_recovery_info *lsm;
 	struct key_part_def *key_parts_copy;
@@ -1257,7 +1257,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 	lsm->key_parts = key_parts_copy;
 	lsm->key_part_count = key_part_count;
 	lsm->is_dropped = false;
-	lsm->commit_lsn = commit_lsn;
+	lsm->create_lsn = create_lsn;
 	lsm->dump_lsn = -1;
 
 	/*
@@ -1756,7 +1756,7 @@ vy_recovery_process_record(struct vy_recovery *recovery,
 		rc = vy_recovery_create_lsm(recovery, record->lsm_id,
 				record->space_id, record->index_id,
 				record->key_parts, record->key_part_count,
-				record->commit_lsn);
+				record->create_lsn);
 		break;
 	case VY_LOG_DROP_LSM:
 		rc = vy_recovery_drop_lsm(recovery, record->lsm_id);
@@ -2005,7 +2005,7 @@ vy_log_append_lsm(struct xlog *xlog, struct vy_lsm_recovery_info *lsm)
 	record.space_id = lsm->space_id;
 	record.key_parts = lsm->key_parts;
 	record.key_part_count = lsm->key_part_count;
-	record.commit_lsn = lsm->commit_lsn;
+	record.create_lsn = lsm->create_lsn;
 	if (vy_log_append_record(xlog, &record) != 0)
 		return -1;
 
diff --git a/src/box/vy_log.h b/src/box/vy_log.h
index 2dad4ee9..702963d5 100644
--- a/src/box/vy_log.h
+++ b/src/box/vy_log.h
@@ -66,7 +66,7 @@ enum vy_log_record_type {
 	/**
 	 * Create a new LSM tree.
 	 * Requires vy_log_record::lsm_id, index_id, space_id,
-	 * key_def (with primary key parts), commit_lsn.
+	 * key_def, create_lsn.
 	 */
 	VY_LOG_CREATE_LSM		= 0,
 	/**
@@ -200,8 +200,8 @@ struct vy_log_record {
 	struct key_part_def *key_parts;
 	/** Number of key parts. */
 	uint32_t key_part_count;
-	/** LSN of the WAL row corresponding to this record. */
-	int64_t commit_lsn;
+	/** LSN of the WAL row that created the LSM tree. */
+	int64_t create_lsn;
 	/** Max LSN stored on disk. */
 	int64_t dump_lsn;
 	/**
@@ -253,8 +253,8 @@ struct vy_lsm_recovery_info {
 	uint32_t key_part_count;
 	/** True if the LSM tree was dropped. */
 	bool is_dropped;
-	/** LSN of the WAL row that committed the LSM tree. */
-	int64_t commit_lsn;
+	/** LSN of the WAL row that created the LSM tree. */
+	int64_t create_lsn;
 	/** LSN of the last LSM tree dump. */
 	int64_t dump_lsn;
 	/**
@@ -503,7 +503,7 @@ vy_log_record_init(struct vy_log_record *record)
 /** Helper to log a vinyl LSM tree creation. */
 static inline void
 vy_log_create_lsm(int64_t id, uint32_t space_id, uint32_t index_id,
-		  const struct key_def *key_def, int64_t commit_lsn)
+		  const struct key_def *key_def, int64_t create_lsn)
 {
 	struct vy_log_record record;
 	vy_log_record_init(&record);
@@ -512,7 +512,7 @@ vy_log_create_lsm(int64_t id, uint32_t space_id, uint32_t index_id,
 	record.space_id = space_id;
 	record.index_id = index_id;
 	record.key_def = key_def;
-	record.commit_lsn = commit_lsn;
+	record.create_lsn = create_lsn;
 	vy_log_write(&record);
 }
 
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 915ffaeb..af6dde50 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -512,17 +512,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 					    (unsigned)lsm->index_id));
 			return -1;
 		}
-		if (lsn > lsm_info->commit_lsn) {
+		if (lsn > lsm_info->create_lsn) {
 			/*
 			 * The last incarnation of the LSM tree was
 			 * created before the last checkpoint, load
 			 * it now.
 			 */
-			lsn = lsm_info->commit_lsn;
+			lsn = lsm_info->create_lsn;
 		}
 	}
 
-	if (lsm_info == NULL || lsn > lsm_info->commit_lsn) {
+	if (lsm_info == NULL || lsn > lsm_info->create_lsn) {
 		/*
 		 * If we failed to log LSM tree creation before restart,
 		 * we won't find it in the log on recovery. This is OK as
@@ -537,7 +537,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 	lsm->id = lsm_info->id;
 	lsm->is_committed = true;
 
-	if (lsn < lsm_info->commit_lsn || lsm_info->is_dropped) {
+	if (lsn < lsm_info->create_lsn || lsm_info->is_dropped) {
 		/*
 		 * Loading a past incarnation of the LSM tree, i.e.
 		 * the LSM tree is going to dropped during final
-- 
2.11.0

[PATCH 11/12] vinyl: do not write VY_LOG_DUMP_LSM record to snapshot

[Vladimir Davydov]

There's no point in writing this record to snapshot, because we can
store LSN of the last index dump right in VY_LOG_CREATE_LSM record.
---
 src/box/vy_log.c         | 17 +++++------------
 test/vinyl/layout.result | 12 ++----------
 2 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 7fa395d5..5552065d 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -1171,7 +1171,8 @@ static int
 vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 		       uint32_t space_id, uint32_t index_id,
 		       const struct key_part_def *key_parts,
-		       uint32_t key_part_count, int64_t create_lsn)
+		       uint32_t key_part_count, int64_t create_lsn,
+		       int64_t dump_lsn)
 {
 	struct vy_lsm_recovery_info *lsm;
 	struct key_part_def *key_parts_copy;
@@ -1258,7 +1259,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 	lsm->key_part_count = key_part_count;
 	lsm->is_dropped = false;
 	lsm->create_lsn = create_lsn;
-	lsm->dump_lsn = -1;
+	lsm->dump_lsn = dump_lsn;
 
 	/*
 	 * Add the LSM tree to the hash.
@@ -1756,7 +1757,7 @@ vy_recovery_process_record(struct vy_recovery *recovery,
 		rc = vy_recovery_create_lsm(recovery, record->lsm_id,
 				record->space_id, record->index_id,
 				record->key_parts, record->key_part_count,
-				record->create_lsn);
+				record->create_lsn, record->dump_lsn);
 		break;
 	case VY_LOG_DROP_LSM:
 		rc = vy_recovery_drop_lsm(recovery, record->lsm_id);
@@ -2006,18 +2007,10 @@ vy_log_append_lsm(struct xlog *xlog, struct vy_lsm_recovery_info *lsm)
 	record.key_parts = lsm->key_parts;
 	record.key_part_count = lsm->key_part_count;
 	record.create_lsn = lsm->create_lsn;
+	record.dump_lsn = lsm->dump_lsn;
 	if (vy_log_append_record(xlog, &record) != 0)
 		return -1;
 
-	if (lsm->dump_lsn >= 0) {
-		vy_log_record_init(&record);
-		record.type = VY_LOG_DUMP_LSM;
-		record.lsm_id = lsm->id;
-		record.dump_lsn = lsm->dump_lsn;
-		if (vy_log_append_record(xlog, &record) != 0)
-			return -1;
-	}
-
 	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
 		vy_log_record_init(&record);
 		if (run->is_incomplete) {
diff --git a/test/vinyl/layout.result b/test/vinyl/layout.result
index c7b14645..4af2c024 100644
--- a/test/vinyl/layout.result
+++ b/test/vinyl/layout.result
@@ -127,15 +127,11 @@ result
     - - HEADER:
           type: INSERT
         BODY:
-          tuple: [0, {7: [{'field': 0, 'collation': 1, 'type': 'string'}], 12: 3,
+          tuple: [0, {7: [{'field': 0, 'collation': 1, 'type': 'string'}], 9: 9, 12: 3,
               6: 512}]
       - HEADER:
           type: INSERT
         BODY:
-          tuple: [10, {9: 9}]
-      - HEADER:
-          type: INSERT
-        BODY:
           tuple: [5, {2: 8, 9: 9}]
       - HEADER:
           type: INSERT
@@ -157,11 +153,7 @@ result
           type: INSERT
         BODY:
           tuple: [0, {0: 2, 5: 1, 6: 512, 7: [{'field': 1, 'is_nullable': true, 'type': 'unsigned'}],
-              12: 4}]
-      - HEADER:
-          type: INSERT
-        BODY:
-          tuple: [10, {0: 2, 9: 9}]
+              9: 9, 12: 4}]
       - HEADER:
           type: INSERT
         BODY:
-- 
2.11.0

[PATCH 12/12] vinyl: allow to modify key definition if it does not require rebuild

[Vladimir Davydov]

To allow extending key definition for non-empty vinyl spaces, this patch
performs the following steps:

 - Revert commit c31dd19a69b5 ("vinyl: forbid vinyl index key definition
   alter") that forbade any key def alter. It isn't needed anymore.

 - Update key_def and cmp_def in vinyl_space_swap_index(). We simply
   swap the definitions between the old and new indexes in memory.
   Since all vinyl objects reference either vy_lsm::key_def or
   vy_lsm::cmp_def or both, and the change is compatible (does not
   change the order for existing tuples), this should work just fine.

 - Update key definition in vylog on ALTER. For this, we introduce a new
   vylog record type, VY_LOG_MODIFY_LSM, which updates key definition.
   To be able to replay it on recovery in case we failed to flush it
   before restart, we also store the LSN of the WAL record that
   triggered the ALTER.

It also adds the following test cases:

 - Modify key definition of primary and secondary indexes of a non-empty
   space (engine/ddl).

 - Modify key definition before snapshot and relay it to a newly joined
   replica (engine/replica_join).

 - Make sure key definition is updated in vylog on ALTER (vinyl/layout).
---
 src/box/key_def.cc                |   9 +++
 src/box/key_def.h                 |   7 +++
 src/box/vinyl.c                   |  74 +++++++++++++++++--------
 src/box/vy_log.c                  |  79 ++++++++++++++++++++++++--
 src/box/vy_log.h                  |  22 ++++++++
 src/box/vy_lsm.c                  |   5 +-
 src/box/vy_lsm.h                  |  10 ++--
 test/engine/ddl.result            | 113 +++++++++++++++++++++++++++++++++++++-
 test/engine/ddl.test.lua          |  35 +++++++++++-
 test/engine/replica_join.result   |  26 ++++++++-
 test/engine/replica_join.test.lua |   2 +
 test/engine/truncate.result       |   2 +-
 test/engine/truncate.test.lua     |   2 +-
 test/vinyl/ddl.result             |  78 +++++++-------------------
 test/vinyl/ddl.test.lua           |  30 +++-------
 test/vinyl/gh.result              |   2 +-
 test/vinyl/layout.result          |  67 +++++++++++-----------
 test/vinyl/layout.test.lua        |   4 +-
 18 files changed, 412 insertions(+), 155 deletions(-)

diff --git a/src/box/key_def.cc b/src/box/key_def.cc
index 2f157419..50786913 100644
--- a/src/box/key_def.cc
+++ b/src/box/key_def.cc
@@ -107,6 +107,15 @@ key_def_dup(const struct key_def *src)
 }
 
 void
+key_def_swap(struct key_def *old_def, struct key_def *new_def)
+{
+	assert(old_def->part_count == new_def->part_count);
+	for (uint32_t i = 0; i < new_def->part_count; i++)
+		SWAP(old_def->parts[i], new_def->parts[i]);
+	SWAP(*old_def, *new_def);
+}
+
+void
 key_def_delete(struct key_def *def)
 {
 	free(def);
diff --git a/src/box/key_def.h b/src/box/key_def.h
index fe9ee56f..84e3e1e5 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -150,6 +150,13 @@ struct key_def *
 key_def_dup(const struct key_def *src);
 
 /**
+ * Swap content of two key definitions in memory.
+ * The two key definitions must have the same size.
+ */
+void
+key_def_swap(struct key_def *old_def, struct key_def *new_def);
+
+/**
  * Delete @a key_def.
  * @param def Key_def to delete.
  */
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 145fde70..886a00f8 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -803,7 +803,7 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 		 * the index isn't in the recovery context and we
 		 * need to retry to log it now.
 		 */
-		if (lsm->is_committed) {
+		if (lsm->commit_lsn >= 0) {
 			vy_scheduler_add_lsm(&env->scheduler, lsm);
 			return;
 		}
@@ -828,8 +828,8 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 	if (lsm->opts.lsn != 0)
 		lsn = lsm->opts.lsn;
 
-	assert(!lsm->is_committed);
-	lsm->is_committed = true;
+	assert(lsm->commit_lsn < 0);
+	lsm->commit_lsn = lsn;
 
 	assert(lsm->range_count == 1);
 	struct vy_range *range = vy_range_tree_first(lsm->tree);
@@ -854,6 +854,34 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 	vy_scheduler_add_lsm(&env->scheduler, lsm);
 }
 
+static void
+vinyl_index_commit_modify(struct index *index, int64_t lsn)
+{
+	struct vy_env *env = vy_env(index->engine);
+	struct vy_lsm *lsm = vy_lsm(index);
+
+	(void)env;
+	assert(env->status == VINYL_ONLINE ||
+	       env->status == VINYL_FINAL_RECOVERY_LOCAL ||
+	       env->status == VINYL_FINAL_RECOVERY_REMOTE);
+
+	if (lsn <= lsm->commit_lsn) {
+		/*
+		 * This must be local recovery from WAL, when
+		 * the operation has already been committed to
+		 * vylog.
+		 */
+		assert(env->status == VINYL_FINAL_RECOVERY_LOCAL);
+		return;
+	}
+
+	lsm->commit_lsn = lsn;
+
+	vy_log_tx_begin();
+	vy_log_modify_lsm(lsm->id, lsm->key_def, lsn);
+	vy_log_tx_try_commit();
+}
+
 /*
  * Delete all runs, ranges, and slices of a given LSM tree
  * from the metadata log.
@@ -945,11 +973,19 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 	 */
 	if (env->status != VINYL_ONLINE)
 		return 0;
-	/*
-	 * Regardless of the space emptyness, key definition of an
-	 * existing index can not be changed, because key
-	 * definition is already in vylog. See #3169.
-	 */
+	/* The space is empty. Allow alter. */
+	if (pk->stat.disk.count.rows == 0 &&
+	    pk->stat.memory.count.rows == 0)
+		return 0;
+	if (space_def_check_compatibility(old_space->def, new_space->def,
+					  false) != 0)
+		return -1;
+	if (old_space->index_count < new_space->index_count) {
+		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
+			 "adding an index to a non-empty space");
+		return -1;
+	}
+
 	if (old_space->index_count == new_space->index_count) {
 		/* Check index_defs to be unchanged. */
 		for (uint32_t i = 0; i < old_space->index_count; ++i) {
@@ -961,25 +997,15 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 			 * vinyl yet.
 			 */
 			if (index_def_change_requires_rebuild(old_def,
-							      new_def) ||
-			    key_part_cmp(old_def->key_def->parts,
-					 old_def->key_def->part_count,
-					 new_def->key_def->parts,
-					 new_def->key_def->part_count) != 0) {
+							      new_def)) {
 				diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-					 "changing the definition of an index");
+					 "changing the definition of "
+					 "a non-empty index");
 				return -1;
 			}
 		}
 	}
-	if (pk->stat.disk.count.rows == 0 &&
-	    pk->stat.memory.count.rows == 0)
-		return 0;
-	/*
-	 * Since space format is not persisted in vylog, it can be
-	 * altered on non-empty space to some state, compatible
-	 * with the old one.
-	 */
+
 	if (space_def_check_compatibility(old_space->def, new_space->def,
 					  false) != 0)
 		return -1;
@@ -1083,6 +1109,8 @@ vinyl_space_swap_index(struct space *old_space, struct space *new_space,
 	     new_lsm->mem_format_with_colmask);
 	SWAP(old_lsm->disk_format, new_lsm->disk_format);
 	SWAP(old_lsm->opts, new_lsm->opts);
+	key_def_swap(old_lsm->key_def, new_lsm->key_def);
+	key_def_swap(old_lsm->cmp_def, new_lsm->cmp_def);
 }
 
 static int
@@ -3931,7 +3959,7 @@ static const struct index_vtab vinyl_index_vtab = {
 	/* .destroy = */ vinyl_index_destroy,
 	/* .commit_create = */ vinyl_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
-	/* .commit_modify = */ generic_index_commit_modify,
+	/* .commit_modify = */ vinyl_index_commit_modify,
 	/* .commit_drop = */ vinyl_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .depends_on_pk = */ vinyl_index_depends_on_pk,
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 5552065d..172a1b01 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -81,6 +81,7 @@ enum vy_log_key {
 	VY_LOG_KEY_GC_LSN		= 10,
 	VY_LOG_KEY_TRUNCATE_COUNT	= 11,
 	VY_LOG_KEY_CREATE_LSN		= 12,
+	VY_LOG_KEY_MODIFY_LSN		= 13,
 };
 
 /** vy_log_key -> human readable name. */
@@ -98,6 +99,7 @@ static const char *vy_log_key_name[] = {
 	[VY_LOG_KEY_GC_LSN]		= "gc_lsn",
 	[VY_LOG_KEY_TRUNCATE_COUNT]	= "truncate_count",
 	[VY_LOG_KEY_CREATE_LSN]		= "create_lsn",
+	[VY_LOG_KEY_MODIFY_LSN]		= "modify_lsn",
 };
 
 /** vy_log_type -> human readable name. */
@@ -115,6 +117,7 @@ static const char *vy_log_type_name[] = {
 	[VY_LOG_DUMP_LSM]		= "dump_lsm",
 	[VY_LOG_SNAPSHOT]		= "snapshot",
 	[VY_LOG_TRUNCATE_LSM]		= "truncate_lsm",
+	[VY_LOG_MODIFY_LSM]		= "modify_lsm",
 };
 
 /** Metadata log object. */
@@ -251,6 +254,10 @@ vy_log_record_snprint(char *buf, int size, const struct vy_log_record *record)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_CREATE_LSN],
 			record->create_lsn);
+	if (record->modify_lsn > 0)
+		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
+			vy_log_key_name[VY_LOG_KEY_MODIFY_LSN],
+			record->modify_lsn);
 	if (record->dump_lsn > 0)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_DUMP_LSN],
@@ -360,6 +367,11 @@ vy_log_record_encode(const struct vy_log_record *record,
 		size += mp_sizeof_uint(record->create_lsn);
 		n_keys++;
 	}
+	if (record->modify_lsn > 0) {
+		size += mp_sizeof_uint(VY_LOG_KEY_MODIFY_LSN);
+		size += mp_sizeof_uint(record->modify_lsn);
+		n_keys++;
+	}
 	if (record->dump_lsn > 0) {
 		size += mp_sizeof_uint(VY_LOG_KEY_DUMP_LSN);
 		size += mp_sizeof_uint(record->dump_lsn);
@@ -432,6 +444,10 @@ vy_log_record_encode(const struct vy_log_record *record,
 		pos = mp_encode_uint(pos, VY_LOG_KEY_CREATE_LSN);
 		pos = mp_encode_uint(pos, record->create_lsn);
 	}
+	if (record->modify_lsn > 0) {
+		pos = mp_encode_uint(pos, VY_LOG_KEY_MODIFY_LSN);
+		pos = mp_encode_uint(pos, record->modify_lsn);
+	}
 	if (record->dump_lsn > 0) {
 		pos = mp_encode_uint(pos, VY_LOG_KEY_DUMP_LSN);
 		pos = mp_encode_uint(pos, record->dump_lsn);
@@ -552,6 +568,9 @@ vy_log_record_decode(struct vy_log_record *record,
 		case VY_LOG_KEY_CREATE_LSN:
 			record->create_lsn = mp_decode_uint(&pos);
 			break;
+		case VY_LOG_KEY_MODIFY_LSN:
+			record->modify_lsn = mp_decode_uint(&pos);
+			break;
 		case VY_LOG_KEY_DUMP_LSN:
 			record->dump_lsn = mp_decode_uint(&pos);
 			break;
@@ -568,7 +587,7 @@ vy_log_record_decode(struct vy_log_record *record,
 			goto fail;
 		}
 	}
-	if (record->type == VY_LOG_CREATE_LSM && record->create_lsn == 0) {
+	if (record->type == VY_LOG_CREATE_LSM) {
 		/*
 		 * We used to use LSN as unique LSM tree identifier
 		 * and didn't store LSN separately so if there's
@@ -577,7 +596,14 @@ vy_log_record_decode(struct vy_log_record *record,
 		 * fact the LSN of the WAL record that committed
 		 * the LSM tree.
 		 */
-		record->create_lsn = record->lsm_id;
+		if (record->create_lsn == 0)
+			record->create_lsn = record->lsm_id;
+		/*
+		 * If the LSM tree has never been modified, initialize
+		 * 'modify_lsn' with 'create_lsn'.
+		 */
+		if (record->modify_lsn == 0)
+			record->modify_lsn = record->create_lsn;
 	}
 	return 0;
 fail:
@@ -1172,7 +1198,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 		       uint32_t space_id, uint32_t index_id,
 		       const struct key_part_def *key_parts,
 		       uint32_t key_part_count, int64_t create_lsn,
-		       int64_t dump_lsn)
+		       int64_t modify_lsn, int64_t dump_lsn)
 {
 	struct vy_lsm_recovery_info *lsm;
 	struct key_part_def *key_parts_copy;
@@ -1259,6 +1285,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 	lsm->key_part_count = key_part_count;
 	lsm->is_dropped = false;
 	lsm->create_lsn = create_lsn;
+	lsm->modify_lsn = modify_lsn;
 	lsm->dump_lsn = dump_lsn;
 
 	/*
@@ -1286,6 +1313,43 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 }
 
 /**
+ * Handle a VY_LOG_MODIFY_LSM log record.
+ * This function updates key definition of the LSM tree with ID @id.
+ * Return 0 on success, -1 on failure.
+ */
+static int
+vy_recovery_modify_lsm(struct vy_recovery *recovery, int64_t id,
+		       const struct key_part_def *key_parts,
+		       uint32_t key_part_count, int64_t modify_lsn)
+{
+	struct vy_lsm_recovery_info *lsm;
+	lsm = vy_recovery_lookup_lsm(recovery, id);
+	if (lsm == NULL) {
+		diag_set(ClientError, ER_INVALID_VYLOG_FILE,
+			 tt_sprintf("Update of unregistered LSM tree %lld",
+				    (long long)id));
+		return -1;
+	}
+	if (lsm->is_dropped) {
+		diag_set(ClientError, ER_INVALID_VYLOG_FILE,
+			 tt_sprintf("Update of deleted LSM tree %lld",
+				    (long long)id));
+		return -1;
+	}
+	free(lsm->key_parts);
+	lsm->key_parts = malloc(sizeof(*key_parts) * key_part_count);
+	if (lsm->key_parts == NULL) {
+		diag_set(OutOfMemory, sizeof(*key_parts) * key_part_count,
+			 "malloc", "struct key_part_def");
+		return -1;
+	}
+	memcpy(lsm->key_parts, key_parts, sizeof(*key_parts) * key_part_count);
+	lsm->key_part_count = key_part_count;
+	lsm->modify_lsn = modify_lsn;
+	return 0;
+}
+
+/**
  * Handle a VY_LOG_DROP_LSM log record.
  * This function marks the LSM tree with ID @id as dropped.
  * All ranges and runs of the LSM tree must have been deleted by now.
@@ -1757,7 +1821,13 @@ vy_recovery_process_record(struct vy_recovery *recovery,
 		rc = vy_recovery_create_lsm(recovery, record->lsm_id,
 				record->space_id, record->index_id,
 				record->key_parts, record->key_part_count,
-				record->create_lsn, record->dump_lsn);
+				record->create_lsn, record->modify_lsn,
+				record->dump_lsn);
+		break;
+	case VY_LOG_MODIFY_LSM:
+		rc = vy_recovery_modify_lsm(recovery, record->lsm_id,
+				record->key_parts, record->key_part_count,
+				record->modify_lsn);
 		break;
 	case VY_LOG_DROP_LSM:
 		rc = vy_recovery_drop_lsm(recovery, record->lsm_id);
@@ -2007,6 +2077,7 @@ vy_log_append_lsm(struct xlog *xlog, struct vy_lsm_recovery_info *lsm)
 	record.key_parts = lsm->key_parts;
 	record.key_part_count = lsm->key_part_count;
 	record.create_lsn = lsm->create_lsn;
+	record.modify_lsn = lsm->modify_lsn;
 	record.dump_lsn = lsm->dump_lsn;
 	if (vy_log_append_record(xlog, &record) != 0)
 		return -1;
diff --git a/src/box/vy_log.h b/src/box/vy_log.h
index 702963d5..1b2b419f 100644
--- a/src/box/vy_log.h
+++ b/src/box/vy_log.h
@@ -164,6 +164,11 @@ enum vy_log_record_type {
 	 * WAL records written after truncation.
 	 */
 	VY_LOG_TRUNCATE_LSM		= 12,
+	/**
+	 * Modify key definition of an LSM tree.
+	 * Requires vy_log_record::lsm_id, key_def, modify_lsn.
+	 */
+	VY_LOG_MODIFY_LSM		= 13,
 
 	vy_log_record_type_MAX
 };
@@ -202,6 +207,8 @@ struct vy_log_record {
 	uint32_t key_part_count;
 	/** LSN of the WAL row that created the LSM tree. */
 	int64_t create_lsn;
+	/** LSN of the WAL row that last modified the LSM tree. */
+	int64_t modify_lsn;
 	/** Max LSN stored on disk. */
 	int64_t dump_lsn;
 	/**
@@ -255,6 +262,8 @@ struct vy_lsm_recovery_info {
 	bool is_dropped;
 	/** LSN of the WAL row that created the LSM tree. */
 	int64_t create_lsn;
+	/** LSN of the WAL row that last modified the LSM tree. */
+	int64_t modify_lsn;
 	/** LSN of the last LSM tree dump. */
 	int64_t dump_lsn;
 	/**
@@ -516,6 +525,19 @@ vy_log_create_lsm(int64_t id, uint32_t space_id, uint32_t index_id,
 	vy_log_write(&record);
 }
 
+/** Helper to log a vinyl LSM tree modification. */
+static inline void
+vy_log_modify_lsm(int64_t id, const struct key_def *key_def, int64_t modify_lsn)
+{
+	struct vy_log_record record;
+	vy_log_record_init(&record);
+	record.type = VY_LOG_MODIFY_LSM;
+	record.lsm_id = id;
+	record.key_def = key_def;
+	record.modify_lsn = modify_lsn;
+	vy_log_write(&record);
+}
+
 /** Helper to log a vinyl LSM tree drop. */
 static inline void
 vy_log_drop_lsm(int64_t id)
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index af6dde50..a8ed39ad 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -182,6 +182,7 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 	lsm->id = -1;
 	lsm->refs = 1;
 	lsm->dump_lsn = -1;
+	lsm->commit_lsn = -1;
 	vy_cache_create(&lsm->cache, cache_env, cmp_def);
 	rlist_create(&lsm->sealed);
 	vy_range_tree_new(lsm->tree);
@@ -479,7 +480,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 		 bool is_checkpoint_recovery, bool force_recovery)
 {
 	assert(lsm->id < 0);
-	assert(!lsm->is_committed);
+	assert(lsm->commit_lsn < 0);
 	assert(lsm->range_count == 0);
 
 	/*
@@ -535,7 +536,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 	}
 
 	lsm->id = lsm_info->id;
-	lsm->is_committed = true;
+	lsm->commit_lsn = lsm_info->modify_lsn;
 
 	if (lsn < lsm_info->create_lsn || lsm_info->is_dropped) {
 		/*
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index f5862a52..201b5a56 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -255,11 +255,13 @@ struct vy_lsm {
 	 * been dumped yet.
 	 */
 	int64_t dump_lsn;
-	/*
-	 * This flag is set if the LSM tree creation was
-	 * committed to the metadata log.
+	/**
+	 * LSN of the WAL row that created or last modified
+	 * this LSM tree. We store it in vylog so that during
+	 * local recovery we can replay vylog records we failed
+	 * to log before restart.
 	 */
-	bool is_committed;
+	int64_t commit_lsn;
 	/**
 	 * This flag is set if the LSM tree was dropped.
 	 * It is also set on local recovery if the LSM tree
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index c3cb1efc..308aefb0 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -391,9 +391,6 @@ s:drop()
 -- gh-3229: update optionality if a space format is changed too,
 -- not only when indexes are updated.
 --
-box.cfg{}
----
-...
 s = box.schema.create_space('test', {engine = engine})
 ---
 ...
@@ -466,3 +463,113 @@ sk:get({5})
 s:drop()
 ---
 ...
+--
+-- Modify key definition without index rebuild.
+--
+s = box.schema.create_space('test', {engine = engine})
+---
+...
+i1 = s:create_index('i1', {unique = true,  parts = {1, 'unsigned'}})
+---
+...
+i2 = s:create_index('i2', {unique = false, parts = {2, 'unsigned'}})
+---
+...
+i3 = s:create_index('i3', {unique = true,  parts = {3, 'unsigned'}})
+---
+...
+_ = s:insert{1, 2, 3}
+---
+...
+box.snapshot()
+---
+- ok
+...
+_ = s:insert{3, 2, 1}
+---
+...
+i1:alter{parts = {1, 'integer'}}
+---
+...
+_ = s:insert{-1, 2, 2}
+---
+...
+i1:select()
+---
+- - [-1, 2, 2]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-1, 2, 2]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [3, 2, 1]
+  - [-1, 2, 2]
+  - [1, 2, 3]
+...
+i2:alter{parts = {2, 'integer'}}
+---
+...
+i3:alter{parts = {3, 'integer'}}
+---
+...
+_ = s:replace{-1, -1, -1}
+---
+...
+i1:select()
+---
+- - [-1, -1, -1]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-1, -1, -1]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [-1, -1, -1]
+  - [3, 2, 1]
+  - [1, 2, 3]
+...
+box.snapshot()
+---
+- ok
+...
+_ = s:replace{-1, -2, -3}
+---
+...
+_ = s:replace{-3, -2, -1}
+---
+...
+i1:select()
+---
+- - [-3, -2, -1]
+  - [-1, -2, -3]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-3, -2, -1]
+  - [-1, -2, -3]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [-1, -2, -3]
+  - [-3, -2, -1]
+  - [3, 2, 1]
+  - [1, 2, 3]
+...
+s:drop()
+---
+...
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 25381a3c..019e18a1 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -141,7 +141,6 @@ s:drop()
 -- gh-3229: update optionality if a space format is changed too,
 -- not only when indexes are updated.
 --
-box.cfg{}
 s = box.schema.create_space('test', {engine = engine})
 format = {}
 format[1] = {'field1', 'unsigned'}
@@ -162,3 +161,37 @@ pk:get({4})
 sk:select({box.NULL})
 sk:get({5})
 s:drop()
+
+--
+-- Modify key definition without index rebuild.
+--
+s = box.schema.create_space('test', {engine = engine})
+i1 = s:create_index('i1', {unique = true,  parts = {1, 'unsigned'}})
+i2 = s:create_index('i2', {unique = false, parts = {2, 'unsigned'}})
+i3 = s:create_index('i3', {unique = true,  parts = {3, 'unsigned'}})
+
+_ = s:insert{1, 2, 3}
+box.snapshot()
+_ = s:insert{3, 2, 1}
+
+i1:alter{parts = {1, 'integer'}}
+_ = s:insert{-1, 2, 2}
+i1:select()
+i2:select()
+i3:select()
+
+i2:alter{parts = {2, 'integer'}}
+i3:alter{parts = {3, 'integer'}}
+_ = s:replace{-1, -1, -1}
+i1:select()
+i2:select()
+i3:select()
+
+box.snapshot()
+_ = s:replace{-1, -2, -3}
+_ = s:replace{-3, -2, -1}
+i1:select()
+i2:select()
+i3:select()
+
+s:drop()
diff --git a/test/engine/replica_join.result b/test/engine/replica_join.result
index a003c0d8..39d857fe 100644
--- a/test/engine/replica_join.result
+++ b/test/engine/replica_join.result
@@ -293,6 +293,12 @@ for k = 8, 1, -1 do box.space.test:update(k, {{'-', 2, 8}}) end
 for k = 9, 16 do box.space.test:delete(k) end
 ---
 ...
+box.space.test.index.primary:alter{parts = {1, 'integer'}}
+---
+...
+for k = -8, -1 do box.space.test:insert{k, 9 + k} end
+---
+...
 _ = box.space.test2:upsert({1, 'test1', 11}, {{'+', 3, 1}})
 ---
 ...
@@ -333,7 +339,15 @@ test_run:cmd('switch replica')
 ...
 box.space.test:select()
 ---
-- - [1, 8]
+- - [-8, 1]
+  - [-7, 2]
+  - [-6, 3]
+  - [-5, 4]
+  - [-4, 5]
+  - [-3, 6]
+  - [-2, 7]
+  - [-1, 8]
+  - [1, 8]
   - [2, 7]
   - [3, 6]
   - [4, 5]
@@ -344,13 +358,21 @@ box.space.test:select()
 ...
 box.space.test.index.secondary:select()
 ---
-- - [8, 1]
+- - [-8, 1]
+  - [8, 1]
+  - [-7, 2]
   - [7, 2]
+  - [-6, 3]
   - [6, 3]
+  - [-5, 4]
   - [5, 4]
+  - [-4, 5]
   - [4, 5]
+  - [-3, 6]
   - [3, 6]
+  - [-2, 7]
   - [2, 7]
+  - [-1, 8]
   - [1, 8]
 ...
 box.space.test2:select()
diff --git a/test/engine/replica_join.test.lua b/test/engine/replica_join.test.lua
index 5c6ed11a..1792281e 100644
--- a/test/engine/replica_join.test.lua
+++ b/test/engine/replica_join.test.lua
@@ -72,6 +72,8 @@ _ = test_run:cmd("cleanup server replica")
 -- new data
 for k = 8, 1, -1 do box.space.test:update(k, {{'-', 2, 8}}) end
 for k = 9, 16 do box.space.test:delete(k) end
+box.space.test.index.primary:alter{parts = {1, 'integer'}}
+for k = -8, -1 do box.space.test:insert{k, 9 + k} end
 _ = box.space.test2:upsert({1, 'test1', 11}, {{'+', 3, 1}})
 _ = box.space.test2:update({2, 'test2'}, {{'+', 3, 2}})
 _ = box.space.test2:delete{3, 'test3'}
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index b6e1a99a..2222804b 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -391,7 +391,7 @@ _ = s3:insert{789, 987}
 ...
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = 'memtx'})
+s4 = box.schema.create_space('test4', {engine = engine})
 ---
 ...
 _ = s4:create_index('i1', {parts = {1, 'string'}})
diff --git a/test/engine/truncate.test.lua b/test/engine/truncate.test.lua
index 66cbd156..df2797a1 100644
--- a/test/engine/truncate.test.lua
+++ b/test/engine/truncate.test.lua
@@ -175,7 +175,7 @@ s3:truncate()
 _ = s3:insert{789, 987}
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = 'memtx'})
+s4 = box.schema.create_space('test4', {engine = engine})
 _ = s4:create_index('i1', {parts = {1, 'string'}})
 _ = s4:create_index('i3', {parts = {3, 'string'}})
 _ = s4:insert{'zzz', 111, 'yyy'}
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 6f393c69..456977e5 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -87,7 +87,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -116,7 +116,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -145,7 +145,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -165,7 +165,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 box.snapshot()
 ---
@@ -174,27 +174,33 @@ box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 ---
 ...
--- After a dump REPLACE + DELETE = nothing, so the space is empty
--- but an index can not be altered.
+-- after a dump REPLACE + DELETE = nothing, so the space is empty now and
+-- can be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
 ...
--- Space format still can be altered.
-format = {}
+#box.space._index:select({space.id})
 ---
+- 2
 ...
-format[1] = {name = 'field1', type = 'unsigned'}
+box.space._index:get{space.id, 0}[6]
 ---
+- [[0, 'unsigned'], [1, 'unsigned']]
 ...
-format[2] = {name = 'field2', type = 'unsigned'}
+space:insert({1, 2})
 ---
+- [1, 2]
 ...
-space:format(format)
+index:select{}
+---
+- - [1, 2]
+...
+index2:select{}
 ---
+- - [1, 2]
 ...
 space:drop()
 ---
@@ -230,7 +236,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 -- After compaction the REPLACE + DELETE + DELETE = nothing, so
 -- the space is now empty and can be altered.
@@ -256,10 +262,8 @@ while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
--- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
 ...
 space:drop()
 ---
@@ -287,7 +291,7 @@ space:auto_increment{3}
 ...
 box.space._index:replace{space.id, 0, 'pk', 'tree', {unique=true}, {{0, 'unsigned'}, {1, 'unsigned'}}}
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 space:select{}
 ---
@@ -828,7 +832,7 @@ s.index.secondary.unique
 ...
 s.index.secondary:alter{unique = true} -- error
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 s.index.secondary.unique
 ---
@@ -846,43 +850,3 @@ s.index.secondary:select(10)
 s:drop()
 ---
 ...
---
--- gh-3169: vinyl index key definition can not be altered even if
--- the index is empty.
---
-s = box.schema.space.create('vinyl', {engine = 'vinyl'})
----
-...
-i = s:create_index('pk')
----
-...
-i:alter{parts = {1, 'integer'}}
----
-- error: Vinyl does not support changing the definition of an index
-...
-_ = s:replace{-1}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-_ = s:replace{1}
----
-...
-_ = s:replace{-2}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-_ = s:replace{3}
----
-...
-_ = s:replace{-3}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-s:select{}
----
-- - [1]
-  - [3]
-...
-s:drop()
----
-...
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index f050799e..8ce8b920 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -59,15 +59,15 @@ space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 
--- After a dump REPLACE + DELETE = nothing, so the space is empty
--- but an index can not be altered.
+-- after a dump REPLACE + DELETE = nothing, so the space is empty now and
+-- can be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
--- Space format still can be altered.
-format = {}
-format[1] = {name = 'field1', type = 'unsigned'}
-format[2] = {name = 'field2', type = 'unsigned'}
-space:format(format)
+#box.space._index:select({space.id})
+box.space._index:get{space.id, 0}[6]
+space:insert({1, 2})
+index:select{}
+index2:select{}
 space:drop()
 
 space = box.schema.space.create('test', { engine = 'vinyl' })
@@ -91,7 +91,6 @@ box.snapshot()
 -- Wait until the dump is finished.
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
--- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 
 space:drop()
@@ -305,18 +304,3 @@ s.index.secondary.unique
 s:insert{2, 10}
 s.index.secondary:select(10)
 s:drop()
-
---
--- gh-3169: vinyl index key definition can not be altered even if
--- the index is empty.
---
-s = box.schema.space.create('vinyl', {engine = 'vinyl'})
-i = s:create_index('pk')
-i:alter{parts = {1, 'integer'}}
-_ = s:replace{-1}
-_ = s:replace{1}
-_ = s:replace{-2}
-_ = s:replace{3}
-_ = s:replace{-3}
-s:select{}
-s:drop()
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 32e4b6fb..9c72acc8 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -144,7 +144,7 @@ s:insert{5, 5}
 ...
 s.index.primary:alter({parts={2,'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 s:drop()
 ---
diff --git a/test/vinyl/layout.result b/test/vinyl/layout.result
index 4af2c024..e6294d3c 100644
--- a/test/vinyl/layout.result
+++ b/test/vinyl/layout.result
@@ -20,7 +20,7 @@ space = box.schema.space.create('test', {engine='vinyl'})
 _ = space:create_index('pk', {parts = {{1, 'string', collation = 'unicode'}}, run_count_per_level=3})
 ---
 ...
-_ = space:create_index('sk', {parts = {{2, 'unsigned', is_nullable = true}}, run_count_per_level=3})
+_ = space:create_index('sk', {parts = {{2, 'unsigned'}}, run_count_per_level=3})
 ---
 ...
 -- Empty run
@@ -35,6 +35,9 @@ box.snapshot()
 ---
 - ok
 ...
+space.index.sk:alter{parts = {{2, 'unsigned', is_nullable = true}}}
+---
+...
 space:replace{'ЭЭЭ', box.NULL}
 ---
 - ['ЭЭЭ', null]
@@ -123,16 +126,16 @@ test_run:cmd("push filter 'offset: .*' to 'offset: <offset>'")
 ...
 result
 ---
-- - - 00000000000000000009.vylog
+- - - 00000000000000000010.vylog
     - - HEADER:
           type: INSERT
         BODY:
-          tuple: [0, {7: [{'field': 0, 'collation': 1, 'type': 'string'}], 9: 9, 12: 3,
-              6: 512}]
+          tuple: [0, {6: 512, 7: [{'field': 0, 'collation': 1, 'type': 'string'}],
+              9: 10, 12: 3, 13: 7}]
       - HEADER:
           type: INSERT
         BODY:
-          tuple: [5, {2: 8, 9: 9}]
+          tuple: [5, {2: 8, 9: 10}]
       - HEADER:
           type: INSERT
         BODY:
@@ -153,11 +156,11 @@ result
           type: INSERT
         BODY:
           tuple: [0, {0: 2, 5: 1, 6: 512, 7: [{'field': 1, 'is_nullable': true, 'type': 'unsigned'}],
-              9: 9, 12: 4}]
+              9: 10, 12: 4, 13: 7}]
       - HEADER:
           type: INSERT
         BODY:
-          tuple: [5, {0: 2, 2: 6, 9: 9}]
+          tuple: [5, {0: 2, 2: 6, 9: 10}]
       - HEADER:
           type: INSERT
         BODY:
@@ -197,7 +200,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [5, {0: 2, 2: 10, 9: 12}]
+          tuple: [5, {0: 2, 2: 10, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -207,7 +210,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [10, {0: 2, 9: 12}]
+          tuple: [10, {0: 2, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -217,7 +220,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [5, {2: 12, 9: 12}]
+          tuple: [5, {2: 12, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -227,16 +230,16 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [10, {9: 12}]
+          tuple: [10, {9: 13}]
   - - 00000000000000000008.index
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 7
+          min_lsn: 8
           max_key: ['ЭЭЭ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 9
+          max_lsn: 10
           min_key: ['ёёё']
       - HEADER:
           type: PAGEINFO
@@ -249,17 +252,17 @@ result
           min_key: ['ёёё']
   - - 00000000000000000008.run
     - - HEADER:
-          lsn: 9
+          lsn: 10
           type: INSERT
         BODY:
           tuple: ['ёёё', null]
       - HEADER:
-          lsn: 8
+          lsn: 9
           type: INSERT
         BODY:
           tuple: ['эээ', null]
       - HEADER:
-          lsn: 7
+          lsn: 8
           type: INSERT
         BODY:
           tuple: ['ЭЭЭ', null]
@@ -271,11 +274,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 10
+          min_lsn: 11
           max_key: ['ЮЮЮ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 12
+          max_lsn: 13
           min_key: ['ёёё']
       - HEADER:
           type: PAGEINFO
@@ -288,17 +291,17 @@ result
           min_key: ['ёёё']
   - - 00000000000000000012.run
     - - HEADER:
-          lsn: 10
+          lsn: 11
           type: REPLACE
         BODY:
           tuple: ['ёёё', 123]
       - HEADER:
-          lsn: 12
+          lsn: 13
           type: INSERT
         BODY:
           tuple: ['ююю', 789]
       - HEADER:
-          lsn: 11
+          lsn: 12
           type: INSERT
         BODY:
           tuple: ['ЮЮЮ', 456]
@@ -310,11 +313,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 7
+          min_lsn: 8
           max_key: [null, 'ЭЭЭ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 9
+          max_lsn: 10
           min_key: [null, 'ёёё']
       - HEADER:
           type: PAGEINFO
@@ -327,17 +330,17 @@ result
           min_key: [null, 'ёёё']
   - - 00000000000000000006.run
     - - HEADER:
-          lsn: 9
+          lsn: 10
           type: INSERT
         BODY:
           tuple: [null, 'ёёё']
       - HEADER:
-          lsn: 8
+          lsn: 9
           type: INSERT
         BODY:
           tuple: [null, 'эээ']
       - HEADER:
-          lsn: 7
+          lsn: 8
           type: INSERT
         BODY:
           tuple: [null, 'ЭЭЭ']
@@ -349,11 +352,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 10
+          min_lsn: 11
           max_key: [789, 'ююю']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 12
+          max_lsn: 13
           min_key: [null, 'ёёё']
       - HEADER:
           type: PAGEINFO
@@ -366,22 +369,22 @@ result
           min_key: [null, 'ёёё']
   - - 00000000000000000010.run
     - - HEADER:
-          lsn: 10
+          lsn: 11
           type: DELETE
         BODY:
           key: [null, 'ёёё']
       - HEADER:
-          lsn: 10
+          lsn: 11
           type: REPLACE
         BODY:
           tuple: [123, 'ёёё']
       - HEADER:
-          lsn: 11
+          lsn: 12
           type: INSERT
         BODY:
           tuple: [456, 'ЮЮЮ']
       - HEADER:
-          lsn: 12
+          lsn: 13
           type: INSERT
         BODY:
           tuple: [789, 'ююю']
diff --git a/test/vinyl/layout.test.lua b/test/vinyl/layout.test.lua
index fd5787b7..27bf5d40 100644
--- a/test/vinyl/layout.test.lua
+++ b/test/vinyl/layout.test.lua
@@ -8,13 +8,15 @@ fun = require 'fun'
 
 space = box.schema.space.create('test', {engine='vinyl'})
 _ = space:create_index('pk', {parts = {{1, 'string', collation = 'unicode'}}, run_count_per_level=3})
-_ = space:create_index('sk', {parts = {{2, 'unsigned', is_nullable = true}}, run_count_per_level=3})
+_ = space:create_index('sk', {parts = {{2, 'unsigned'}}, run_count_per_level=3})
 
 -- Empty run
 space:insert{'ЁЁЁ', 777}
 space:delete{'ЁЁЁ'}
 box.snapshot()
 
+space.index.sk:alter{parts = {{2, 'unsigned', is_nullable = true}}}
+
 space:replace{'ЭЭЭ', box.NULL}
 space:replace{'эээ', box.NULL}
 space:replace{'ёёё', box.NULL}
-- 
2.11.0

Re: [tarantool-patches] [PATCH 05/12] vinyl: do not reallocate tuple formats on alter

[v.shpilevoy]

> 1 апр. 2018 г., в 12:05, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> We create new formats for all indexes of the new space in
> vinyl_space_commit_alter() while we don't actually need to
> do this, because the new formats have already been created
> by vy_lsm_new() - all we need to do is reuse them somehow.
> 
> This patch does the trick: it implements the swap_index()
> space virtual method for vinyl so that it swaps tuple formats
> between the old and new spaces.
> ---
> src/box/vinyl.c | 58 ++++++++++++++++++++++-----------------------------------
> 1 file changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index fc37283a..f3cef52f 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> +static void
> +vinyl_space_swap_index(struct space *old_space, struct space *new_space,
> +		       uint32_t old_index_id, uint32_t new_index_id)
> +{
> +	struct vy_lsm *old_lsm = vy_lsm(old_space->index_map[old_index_id]);
> +	struct vy_lsm *new_lsm = vy_lsm(new_space->index_map[new_index_id]);
> +
> +	/*
> +	 * Swap the two indexes between the two spaces,
> +	 * but leave tuple formats.
> +	 */
> +	generic_space_swap_index(old_space, new_space,
> +				 old_index_id, new_index_id);
> +
> +	SWAP(old_lsm->mem_format, new_lsm->mem_format);

How do you swap the formats, if they are variable length structures? 1) Their fields[] arrays are not swapped here,
2) even if you will try to swap - they can have different length.

> +	SWAP(old_lsm->mem_format_with_colmask,
> +	     new_lsm->mem_format_with_colmask);
> +	SWAP(old_lsm->disk_format, new_lsm->disk_format);
> +	SWAP(old_lsm->opts, new_lsm->opts);
> }
> 
> static int
> @@ -3938,7 +3924,7 @@ static const struct space_vtab vinyl_space_vtab = {
> 	/* .drop_primary_key = */ vinyl_space_drop_primary_key,
> 	/* .check_format = */ vinyl_space_check_format,
> 	/* .build_secondary_key = */ vinyl_space_build_secondary_key,
> -	/* .swap_index = */ generic_space_swap_index,
> +	/* .swap_index = */ vinyl_space_swap_index,
> 	/* .prepare_alter = */ vinyl_space_prepare_alter,
> 	/* .commit_alter = */ vinyl_space_commit_alter,
> };
> -- 
> 2.11.0
> 
> 

Re: [tarantool-patches] [PATCH 05/12] vinyl: do not reallocate tuple formats on alter

[Vladimir Davydov]

On Sun, Apr 01, 2018 at 02:12:26PM +0300, v.shpilevoy@tarantool.org wrote:
> > diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> > index fc37283a..f3cef52f 100644
> > --- a/src/box/vinyl.c
> > +++ b/src/box/vinyl.c
> > +static void
> > +vinyl_space_swap_index(struct space *old_space, struct space *new_space,
> > +		       uint32_t old_index_id, uint32_t new_index_id)
> > +{
> > +	struct vy_lsm *old_lsm = vy_lsm(old_space->index_map[old_index_id]);
> > +	struct vy_lsm *new_lsm = vy_lsm(new_space->index_map[new_index_id]);
> > +
> > +	/*
> > +	 * Swap the two indexes between the two spaces,
> > +	 * but leave tuple formats.
> > +	 */
> > +	generic_space_swap_index(old_space, new_space,
> > +				 old_index_id, new_index_id);
> > +
> > +	SWAP(old_lsm->mem_format, new_lsm->mem_format);
> 
> How do you swap the formats, if they are variable length structures?
> 1) Their fields[] arrays are not swapped here, 2) even if you will try
> to swap - they can have different length.

I swap pointers, not objects:

struct vy_lsm {
  	...
	struct tuple_format *disk_format;
	struct tuple_format *mem_format;
	struct tuple_format *mem_format_with_colmask;
};

> 
> > +	SWAP(old_lsm->mem_format_with_colmask,
> > +	     new_lsm->mem_format_with_colmask);
> > +	SWAP(old_lsm->disk_format, new_lsm->disk_format);
> > +	SWAP(old_lsm->opts, new_lsm->opts);
> > }

Re: [PATCH 12/12] vinyl: allow to modify key definition if it does not require rebuild

[Vladimir Davydov]

On Sun, Apr 01, 2018 at 12:05:39PM +0300, Vladimir Davydov wrote:
> @@ -945,11 +973,19 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
>  	 */
>  	if (env->status != VINYL_ONLINE)
>  		return 0;
> -	/*
> -	 * Regardless of the space emptyness, key definition of an
> -	 * existing index can not be changed, because key
> -	 * definition is already in vylog. See #3169.
> -	 */
> +	/* The space is empty. Allow alter. */
> +	if (pk->stat.disk.count.rows == 0 &&
> +	    pk->stat.memory.count.rows == 0)
> +		return 0;

> +	if (space_def_check_compatibility(old_space->def, new_space->def,
> +					  false) != 0)
> +		return -1;
> +	if (old_space->index_count < new_space->index_count) {
> +		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
> +			 "adding an index to a non-empty space");
> +		return -1;
> +	}
> +

These two checks got added by git-revert. They are pointless as we
already have them in vinyl_space_prepare_alter(). I removed them on
the branch. Here's the updated diff:

diff --git a/src/box/key_def.cc b/src/box/key_def.cc
index 2f157419..50786913 100644
--- a/src/box/key_def.cc
+++ b/src/box/key_def.cc
@@ -107,6 +107,15 @@ key_def_dup(const struct key_def *src)
 }
 
 void
+key_def_swap(struct key_def *old_def, struct key_def *new_def)
+{
+	assert(old_def->part_count == new_def->part_count);
+	for (uint32_t i = 0; i < new_def->part_count; i++)
+		SWAP(old_def->parts[i], new_def->parts[i]);
+	SWAP(*old_def, *new_def);
+}
+
+void
 key_def_delete(struct key_def *def)
 {
 	free(def);
diff --git a/src/box/key_def.h b/src/box/key_def.h
index fe9ee56f..84e3e1e5 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -150,6 +150,13 @@ struct key_def *
 key_def_dup(const struct key_def *src);
 
 /**
+ * Swap content of two key definitions in memory.
+ * The two key definitions must have the same size.
+ */
+void
+key_def_swap(struct key_def *old_def, struct key_def *new_def);
+
+/**
  * Delete @a key_def.
  * @param def Key_def to delete.
  */
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 145fde70..d503704e 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -803,7 +803,7 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 		 * the index isn't in the recovery context and we
 		 * need to retry to log it now.
 		 */
-		if (lsm->is_committed) {
+		if (lsm->commit_lsn >= 0) {
 			vy_scheduler_add_lsm(&env->scheduler, lsm);
 			return;
 		}
@@ -828,8 +828,8 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 	if (lsm->opts.lsn != 0)
 		lsn = lsm->opts.lsn;
 
-	assert(!lsm->is_committed);
-	lsm->is_committed = true;
+	assert(lsm->commit_lsn < 0);
+	lsm->commit_lsn = lsn;
 
 	assert(lsm->range_count == 1);
 	struct vy_range *range = vy_range_tree_first(lsm->tree);
@@ -854,6 +854,34 @@ vinyl_index_commit_create(struct index *index, int64_t lsn)
 	vy_scheduler_add_lsm(&env->scheduler, lsm);
 }
 
+static void
+vinyl_index_commit_modify(struct index *index, int64_t lsn)
+{
+	struct vy_env *env = vy_env(index->engine);
+	struct vy_lsm *lsm = vy_lsm(index);
+
+	(void)env;
+	assert(env->status == VINYL_ONLINE ||
+	       env->status == VINYL_FINAL_RECOVERY_LOCAL ||
+	       env->status == VINYL_FINAL_RECOVERY_REMOTE);
+
+	if (lsn <= lsm->commit_lsn) {
+		/*
+		 * This must be local recovery from WAL, when
+		 * the operation has already been committed to
+		 * vylog.
+		 */
+		assert(env->status == VINYL_FINAL_RECOVERY_LOCAL);
+		return;
+	}
+
+	lsm->commit_lsn = lsn;
+
+	vy_log_tx_begin();
+	vy_log_modify_lsm(lsm->id, lsm->key_def, lsn);
+	vy_log_tx_try_commit();
+}
+
 /*
  * Delete all runs, ranges, and slices of a given LSM tree
  * from the metadata log.
@@ -945,11 +973,10 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 	 */
 	if (env->status != VINYL_ONLINE)
 		return 0;
-	/*
-	 * Regardless of the space emptyness, key definition of an
-	 * existing index can not be changed, because key
-	 * definition is already in vylog. See #3169.
-	 */
+	/* The space is empty. Allow alter. */
+	if (pk->stat.disk.count.rows == 0 &&
+	    pk->stat.memory.count.rows == 0)
+		return 0;
 	if (old_space->index_count == new_space->index_count) {
 		/* Check index_defs to be unchanged. */
 		for (uint32_t i = 0; i < old_space->index_count; ++i) {
@@ -961,25 +988,14 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 			 * vinyl yet.
 			 */
 			if (index_def_change_requires_rebuild(old_def,
-							      new_def) ||
-			    key_part_cmp(old_def->key_def->parts,
-					 old_def->key_def->part_count,
-					 new_def->key_def->parts,
-					 new_def->key_def->part_count) != 0) {
+							      new_def)) {
 				diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-					 "changing the definition of an index");
+					 "changing the definition of "
+					 "a non-empty index");
 				return -1;
 			}
 		}
 	}
-	if (pk->stat.disk.count.rows == 0 &&
-	    pk->stat.memory.count.rows == 0)
-		return 0;
-	/*
-	 * Since space format is not persisted in vylog, it can be
-	 * altered on non-empty space to some state, compatible
-	 * with the old one.
-	 */
 	if (space_def_check_compatibility(old_space->def, new_space->def,
 					  false) != 0)
 		return -1;
@@ -1083,6 +1099,8 @@ vinyl_space_swap_index(struct space *old_space, struct space *new_space,
 	     new_lsm->mem_format_with_colmask);
 	SWAP(old_lsm->disk_format, new_lsm->disk_format);
 	SWAP(old_lsm->opts, new_lsm->opts);
+	key_def_swap(old_lsm->key_def, new_lsm->key_def);
+	key_def_swap(old_lsm->cmp_def, new_lsm->cmp_def);
 }
 
 static int
@@ -3931,7 +3949,7 @@ static const struct index_vtab vinyl_index_vtab = {
 	/* .destroy = */ vinyl_index_destroy,
 	/* .commit_create = */ vinyl_index_commit_create,
 	/* .abort_create = */ generic_index_abort_create,
-	/* .commit_modify = */ generic_index_commit_modify,
+	/* .commit_modify = */ vinyl_index_commit_modify,
 	/* .commit_drop = */ vinyl_index_commit_drop,
 	/* .update_def = */ generic_index_update_def,
 	/* .depends_on_pk = */ vinyl_index_depends_on_pk,
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 5552065d..172a1b01 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -81,6 +81,7 @@ enum vy_log_key {
 	VY_LOG_KEY_GC_LSN		= 10,
 	VY_LOG_KEY_TRUNCATE_COUNT	= 11,
 	VY_LOG_KEY_CREATE_LSN		= 12,
+	VY_LOG_KEY_MODIFY_LSN		= 13,
 };
 
 /** vy_log_key -> human readable name. */
@@ -98,6 +99,7 @@ static const char *vy_log_key_name[] = {
 	[VY_LOG_KEY_GC_LSN]		= "gc_lsn",
 	[VY_LOG_KEY_TRUNCATE_COUNT]	= "truncate_count",
 	[VY_LOG_KEY_CREATE_LSN]		= "create_lsn",
+	[VY_LOG_KEY_MODIFY_LSN]		= "modify_lsn",
 };
 
 /** vy_log_type -> human readable name. */
@@ -115,6 +117,7 @@ static const char *vy_log_type_name[] = {
 	[VY_LOG_DUMP_LSM]		= "dump_lsm",
 	[VY_LOG_SNAPSHOT]		= "snapshot",
 	[VY_LOG_TRUNCATE_LSM]		= "truncate_lsm",
+	[VY_LOG_MODIFY_LSM]		= "modify_lsm",
 };
 
 /** Metadata log object. */
@@ -251,6 +254,10 @@ vy_log_record_snprint(char *buf, int size, const struct vy_log_record *record)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_CREATE_LSN],
 			record->create_lsn);
+	if (record->modify_lsn > 0)
+		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
+			vy_log_key_name[VY_LOG_KEY_MODIFY_LSN],
+			record->modify_lsn);
 	if (record->dump_lsn > 0)
 		SNPRINT(total, snprintf, buf, size, "%s=%"PRIi64", ",
 			vy_log_key_name[VY_LOG_KEY_DUMP_LSN],
@@ -360,6 +367,11 @@ vy_log_record_encode(const struct vy_log_record *record,
 		size += mp_sizeof_uint(record->create_lsn);
 		n_keys++;
 	}
+	if (record->modify_lsn > 0) {
+		size += mp_sizeof_uint(VY_LOG_KEY_MODIFY_LSN);
+		size += mp_sizeof_uint(record->modify_lsn);
+		n_keys++;
+	}
 	if (record->dump_lsn > 0) {
 		size += mp_sizeof_uint(VY_LOG_KEY_DUMP_LSN);
 		size += mp_sizeof_uint(record->dump_lsn);
@@ -432,6 +444,10 @@ vy_log_record_encode(const struct vy_log_record *record,
 		pos = mp_encode_uint(pos, VY_LOG_KEY_CREATE_LSN);
 		pos = mp_encode_uint(pos, record->create_lsn);
 	}
+	if (record->modify_lsn > 0) {
+		pos = mp_encode_uint(pos, VY_LOG_KEY_MODIFY_LSN);
+		pos = mp_encode_uint(pos, record->modify_lsn);
+	}
 	if (record->dump_lsn > 0) {
 		pos = mp_encode_uint(pos, VY_LOG_KEY_DUMP_LSN);
 		pos = mp_encode_uint(pos, record->dump_lsn);
@@ -552,6 +568,9 @@ vy_log_record_decode(struct vy_log_record *record,
 		case VY_LOG_KEY_CREATE_LSN:
 			record->create_lsn = mp_decode_uint(&pos);
 			break;
+		case VY_LOG_KEY_MODIFY_LSN:
+			record->modify_lsn = mp_decode_uint(&pos);
+			break;
 		case VY_LOG_KEY_DUMP_LSN:
 			record->dump_lsn = mp_decode_uint(&pos);
 			break;
@@ -568,7 +587,7 @@ vy_log_record_decode(struct vy_log_record *record,
 			goto fail;
 		}
 	}
-	if (record->type == VY_LOG_CREATE_LSM && record->create_lsn == 0) {
+	if (record->type == VY_LOG_CREATE_LSM) {
 		/*
 		 * We used to use LSN as unique LSM tree identifier
 		 * and didn't store LSN separately so if there's
@@ -577,7 +596,14 @@ vy_log_record_decode(struct vy_log_record *record,
 		 * fact the LSN of the WAL record that committed
 		 * the LSM tree.
 		 */
-		record->create_lsn = record->lsm_id;
+		if (record->create_lsn == 0)
+			record->create_lsn = record->lsm_id;
+		/*
+		 * If the LSM tree has never been modified, initialize
+		 * 'modify_lsn' with 'create_lsn'.
+		 */
+		if (record->modify_lsn == 0)
+			record->modify_lsn = record->create_lsn;
 	}
 	return 0;
 fail:
@@ -1172,7 +1198,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 		       uint32_t space_id, uint32_t index_id,
 		       const struct key_part_def *key_parts,
 		       uint32_t key_part_count, int64_t create_lsn,
-		       int64_t dump_lsn)
+		       int64_t modify_lsn, int64_t dump_lsn)
 {
 	struct vy_lsm_recovery_info *lsm;
 	struct key_part_def *key_parts_copy;
@@ -1259,6 +1285,7 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 	lsm->key_part_count = key_part_count;
 	lsm->is_dropped = false;
 	lsm->create_lsn = create_lsn;
+	lsm->modify_lsn = modify_lsn;
 	lsm->dump_lsn = dump_lsn;
 
 	/*
@@ -1286,6 +1313,43 @@ vy_recovery_create_lsm(struct vy_recovery *recovery, int64_t id,
 }
 
 /**
+ * Handle a VY_LOG_MODIFY_LSM log record.
+ * This function updates key definition of the LSM tree with ID @id.
+ * Return 0 on success, -1 on failure.
+ */
+static int
+vy_recovery_modify_lsm(struct vy_recovery *recovery, int64_t id,
+		       const struct key_part_def *key_parts,
+		       uint32_t key_part_count, int64_t modify_lsn)
+{
+	struct vy_lsm_recovery_info *lsm;
+	lsm = vy_recovery_lookup_lsm(recovery, id);
+	if (lsm == NULL) {
+		diag_set(ClientError, ER_INVALID_VYLOG_FILE,
+			 tt_sprintf("Update of unregistered LSM tree %lld",
+				    (long long)id));
+		return -1;
+	}
+	if (lsm->is_dropped) {
+		diag_set(ClientError, ER_INVALID_VYLOG_FILE,
+			 tt_sprintf("Update of deleted LSM tree %lld",
+				    (long long)id));
+		return -1;
+	}
+	free(lsm->key_parts);
+	lsm->key_parts = malloc(sizeof(*key_parts) * key_part_count);
+	if (lsm->key_parts == NULL) {
+		diag_set(OutOfMemory, sizeof(*key_parts) * key_part_count,
+			 "malloc", "struct key_part_def");
+		return -1;
+	}
+	memcpy(lsm->key_parts, key_parts, sizeof(*key_parts) * key_part_count);
+	lsm->key_part_count = key_part_count;
+	lsm->modify_lsn = modify_lsn;
+	return 0;
+}
+
+/**
  * Handle a VY_LOG_DROP_LSM log record.
  * This function marks the LSM tree with ID @id as dropped.
  * All ranges and runs of the LSM tree must have been deleted by now.
@@ -1757,7 +1821,13 @@ vy_recovery_process_record(struct vy_recovery *recovery,
 		rc = vy_recovery_create_lsm(recovery, record->lsm_id,
 				record->space_id, record->index_id,
 				record->key_parts, record->key_part_count,
-				record->create_lsn, record->dump_lsn);
+				record->create_lsn, record->modify_lsn,
+				record->dump_lsn);
+		break;
+	case VY_LOG_MODIFY_LSM:
+		rc = vy_recovery_modify_lsm(recovery, record->lsm_id,
+				record->key_parts, record->key_part_count,
+				record->modify_lsn);
 		break;
 	case VY_LOG_DROP_LSM:
 		rc = vy_recovery_drop_lsm(recovery, record->lsm_id);
@@ -2007,6 +2077,7 @@ vy_log_append_lsm(struct xlog *xlog, struct vy_lsm_recovery_info *lsm)
 	record.key_parts = lsm->key_parts;
 	record.key_part_count = lsm->key_part_count;
 	record.create_lsn = lsm->create_lsn;
+	record.modify_lsn = lsm->modify_lsn;
 	record.dump_lsn = lsm->dump_lsn;
 	if (vy_log_append_record(xlog, &record) != 0)
 		return -1;
diff --git a/src/box/vy_log.h b/src/box/vy_log.h
index 702963d5..1b2b419f 100644
--- a/src/box/vy_log.h
+++ b/src/box/vy_log.h
@@ -164,6 +164,11 @@ enum vy_log_record_type {
 	 * WAL records written after truncation.
 	 */
 	VY_LOG_TRUNCATE_LSM		= 12,
+	/**
+	 * Modify key definition of an LSM tree.
+	 * Requires vy_log_record::lsm_id, key_def, modify_lsn.
+	 */
+	VY_LOG_MODIFY_LSM		= 13,
 
 	vy_log_record_type_MAX
 };
@@ -202,6 +207,8 @@ struct vy_log_record {
 	uint32_t key_part_count;
 	/** LSN of the WAL row that created the LSM tree. */
 	int64_t create_lsn;
+	/** LSN of the WAL row that last modified the LSM tree. */
+	int64_t modify_lsn;
 	/** Max LSN stored on disk. */
 	int64_t dump_lsn;
 	/**
@@ -255,6 +262,8 @@ struct vy_lsm_recovery_info {
 	bool is_dropped;
 	/** LSN of the WAL row that created the LSM tree. */
 	int64_t create_lsn;
+	/** LSN of the WAL row that last modified the LSM tree. */
+	int64_t modify_lsn;
 	/** LSN of the last LSM tree dump. */
 	int64_t dump_lsn;
 	/**
@@ -516,6 +525,19 @@ vy_log_create_lsm(int64_t id, uint32_t space_id, uint32_t index_id,
 	vy_log_write(&record);
 }
 
+/** Helper to log a vinyl LSM tree modification. */
+static inline void
+vy_log_modify_lsm(int64_t id, const struct key_def *key_def, int64_t modify_lsn)
+{
+	struct vy_log_record record;
+	vy_log_record_init(&record);
+	record.type = VY_LOG_MODIFY_LSM;
+	record.lsm_id = id;
+	record.key_def = key_def;
+	record.modify_lsn = modify_lsn;
+	vy_log_write(&record);
+}
+
 /** Helper to log a vinyl LSM tree drop. */
 static inline void
 vy_log_drop_lsm(int64_t id)
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index af6dde50..a8ed39ad 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -182,6 +182,7 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 	lsm->id = -1;
 	lsm->refs = 1;
 	lsm->dump_lsn = -1;
+	lsm->commit_lsn = -1;
 	vy_cache_create(&lsm->cache, cache_env, cmp_def);
 	rlist_create(&lsm->sealed);
 	vy_range_tree_new(lsm->tree);
@@ -479,7 +480,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 		 bool is_checkpoint_recovery, bool force_recovery)
 {
 	assert(lsm->id < 0);
-	assert(!lsm->is_committed);
+	assert(lsm->commit_lsn < 0);
 	assert(lsm->range_count == 0);
 
 	/*
@@ -535,7 +536,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 	}
 
 	lsm->id = lsm_info->id;
-	lsm->is_committed = true;
+	lsm->commit_lsn = lsm_info->modify_lsn;
 
 	if (lsn < lsm_info->create_lsn || lsm_info->is_dropped) {
 		/*
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index f5862a52..201b5a56 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -255,11 +255,13 @@ struct vy_lsm {
 	 * been dumped yet.
 	 */
 	int64_t dump_lsn;
-	/*
-	 * This flag is set if the LSM tree creation was
-	 * committed to the metadata log.
+	/**
+	 * LSN of the WAL row that created or last modified
+	 * this LSM tree. We store it in vylog so that during
+	 * local recovery we can replay vylog records we failed
+	 * to log before restart.
 	 */
-	bool is_committed;
+	int64_t commit_lsn;
 	/**
 	 * This flag is set if the LSM tree was dropped.
 	 * It is also set on local recovery if the LSM tree
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index c3cb1efc..308aefb0 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -391,9 +391,6 @@ s:drop()
 -- gh-3229: update optionality if a space format is changed too,
 -- not only when indexes are updated.
 --
-box.cfg{}
----
-...
 s = box.schema.create_space('test', {engine = engine})
 ---
 ...
@@ -466,3 +463,113 @@ sk:get({5})
 s:drop()
 ---
 ...
+--
+-- Modify key definition without index rebuild.
+--
+s = box.schema.create_space('test', {engine = engine})
+---
+...
+i1 = s:create_index('i1', {unique = true,  parts = {1, 'unsigned'}})
+---
+...
+i2 = s:create_index('i2', {unique = false, parts = {2, 'unsigned'}})
+---
+...
+i3 = s:create_index('i3', {unique = true,  parts = {3, 'unsigned'}})
+---
+...
+_ = s:insert{1, 2, 3}
+---
+...
+box.snapshot()
+---
+- ok
+...
+_ = s:insert{3, 2, 1}
+---
+...
+i1:alter{parts = {1, 'integer'}}
+---
+...
+_ = s:insert{-1, 2, 2}
+---
+...
+i1:select()
+---
+- - [-1, 2, 2]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-1, 2, 2]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [3, 2, 1]
+  - [-1, 2, 2]
+  - [1, 2, 3]
+...
+i2:alter{parts = {2, 'integer'}}
+---
+...
+i3:alter{parts = {3, 'integer'}}
+---
+...
+_ = s:replace{-1, -1, -1}
+---
+...
+i1:select()
+---
+- - [-1, -1, -1]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-1, -1, -1]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [-1, -1, -1]
+  - [3, 2, 1]
+  - [1, 2, 3]
+...
+box.snapshot()
+---
+- ok
+...
+_ = s:replace{-1, -2, -3}
+---
+...
+_ = s:replace{-3, -2, -1}
+---
+...
+i1:select()
+---
+- - [-3, -2, -1]
+  - [-1, -2, -3]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i2:select()
+---
+- - [-3, -2, -1]
+  - [-1, -2, -3]
+  - [1, 2, 3]
+  - [3, 2, 1]
+...
+i3:select()
+---
+- - [-1, -2, -3]
+  - [-3, -2, -1]
+  - [3, 2, 1]
+  - [1, 2, 3]
+...
+s:drop()
+---
+...
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 25381a3c..019e18a1 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -141,7 +141,6 @@ s:drop()
 -- gh-3229: update optionality if a space format is changed too,
 -- not only when indexes are updated.
 --
-box.cfg{}
 s = box.schema.create_space('test', {engine = engine})
 format = {}
 format[1] = {'field1', 'unsigned'}
@@ -162,3 +161,37 @@ pk:get({4})
 sk:select({box.NULL})
 sk:get({5})
 s:drop()
+
+--
+-- Modify key definition without index rebuild.
+--
+s = box.schema.create_space('test', {engine = engine})
+i1 = s:create_index('i1', {unique = true,  parts = {1, 'unsigned'}})
+i2 = s:create_index('i2', {unique = false, parts = {2, 'unsigned'}})
+i3 = s:create_index('i3', {unique = true,  parts = {3, 'unsigned'}})
+
+_ = s:insert{1, 2, 3}
+box.snapshot()
+_ = s:insert{3, 2, 1}
+
+i1:alter{parts = {1, 'integer'}}
+_ = s:insert{-1, 2, 2}
+i1:select()
+i2:select()
+i3:select()
+
+i2:alter{parts = {2, 'integer'}}
+i3:alter{parts = {3, 'integer'}}
+_ = s:replace{-1, -1, -1}
+i1:select()
+i2:select()
+i3:select()
+
+box.snapshot()
+_ = s:replace{-1, -2, -3}
+_ = s:replace{-3, -2, -1}
+i1:select()
+i2:select()
+i3:select()
+
+s:drop()
diff --git a/test/engine/replica_join.result b/test/engine/replica_join.result
index a003c0d8..39d857fe 100644
--- a/test/engine/replica_join.result
+++ b/test/engine/replica_join.result
@@ -293,6 +293,12 @@ for k = 8, 1, -1 do box.space.test:update(k, {{'-', 2, 8}}) end
 for k = 9, 16 do box.space.test:delete(k) end
 ---
 ...
+box.space.test.index.primary:alter{parts = {1, 'integer'}}
+---
+...
+for k = -8, -1 do box.space.test:insert{k, 9 + k} end
+---
+...
 _ = box.space.test2:upsert({1, 'test1', 11}, {{'+', 3, 1}})
 ---
 ...
@@ -333,7 +339,15 @@ test_run:cmd('switch replica')
 ...
 box.space.test:select()
 ---
-- - [1, 8]
+- - [-8, 1]
+  - [-7, 2]
+  - [-6, 3]
+  - [-5, 4]
+  - [-4, 5]
+  - [-3, 6]
+  - [-2, 7]
+  - [-1, 8]
+  - [1, 8]
   - [2, 7]
   - [3, 6]
   - [4, 5]
@@ -344,13 +358,21 @@ box.space.test:select()
 ...
 box.space.test.index.secondary:select()
 ---
-- - [8, 1]
+- - [-8, 1]
+  - [8, 1]
+  - [-7, 2]
   - [7, 2]
+  - [-6, 3]
   - [6, 3]
+  - [-5, 4]
   - [5, 4]
+  - [-4, 5]
   - [4, 5]
+  - [-3, 6]
   - [3, 6]
+  - [-2, 7]
   - [2, 7]
+  - [-1, 8]
   - [1, 8]
 ...
 box.space.test2:select()
diff --git a/test/engine/replica_join.test.lua b/test/engine/replica_join.test.lua
index 5c6ed11a..1792281e 100644
--- a/test/engine/replica_join.test.lua
+++ b/test/engine/replica_join.test.lua
@@ -72,6 +72,8 @@ _ = test_run:cmd("cleanup server replica")
 -- new data
 for k = 8, 1, -1 do box.space.test:update(k, {{'-', 2, 8}}) end
 for k = 9, 16 do box.space.test:delete(k) end
+box.space.test.index.primary:alter{parts = {1, 'integer'}}
+for k = -8, -1 do box.space.test:insert{k, 9 + k} end
 _ = box.space.test2:upsert({1, 'test1', 11}, {{'+', 3, 1}})
 _ = box.space.test2:update({2, 'test2'}, {{'+', 3, 2}})
 _ = box.space.test2:delete{3, 'test3'}
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index b6e1a99a..2222804b 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -391,7 +391,7 @@ _ = s3:insert{789, 987}
 ...
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = 'memtx'})
+s4 = box.schema.create_space('test4', {engine = engine})
 ---
 ...
 _ = s4:create_index('i1', {parts = {1, 'string'}})
diff --git a/test/engine/truncate.test.lua b/test/engine/truncate.test.lua
index 66cbd156..df2797a1 100644
--- a/test/engine/truncate.test.lua
+++ b/test/engine/truncate.test.lua
@@ -175,7 +175,7 @@ s3:truncate()
 _ = s3:insert{789, 987}
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = 'memtx'})
+s4 = box.schema.create_space('test4', {engine = engine})
 _ = s4:create_index('i1', {parts = {1, 'string'}})
 _ = s4:create_index('i3', {parts = {3, 'string'}})
 _ = s4:insert{'zzz', 111, 'yyy'}
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 6f393c69..456977e5 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -87,7 +87,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -116,7 +116,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -145,7 +145,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 #box.space._index:select({space.id})
 ---
@@ -165,7 +165,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 box.snapshot()
 ---
@@ -174,27 +174,33 @@ box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 ---
 ...
--- After a dump REPLACE + DELETE = nothing, so the space is empty
--- but an index can not be altered.
+-- after a dump REPLACE + DELETE = nothing, so the space is empty now and
+-- can be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
 ...
--- Space format still can be altered.
-format = {}
+#box.space._index:select({space.id})
 ---
+- 2
 ...
-format[1] = {name = 'field1', type = 'unsigned'}
+box.space._index:get{space.id, 0}[6]
 ---
+- [[0, 'unsigned'], [1, 'unsigned']]
 ...
-format[2] = {name = 'field2', type = 'unsigned'}
+space:insert({1, 2})
 ---
+- [1, 2]
 ...
-space:format(format)
+index:select{}
+---
+- - [1, 2]
+...
+index2:select{}
 ---
+- - [1, 2]
 ...
 space:drop()
 ---
@@ -230,7 +236,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 -- After compaction the REPLACE + DELETE + DELETE = nothing, so
 -- the space is now empty and can be altered.
@@ -256,10 +262,8 @@ while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
--- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
 ...
 space:drop()
 ---
@@ -287,7 +291,7 @@ space:auto_increment{3}
 ...
 box.space._index:replace{space.id, 0, 'pk', 'tree', {unique=true}, {{0, 'unsigned'}, {1, 'unsigned'}}}
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 space:select{}
 ---
@@ -828,7 +832,7 @@ s.index.secondary.unique
 ...
 s.index.secondary:alter{unique = true} -- error
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 s.index.secondary.unique
 ---
@@ -846,43 +850,3 @@ s.index.secondary:select(10)
 s:drop()
 ---
 ...
---
--- gh-3169: vinyl index key definition can not be altered even if
--- the index is empty.
---
-s = box.schema.space.create('vinyl', {engine = 'vinyl'})
----
-...
-i = s:create_index('pk')
----
-...
-i:alter{parts = {1, 'integer'}}
----
-- error: Vinyl does not support changing the definition of an index
-...
-_ = s:replace{-1}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-_ = s:replace{1}
----
-...
-_ = s:replace{-2}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-_ = s:replace{3}
----
-...
-_ = s:replace{-3}
----
-- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
-...
-s:select{}
----
-- - [1]
-  - [3]
-...
-s:drop()
----
-...
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index f050799e..8ce8b920 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -59,15 +59,15 @@ space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 
--- After a dump REPLACE + DELETE = nothing, so the space is empty
--- but an index can not be altered.
+-- after a dump REPLACE + DELETE = nothing, so the space is empty now and
+-- can be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
--- Space format still can be altered.
-format = {}
-format[1] = {name = 'field1', type = 'unsigned'}
-format[2] = {name = 'field2', type = 'unsigned'}
-space:format(format)
+#box.space._index:select({space.id})
+box.space._index:get{space.id, 0}[6]
+space:insert({1, 2})
+index:select{}
+index2:select{}
 space:drop()
 
 space = box.schema.space.create('test', { engine = 'vinyl' })
@@ -91,7 +91,6 @@ box.snapshot()
 -- Wait until the dump is finished.
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
--- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 
 space:drop()
@@ -305,18 +304,3 @@ s.index.secondary.unique
 s:insert{2, 10}
 s.index.secondary:select(10)
 s:drop()
-
---
--- gh-3169: vinyl index key definition can not be altered even if
--- the index is empty.
---
-s = box.schema.space.create('vinyl', {engine = 'vinyl'})
-i = s:create_index('pk')
-i:alter{parts = {1, 'integer'}}
-_ = s:replace{-1}
-_ = s:replace{1}
-_ = s:replace{-2}
-_ = s:replace{3}
-_ = s:replace{-3}
-s:select{}
-s:drop()
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 32e4b6fb..9c72acc8 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -144,7 +144,7 @@ s:insert{5, 5}
 ...
 s.index.primary:alter({parts={2,'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of an index
+- error: Vinyl does not support changing the definition of a non-empty index
 ...
 s:drop()
 ---
diff --git a/test/vinyl/layout.result b/test/vinyl/layout.result
index 4af2c024..e6294d3c 100644
--- a/test/vinyl/layout.result
+++ b/test/vinyl/layout.result
@@ -20,7 +20,7 @@ space = box.schema.space.create('test', {engine='vinyl'})
 _ = space:create_index('pk', {parts = {{1, 'string', collation = 'unicode'}}, run_count_per_level=3})
 ---
 ...
-_ = space:create_index('sk', {parts = {{2, 'unsigned', is_nullable = true}}, run_count_per_level=3})
+_ = space:create_index('sk', {parts = {{2, 'unsigned'}}, run_count_per_level=3})
 ---
 ...
 -- Empty run
@@ -35,6 +35,9 @@ box.snapshot()
 ---
 - ok
 ...
+space.index.sk:alter{parts = {{2, 'unsigned', is_nullable = true}}}
+---
+...
 space:replace{'ЭЭЭ', box.NULL}
 ---
 - ['ЭЭЭ', null]
@@ -123,16 +126,16 @@ test_run:cmd("push filter 'offset: .*' to 'offset: <offset>'")
 ...
 result
 ---
-- - - 00000000000000000009.vylog
+- - - 00000000000000000010.vylog
     - - HEADER:
           type: INSERT
         BODY:
-          tuple: [0, {7: [{'field': 0, 'collation': 1, 'type': 'string'}], 9: 9, 12: 3,
-              6: 512}]
+          tuple: [0, {6: 512, 7: [{'field': 0, 'collation': 1, 'type': 'string'}],
+              9: 10, 12: 3, 13: 7}]
       - HEADER:
           type: INSERT
         BODY:
-          tuple: [5, {2: 8, 9: 9}]
+          tuple: [5, {2: 8, 9: 10}]
       - HEADER:
           type: INSERT
         BODY:
@@ -153,11 +156,11 @@ result
           type: INSERT
         BODY:
           tuple: [0, {0: 2, 5: 1, 6: 512, 7: [{'field': 1, 'is_nullable': true, 'type': 'unsigned'}],
-              9: 9, 12: 4}]
+              9: 10, 12: 4, 13: 7}]
       - HEADER:
           type: INSERT
         BODY:
-          tuple: [5, {0: 2, 2: 6, 9: 9}]
+          tuple: [5, {0: 2, 2: 6, 9: 10}]
       - HEADER:
           type: INSERT
         BODY:
@@ -197,7 +200,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [5, {0: 2, 2: 10, 9: 12}]
+          tuple: [5, {0: 2, 2: 10, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -207,7 +210,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [10, {0: 2, 9: 12}]
+          tuple: [10, {0: 2, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -217,7 +220,7 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [5, {2: 12, 9: 12}]
+          tuple: [5, {2: 12, 9: 13}]
       - HEADER:
           timestamp: <timestamp>
           type: INSERT
@@ -227,16 +230,16 @@ result
           timestamp: <timestamp>
           type: INSERT
         BODY:
-          tuple: [10, {9: 12}]
+          tuple: [10, {9: 13}]
   - - 00000000000000000008.index
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 7
+          min_lsn: 8
           max_key: ['ЭЭЭ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 9
+          max_lsn: 10
           min_key: ['ёёё']
       - HEADER:
           type: PAGEINFO
@@ -249,17 +252,17 @@ result
           min_key: ['ёёё']
   - - 00000000000000000008.run
     - - HEADER:
-          lsn: 9
+          lsn: 10
           type: INSERT
         BODY:
           tuple: ['ёёё', null]
       - HEADER:
-          lsn: 8
+          lsn: 9
           type: INSERT
         BODY:
           tuple: ['эээ', null]
       - HEADER:
-          lsn: 7
+          lsn: 8
           type: INSERT
         BODY:
           tuple: ['ЭЭЭ', null]
@@ -271,11 +274,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 10
+          min_lsn: 11
           max_key: ['ЮЮЮ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 12
+          max_lsn: 13
           min_key: ['ёёё']
       - HEADER:
           type: PAGEINFO
@@ -288,17 +291,17 @@ result
           min_key: ['ёёё']
   - - 00000000000000000012.run
     - - HEADER:
-          lsn: 10
+          lsn: 11
           type: REPLACE
         BODY:
           tuple: ['ёёё', 123]
       - HEADER:
-          lsn: 12
+          lsn: 13
           type: INSERT
         BODY:
           tuple: ['ююю', 789]
       - HEADER:
-          lsn: 11
+          lsn: 12
           type: INSERT
         BODY:
           tuple: ['ЮЮЮ', 456]
@@ -310,11 +313,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 7
+          min_lsn: 8
           max_key: [null, 'ЭЭЭ']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 9
+          max_lsn: 10
           min_key: [null, 'ёёё']
       - HEADER:
           type: PAGEINFO
@@ -327,17 +330,17 @@ result
           min_key: [null, 'ёёё']
   - - 00000000000000000006.run
     - - HEADER:
-          lsn: 9
+          lsn: 10
           type: INSERT
         BODY:
           tuple: [null, 'ёёё']
       - HEADER:
-          lsn: 8
+          lsn: 9
           type: INSERT
         BODY:
           tuple: [null, 'эээ']
       - HEADER:
-          lsn: 7
+          lsn: 8
           type: INSERT
         BODY:
           tuple: [null, 'ЭЭЭ']
@@ -349,11 +352,11 @@ result
     - - HEADER:
           type: RUNINFO
         BODY:
-          min_lsn: 10
+          min_lsn: 11
           max_key: [789, 'ююю']
           page_count: 1
           bloom_filter: <bloom_filter>
-          max_lsn: 12
+          max_lsn: 13
           min_key: [null, 'ёёё']
       - HEADER:
           type: PAGEINFO
@@ -366,22 +369,22 @@ result
           min_key: [null, 'ёёё']
   - - 00000000000000000010.run
     - - HEADER:
-          lsn: 10
+          lsn: 11
           type: DELETE
         BODY:
           key: [null, 'ёёё']
       - HEADER:
-          lsn: 10
+          lsn: 11
           type: REPLACE
         BODY:
           tuple: [123, 'ёёё']
       - HEADER:
-          lsn: 11
+          lsn: 12
           type: INSERT
         BODY:
           tuple: [456, 'ЮЮЮ']
       - HEADER:
-          lsn: 12
+          lsn: 13
           type: INSERT
         BODY:
           tuple: [789, 'ююю']
diff --git a/test/vinyl/layout.test.lua b/test/vinyl/layout.test.lua
index fd5787b7..27bf5d40 100644
--- a/test/vinyl/layout.test.lua
+++ b/test/vinyl/layout.test.lua
@@ -8,13 +8,15 @@ fun = require 'fun'
 
 space = box.schema.space.create('test', {engine='vinyl'})
 _ = space:create_index('pk', {parts = {{1, 'string', collation = 'unicode'}}, run_count_per_level=3})
-_ = space:create_index('sk', {parts = {{2, 'unsigned', is_nullable = true}}, run_count_per_level=3})
+_ = space:create_index('sk', {parts = {{2, 'unsigned'}}, run_count_per_level=3})
 
 -- Empty run
 space:insert{'ЁЁЁ', 777}
 space:delete{'ЁЁЁ'}
 box.snapshot()
 
+space.index.sk:alter{parts = {{2, 'unsigned', is_nullable = true}}}
+
 space:replace{'ЭЭЭ', box.NULL}
 space:replace{'эээ', box.NULL}
 space:replace{'ёёё', box.NULL}

Re: [PATCH 03/12] alter: do not rebuild secondary indexes on compatible pk changes

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/01 12:08]:

Please add a test case. 

I guess there is no infrastructure to test how well ALTER is
optimized. You will need to come up with one.

Let's come up with an engine-independent one.

The patch itself I've pushed.

> If the new cmp_def of a secondary index is compatible with the old one
> after the primary key parts have changed, we don't need to rebuild it,
> we just need to update its definition.
> ---
>  src/box/alter.cc | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 7f130297..174d53fa 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1432,7 +1432,13 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
>  					old_def->key_def, alter->pk_def);
>  		index_def_update_optionality(new_def, min_field_count);
>  		auto guard = make_scoped_guard([=] { index_def_delete(new_def); });
> -		(void) new RebuildIndex(alter, new_def, old_def);
> +		if (key_part_check_compatibility(old_def->cmp_def->parts,
> +						 old_def->cmp_def->part_count,
> +						 new_def->cmp_def->parts,
> +						 new_def->cmp_def->part_count))
> +			(void) new ModifyIndex(alter, new_def, old_def);
> +		else
> +			(void) new RebuildIndex(alter, new_def, old_def);
>  		guard.is_active = false;
>  	}
>  }
> -- 
> 2.11.0
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [PATCH 12/12] vinyl: allow to modify key definition if it does not require rebuild

[Konstantin Osipov]

I'm pushing this patch, but I dislike the strong similarity of two
msgpack keys in vylog (VY_LOG_MODIFY_LSM and
VY_LOG_KEY_MODIFY_LSN).

Please find a name to distance the two keys from each other in the
way they named, so it's harder to mistake one for another.

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/02 11:49]:

> +	VY_LOG_KEY_MODIFY_LSN		= 13,
>  };
>  
>  /** vy_log_key -> human readable name. */
> @@ -98,6 +99,7 @@ static const char *vy_log_key_name[] = {
>  	[VY_LOG_KEY_GC_LSN]		= "gc_lsn",
>  	[VY_LOG_KEY_TRUNCATE_COUNT]	= "truncate_count",
>  	[VY_LOG_KEY_CREATE_LSN]		= "create_lsn",
> +	[VY_LOG_KEY_MODIFY_LSN]		= "modify_lsn",
>  };
>  
>  /** vy_log_type -> human readable name. */
> @@ -115,6 +117,7 @@ static const char *vy_log_type_name[] = {
>  	[VY_LOG_DUMP_LSM]		= "dump_lsm",
>  	[VY_LOG_SNAPSHOT]		= "snapshot",
>  	[VY_LOG_TRUNCATE_LSM]		= "truncate_lsm",
> +	[VY_LOG_MODIFY_LSM]		= "modify_lsm",
>  };

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [PATCH 12/12] vinyl: allow to modify key definition if it does not require rebuild

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/02 11:49]:

> +--
> +-- Modify key definition without index rebuild.
> +--

One more thing, please, we need more functional tests.

Let's create a test plan which we will then reuse for non-empty
spaces as well:

Modify:
 - unique -> non unique, non-unique -> unique, or no change
 - nullable -> not-nullable, not-nullable -> nullable, or no
   change
 - a wider type is altered to a more narrow type, or vice versa,
   or the type types are incompatible, or no change,
 - key part added, removed, or all key parts changed, or no key
   parts changed.

Each line above creates a testing dimension, so in the end we
should have at least 3*4*4*4  = 192 different test cases.
Let's construct a test case in a way allowing us to add different
dimensions to it in the future.

> +s = box.schema.create_space('test', {engine = engine})
> +---
> +...
> +i1 = s:create_index('i1', {unique = true,  parts = {1, 'unsigned'}})
> +---
> +...
> +i2 = s:create_index('i2', {unique = false, parts = {2, 'unsigned'}})
> +---
> +...
> +i3 = s:create_index('i3', {unique = true,  parts = {3, 'unsigned'}})
> +---
> +...
> +_ = s:insert{1, 2, 3}
> +---
> +...
> +box.snapshot()
> +---
> +- ok
> +...
> +_ = s:insert{3, 2, 1}
> +---
> +...
> +i1:alter{parts = {1, 'integer'}}
> +---
> +...
> +_ = s:insert{-1, 2, 2}
> +---
> +...
> +i1:select()
> +---
> +- - [-1, 2, 2]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i2:select()
> +---
> +- - [-1, 2, 2]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i3:select()
> +---
> +- - [3, 2, 1]
> +  - [-1, 2, 2]
> +  - [1, 2, 3]
> +...
> +i2:alter{parts = {2, 'integer'}}
> +---
> +...
> +i3:alter{parts = {3, 'integer'}}
> +---
> +...
> +_ = s:replace{-1, -1, -1}
> +---
> +...
> +i1:select()
> +---
> +- - [-1, -1, -1]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i2:select()
> +---
> +- - [-1, -1, -1]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i3:select()
> +---
> +- - [-1, -1, -1]
> +  - [3, 2, 1]
> +  - [1, 2, 3]
> +...
> +box.snapshot()
> +---
> +- ok
> +...
> +_ = s:replace{-1, -2, -3}
> +---
> +...
> +_ = s:replace{-3, -2, -1}
> +---
> +...
> +i1:select()
> +---
> +- - [-3, -2, -1]
> +  - [-1, -2, -3]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i2:select()
> +---
> +- - [-3, -2, -1]
> +  - [-1, -2, -3]
> +  - [1, 2, 3]
> +  - [3, 2, 1]
> +...
> +i3:select()
> +---
> +- - [-1, -2, -3]
> +  - [-3, -2, -1]
> +  - [3, 2, 1]
> +  - [1, 2, 3]
> +...
> +s:drop()
> +---
> +...

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [PATCH 12/12] vinyl: allow to modify key definition if it does not require rebuild

[Konstantin Osipov]

* Konstantin Osipov <kostja@tarantool.org> [18/04/05 17:45]:

> One more thing, please, we need more functional tests.
> 
> Let's create a test plan which we will then reuse for non-empty
> spaces as well:
> 
> Modify:
>  - unique -> non unique, non-unique -> unique, or no change
>  - nullable -> not-nullable, not-nullable -> nullable, or no
>    change
>  - a wider type is altered to a more narrow type, or vice versa,
>    or the type types are incompatible, or no change,
>  - key part added, removed, or all key parts changed, or no key
>    parts changed.

A few more dimensions: 

 - engine=memtx or engine=vinyl.
 - a space format is defined or not
 - a space is empty or not
 - the alter happens during run time, replica join, or WAL recovery

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov