txm: disallow yields after DDL operation in TX

To avoid sharing (ergo phantom reads) metadata object for different
transactions in MVCC mode, let's do following things.
Firstly, let's set on replace trigger on all system spaces (content's change in
system space is considered to be DDL operation) which disables yields until
transaction is committed. The only exceptions are index build and space format
check: during these operations yields are allowed since they may take a while
(so without yields they block execution). Actually it is not a problem 'cause
these two operations must be first-in-transaction: as a result transaction
can't contain two yielding statements. So after any cache modification no
yields take place for sure.
Secondly, on committing transaction that provides DDL changes let's abort all
other transaction since they may refer to obsolete schema objects. The last
restriction may seem too strict, but it is OK as primitive workaround until
transactional DDL is introduced. In fact we should only abort transactions that
have read dirty (i.e. modified) objects.

Closes #5998
Closes #6140
Workaround for #6138

changelogs/unreleased/gh-5998-disallow-yields-ddl.md

@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Disallow yields after DDL operations in MVCC mode. It fixes crash which takes
+  place in case several transactions refer to system spaces (gh-5998).

changelogs/unreleased/gh-6140-fix-mvcc-crash-in-ddl-rollback.md

@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed bug in MVCC connected which happens on rollback after
+  DDL operation (gh-5998).

src/box/alter.cc

@@ -1192,6 +1192,18 @@ class CheckSpaceFormat: public AlterSpaceOp
 	virtual void prepare(struct alter_space *alter);
 };
 
+static inline void
+space_check_format_with_yield(struct space *space,
+			      struct tuple_format *format)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL);
+	(void) txn_can_yield(txn, true);
+	auto yield_guard =
+		make_scoped_guard([=] { txn_can_yield(txn, false); });
+	space_check_format_xc(space, format);
+}
+
 void
 CheckSpaceFormat::prepare(struct alter_space *alter)
 {
@@ -1203,7 +1215,7 @@ CheckSpaceFormat::prepare(struct alter_space *alter)
 		assert(new_format != NULL);
 		if (!tuple_format1_can_store_format2_tuples(new_format,
 							    old_format))
-		    space_check_format_xc(old_space, new_format);
+			space_check_format_with_yield(old_space, new_format);
 	}
 }
 
@@ -1444,6 +1456,18 @@ CreateIndex::alter_def(struct alter_space *alter)
 	index_def_list_add(&alter->key_list, new_index_def);
 }
 
+static inline void
+space_build_index_with_yield(struct space *old_space, struct space *new_space,
+			     struct index *new_index)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL);
+	(void) txn_can_yield(txn, true);
+	auto yield_guard =
+		make_scoped_guard([=] { txn_can_yield(txn, false); });
+	space_build_index_xc(old_space, new_space, new_index);
+}
+
 /**
  * Optionally build the new index.
  *
@@ -1475,7 +1499,8 @@ CreateIndex::prepare(struct alter_space *alter)
 		space_add_primary_key_xc(alter->new_space);
 		return;
 	}
-	space_build_index_xc(alter->old_space, alter->new_space, new_index);
+	space_build_index_with_yield(alter->old_space, alter->new_space,
+				     new_index);
 }
 
 void
@@ -1540,7 +1565,8 @@ 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, alter->new_space, new_index);
+	space_build_index_with_yield(alter->old_space, alter->new_space,
+				     new_index);
 }
 
 void
@@ -1627,7 +1653,8 @@ TruncateIndex::prepare(struct alter_space *alter)
 	 * callback to load indexes during local recovery.
 	 */
 	assert(new_index != NULL);
-	space_build_index_xc(alter->new_space, alter->new_space, new_index);
+	space_build_index_with_yield(alter->new_space, alter->new_space,
+				     new_index);
 }
 
 void

src/box/memtx_engine.c

@@ -400,6 +400,8 @@ memtx_engine_prepare(struct engine *engine, struct txn *txn)
 		if (stmt->add_story != NULL || stmt->del_story != NULL)
 			memtx_tx_history_prepare_stmt(stmt);
 	}
+	if (txn->is_schema_changed)
+		memtx_tx_abort_all_for_ddl(txn);
 	return 0;
 }
 

src/box/memtx_space.c

@@ -915,8 +915,6 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	if (txn_check_singlestatement(txn, "space format check") != 0)
 		return -1;
 
-	bool could_yield = txn_can_yield(txn, true);
-
 	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
 	struct memtx_ddl_state state;
 	state.format = format;
@@ -959,7 +957,6 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	iterator_delete(it);
 	diag_destroy(&state.diag);
 	trigger_clear(&on_replace);
-	txn_can_yield(txn, could_yield);
 	return rc;
 }
 
@@ -1074,8 +1071,6 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (txn_check_singlestatement(txn, "index build") != 0)
 		return -1;
 
-	bool could_yield = txn_can_yield(txn, true);
-
 	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
 	struct memtx_ddl_state state;
 	state.index = new_index;
@@ -1153,7 +1148,6 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	iterator_delete(it);
 	diag_destroy(&state.diag);
 	trigger_clear(&on_replace);
-	txn_can_yield(txn, could_yield);
 	return rc;
 }
 

src/box/memtx_tx.c

@@ -277,6 +277,27 @@ memtx_tx_register_tx(struct txn *tx)
 	rlist_add_tail(&txm.all_txs, &tx->in_all_txs);
 }
 
+void
+memtx_tx_acquire_ddl(struct txn *tx)
+{
+	tx->is_schema_changed = true;
+	(void) txn_can_yield(tx, false);
+}
+
+void
+memtx_tx_abort_all_for_ddl(struct txn *ddl_owner)
+{
+	struct txn *to_be_aborted;
+	rlist_foreach_entry(to_be_aborted, &txm.all_txs, in_all_txs) {
+		if (to_be_aborted == ddl_owner)
+			continue;
+		to_be_aborted->status = TXN_CONFLICTED;
+		say_warn("Transaction committing DDL (id=%lld) has aborted "
+			 "another TX (id=%lld)", (long long) ddl_owner->id,
+			 (long long) to_be_aborted->id);
+	}
+}
+
 int
 memtx_tx_cause_conflict(struct txn *breaker, struct txn *victim)
 {

src/box/memtx_tx.h

@@ -182,6 +182,21 @@ memtx_tx_manager_init();
 void
 memtx_tx_manager_free();
 
+/**
+ * Transaction providing DDL changes is disallowed to yield after
+ * modifications of internal caches (i.e. after ALTER operation finishes).
+ */
+void
+memtx_tx_acquire_ddl(struct txn *tx);
+
+/**
+ * Mark all transactions except for a given as aborted due to conflict:
+ * when DDL operation is about to be committed other transactions are
+ * considered to use obsolete schema so that should be aborted.
+ */
+void
+memtx_tx_abort_all_for_ddl(struct txn *ddl_owner);
+
 /**
  * Notify TX manager that if transaction @a breaker is committed then the
  * transaction @a victim must be aborted due to conflict. It is achieved

src/box/schema.cc

@@ -38,6 +38,7 @@
 #include "user.h"
 #include "vclock/vclock.h"
 #include "fiber.h"
+#include "memtx_tx.h"
 
 /**
  * @module Data Dictionary
@@ -262,6 +263,15 @@ space_cache_replace(struct space *old_space, struct space *new_space)
 		space_invalidate(old_space);
 }
 
+static int
+on_replace_dd_system_space(struct trigger *trigger, void *event)
+{
+	(void) trigger;
+	struct txn *txn = (struct txn *) event;
+	memtx_tx_acquire_ddl(txn);
+	return 0;
+}
+
 /** A wrapper around space_new() for data dictionary spaces. */
 static void
 sc_space_new(uint32_t id, const char *name,
@@ -296,6 +306,9 @@ sc_space_new(uint32_t id, const char *name,
 	space_cache_replace(NULL, space);
 	if (replace_trigger)
 		trigger_add(&space->on_replace, replace_trigger);
+	struct trigger *t = (struct trigger *) malloc(sizeof(*t));
+	trigger_create(t, on_replace_dd_system_space, NULL, (trigger_f0) free);
+	trigger_add(&space->on_replace, t);
 	/*
 	 * Data dictionary spaces are fully built since:
 	 * - they contain data right from the start

src/box/txn.c

@@ -302,6 +302,7 @@ txn_begin(void)
 	txn->engine = NULL;
 	txn->engine_tx = NULL;
 	txn->fk_deferred_count = 0;
+	txn->is_schema_changed = false;
 	rlist_create(&txn->savepoints);
 	memtx_tx_register_tx(txn);
 	txn->fiber = NULL;

src/box/txn.h

@@ -426,6 +426,8 @@ struct txn {
 	struct rlist full_scan_list;
 	/** Link in tx_manager::all_txs. */
 	struct rlist in_all_txs;
+	/** True in case transaction provides any DDL change. */
+	bool is_schema_changed;
 };
 
 static inline bool

src/box/vinyl.c

@@ -1078,9 +1078,6 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	if (txn_check_singlestatement(txn, "space format check") != 0)
 		return -1;
 
-	/* See the comment in vinyl_space_build_index(). */
-	bool could_yield = txn_can_yield(txn, true);
-
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
 	ctx.format = format;
@@ -1131,7 +1128,6 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 out:
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
-	txn_can_yield(txn, could_yield);
 	return rc;
 }
 
@@ -4196,23 +4192,6 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	if (txn_check_singlestatement(txn, "index build") != 0)
 		return -1;
 
-	/*
-	 * Tarantool doesn't support multi-engine transactions, and
-	 * DDL system tables are memtx tables. Memtx transactions,
-	 * generally, can't yield. So here we're in the middle of
-	 * a *memtx* transaction. We don't start a hidden vinyl
-	 * transaction for DDL to avoid its overhead, but some long
-	 * DDL operations can yield, like checking a format or
-	 * building an index. Unless we switch off memtx yield
-	 * rollback triggers, such yield leads to memtx transaction
-	 * rollback. It is safe to switch the trigger off though:
-	 * it protects subsequent memtx transactions from reading
-	 * a dirty state, and at this phase vinyl DDL does not
-	 * change the data dictionary, so there is no dirty state
-	 * that can be observed.
-	 */
-	bool could_yield = txn_can_yield(txn, true);
-
 	/*
 	 * Iterate over all tuples stored in the space and insert
 	 * each of them into the new LSM tree. Since read iterator
@@ -4315,7 +4294,6 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 out:
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
-	txn_can_yield(txn, could_yield);
 	return rc;
 }