[PATCH 0/3] Fix crash on rollback of mixed DML/DDL transaction

[PATCH 0/3] Fix crash on rollback of mixed DML/DDL transaction

[Vladimir Davydov]

Struct txn_stmt contains a pointer to struct space, which is used by
memtx to revert the statement on transaction rollback. The pointer is
set when a statement is executed. The space a txn_stmt refers to may be
replaced in the space cache by a DDL operation (in case of memtx DDL and
DML statements may be mixed in the same transaction). This is okay as
long as the DDL operation guarantees that the old space remains valid.
All DDL operation observe this rule except MoveIndex and ModifyIndex.
The latter two swap indexes between the old and the new spaces thus
making the old space refer to a dummy place-holder index. This breaks
rollback code assumptions and may lead to a crash.

So this patch set reworks the above-mentioned DDL operations in such
a way that rather than swapping indexes, we copy the old index to
the new space, thus making sure that both spaces stay valid and can
be used for rollback. To achieve that we need to do some refactoring
at the side of the vinyl engine, which actively uses DDL callbacks.
The refactoring is done by the first two patches. The bug is fixed
by the final patch in the series.

https://github.com/tarantool/tarantool/issues/4368
https://github.com/tarantool/tarantool/commits/dv/gh-4368-fix-mixed-ddl-dml-crash

Vladimir Davydov (3):
  vinyl: use update_def index method to update vy_lsm on ddl
  vinyl: move unique check optimization setup to the space level
  alter: fix rollback in case DDL and DML are used in the same
    transaction

 src/box/alter.cc              |  85 +++++++++++++++++---------
 src/box/blackhole.c           |   2 +-
 src/box/key_def.c             |  56 +++++++++++-------
 src/box/key_def.h             |   4 +-
 src/box/memtx_space.c         |   7 ++-
 src/box/space.c               |  53 ++++++++++++++---
 src/box/space.h               |  77 +++++++++++++++---------
 src/box/sysview.c             |   2 +-
 src/box/vinyl.c               | 108 ++++++++++++++--------------------
 src/box/vy_lsm.c              |  18 +++++-
 src/box/vy_lsm.h              |  17 +++---
 test/box/transaction.result   |  27 +++++++++
 test/box/transaction.test.lua |  20 +++++++
 13 files changed, 310 insertions(+), 166 deletions(-)

-- 
2.20.1

[PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl

[Vladimir Davydov]

When an index definition is modified by DDL in such a way that doesn't
require index rebuild (e.g. a key part type is changed from unsigned to
integer), we move the index from the old space container to the new one,
see ModifyIndex. In case of Vinyl we also need to update key definitions
and options stored in vy_lsm. We do that in swap_index space method, but
this isn't entirely correct, as this method is also called when an index
is moved without any modifications, see MoveIndex. Let's do this from
update_def index method instead, which seems to be more suitable.

The only reason this code lives in swap_index space method now is that
we didn't have update_def index method when this functionality was
introduced in the first place.
---
 src/box/key_def.c | 56 ++++++++++++++++++++++++++++-------------------
 src/box/key_def.h |  4 ++--
 src/box/vinyl.c   | 14 ++++++++----
 3 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/src/box/key_def.c b/src/box/key_def.c
index ee758eef..9fa1ca49 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -81,18 +81,26 @@ const struct opt_def part_def_reg[] = {
 	OPT_END,
 };
 
-struct key_def *
-key_def_dup(const struct key_def *src)
+/**
+ * Return the size of memory occupied by the given key definition.
+ */
+static inline size_t
+key_def_copy_size(const struct key_def *def)
 {
 	size_t sz = 0;
-	for (uint32_t i = 0; i < src->part_count; i++)
-		sz += src->parts[i].path_len;
-	sz = key_def_sizeof(src->part_count, sz);
-	struct key_def *res = (struct key_def *)malloc(sz);
-	if (res == NULL) {
-		diag_set(OutOfMemory, sz, "malloc", "res");
-		return NULL;
-	}
+	for (uint32_t i = 0; i < def->part_count; i++)
+		sz += def->parts[i].path_len;
+	return key_def_sizeof(def->part_count, sz);
+}
+
+/**
+ * A helper function for key_def_copy() and key_def_dup() that
+ * copies key definition src of size sz to res without checking
+ * that the two key definitions have the same allocation size.
+ */
+static struct key_def *
+key_def_do_copy(struct key_def *res, const struct key_def *src, size_t sz)
+{
 	memcpy(res, src, sz);
 	/*
 	 * Update the paths pointers so that they refer to the
@@ -112,20 +120,24 @@ key_def_dup(const struct key_def *src)
 }
 
 void
-key_def_swap(struct key_def *old_def, struct key_def *new_def)
+key_def_copy(struct key_def *dest, const struct key_def *src)
 {
-	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]);
-		/*
-		 * Paths are allocated as a part of key_def so
-		 * we need to swap path pointers back - it's OK
-		 * as paths aren't supposed to change.
-		 */
-		assert(old_def->parts[i].path_len == new_def->parts[i].path_len);
-		SWAP(old_def->parts[i].path, new_def->parts[i].path);
+	size_t sz = key_def_copy_size(src);
+	assert(sz = key_def_copy_size(dest));
+	key_def_do_copy(dest, src, sz);
+}
+
+struct key_def *
+key_def_dup(const struct key_def *src)
+{
+	size_t sz = key_def_copy_size(src);
+	struct key_def *res = malloc(sz);
+	if (res == NULL) {
+		diag_set(OutOfMemory, sz, "malloc", "res");
+		return NULL;
 	}
-	SWAP(*old_def, *new_def);
+	key_def_do_copy(res, src, sz);
+	return res;
 }
 
 void
diff --git a/src/box/key_def.h b/src/box/key_def.h
index df83d055..73aefb9a 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -250,11 +250,11 @@ struct key_def *
 key_def_dup(const struct key_def *src);
 
 /**
- * Swap content of two key definitions in memory.
+ * Copy content of key definition src to dest.
  * The two key definitions must have the same size.
  */
 void
-key_def_swap(struct key_def *old_def, struct key_def *new_def);
+key_def_copy(struct key_def *dest, const struct key_def *src);
 
 /**
  * Delete @a key_def.
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index cf7af26b..4ac54138 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -963,6 +963,15 @@ vinyl_index_commit_drop(struct index *index, int64_t lsn)
 	vy_log_tx_try_commit();
 }
 
+static void
+vinyl_index_update_def(struct index *index)
+{
+	struct vy_lsm *lsm = vy_lsm(index);
+	lsm->opts = index->def->opts;
+	key_def_copy(lsm->key_def, index->def->key_def);
+	key_def_copy(lsm->cmp_def, index->def->cmp_def);
+}
+
 static bool
 vinyl_index_depends_on_pk(struct index *index)
 {
@@ -1200,9 +1209,6 @@ vinyl_space_swap_index(struct space *old_space, struct space *new_space,
 	SWAP(old_lsm->check_is_unique, new_lsm->check_is_unique);
 	SWAP(old_lsm->mem_format, new_lsm->mem_format);
 	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);
 
 	/* Update pointer to the primary key. */
 	vy_lsm_update_pk(old_lsm, vy_lsm(old_space->index_map[0]));
@@ -4741,7 +4747,7 @@ static const struct index_vtab vinyl_index_vtab = {
 	/* .abort_create = */ vinyl_index_abort_create,
 	/* .commit_modify = */ vinyl_index_commit_modify,
 	/* .commit_drop = */ vinyl_index_commit_drop,
-	/* .update_def = */ generic_index_update_def,
+	/* .update_def = */ vinyl_index_update_def,
 	/* .depends_on_pk = */ vinyl_index_depends_on_pk,
 	/* .def_change_requires_rebuild = */
 		vinyl_index_def_change_requires_rebuild,
-- 
2.20.1

[PATCH 2/3] vinyl: move unique check optimization setup to the space level

[Vladimir Davydov]

If any sub-set of fields indexed by a unique index is indexed by another
unique index, we can skip the uniqueness check for it. We use this to
optimize out unnecessary uniqueness checks in vinyl, where they may be
pretty costly, especially if the bloom filter doesn't reflect them.

Currently, whether to check if an index is unique or not is determined
in vinyl space constructor, which sets vy_lsm::check_is_unique flag for
the space indexes. This looks ugly, because this means that no other
engine can make use of this optimization without duplicating the code
setting up the flags. True, no other engine needs it now, but still it
doesn't feel right. Besides, the check_is_unique flag isn't actually an
index property - it's rather a property of a space so storing it in
vy_lsm looks wrong. Because of that we have to update the flag when an
index is reassigned to another space by MoveIndex DDL operation, see
vinyl_space_swap_index().

So let's store the flags indicating whether a uniqueness check is
required for a particular index in a bitmap in the space struct and
set it up in the generic space constructor, space_create().
---
 src/box/alter.cc      | 20 +++++++++----
 src/box/memtx_space.c |  5 +++-
 src/box/space.c       | 38 +++++++++++++++++++++++-
 src/box/space.h       | 51 +++++++++++++++++++++++++-------
 src/box/vinyl.c       | 68 ++++++++++++++++++-------------------------
 src/box/vy_lsm.c      |  1 -
 src/box/vy_lsm.h      |  8 -----
 7 files changed, 124 insertions(+), 67 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index f98a77a5..8668688c 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -733,6 +733,17 @@ alter_space_delete(struct alter_space *alter)
 	space_def_delete(alter->space_def);
 }
 
+/** A trivial wrapper around space_build_index(). */
+static void
+alter_space_build_index(struct space *src_space, struct space *new_space,
+			struct index *new_index)
+{
+	bool check_unique = space_needs_check_unique(new_space,
+						     new_index->def->iid);
+	space_build_index_xc(src_space, new_index, new_space->format,
+			     check_unique);
+}
+
 AlterSpaceOp::AlterSpaceOp(struct alter_space *alter)
 {
 	/* Add to the tail: operations must be processed in order. */
@@ -1280,8 +1291,7 @@ CreateIndex::prepare(struct alter_space *alter)
 		space_add_primary_key_xc(alter->new_space);
 		return;
 	}
-	space_build_index_xc(alter->old_space, new_index,
-			     alter->new_space->format);
+	alter_space_build_index(alter->old_space, alter->new_space, new_index);
 }
 
 void
@@ -1346,8 +1356,7 @@ RebuildIndex::prepare(struct alter_space *alter)
 	/* Get the new index and build it.  */
 	new_index = space_index(alter->new_space, new_index_def->iid);
 	assert(new_index != NULL);
-	space_build_index_xc(alter->old_space, new_index,
-			     alter->new_space->format);
+	alter_space_build_index(alter->old_space, alter->new_space, new_index);
 }
 
 void
@@ -1413,8 +1422,7 @@ TruncateIndex::prepare(struct alter_space *alter)
 	 * callback to load indexes during local recovery.
 	 */
 	assert(new_index != NULL);
-	space_build_index_xc(alter->new_space, new_index,
-			     alter->new_space->format);
+	alter_space_build_index(alter->new_space, alter->new_space, new_index);
 }
 
 void
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index a8e7b808..f6cc943f 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -1008,8 +1008,11 @@ memtx_build_on_replace(struct trigger *trigger, void *event)
 
 static int
 memtx_space_build_index(struct space *src_space, struct index *new_index,
-			struct tuple_format *new_format)
+			struct tuple_format *new_format, bool check_unique)
 {
+	/* In memtx unique check comes for free so we never skip it. */
+	(void)check_unique;
+
 	struct txn *txn = in_txn();
 	/**
 	 * If it's a secondary key, and we're not building them
diff --git a/src/box/space.c b/src/box/space.c
index e7babb2a..f520b307 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -32,6 +32,7 @@
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
+#include "bit/bit.h"
 #include "tuple_format.h"
 #include "trigger.h"
 #include "user.h"
@@ -156,16 +157,48 @@ space_create(struct space *space, struct engine *engine,
 		goto fail;
 	}
 	space->index = space->index_map + index_id_max + 1;
+	space->unique_index_bitmap = calloc(bitmap_size(index_id_max + 1), 1);
+	if (space->unique_index_bitmap == NULL) {
+		diag_set(OutOfMemory, bitmap_size(index_id_max + 1),
+			 "malloc", "unique_index_bitmap");
+		goto fail;
+	}
 	rlist_foreach_entry(index_def, key_list, link) {
 		struct index *index = space_create_index(space, index_def);
 		if (index == NULL)
 			goto fail_free_indexes;
 		space->index_map[index_def->iid] = index;
+		if (index_def->opts.is_unique)
+			bit_set(space->unique_index_bitmap, index_def->iid);
 	}
 	space_fill_index_map(space);
 	rlist_create(&space->parent_fk_constraint);
 	rlist_create(&space->child_fk_constraint);
 	rlist_create(&space->ck_constraint);
+
+	/*
+	 * Check if there are unique indexes that are contained
+	 * by other unique indexes. For them, we can skip check
+	 * for duplicates on INSERT. Prefer indexes with higher
+	 * ids for uniqueness check optimization as they are
+	 * likelier to have a "colder" cache.
+	 */
+	for (int i = space->index_count - 1; i >= 0; i--) {
+		struct index *index = space->index[i];
+		if (!bit_test(space->unique_index_bitmap, index->def->iid))
+			continue;
+		for (int j = 0; j < (int)space->index_count; j++) {
+			struct index *other = space->index[j];
+			if (i != j && bit_test(space->unique_index_bitmap,
+					       other->def->iid) &&
+			    key_def_contains(index->def->key_def,
+					     other->def->key_def)) {
+				bit_clear(space->unique_index_bitmap,
+					  index->def->iid);
+				break;
+			}
+		}
+	}
 	return 0;
 
 fail_free_indexes:
@@ -176,6 +209,7 @@ fail_free_indexes:
 	}
 fail:
 	free(space->index_map);
+	free(space->unique_index_bitmap);
 	if (space->def != NULL)
 		space_def_delete(space->def);
 	if (space->format != NULL)
@@ -214,6 +248,7 @@ space_delete(struct space *space)
 			index_delete(index);
 	}
 	free(space->index_map);
+	free(space->unique_index_bitmap);
 	if (space->format != NULL)
 		tuple_format_unref(space->format);
 	trigger_destroy(&space->before_replace);
@@ -635,11 +670,12 @@ generic_space_check_format(struct space *space, struct tuple_format *format)
 
 int
 generic_space_build_index(struct space *src_space, struct index *new_index,
-			  struct tuple_format *new_format)
+			  struct tuple_format *new_format, bool check_unique)
 {
 	(void)src_space;
 	(void)new_index;
 	(void)new_format;
+	(void)check_unique;
 	return 0;
 }
 
diff --git a/src/box/space.h b/src/box/space.h
index 88db4ec5..d44c9fdc 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -33,6 +33,7 @@
 #include "user_def.h"
 #include "space_def.h"
 #include "small/rlist.h"
+#include "bit/bit.h"
 #include "engine.h"
 #include "index.h"
 #include "error.h"
@@ -112,14 +113,20 @@ struct space_vtab {
 	 * supposed to assure that all tuples conform to the new
 	 * format.
 	 *
-	 * @param src_space   space to use as build source
-	 * @param new_index   index to build
-	 * @param new_format  format for validating tuples
-	 * @retval  0         success
-	 * @retval -1         build failed
+	 * @param src_space     space to use as build source
+	 * @param new_index     index to build
+	 * @param new_format    format for validating tuples
+	 * @param check_unique  if this flag is set the build procedure
+	 *                      must check the uniqueness constraint of
+	 *                      the new index, otherwise the check may
+	 *                      be optimized out even if the index is
+	 *                      marked as unique
+	 *
+	 * @retval  0           success
+	 * @retval -1           build failed
 	 */
 	int (*build_index)(struct space *src_space, struct index *new_index,
-			   struct tuple_format *new_format);
+			   struct tuple_format *new_format, bool check_unique);
 	/**
 	 * Exchange two index objects in two spaces. Used
 	 * to update a space with a newly built index, while
@@ -195,6 +202,15 @@ struct space {
 	 * of index id.
 	 */
 	struct index **index;
+	/**
+	 * If bit i is set, the unique constraint of index i must
+	 * be checked before inserting a tuple into this space.
+	 * Note, it isn't quite the same as index_opts::is_unique,
+	 * as we don't need to check the unique constraint of
+	 * a unique index in case the uniqueness of the indexed
+	 * fields is guaranteed by another unique index.
+	 */
+	void *unique_index_bitmap;
 	/**
 	 * List of check constraints linked with
 	 * ck_constraint::link.
@@ -265,6 +281,17 @@ space_index(struct space *space, uint32_t id)
 	return NULL;
 }
 
+/**
+ * Return true if the unique constraint must be checked for
+ * the index with the given id before inserting a tuple into
+ * the space.
+ */
+static inline bool
+space_needs_check_unique(struct space *space, uint32_t index_id)
+{
+	return bit_test(space->unique_index_bitmap, index_id);
+}
+
 /**
  * Return key_def of the index identified by id or NULL
  * if there is no such index.
@@ -403,9 +430,10 @@ space_drop_primary_key(struct space *space)
 
 static inline int
 space_build_index(struct space *src_space, struct index *new_index,
-		  struct tuple_format *new_format)
+		  struct tuple_format *new_format, bool check_unique)
 {
-	return src_space->vtab->build_index(src_space, new_index, new_format);
+	return src_space->vtab->build_index(src_space, new_index,
+					    new_format, check_unique);
 }
 
 static inline void
@@ -490,7 +518,7 @@ int generic_space_add_primary_key(struct space *space);
 void generic_space_drop_primary_key(struct space *space);
 int generic_space_check_format(struct space *, struct tuple_format *);
 int generic_space_build_index(struct space *, struct index *,
-			      struct tuple_format *);
+			      struct tuple_format *, bool);
 int generic_space_prepare_alter(struct space *, struct space *);
 void generic_space_invalidate(struct space *);
 
@@ -588,9 +616,10 @@ space_check_format_xc(struct space *space, struct tuple_format *format)
 
 static inline void
 space_build_index_xc(struct space *src_space, struct index *new_index,
-		     struct tuple_format *new_format)
+		     struct tuple_format *new_format, bool check_unique)
 {
-	if (space_build_index(src_space, new_index, new_format) != 0)
+	if (space_build_index(src_space, new_index,
+			      new_format, check_unique) != 0)
 		diag_raise();
 }
 
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 4ac54138..d2a02a21 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -647,27 +647,6 @@ vinyl_engine_create_space(struct engine *engine, struct space_def *def,
 
 	/* Format is now referenced by the space. */
 	tuple_format_unref(format);
-
-	/*
-	 * Check if there are unique indexes that are contained
-	 * by other unique indexes. For them, we can skip check
-	 * for duplicates on INSERT. Prefer indexes with higher
-	 * ids for uniqueness check optimization as they are
-	 * likelier to have a "colder" cache.
-	 */
-	for (int i = space->index_count - 1; i >= 0; i--) {
-		struct vy_lsm *lsm = vy_lsm(space->index[i]);
-		if (!lsm->check_is_unique)
-			continue;
-		for (int j = 0; j < (int)space->index_count; j++) {
-			struct vy_lsm *other = vy_lsm(space->index[j]);
-			if (other != lsm && other->check_is_unique &&
-			    key_def_contains(lsm->key_def, other->key_def)) {
-				lsm->check_is_unique = false;
-				break;
-			}
-		}
-	}
 	return space;
 }
 
@@ -1206,7 +1185,6 @@ vinyl_space_swap_index(struct space *old_space, struct space *new_space,
 				 old_index_id, new_index_id);
 
 	SWAP(old_lsm, new_lsm);
-	SWAP(old_lsm->check_is_unique, new_lsm->check_is_unique);
 	SWAP(old_lsm->mem_format, new_lsm->mem_format);
 	SWAP(old_lsm->disk_format, new_lsm->disk_format);
 
@@ -1564,10 +1542,9 @@ vy_check_is_unique_primary(struct vy_tx *tx, const struct vy_read_view **rv,
 			   struct vy_lsm *lsm, struct tuple *stmt)
 {
 	assert(lsm->index_id == 0);
+	assert(lsm->opts.is_unique);
 	assert(vy_stmt_type(stmt) == IPROTO_INSERT);
 
-	if (!lsm->check_is_unique)
-		return 0;
 	struct tuple *found;
 	if (vy_get(lsm, tx, rv, stmt, &found))
 		return -1;
@@ -1610,7 +1587,7 @@ vy_check_is_unique_secondary_one(struct vy_tx *tx, const struct vy_read_view **r
 	 * without modifying the secondary key and so there's
 	 * actually no conflict. For INSERT this can only happen
 	 * if we optimized out the primary index uniqueness check
-	 * (see vy_lsm::check_is_unique), in which case we must
+	 * (see space_needs_check_unique()), in which case we must
 	 * fail here.
 	 */
 	if (found != NULL && vy_stmt_type(stmt) == IPROTO_REPLACE &&
@@ -1646,8 +1623,7 @@ vy_check_is_unique_secondary(struct vy_tx *tx, const struct vy_read_view **rv,
 			     const char *space_name, const char *index_name,
 			     struct vy_lsm *lsm, struct tuple *stmt)
 {
-	if (!lsm->check_is_unique)
-		return 0;
+	assert(lsm->opts.is_unique);
 	if (!lsm->cmp_def->is_multikey) {
 		return vy_check_is_unique_secondary_one(tx, rv,
 				space_name, index_name, lsm, stmt,
@@ -1699,7 +1675,8 @@ vy_check_is_unique(struct vy_env *env, struct vy_tx *tx,
 	 * if this is INSERT, because REPLACE will silently overwrite
 	 * the existing tuple, if any.
 	 */
-	if (vy_stmt_type(stmt) == IPROTO_INSERT) {
+	if (space_needs_check_unique(space, 0) &&
+	    vy_stmt_type(stmt) == IPROTO_INSERT) {
 		struct vy_lsm *lsm = vy_lsm(space->index[0]);
 		if (vy_check_is_unique_primary(tx, rv, space_name(space),
 					       index_name_by_id(space, 0),
@@ -1713,6 +1690,8 @@ vy_check_is_unique(struct vy_env *env, struct vy_tx *tx,
 	 */
 	for (uint32_t i = 1; i < space->index_count; i++) {
 		struct vy_lsm *lsm = vy_lsm(space->index[i]);
+		if (!space_needs_check_unique(space, lsm->index_id))
+			continue;
 		if (key_update_can_be_skipped(lsm->key_def->column_mask,
 					      column_mask))
 			continue;
@@ -4028,6 +4007,11 @@ struct vy_build_ctx {
 	struct vy_lsm *lsm;
 	/** Format to check new tuples against. */
 	struct tuple_format *format;
+	/**
+	 * Check the unique constraint of the new index
+	 * if this flag is set.
+	 */
+	bool check_unique;
 	/**
 	 * Names of the altered space and the new index.
 	 * Used for error reporting.
@@ -4063,7 +4047,7 @@ vy_build_on_replace(struct trigger *trigger, void *event)
 		goto err;
 
 	/* Check key uniqueness if necessary. */
-	if (stmt->new_tuple != NULL &&
+	if (ctx->check_unique && stmt->new_tuple != NULL &&
 	    vy_check_is_unique_secondary(tx, vy_tx_read_view(tx),
 					 ctx->space_name, ctx->index_name,
 					 lsm, stmt->new_tuple) != 0)
@@ -4138,7 +4122,8 @@ vy_build_insert_stmt(struct vy_lsm *lsm, struct vy_mem *mem,
 static int
 vy_build_insert_tuple(struct vy_env *env, struct vy_lsm *lsm,
 		      const char *space_name, const char *index_name,
-		      struct tuple_format *new_format, struct tuple *tuple)
+		      struct tuple_format *new_format, bool check_unique,
+		      struct tuple *tuple)
 {
 	int rc;
 	struct vy_mem *mem = lsm->mem;
@@ -4175,13 +4160,16 @@ vy_build_insert_tuple(struct vy_env *env, struct vy_lsm *lsm,
 	 * uniqueness check helper is sensitive to the statement
 	 * type, we must not use the original tuple for the check.
 	 */
-	vy_mem_pin(mem);
-	rc = vy_check_is_unique_secondary(NULL, &env->xm->p_committed_read_view,
-					  space_name, index_name, lsm, stmt);
-	vy_mem_unpin(mem);
-	if (rc != 0) {
-		tuple_unref(stmt);
-		return -1;
+	if (check_unique) {
+		vy_mem_pin(mem);
+		rc = vy_check_is_unique_secondary(NULL,
+				&env->xm->p_committed_read_view,
+				space_name, index_name, lsm, stmt);
+		vy_mem_unpin(mem);
+		if (rc != 0) {
+			tuple_unref(stmt);
+			return -1;
+		}
 	}
 
 	/* Insert the new tuple into the in-memory index. */
@@ -4329,7 +4317,7 @@ vy_build_recover(struct vy_env *env, struct vy_lsm *lsm, struct vy_lsm *pk)
 
 static int
 vinyl_space_build_index(struct space *src_space, struct index *new_index,
-			struct tuple_format *new_format)
+			struct tuple_format *new_format, bool check_unique)
 {
 	struct vy_env *env = vy_env(src_space->engine);
 	struct vy_lsm *pk = vy_lsm(src_space->index[0]);
@@ -4398,6 +4386,7 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	struct vy_build_ctx ctx;
 	ctx.lsm = new_lsm;
 	ctx.format = new_format;
+	ctx.check_unique = check_unique;
 	ctx.space_name = space_name(src_space);
 	ctx.index_name = new_index->def->name;
 	ctx.is_failed = false;
@@ -4441,7 +4430,8 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 			rc = vy_build_insert_tuple(env, new_lsm,
 						   space_name(src_space),
 						   new_index->def->name,
-						   new_format, tuple);
+						   new_format, check_unique,
+						   tuple);
 			if (rc != 0)
 				break;
 		}
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 48590521..c67db87a 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -207,7 +207,6 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 	lsm->index_id = index_def->iid;
 	lsm->group_id = group_id;
 	lsm->opts = index_def->opts;
-	lsm->check_is_unique = lsm->opts.is_unique;
 	vy_lsm_read_set_new(&lsm->read_set);
 
 	lsm_env->lsm_count++;
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index 0a8e9100..d9e4b582 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -206,14 +206,6 @@ struct vy_lsm {
 	 * to a primary index.
 	 */
 	struct key_def *pk_in_cmp_def;
-	/**
-	 * If the following flag is set, the index this LSM tree
-	 * is created for is unique and it must be checked for
-	 * duplicates on INSERT. Otherwise, the check can be skipped,
-	 * either because the index is not unique or it is a part
-	 * of another unique index.
-	 */
-	bool check_is_unique;
 	/**
 	 * Tuple format for tuples of this LSM tree created when
 	 * reading pages from disk.
-- 
2.20.1

[PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction

[Vladimir Davydov]

A txn_stmt keeps a reference to the space it modifies. Memtx uses this
space reference to revert the statement on error or voluntary rollback
so the space must stay valid throughout the whole transaction.

The problem is DML statements may be mixed in a DDL transaction, in
which case we always roll back DML statements first and only then run
rollback triggers to revert changes done to the schema by the DDL
statements. This means every DDL statement modifying the space cache
must be implemented in such a way that it leaves both the old space
and the new space valid until commit or rollback. All DDL operations
follow this rule except MoveIndex and ModifyIndex, which swap indexes
between the old and the new spaces thus making the old space refer to
a place-holder index. An attempt to revert a change from such a dummy
index will most likely lead to a crash.

To fix this, let's split the index swapping operation into two steps:
first assign the old index to the new space without removing it from
the old space; then, on commit, replace the old index with the new
(place-holder) index in the old space so that it gets destroyed by the
alter_space destructor.

Closes #4368
---
 src/box/alter.cc              | 65 +++++++++++++++++++++++------------
 src/box/blackhole.c           |  2 +-
 src/box/memtx_space.c         |  2 +-
 src/box/space.c               | 15 ++++----
 src/box/space.h               | 26 +++++---------
 src/box/sysview.c             |  2 +-
 src/box/vinyl.c               | 26 ++++----------
 src/box/vy_lsm.c              | 17 +++++++++
 src/box/vy_lsm.h              |  9 +++++
 test/box/transaction.result   | 27 +++++++++++++++
 test/box/transaction.test.lua | 20 +++++++++++
 11 files changed, 140 insertions(+), 71 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 8668688c..cd01d734 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1134,24 +1134,46 @@ DropIndex::commit(struct alter_space *alter, int64_t signature)
 class MoveIndex: public AlterSpaceOp
 {
 public:
-	MoveIndex(struct alter_space *alter, uint32_t iid_arg)
-		:AlterSpaceOp(alter), iid(iid_arg) {}
-	/** id of the index on the move. */
-	uint32_t iid;
+	MoveIndex(struct alter_space *alter, struct index *index)
+		: AlterSpaceOp(alter), old_index(index), new_index(NULL) {}
+	struct index *old_index;
+	struct index *new_index;
 	virtual void alter(struct alter_space *alter);
+	virtual void commit(struct alter_space *alter, int64_t lsn);
 	virtual void rollback(struct alter_space *alter);
 };
 
 void
 MoveIndex::alter(struct alter_space *alter)
 {
-	space_swap_index(alter->old_space, alter->new_space, iid, iid);
+	/*
+	 * Replace the new index with the old index in the new space,
+	 * but don't remove the old index from the old space so that
+	 * the old space stays valid. We must guarantee the validity
+	 * of the old space, because txn statements corresponding to
+	 * DML requests processed in the same transaction may still
+	 * use it for rollback.
+	 */
+	new_index = space_index(alter->new_space, old_index->def->iid);
+	alter->new_space->index_map[new_index->def->iid] = old_index;
+	space_move_index(alter->new_space, old_index);
+}
+
+void
+MoveIndex::commit(struct alter_space *alter, int64_t)
+{
+	/*
+	 * Assign the new index to the old space so that it gets
+	 * destroyed by the alter_space destructor.
+	 */
+	alter->old_space->index_map[old_index->def->iid] = new_index;
 }
 
 void
 MoveIndex::rollback(struct alter_space *alter)
 {
-	space_swap_index(alter->old_space, alter->new_space, iid, iid);
+	alter->new_space->index_map[new_index->def->iid] = new_index;
+	space_move_index(alter->old_space, old_index);
 }
 
 /**
@@ -1201,34 +1223,33 @@ ModifyIndex::alter(struct alter_space *alter)
 	new_index = space_index(alter->new_space, new_index_def->iid);
 	assert(old_index->def->iid == new_index->def->iid);
 	/*
-	 * Move the old index to the new space to preserve the
-	 * original data, but use the new definition.
+	 * Update the old index definition and copy it to the new
+	 * space. See also MoveIndex::alter.
 	 */
-	space_swap_index(alter->old_space, alter->new_space,
-			 old_index->def->iid, new_index->def->iid);
-	SWAP(old_index, new_index);
 	SWAP(old_index->def, new_index->def);
-	index_update_def(new_index);
+	index_update_def(old_index);
+	alter->new_space->index_map[new_index->def->iid] = old_index;
+	space_move_index(alter->new_space, old_index);
 }
 
 void
 ModifyIndex::commit(struct alter_space *alter, int64_t signature)
 {
-	(void)alter;
-	index_commit_modify(new_index, signature);
+	index_commit_modify(old_index, signature);
+	/*
+	 * Assign the new index to the old space so that it gets
+	 * destroyed by the alter_space destructor.
+	 */
+	alter->old_space->index_map[old_index->def->iid] = new_index;
 }
 
 void
 ModifyIndex::rollback(struct alter_space *alter)
 {
-	/*
-	 * Restore indexes.
-	 */
-	space_swap_index(alter->old_space, alter->new_space,
-			 old_index->def->iid, new_index->def->iid);
-	SWAP(old_index, new_index);
 	SWAP(old_index->def, new_index->def);
 	index_update_def(old_index);
+	alter->new_space->index_map[new_index->def->iid] = new_index;
+	space_move_index(alter->old_space, old_index);
 }
 
 ModifyIndex::~ModifyIndex()
@@ -1647,7 +1668,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 							     min_field_count);
 				(void) new ModifyIndex(alter, old_index, new_def);
 			} else {
-				(void) new MoveIndex(alter, old_def->iid);
+				(void) new MoveIndex(alter, old_index);
 			}
 			continue;
 		}
@@ -2251,7 +2272,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 		alter_space_move_indexes(alter, 0, iid);
 		if (index_def_cmp(index_def, old_index->def) == 0) {
 			/* Index is not changed so just move it. */
-			(void) new MoveIndex(alter, old_index->def->iid);
+			(void) new MoveIndex(alter, old_index);
 		} else if (index_def_change_requires_rebuild(old_index,
 							     index_def)) {
 			if (index_is_used_by_fk_constraint(&old_space->parent_fk_constraint,
diff --git a/src/box/blackhole.c b/src/box/blackhole.c
index b69e543a..c0652fe1 100644
--- a/src/box/blackhole.c
+++ b/src/box/blackhole.c
@@ -127,7 +127,7 @@ static const struct space_vtab blackhole_space_vtab = {
 	/* .drop_primary_key = */ generic_space_drop_primary_key,
 	/* .check_format = */ generic_space_check_format,
 	/* .build_index = */ generic_space_build_index,
-	/* .swap_index = */ generic_space_swap_index,
+	/* .move_index = */ generic_space_move_index,
 	/* .prepare_alter = */ generic_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f6cc943f..65f0bc6f 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -1165,7 +1165,7 @@ static const struct space_vtab memtx_space_vtab = {
 	/* .drop_primary_key = */ memtx_space_drop_primary_key,
 	/* .check_format  = */ memtx_space_check_format,
 	/* .build_index = */ memtx_space_build_index,
-	/* .swap_index = */ generic_space_swap_index,
+	/* .move_index = */ generic_space_move_index,
 	/* .prepare_alter = */ memtx_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
diff --git a/src/box/space.c b/src/box/space.c
index f520b307..9dcba2fe 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -283,14 +283,6 @@ space_index_key_def(struct space *space, uint32_t id)
 	return NULL;
 }
 
-void
-generic_space_swap_index(struct space *old_space, struct space *new_space,
-			 uint32_t old_index_id, uint32_t new_index_id)
-{
-	SWAP(old_space->index_map[old_index_id],
-	     new_space->index_map[new_index_id]);
-}
-
 void
 space_run_triggers(struct space *space, bool yesno)
 {
@@ -679,6 +671,13 @@ generic_space_build_index(struct space *src_space, struct index *new_index,
 	return 0;
 }
 
+void
+generic_space_move_index(struct space *space, struct index *index)
+{
+	(void)space;
+	(void)index;
+}
+
 int
 generic_space_prepare_alter(struct space *old_space, struct space *new_space)
 {
diff --git a/src/box/space.h b/src/box/space.h
index d44c9fdc..355918d2 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -128,12 +128,12 @@ struct space_vtab {
 	int (*build_index)(struct space *src_space, struct index *new_index,
 			   struct tuple_format *new_format, bool check_unique);
 	/**
-	 * 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.
+	 * Called after assigning the given index to the space to
+	 * update engine-specific data that depends on the space
+	 * definition. E.g. the vinyl engine uses this method to
+	 * update tuple formats referenced by the index.
 	 */
-	void (*swap_index)(struct space *old_space, struct space *new_space,
-			   uint32_t old_index_id, uint32_t new_index_id);
+	void (*move_index)(struct space *space, struct index *index);
 	/**
 	 * Notify the engine about the changed space,
 	 * before it's done, to prepare 'new_space' object.
@@ -384,14 +384,6 @@ space_ephemeral_delete(struct space *space, const char *key)
 	return space->vtab->ephemeral_delete(space, key);
 }
 
-/**
- * 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)
 {
@@ -437,12 +429,9 @@ space_build_index(struct space *src_space, struct index *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)
+space_move_index(struct space *space, struct index *index)
 {
-	assert(old_space->vtab == new_space->vtab);
-	return new_space->vtab->swap_index(old_space, new_space,
-					   old_index_id, new_index_id);
+	return space->vtab->move_index(space, index);
 }
 
 static inline int
@@ -519,6 +508,7 @@ void generic_space_drop_primary_key(struct space *space);
 int generic_space_check_format(struct space *, struct tuple_format *);
 int generic_space_build_index(struct space *, struct index *,
 			      struct tuple_format *, bool);
+void generic_space_move_index(struct space *, struct index *);
 int generic_space_prepare_alter(struct space *, struct space *);
 void generic_space_invalidate(struct space *);
 
diff --git a/src/box/sysview.c b/src/box/sysview.c
index 46cf1e13..13ade9b1 100644
--- a/src/box/sysview.c
+++ b/src/box/sysview.c
@@ -506,7 +506,7 @@ static const struct space_vtab sysview_space_vtab = {
 	/* .drop_primary_key = */ generic_space_drop_primary_key,
 	/* .check_format = */ generic_space_check_format,
 	/* .build_index = */ generic_space_build_index,
-	/* .swap_index = */ generic_space_swap_index,
+	/* .move_index = */ generic_space_move_index,
 	/* .prepare_alter = */ generic_space_prepare_alter,
 	/* .invalidate = */ generic_space_invalidate,
 };
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index d2a02a21..d3658a31 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1171,26 +1171,12 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 }
 
 static void
-vinyl_space_swap_index(struct space *old_space, struct space *new_space,
-		       uint32_t old_index_id, uint32_t new_index_id)
+vinyl_space_move_index(struct space *space, struct index *index)
 {
-	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, new_lsm);
-	SWAP(old_lsm->mem_format, new_lsm->mem_format);
-	SWAP(old_lsm->disk_format, new_lsm->disk_format);
-
-	/* Update pointer to the primary key. */
-	vy_lsm_update_pk(old_lsm, vy_lsm(old_space->index_map[0]));
-	vy_lsm_update_pk(new_lsm, vy_lsm(new_space->index_map[0]));
+	struct vy_lsm *lsm = vy_lsm(index);
+	struct vy_lsm *pk = vy_lsm(space_index(space, 0));
+	vy_lsm_update_pk(lsm, pk);
+	vy_lsm_update_format(lsm, space->format);
 }
 
 static int
@@ -4726,7 +4712,7 @@ static const struct space_vtab vinyl_space_vtab = {
 	/* .drop_primary_key = */ generic_space_drop_primary_key,
 	/* .check_format = */ vinyl_space_check_format,
 	/* .build_index = */ vinyl_space_build_index,
-	/* .swap_index = */ vinyl_space_swap_index,
+	/* .move_index = */ vinyl_space_move_index,
 	/* .prepare_alter = */ vinyl_space_prepare_alter,
 	/* .invalidate = */ vinyl_space_invalidate,
 };
diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index c67db87a..e824a6aa 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -284,6 +284,23 @@ vy_lsm_delete(struct vy_lsm *lsm)
 	free(lsm);
 }
 
+void
+vy_lsm_update_format(struct vy_lsm *lsm, struct tuple_format *format)
+{
+	tuple_format_ref(format);
+	tuple_format_unref(lsm->mem_format);
+	lsm->mem_format = format;
+	/*
+	 * Disk format is the same across all secondary indexes
+	 * and it doesn't depend on the space format.
+	 */
+	if (lsm->index_id == 0) {
+		tuple_format_ref(format);
+		tuple_format_unref(lsm->disk_format);
+		lsm->disk_format = format;
+	}
+}
+
 int
 vy_lsm_create(struct vy_lsm *lsm)
 {
diff --git a/src/box/vy_lsm.h b/src/box/vy_lsm.h
index d9e4b582..597ad099 100644
--- a/src/box/vy_lsm.h
+++ b/src/box/vy_lsm.h
@@ -340,6 +340,15 @@ vy_lsm_new(struct vy_lsm_env *lsm_env, struct vy_cache_env *cache_env,
 void
 vy_lsm_delete(struct vy_lsm *lsm);
 
+/**
+ * Update the formats referenced by the given LSM tree after
+ * it has been assigned to another space container on DDL.
+ * The new format must be compatible with tuples stored in
+ * the space.
+ */
+void
+vy_lsm_update_format(struct vy_lsm *lsm, struct tuple_format *format);
+
 /**
  * Return true if the LSM tree has no statements, neither on disk
  * nor in memory.
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 9a48f2f7..4ebd820e 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -778,3 +778,30 @@ box.begin() drop() box.rollback()
 box.begin() drop() box.commit()
 ---
 ...
+--
+-- gh-4368: transaction rollback leads to a crash if DDL and DML statements
+-- are mixed in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+for i = 1, 100 do
+    box.begin()
+    s = box.schema.space.create('test')
+    s:create_index('pk')
+    s:create_index('sk', {unique = true, parts = {2, 'unsigned'}})
+    s:insert{1, 1}
+    s.index.sk:alter{unique = false}
+    s:insert{2, 1}
+    s.index.sk:drop()
+    s:delete{2}
+    box.rollback()
+    fiber.sleep(0)
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 0792cc3c..79929b93 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -415,3 +415,23 @@ box.begin() create() box.rollback()
 box.begin() create() box.commit()
 box.begin() drop() box.rollback()
 box.begin() drop() box.commit()
+
+--
+-- gh-4368: transaction rollback leads to a crash if DDL and DML statements
+-- are mixed in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+for i = 1, 100 do
+    box.begin()
+    s = box.schema.space.create('test')
+    s:create_index('pk')
+    s:create_index('sk', {unique = true, parts = {2, 'unsigned'}})
+    s:insert{1, 1}
+    s.index.sk:alter{unique = false}
+    s:insert{2, 1}
+    s.index.sk:drop()
+    s:delete{2}
+    box.rollback()
+    fiber.sleep(0)
+end;
+test_run:cmd("setopt delimiter ''");
-- 
2.20.1

[tarantool-patches] Re: [PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> When an index definition is modified by DDL in such a way that doesn't
> require index rebuild (e.g. a key part type is changed from unsigned to
> integer), we move the index from the old space container to the new one,
> see ModifyIndex. In case of Vinyl we also need to update key definitions
> and options stored in vy_lsm. We do that in swap_index space method, but
> this isn't entirely correct, as this method is also called when an index
> is moved without any modifications, see MoveIndex. Let's do this from
> update_def index method instead, which seems to be more suitable.
> 
> The only reason this code lives in swap_index space method now is that
> we didn't have update_def index method when this functionality was
> introduced in the first place.

First of all, it is extremely annoying that instead of testing
this properly despite my humble, repeated requests we waited for
these bugs to surface and now address them with urgent
patches.

I see if I could find anything wrong with this patch but I can't.

> +static struct key_def *
> +key_def_do_copy(struct key_def *res, const struct key_def *src, size_t sz)

I personally prefer _impl to do_.

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/3] vinyl: move unique check optimization setup to the space level

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> If any sub-set of fields indexed by a unique index is indexed by another
> unique index, we can skip the uniqueness check for it. We use this to
> optimize out unnecessary uniqueness checks in vinyl, where they may be
> pretty costly, especially if the bloom filter doesn't reflect them.
> 
> Currently, whether to check if an index is unique or not is determined
> in vinyl space constructor, which sets vy_lsm::check_is_unique flag for
> the space indexes. This looks ugly, because this means that no other
> engine can make use of this optimization without duplicating the code
> setting up the flags. True, no other engine needs it now, but still it
> doesn't feel right. Besides, the check_is_unique flag isn't actually an
> index property - it's rather a property of a space so storing it in
> vy_lsm looks wrong. Because of that we have to update the flag when an
> index is reassigned to another space by MoveIndex DDL operation, see
> vinyl_space_swap_index().
> 
> So let's store the flags indicating whether a uniqueness check is
> required for a particular index in a bitmap in the space struct and
> set it up in the generic space constructor, space_create().

LGTM
> +/** A trivial wrapper around space_build_index(). */
> +static void
> +alter_space_build_index(struct space *src_space, struct space *new_space,
> +			struct index *new_index)
> +{
> +	bool check_unique = space_needs_check_unique(new_space,
> +						     new_index->def->iid);

check_unique is not proper English, please use
check_unique_constraint or check_unique_ck.

> +	space_build_index_xc(src_space, new_index, new_space->format,
> +			     check_unique);

Why can't you move testing check_unique property inside space_build_index
or even the engine specific implementation of space_build_index?

> +	space->unique_index_bitmap = calloc(bitmap_size(index_id_max + 1), 1);

Let's use check_unique_constraint_map
> +/**
> + * Return true if the unique constraint must be checked for
> + * the index with the given id before inserting a tuple into
> + * the space.
> + */
> +static inline bool
> +space_needs_check_unique(struct space *space, uint32_t index_id)

space_needs_unique_constraint_check

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> A txn_stmt keeps a reference to the space it modifies. Memtx uses this
> space reference to revert the statement on error or voluntary rollback
> so the space must stay valid throughout the whole transaction.
> 
> The problem is DML statements may be mixed in a DDL transaction, in
> which case we always roll back DML statements first and only then run
> rollback triggers to revert changes done to the schema by the DDL
> statements. This means every DDL statement modifying the space cache
> must be implemented in such a way that it leaves both the old space
> and the new space valid until commit or rollback.

How are we going to enforce this invariant by code structure going
forward other than patching it hastily when it breaks?


-- 
Konstantin Osipov, Moscow, Russia

Re: [tarantool-patches] Re: [PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl

[Vladimir Davydov]

On Thu, Jul 25, 2019 at 01:25:01AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> > When an index definition is modified by DDL in such a way that doesn't
> > require index rebuild (e.g. a key part type is changed from unsigned to
> > integer), we move the index from the old space container to the new one,
> > see ModifyIndex. In case of Vinyl we also need to update key definitions
> > and options stored in vy_lsm. We do that in swap_index space method, but
> > this isn't entirely correct, as this method is also called when an index
> > is moved without any modifications, see MoveIndex. Let's do this from
> > update_def index method instead, which seems to be more suitable.
> > 
> > The only reason this code lives in swap_index space method now is that
> > we didn't have update_def index method when this functionality was
> > introduced in the first place.
> 
> First of all, it is extremely annoying that instead of testing
> this properly despite my humble, repeated requests we waited for
> these bugs to surface and now address them with urgent
> patches.

Well, that's how things typically go. We write code, we find bugs, we
fix them. It's impossible to write bug-free code. Tests can reduce the
possibility of a bug, but they aren't a silver bullet, especially when
written by a developer whose goal is to commit a feature ;)

Regarding this particular bug, to tell you the truth, I knew it existed
when I submitted the original patch (it was me who filed the ticket
after all), but I also knew that it could be fixed without redesign so
I proceeded with commit ;)

I often deliberately commit buggy code, because it's easier to proceed
knowing that the design is agreed upon and committed, easier to build
on top of that.

> 
> I see if I could find anything wrong with this patch but I can't.
> 
> > +static struct key_def *
> > +key_def_do_copy(struct key_def *res, const struct key_def *src, size_t sz)
> 
> I personally prefer _impl to do_.

NP, will rename if you like.

Re: [tarantool-patches] Re: [PATCH 2/3] vinyl: move unique check optimization setup to the space level

[Vladimir Davydov]

On Thu, Jul 25, 2019 at 01:31:41AM +0300, Konstantin Osipov wrote:
> > +/** A trivial wrapper around space_build_index(). */
> > +static void
> > +alter_space_build_index(struct space *src_space, struct space *new_space,
> > +			struct index *new_index)
> > +{
> > +	bool check_unique = space_needs_check_unique(new_space,
> > +						     new_index->def->iid);
> 
> check_unique is not proper English, please use
> check_unique_constraint or check_unique_ck.

Yeah, sure, will rename to check_unique_constraint.

> 
> > +	space_build_index_xc(src_space, new_index, new_space->format,
> > +			     check_unique);
> 
> Why can't you move testing check_unique property inside space_build_index
> or even the engine specific implementation of space_build_index?

Then I would need to pass the old space to the function, which I'd like
to avoid, because this would make the function proto less obvious.

> 
> > +	space->unique_index_bitmap = calloc(bitmap_size(index_id_max + 1), 1);
> 
> Let's use check_unique_constraint_map

Okay.

> > +/**
> > + * Return true if the unique constraint must be checked for
> > + * the index with the given id before inserting a tuple into
> > + * the space.
> > + */
> > +static inline bool
> > +space_needs_check_unique(struct space *space, uint32_t index_id)
> 
> space_needs_unique_constraint_check

Okay.

Re: [tarantool-patches] Re: [PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction

[Vladimir Davydov]

On Thu, Jul 25, 2019 at 01:32:55AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> > A txn_stmt keeps a reference to the space it modifies. Memtx uses this
> > space reference to revert the statement on error or voluntary rollback
> > so the space must stay valid throughout the whole transaction.
> > 
> > The problem is DML statements may be mixed in a DDL transaction, in
> > which case we always roll back DML statements first and only then run
> > rollback triggers to revert changes done to the schema by the DDL
> > statements. This means every DDL statement modifying the space cache
> > must be implemented in such a way that it leaves both the old space
> > and the new space valid until commit or rollback.
> 
> How are we going to enforce this invariant by code structure going
> forward other than patching it hastily when it breaks?

No way, I guess. We'll need to rethink the whole ALTER infrastructure
if things go south. Just like we always do.

Re: [PATCH 1/3] vinyl: use update_def index method to update vy_lsm on ddl

[Vladimir Davydov]

Renamed key_def_do_copy to key_def_copy_impl and pushed to master.

Re: [PATCH 2/3] vinyl: move unique check optimization setup to the space level

[Vladimir Davydov]

Renamed check_unique to check_unique_constraint and unique_index_bitmap
to check_unique_constraint_map and pushed to master.

Re: [tarantool-patches] Re: [PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction

[Vladimir Davydov]

On Thu, Jul 25, 2019 at 11:51:17AM +0300, Vladimir Davydov wrote:
> On Thu, Jul 25, 2019 at 01:32:55AM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/23 18:49]:
> > > A txn_stmt keeps a reference to the space it modifies. Memtx uses this
> > > space reference to revert the statement on error or voluntary rollback
> > > so the space must stay valid throughout the whole transaction.
> > > 
> > > The problem is DML statements may be mixed in a DDL transaction, in
> > > which case we always roll back DML statements first and only then run
> > > rollback triggers to revert changes done to the schema by the DDL
> > > statements. This means every DDL statement modifying the space cache
> > > must be implemented in such a way that it leaves both the old space
> > > and the new space valid until commit or rollback.
> > 
> > How are we going to enforce this invariant by code structure going
> > forward other than patching it hastily when it breaks?
> 
> No way, I guess. We'll need to rethink the whole ALTER infrastructure
> if things go south. Just like we always do.

An alternative approach to this issue is attaching DDL triggers to txn
statements and running rollback triggers "synchronously" while rolling
back statements. This would allow us not to impose any strict
requirements on how DDL triggers are implemented.

Re: [tarantool-patches] Re: [PATCH 3/3] alter: fix rollback in case DDL and DML are used in the same transaction

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/30 15:04]:
> An alternative approach to this issue is attaching DDL triggers to txn
> statements and running rollback triggers "synchronously" while rolling
> back statements. This would allow us not to impose any strict
> requirements on how DDL triggers are implemented.

I like it!

-- 
Konstantin Osipov, Moscow, Russia