[PATCH v2 0/3] Transactional DDL

[PATCH v2 0/3] Transactional DDL

[Vladimir Davydov]

This patch set implements transactional DDL for the case when all
statements are non-yielding. There are only two DDL statements that
may yield implicitly - building of a new index and checking a space
format - so it must cover most user stories. For more details re
the implementation, see comments to the individual patches.

https://github.com/tarantool/tarantool/issues/4083
https://github.com/tarantool/tarantool/commits/dv/gh-4083-transactional-ddl

v1: https://www.freelists.org/post/tarantool-patches/PATCH-05-Transactional-DDL

Changes in v2:
 - Committed approved patches and rebased.
 - Fixed wrong index usage in AlterSpaceOp::commit,rollback
 - Reworked error reporting.
 - Reworked txn_on_yield management.

Vladimir Davydov (3):
  memtx: fix txn_on_yield for DDL transactions
  ddl: don't use space_index from AlterSpaceOp::commit,rollback
  ddl: allow to execute non-yielding DDL statements in transactions

 src/box/alter.cc              | 91 ++++++++++++++--------------------------
 src/box/errcode.h             |  2 +-
 src/box/memtx_engine.c        | 96 ++----------------------------------------
 src/box/memtx_space.c         | 10 +++++
 src/box/txn.c                 | 89 ++++++++++++++++++++++++++++++++++++---
 src/box/txn.h                 | 63 +++++++++++++++-------------
 src/box/user.cc               |  1 -
 src/box/vinyl.c               | 25 ++++++++---
 test/box/misc.result          |  1 +
 test/box/on_replace.result    | 53 +++++++++++------------
 test/box/on_replace.test.lua  | 13 +++---
 test/box/transaction.result   | 98 +++++++++++++++++++++++++++++++++----------
 test/box/transaction.test.lua | 64 +++++++++++++++++++++++-----
 test/engine/ddl.result        | 58 ++++++++++++++++++++++++-
 test/engine/ddl.test.lua      | 39 ++++++++++++++++-
 test/engine/truncate.result   | 10 +----
 test/engine/truncate.test.lua |  6 +--
 17 files changed, 440 insertions(+), 279 deletions(-)

-- 
2.11.0

[PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

Memtx engine doesn't allow yielding inside a transaction. To achieve
that, it installs fiber->on_yield trigger that aborts the current
transaction (rolls it back, but leaves it be so that commit fails).

There's an exception though - DDL statements are allowed to yield.
This is required so as not to block the event loop while a new index
is built or a space format is checked. Currently, we handle this
exception by checking space id and omitting installation of the
trigger for system spaces. This isn't entirely correct, because we
may yield after a DDL statement is complete, in which case the
transaction won't be aborted though it should:

  box.begin()
  box.space.my_space:create_index('my_index')
  fiber.sleep(0) -- doesn't abort the transaction!

This patch fixes the problem by making the memtx engine install the
on_yield trigger unconditionally, for all kinds of transactions, and
instead explicitly disabling the trigger for yielding DDL operations.

In order not to spread the yield-in-transaction logic between memtx
and txn code, let's move all fiber_on_yield related stuff to txn,
export a method to disable yields, and use the method in memtx.
---
 src/box/memtx_engine.c        | 96 ++-----------------------------------------
 src/box/memtx_space.c         |  6 +++
 src/box/txn.c                 | 82 ++++++++++++++++++++++++++++++++++--
 src/box/txn.h                 | 50 ++++++++++++++--------
 src/box/vinyl.c               |  6 +++
 test/box/transaction.result   | 16 ++++++++
 test/box/transaction.test.lua |  8 ++++
 test/engine/truncate.result   | 10 +----
 test/engine/truncate.test.lua |  6 +--
 9 files changed, 155 insertions(+), 125 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index c8376110..6ed38a91 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -55,58 +55,6 @@
 static void
 checkpoint_cancel(struct checkpoint *ckpt);
 
-/*
- * Memtx yield-in-transaction trigger: roll back the effects
- * of the transaction and mark the transaction as aborted.
- */
-static void
-txn_on_yield(struct trigger *trigger, void *event)
-{
-	(void) trigger;
-	(void) event;
-
-	struct txn *txn = in_txn();
-	assert(txn && txn->engine_tx);
-	if (txn == NULL || txn->engine_tx == NULL)
-		return;
-	txn_abort(txn);                 /* doesn't yield or fail */
-}
-
-/**
- * Initialize context for yield triggers.
- * In case of a yield inside memtx multi-statement transaction
- * we must, first of all, roll back the effects of the transaction
- * so that concurrent transactions won't see dirty, uncommitted
- * data.
- * Second, we must abort the transaction, since it has been rolled
- * back implicitly. The transaction can not be rolled back
- * completely from within a yield trigger, since a yield trigger
- * can't fail. Instead, we mark the transaction as aborted and
- * raise an error on attempt to commit it.
- *
- * So much hassle to be user-friendly until we have a true
- * interactive transaction support in memtx.
- */
-void
-memtx_init_txn(struct txn *txn)
-{
-	struct fiber *fiber = fiber();
-
-	trigger_create(&txn->fiber_on_yield, txn_on_yield,
-		       NULL, NULL);
-	/*
-	 * Memtx doesn't allow yields between statements of
-	 * a transaction. Set a trigger which would roll
-	 * back the transaction if there is a yield.
-	 */
-	trigger_add(&fiber->on_yield, &txn->fiber_on_yield);
-	/*
-	 * This serves as a marker that the triggers are
-	 * initialized.
-	 */
-	txn->engine_tx = txn;
-}
-
 struct PACKED memtx_tuple {
 	/*
 	 * sic: the header of the tuple is used
@@ -372,45 +320,10 @@ memtx_engine_create_space(struct engine *engine, struct space_def *def,
 }
 
 static int
-memtx_engine_prepare(struct engine *engine, struct txn *txn)
-{
-	(void)engine;
-	if (txn->engine_tx == 0)
-		return 0;
-	/*
-	 * These triggers are only used for memtx and only
-	 * when autocommit == false, so we are saving
-	 * on calls to trigger_create/trigger_clear.
-	 */
-	trigger_clear(&txn->fiber_on_yield);
-	if (txn->is_aborted) {
-		diag_set(ClientError, ER_TRANSACTION_YIELD);
-		diag_log();
-		return -1;
-	}
-	return 0;
-}
-
-static int
 memtx_engine_begin(struct engine *engine, struct txn *txn)
 {
 	(void)engine;
-	(void)txn;
-	return 0;
-}
-
-static int
-memtx_engine_begin_statement(struct engine *engine, struct txn *txn)
-{
-	(void)engine;
-	(void)txn;
-	if (txn->engine_tx == NULL) {
-		struct space *space = txn_last_stmt(txn)->space;
-
-		if (space->def->id > BOX_SYSTEM_ID_MAX)
-			/* Setup triggers for non-ddl transactions. */
-			memtx_init_txn(txn);
-	}
+	txn_disable_yield(txn);
 	return 0;
 }
 
@@ -459,9 +372,6 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn,
 static void
 memtx_engine_rollback(struct engine *engine, struct txn *txn)
 {
-	if (txn->engine_tx != NULL) {
-		trigger_clear(&txn->fiber_on_yield);
-	}
 	struct txn_stmt *stmt;
 	stailq_reverse(&txn->stmts);
 	stailq_foreach_entry(stmt, &txn->stmts, next)
@@ -948,8 +858,8 @@ static const struct engine_vtab memtx_engine_vtab = {
 	/* .create_space = */ memtx_engine_create_space,
 	/* .join = */ memtx_engine_join,
 	/* .begin = */ memtx_engine_begin,
-	/* .begin_statement = */ memtx_engine_begin_statement,
-	/* .prepare = */ memtx_engine_prepare,
+	/* .begin_statement = */ generic_engine_begin_statement,
+	/* .prepare = */ generic_engine_prepare,
 	/* .commit = */ generic_engine_commit,
 	/* .rollback_statement = */ memtx_engine_rollback_statement,
 	/* .rollback = */ memtx_engine_rollback,
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f0e1cfd2..921dbcbf 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -888,6 +888,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	if (it == NULL)
 		return -1;
 
+	txn_enable_yield_for_ddl();
+
 	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
 	struct memtx_ddl_state state;
 	state.format = format;
@@ -930,6 +932,7 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	iterator_delete(it);
 	diag_destroy(&state.diag);
 	trigger_clear(&on_replace);
+	txn_disable_yield_after_ddl();
 	return rc;
 }
 
@@ -1027,6 +1030,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (it == NULL)
 		return -1;
 
+	txn_enable_yield_for_ddl();
+
 	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
 	struct memtx_ddl_state state;
 	state.index = new_index;
@@ -1103,6 +1108,7 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	iterator_delete(it);
 	diag_destroy(&state.diag);
 	trigger_clear(&on_replace);
+	txn_disable_yield_after_ddl();
 	return rc;
 }
 
diff --git a/src/box/txn.c b/src/box/txn.c
index c605345d..bab98d7e 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -40,6 +40,12 @@ double too_long_threshold;
 /* Txn cache. */
 static struct stailq txn_cache = {NULL, &txn_cache.first};
 
+static void
+txn_on_stop(struct trigger *trigger, void *event);
+
+static void
+txn_on_yield(struct trigger *trigger, void *event);
+
 static inline void
 fiber_set_txn(struct fiber *fiber, struct txn *txn)
 {
@@ -194,6 +200,7 @@ txn_begin()
 	txn->has_triggers  = false;
 	txn->is_done = false;
 	txn->is_aborted = false;
+	txn->is_yield_disabled = false;
 	txn->in_sub_stmt = 0;
 	txn->id = ++tsn;
 	txn->signature = -1;
@@ -201,8 +208,8 @@ txn_begin()
 	txn->engine_tx = NULL;
 	txn->psql_txn = NULL;
 	txn->fiber = NULL;
-	/* fiber_on_yield/fiber_on_stop initialized by engine on demand */
 	fiber_set_txn(fiber(), txn);
+	/* fiber_on_yield is initialized by engine on demand */
 	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
 	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
 	return txn;
@@ -463,6 +470,12 @@ txn_write_to_wal(struct txn *txn)
 static int
 txn_prepare(struct txn *txn)
 {
+	if (txn->is_aborted) {
+		assert(txn->is_yield_disabled);
+		diag_set(ClientError, ER_TRANSACTION_YIELD);
+		diag_log();
+		return -1;
+	}
 	/*
 	 * If transaction has been started in SQL, deferred
 	 * foreign key constraints must not be violated.
@@ -484,6 +497,8 @@ txn_prepare(struct txn *txn)
 			return -1;
 	}
 	trigger_clear(&txn->fiber_on_stop);
+	if (txn->is_yield_disabled)
+		trigger_clear(&txn->fiber_on_yield);
 	return 0;
 }
 
@@ -550,12 +565,22 @@ txn_rollback(struct txn *txn)
 {
 	assert(txn == in_txn());
 	trigger_clear(&txn->fiber_on_stop);
+	if (txn->is_yield_disabled)
+		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = -1;
 	txn_complete(txn);
 	fiber_set_txn(fiber(), NULL);
 }
 
-void
+/**
+ * Roll back the transaction but keep the object around.
+ * A special case for memtx transaction abort on yield. In this
+ * case we need to abort the transaction to avoid dirty reads but
+ * need to keep it around to ensure a new one is not implicitly
+ * started and committed by the user program. Later, at
+ * transaction commit we will raise an exception.
+ */
+static void
 txn_abort(struct txn *txn)
 {
 	assert(in_txn() == txn);
@@ -578,6 +603,31 @@ txn_check_singlestatement(struct txn *txn, const char *where)
 	return 0;
 }
 
+void
+txn_disable_yield(struct txn *txn)
+{
+	assert(!txn->is_yield_disabled);
+	txn->is_yield_disabled = true;
+	trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
+	trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
+}
+
+void
+txn_enable_yield_for_ddl(void)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL && txn->is_yield_disabled);
+	trigger_clear(&txn->fiber_on_yield);
+}
+
+void
+txn_disable_yield_after_ddl(void)
+{
+	struct txn *txn = in_txn();
+	assert(txn != NULL && txn->is_yield_disabled);
+	trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
+}
+
 int64_t
 box_txn_id(void)
 {
@@ -711,7 +761,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 	return 0;
 }
 
-void
+static void
 txn_on_stop(struct trigger *trigger, void *event)
 {
 	(void) trigger;
@@ -719,3 +769,29 @@ txn_on_stop(struct trigger *trigger, void *event)
 	txn_rollback(in_txn());                 /* doesn't yield or fail */
 }
 
+/**
+ * Memtx yield-in-transaction trigger callback.
+ *
+ * In case of a yield inside memtx multi-statement transaction
+ * we must, first of all, roll back the effects of the transaction
+ * so that concurrent transactions won't see dirty, uncommitted
+ * data.
+ *
+ * Second, we must abort the transaction, since it has been rolled
+ * back implicitly. The transaction can not be rolled back
+ * completely from within a yield trigger, since a yield trigger
+ * can't fail. Instead, we mark the transaction as aborted and
+ * raise an error on attempt to commit it.
+ *
+ * So much hassle to be user-friendly until we have a true
+ * interactive transaction support in memtx.
+ */
+static void
+txn_on_yield(struct trigger *trigger, void *event)
+{
+	(void) trigger;
+	(void) event;
+	struct txn *txn = in_txn();
+	assert(txn != NULL && txn->is_yield_disabled);
+	txn_abort(txn);                         /* doesn't yield or fail */
+}
diff --git a/src/box/txn.h b/src/box/txn.h
index d1ef220a..258a8db7 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -169,6 +169,11 @@ struct txn {
 	 * rolled back at commit.
 	 */
 	bool is_aborted;
+	/**
+	 * True if the engine doesn't support yields inside
+	 * transactions, see txn_disable_yield().
+	 */
+	bool is_yield_disabled;
 	/** True if on_commit and on_rollback lists are non-empty. */
 	bool has_triggers;
 	/** The number of active nested statement-level transactions. */
@@ -259,17 +264,6 @@ int
 txn_write(struct txn *txn);
 
 /**
- * Roll back the transaction but keep the object around.
- * A special case for memtx transaction abort on yield. In this
- * case we need to abort the transaction to avoid dirty reads but
- * need to keep it around to ensure a new one is not implicitly
- * started and committed by the user program. Later, at
- * transaction commit we will raise an exception.
- */
-void
-txn_abort(struct txn *txn);
-
-/**
  * Most txns don't have triggers, and txn objects
  * are created on every access to data, so txns
  * are partially initialized.
@@ -371,6 +365,34 @@ int
 txn_check_singlestatement(struct txn *txn, const char *where);
 
 /**
+ * Installs yield-in-transaction trigger: roll back the effects
+ * of the transaction and mark the transaction as aborted.
+ *
+ * Used by the memtx engine which doesn't support yields in
+ * transactions.
+ */
+void
+txn_disable_yield(struct txn *txn);
+
+/**
+ * Since memtx engine doesn't support yields inside transactions,
+ * it installs a trigger that aborts the current transaction on
+ * fiber yield. However, we want to allow yields while executing
+ * certain DDL statements, such as building an index or checking
+ * space format, so as not to block the event loop for too long.
+ *
+ * This function temporarily disables the trigger for this purpose.
+ * One must call txn_disable_yield_after_ddl() once the DDL request
+ * has been complete.
+ */
+void
+txn_enable_yield_for_ddl(void);
+
+/** See txn_enable_yield_for_ddl(). */
+void
+txn_disable_yield_after_ddl(void);
+
+/**
  * Returns true if the transaction has a single statement.
  * Supposed to be used from a space on_replace trigger to
  * detect transaction boundaries.
@@ -400,12 +422,6 @@ txn_last_stmt(struct txn *txn)
 }
 
 /**
- * Fiber-stop trigger: roll back the transaction.
- */
-void
-txn_on_stop(struct trigger *trigger, void *event);
-
-/**
  * Return VDBE that is being currently executed.
  *
  * @retval VDBE that is being currently executed.
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index e0de65d0..8629aa8e 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1121,6 +1121,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
 
+	txn_enable_yield_for_ddl();
+
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
 	ctx.format = format;
@@ -1168,6 +1170,7 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
+	txn_disable_yield_after_ddl();
 	return rc;
 }
 
@@ -4345,6 +4348,8 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, src_space, &need_wal_sync);
 
+	txn_enable_yield_for_ddl();
+
 	/*
 	 * Iterate over all tuples stored in the space and insert
 	 * each of them into the new LSM tree. Since read iterator
@@ -4438,6 +4443,7 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
+	txn_disable_yield_after_ddl();
 	return rc;
 }
 
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 857314b7..ad2d650c 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -722,3 +722,19 @@ box.commit() -- error
 s:drop()
 ---
 ...
+--
+-- Check that a DDL transaction is rolled back on fiber yield.
+--
+s = box.schema.space.create('test')
+---
+...
+box.begin() s:create_index('pk') fiber.sleep(0)
+---
+...
+box.commit() -- error
+---
+- error: Transaction has been aborted by a fiber yield
+...
+s:drop()
+---
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 8ffae2fe..8cd3e8ba 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -373,3 +373,11 @@ fiber.sleep(0)
 s.index.pk == nil
 box.commit() -- error
 s:drop()
+
+--
+-- Check that a DDL transaction is rolled back on fiber yield.
+--
+s = box.schema.space.create('test')
+box.begin() s:create_index('pk') fiber.sleep(0)
+box.commit() -- error
+s:drop()
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index 277e7bda..d65a1df1 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -8,7 +8,7 @@ fiber =  require('fiber')
 ---
 ...
 --
--- Check that space truncation is forbidden in a transaction.
+-- Check that space truncation works fine in a transaction.
 --
 s = box.schema.create_space('test', {engine = engine})
 ---
@@ -19,13 +19,7 @@ _ = s:create_index('pk')
 _ = s:insert{123}
 ---
 ...
-box.begin()
----
-...
-s:truncate()
----
-...
-box.commit()
+box.begin() s:truncate() box.commit()
 ---
 ...
 s:select()
diff --git a/test/engine/truncate.test.lua b/test/engine/truncate.test.lua
index 74fdd52b..fe897fb6 100644
--- a/test/engine/truncate.test.lua
+++ b/test/engine/truncate.test.lua
@@ -4,14 +4,12 @@ engine = test_run:get_cfg('engine')
 fiber =  require('fiber')
 
 --
--- Check that space truncation is forbidden in a transaction.
+-- Check that space truncation works fine in a transaction.
 --
 s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
 _ = s:insert{123}
-box.begin()
-s:truncate()
-box.commit()
+box.begin() s:truncate() box.commit()
 s:select()
 s:drop()
 
-- 
2.11.0

[PATCH v2 2/3] ddl: don't use space_index from AlterSpaceOp::commit,rollback

[Vladimir Davydov]

If there are multiple DDL operations in the same transactions, which is
impossible now, but will be implemented soon, AlterSpaceOp::commit and
rollback methods must not access space index map. To understand that,
consider the following example:

  - on_replace: AlterSpaceOp1 creates index I1 for space S1
  - on_replace: AlterSpaceOp2 moves index I1 from space S1 to space S2
  - on_commit:  AlterSpaceOp1 commits creation of index I1

AlterSpaceOp1 can't lookup I1 in S1 by id, because the index was moved
from S1 to S2 by AlterSpaceOp2. If AlterSpaceOp1 attempts to look it up,
it will access a wrong index.

Fix that by storing pointers to old and new indexes in AlterSpaceOp if
required.
---
 src/box/alter.cc | 77 +++++++++++++++++++++++---------------------------------
 1 file changed, 32 insertions(+), 45 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index ce0cf2d9..612bcd89 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1088,10 +1088,9 @@ ModifySpace::~ModifySpace()
 class DropIndex: public AlterSpaceOp
 {
 public:
-	DropIndex(struct alter_space *alter, struct index_def *def_arg)
-		:AlterSpaceOp(alter), old_index_def(def_arg) {}
-	/** A reference to the definition of the dropped index. */
-	struct index_def *old_index_def;
+	DropIndex(struct alter_space *alter, struct index *index)
+		:AlterSpaceOp(alter), old_index(index) {}
+	struct index *old_index;
 	virtual void alter_def(struct alter_space *alter);
 	virtual void prepare(struct alter_space *alter);
 	virtual void commit(struct alter_space *alter, int64_t lsn);
@@ -1104,24 +1103,22 @@ public:
 void
 DropIndex::alter_def(struct alter_space * /* alter */)
 {
-	rlist_del_entry(old_index_def, link);
+	rlist_del_entry(old_index->def, link);
 }
 
 /* Do the drop. */
 void
 DropIndex::prepare(struct alter_space *alter)
 {
-	if (old_index_def->iid == 0)
+	if (old_index->def->iid == 0)
 		space_drop_primary_key(alter->new_space);
 }
 
 void
 DropIndex::commit(struct alter_space *alter, int64_t signature)
 {
-	struct index *index = space_index(alter->old_space,
-					  old_index_def->iid);
-	assert(index != NULL);
-	index_commit_drop(index, signature);
+	(void)alter;
+	index_commit_drop(old_index, signature);
 }
 
 /**
@@ -1160,15 +1157,14 @@ class ModifyIndex: public AlterSpaceOp
 {
 public:
 	ModifyIndex(struct alter_space *alter,
-		    struct index_def *new_index_def_arg,
-		    struct index_def *old_index_def_arg)
-		: AlterSpaceOp(alter),new_index_def(new_index_def_arg),
-		  old_index_def(old_index_def_arg) {
+		    struct index *index, struct index_def *def)
+		: AlterSpaceOp(alter), old_index(index),
+		  new_index(NULL), new_index_def(def) {
 	        if (new_index_def->iid == 0 &&
 	            key_part_cmp(new_index_def->key_def->parts,
 	                         new_index_def->key_def->part_count,
-	                         old_index_def->key_def->parts,
-	                         old_index_def->key_def->part_count) != 0) {
+	                         old_index->def->key_def->parts,
+	                         old_index->def->key_def->part_count) != 0) {
 	                /*
 	                 * Primary parts have been changed -
 	                 * update secondary indexes.
@@ -1176,8 +1172,9 @@ public:
 	                alter->pk_def = new_index_def->key_def;
 	        }
 	}
+	struct index *old_index;
+	struct index *new_index;
 	struct index_def *new_index_def;
-	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);
@@ -1189,26 +1186,22 @@ public:
 void
 ModifyIndex::alter_def(struct alter_space *alter)
 {
-	rlist_del_entry(old_index_def, link);
+	rlist_del_entry(old_index->def, link);
 	index_def_list_add(&alter->key_list, new_index_def);
 }
 
 void
 ModifyIndex::alter(struct alter_space *alter)
 {
-	assert(old_index_def->iid == new_index_def->iid);
+	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.
 	 */
 	space_swap_index(alter->old_space, alter->new_space,
-			 old_index_def->iid, new_index_def->iid);
-	struct index *old_index = space_index(alter->old_space,
-					      old_index_def->iid);
-	assert(old_index != NULL);
-	struct index *new_index = space_index(alter->new_space,
-					      new_index_def->iid);
-	assert(new_index != NULL);
+			 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);
 }
@@ -1216,27 +1209,19 @@ 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);
-	assert(new_index != NULL);
+	(void)alter;
 	index_commit_modify(new_index, signature);
 }
 
 void
 ModifyIndex::rollback(struct alter_space *alter)
 {
-	assert(old_index_def->iid == new_index_def->iid);
 	/*
 	 * Restore indexes.
 	 */
 	space_swap_index(alter->old_space, alter->new_space,
-			 old_index_def->iid, new_index_def->iid);
-	struct index *old_index = space_index(alter->old_space,
-					      old_index_def->iid);
-	assert(old_index != NULL);
-	struct index *new_index = space_index(alter->new_space,
-					      new_index_def->iid);
-	assert(new_index != NULL);
+			 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);
 }
@@ -1400,10 +1385,12 @@ class TruncateIndex: public AlterSpaceOp
 	 * In case TRUNCATE fails, we need to clean up the new
 	 * index data in the engine.
 	 */
+	struct index *old_index;
 	struct index *new_index;
 public:
 	TruncateIndex(struct alter_space *alter, uint32_t iid)
-		: AlterSpaceOp(alter), iid(iid) {}
+		: AlterSpaceOp(alter), iid(iid),
+		  old_index(NULL), new_index(NULL) {}
 	virtual void prepare(struct alter_space *alter);
 	virtual void commit(struct alter_space *alter, int64_t signature);
 	virtual ~TruncateIndex();
@@ -1412,7 +1399,9 @@ public:
 void
 TruncateIndex::prepare(struct alter_space *alter)
 {
+	old_index = space_index(alter->old_space, iid);
 	new_index = space_index(alter->new_space, iid);
+
 	if (iid == 0) {
 		/*
 		 * Notify the engine that the primary index
@@ -1437,8 +1426,7 @@ TruncateIndex::prepare(struct alter_space *alter)
 void
 TruncateIndex::commit(struct alter_space *alter, int64_t signature)
 {
-	struct index *old_index = space_index(alter->old_space, iid);
-
+	(void)alter;
 	index_commit_drop(old_index, signature);
 	index_commit_create(new_index, signature);
 	new_index = NULL;
@@ -1655,7 +1643,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 				new_def = index_def_dup(old_def);
 				index_def_update_optionality(new_def,
 							     min_field_count);
-				(void) new ModifyIndex(alter, new_def, old_def);
+				(void) new ModifyIndex(alter, old_index, new_def);
 			} else {
 				(void) new MoveIndex(alter, old_def->iid);
 			}
@@ -1672,7 +1660,7 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 		index_def_update_optionality(new_def, min_field_count);
 		auto guard = make_scoped_guard([=] { index_def_delete(new_def); });
 		if (!index_def_change_requires_rebuild(old_index, new_def))
-			(void) new ModifyIndex(alter, new_def, old_def);
+			(void) new ModifyIndex(alter, old_index, new_def);
 		else
 			(void) new RebuildIndex(alter, new_def, old_def);
 		guard.is_active = false;
@@ -2207,7 +2195,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 				  "can not drop a referenced index");
 		}
 		alter_space_move_indexes(alter, 0, iid);
-		(void) new DropIndex(alter, old_index->def);
+		(void) new DropIndex(alter, old_index);
 	}
 	/* Case 2: create an index, if it is simply created. */
 	if (old_index == NULL && new_tuple != NULL) {
@@ -2285,8 +2273,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			 * in the space conform to the new format.
 			 */
 			(void) new CheckSpaceFormat(alter);
-			(void) new ModifyIndex(alter, index_def,
-					       old_index->def);
+			(void) new ModifyIndex(alter, old_index, index_def);
 			index_def_guard.is_active = false;
 		}
 	}
-- 
2.11.0

[PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

The patch is pretty straightforward - all it does is moves checks for
single statement transactions from alter.cc to txn_enable_yield_for_ddl
so that now any DDL request may be executed in a transaction unless it
builds an index or checks the format of a non-empty space (those are the
only two operations that may yield).

There's two things that must be noted explicitly. The first is removal
of an assertion from priv_grant. The assertion ensured that a revoked
privilege was in the cache. The problem is the cache is built from the
contents of the space, see user_reload_privs. On rollback, we first
revert the content of the space to the original state, and only then
start invoking rollback triggers, which call priv_grant. As a result, we
will revert the cache to the original state right after the first
trigger is invoked and the following triggers will have no effect on it.
Thus we have to remove this assertion.

The second subtlety lays in vinyl_index_commit_modify. Before the commit
we assumed that if statement lsn is <= vy_lsm::commit_lsn, then it must
be local recovery from WAL. Now it's not true, because there may be
several operations for the same index in a transaction, and they all
will receive the same signature in on_commit trigger. We could, of
course, try to assign different signatures to them, but that would look
cumbersome - better simply allow lsn <= vy_lsm::commit_lsn after local
recovery, there's actually nothing wrong about that.

Closes #4083

@TarantoolBot document
Title: Transactional DDL

Now it's possible to group non-yielding DDL statements into
transactions, e.g.

```Lua
box.begin()
box.schema.space.create('my_space')
box.space.my_space:create_index('primary')
box.commit() -- or box.rollback()
```

Most DDL statements don't yield and hence can be run from transactions.
There are just two exceptions: creation of a new index and changing the
format of a non-empty space. Those are long operations that may yield
so as not to block the event loop for too long. Those statements can't
be executed from transactions (to be more exact, such a statement must
go first in any transaction).

Also, just like in case of DML transactions in memtx, it's forbidden to
explicitly yield in a DDL transaction by calling fiber.sleep or any
other yielding function. If this happens, the transaction will be
aborted and an attempt to commit it will fail.
---
 src/box/alter.cc              | 14 --------
 src/box/errcode.h             |  2 +-
 src/box/memtx_space.c         |  8 +++--
 src/box/txn.c                 | 27 +++++++-------
 src/box/txn.h                 | 25 ++++---------
 src/box/user.cc               |  1 -
 src/box/vinyl.c               | 23 ++++++++----
 test/box/misc.result          |  1 +
 test/box/on_replace.result    | 53 +++++++++++++---------------
 test/box/on_replace.test.lua  | 13 +++----
 test/box/transaction.result   | 82 ++++++++++++++++++++++++++++++++-----------
 test/box/transaction.test.lua | 56 +++++++++++++++++++++++------
 test/engine/ddl.result        | 58 ++++++++++++++++++++++++++++--
 test/engine/ddl.test.lua      | 39 ++++++++++++++++++--
 14 files changed, 273 insertions(+), 129 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 612bcd89..c7be6b77 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1816,7 +1816,6 @@ static void
 on_replace_dd_space(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _space");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -2113,7 +2112,6 @@ static void
 on_replace_dd_index(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _index");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -2305,7 +2303,6 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
-	txn_check_singlestatement_xc(txn, "Space _truncate");
 	struct tuple *new_tuple = stmt->new_tuple;
 
 	if (new_tuple == NULL) {
@@ -2535,7 +2532,6 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
-	txn_check_singlestatement_xc(txn, "Space _user");
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
 
@@ -2686,7 +2682,6 @@ static void
 on_replace_dd_func(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _func");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -2880,7 +2875,6 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
-	txn_check_singlestatement_xc(txn, "Space _collation");
 	if (new_tuple == NULL && old_tuple != NULL) {
 		/* DELETE */
 		struct trigger *on_commit =
@@ -3178,7 +3172,6 @@ static void
 on_replace_dd_priv(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _priv");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -3227,7 +3220,6 @@ static void
 on_replace_dd_schema(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _schema");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -3305,7 +3297,6 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 {
 	(void) trigger;
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _cluster");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -3458,7 +3449,6 @@ static void
 on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _sequence");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -3661,7 +3651,6 @@ static void
 on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _space_sequence");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *tuple = stmt->new_tuple ? stmt->new_tuple : stmt->old_tuple;
 
@@ -3806,7 +3795,6 @@ static void
 on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _trigger");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -4189,7 +4177,6 @@ static void
 on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _fk_constraint");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
@@ -4480,7 +4467,6 @@ static void
 on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 {
 	struct txn *txn = (struct txn *) event;
-	txn_check_singlestatement_xc(txn, "Space _ck_constraint");
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index be8dab27..97fc9675 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -106,7 +106,7 @@ struct errcode_record {
 	/* 51 */_(ER_NO_SUCH_FUNCTION,		"Function '%s' does not exist") \
 	/* 52 */_(ER_FUNCTION_EXISTS,		"Function '%s' already exists") \
 	/* 53 */_(ER_BEFORE_REPLACE_RET,	"Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
-	/* 54 */_(ER_UNUSED2,			"") \
+	/* 54 */_(ER_MULTISTATEMENT_DDL,	"Can not perform %s in a multi-statement transaction") \
 	/* 55 */_(ER_TRIGGER_EXISTS,		"Trigger '%s' already exists") \
 	/* 56 */_(ER_USER_MAX,			"A limit on the total number of users has been reached: %u") \
 	/* 57 */_(ER_NO_SUCH_ENGINE,		"Space engine '%s' does not exist") \
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 921dbcbf..47369994 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -888,7 +888,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	if (it == NULL)
 		return -1;
 
-	txn_enable_yield_for_ddl();
+	if (txn_enable_yield_for_ddl("space format check") != 0)
+		return -1;
 
 	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
 	struct memtx_ddl_state state;
@@ -1018,6 +1019,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	struct index *pk = index_find(src_space, 0);
 	if (pk == NULL)
 		return -1;
+	if (index_size(pk) == 0)
+		return 0;
 
 	struct errinj *inj = errinj(ERRINJ_BUILD_INDEX, ERRINJ_INT);
 	if (inj != NULL && inj->iparam == (int)new_index->def->iid) {
@@ -1030,7 +1033,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (it == NULL)
 		return -1;
 
-	txn_enable_yield_for_ddl();
+	if (txn_enable_yield_for_ddl("index build") != 0)
+		return -1;
 
 	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
 	struct memtx_ddl_state state;
diff --git a/src/box/txn.c b/src/box/txn.c
index bab98d7e..9ae11aa7 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -592,17 +592,6 @@ txn_abort(struct txn *txn)
 	txn->is_aborted = true;
 }
 
-int
-txn_check_singlestatement(struct txn *txn, const char *where)
-{
-	if (!txn_is_first_statement(txn)) {
-		diag_set(ClientError, ER_UNSUPPORTED,
-			 where, "multi-statement transactions");
-		return -1;
-	}
-	return 0;
-}
-
 void
 txn_disable_yield(struct txn *txn)
 {
@@ -612,12 +601,24 @@ txn_disable_yield(struct txn *txn)
 	trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
 }
 
-void
-txn_enable_yield_for_ddl(void)
+int
+txn_enable_yield_for_ddl(const char *what)
 {
 	struct txn *txn = in_txn();
 	assert(txn != NULL && txn->is_yield_disabled);
+	/*
+	 * It's okay to yield while executing the first DDL statement
+	 * in a transaction, because the schema hasn't been updated
+	 * yet and so other transactions can't see uncommitted objects.
+	 * Yielding in subsequent statements is not safe, as there
+	 * may be uncommitted objects in the schema cache.
+	 */
+	if (!txn_is_first_statement(txn)) {
+		diag_set(ClientError, ER_MULTISTATEMENT_DDL, what);
+		return -1;
+	}
 	trigger_clear(&txn->fiber_on_yield);
+	return 0;
 }
 
 void
diff --git a/src/box/txn.h b/src/box/txn.h
index 258a8db7..92366038 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -358,13 +358,6 @@ void
 txn_rollback_stmt(struct txn *txn);
 
 /**
- * Raise an error if this is a multi-statement transaction: DDL
- * can not be part of a multi-statement transaction.
- */
-int
-txn_check_singlestatement(struct txn *txn, const char *where);
-
-/**
  * Installs yield-in-transaction trigger: roll back the effects
  * of the transaction and mark the transaction as aborted.
  *
@@ -384,9 +377,13 @@ txn_disable_yield(struct txn *txn);
  * This function temporarily disables the trigger for this purpose.
  * One must call txn_disable_yield_after_ddl() once the DDL request
  * has been complete.
+ *
+ * Before enabling yields, this function checks if it doesn't
+ * violate transaction isolation. If it does, it returns -1 and
+ * sets diag. The 'what' message is used for error reporting.
  */
-void
-txn_enable_yield_for_ddl(void);
+int
+txn_enable_yield_for_ddl(const char *what);
 
 /** See txn_enable_yield_for_ddl(). */
 void
@@ -525,16 +522,6 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *savepoint);
 
 #if defined(__cplusplus)
 } /* extern "C" */
-
-#include "diag.h"
-
-static inline void
-txn_check_singlestatement_xc(struct txn *txn, const char *where)
-{
-	if (txn_check_singlestatement(txn, where) != 0)
-		diag_raise();
-}
-
 #endif /* defined(__cplusplus) */
 
 #endif /* TARANTOOL_BOX_TXN_H_INCLUDED */
diff --git a/src/box/user.cc b/src/box/user.cc
index 48bdf18e..c46ff67d 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -709,7 +709,6 @@ priv_grant(struct user *grantee, struct priv_def *priv)
 	if (object == NULL)
 		return;
 	struct access *access = &object[grantee->auth_token];
-	assert(privset_search(&grantee->privs, priv) || access->granted == 0);
 	access->granted = priv->access;
 	rebuild_effective_grants(grantee);
 }
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 8629aa8e..af044f0c 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -919,16 +919,17 @@ vinyl_index_commit_modify(struct index *index, int64_t lsn)
 	       env->status == VINYL_FINAL_RECOVERY_LOCAL ||
 	       env->status == VINYL_FINAL_RECOVERY_REMOTE);
 
-	if (lsn <= lsm->commit_lsn) {
+	if (env->status == VINYL_FINAL_RECOVERY_LOCAL &&
+	    lsn <= lsm->commit_lsn) {
 		/*
-		 * This must be local recovery from WAL, when
-		 * the operation has already been committed to
-		 * vylog.
+		 * The statement we are recovering from WAL has
+		 * been successfully written to vylog so we must
+		 * not replay it.
 		 */
-		assert(env->status == VINYL_FINAL_RECOVERY_LOCAL);
 		return;
 	}
 
+	assert(lsm->commit_lsn <= lsn);
 	lsm->commit_lsn = lsn;
 
 	vy_log_tx_begin();
@@ -1121,7 +1122,11 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
 
-	txn_enable_yield_for_ddl();
+	if (!need_wal_sync && vy_lsm_is_empty(pk))
+		return 0; /* space is empty, nothing to do */
+
+	if (txn_enable_yield_for_ddl("space format check") != 0)
+		return -1;
 
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
@@ -4348,7 +4353,11 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, src_space, &need_wal_sync);
 
-	txn_enable_yield_for_ddl();
+	if (!need_wal_sync && vy_lsm_is_empty(pk))
+		return 0; /* space is empty, nothing to do */
+
+	if (txn_enable_yield_for_ddl("index build") != 0)
+		return -1;
 
 	/*
 	 * Iterate over all tuples stored in the space and insert
diff --git a/test/box/misc.result b/test/box/misc.result
index dab8549b..3c8e6994 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -385,6 +385,7 @@ t;
   51: box.error.NO_SUCH_FUNCTION
   52: box.error.FUNCTION_EXISTS
   53: box.error.BEFORE_REPLACE_RET
+  54: box.error.MULTISTATEMENT_DDL
   55: box.error.TRIGGER_EXISTS
   56: box.error.USER_MAX
   57: box.error.NO_SUCH_ENGINE
diff --git a/test/box/on_replace.result b/test/box/on_replace.result
index b71c9878..6334d9a2 100644
--- a/test/box/on_replace.result
+++ b/test/box/on_replace.result
@@ -465,86 +465,83 @@ s = box.schema.space.create('test_on_repl_ddl')
 _ = s:create_index('pk')
 ---
 ...
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
+---
+...
 t = s:on_replace(function () box.schema.space.create('some_space') end)
 ---
 ...
 s:replace({1, 2})
 ---
-- error: Space _schema does not support multi-statement transactions
+- [1, 2]
 ...
-t = s:on_replace(function () s:create_index('sec') end, t)
+t = s:on_replace(function () s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) end, t)
 ---
 ...
 s:replace({2, 3})
 ---
-- error: Space _index does not support multi-statement transactions
+- [2, 3]
 ...
-t = s:on_replace(function () box.schema.user.create('newu') end, t)
+t = s:on_replace(function () box.schema.user.create('newu') box.schema.role.create('newr') end, t)
 ---
 ...
 s:replace({3, 4})
 ---
-- error: Space _user does not support multi-statement transactions
+- [3, 4]
 ...
-t = s:on_replace(function () box.schema.role.create('newr') end, t)
+t = s:on_replace(function () box.schema.user.drop('newu') box.schema.role.drop('newr') end, t)
 ---
 ...
 s:replace({4, 5})
 ---
-- error: Space _user does not support multi-statement transactions
+- [4, 5]
 ...
-t = s:on_replace(function () box.space._user:delete{box.schema.GUEST_ID} end, t)
----
-...
-s:replace({4, 5})
----
-- error: Space _user does not support multi-statement transactions
-...
-t = s:on_replace(function () box.space._user:delete{box.schema.SUPER_ROLE_ID} end, t)
----
-...
-s:replace({4, 5})
----
-- error: Space _user does not support multi-statement transactions
-...
-t = s:on_replace(function () s:drop() end, t)
+t = s:on_replace(function () s.index.sk:drop() end, t)
 ---
 ...
 s:replace({5, 6})
 ---
-- error: Space _index does not support multi-statement transactions
+- [5, 6]
 ...
 t = s:on_replace(function () box.schema.func.create('newf') end, t)
 ---
 ...
 s:replace({6, 7})
 ---
-- error: Space _func does not support multi-statement transactions
+- [6, 7]
 ...
 t = s:on_replace(function () box.schema.user.grant('guest', 'read,write', 'space', 'test_on_repl_ddl') end, t)
 ---
 ...
 s:replace({7, 8})
 ---
-- error: Space _priv does not support multi-statement transactions
+- [7, 8]
 ...
 t = s:on_replace(function () s:rename('newname') end, t)
 ---
 ...
 s:replace({8, 9})
 ---
-- error: Space _space does not support multi-statement transactions
+- [8, 9]
 ...
 t = s:on_replace(function () s.index.pk:rename('newname') end, t)
 ---
 ...
 s:replace({9, 10})
 ---
-- error: Space _index does not support multi-statement transactions
+- [9, 10]
 ...
 s:select()
 ---
-- []
+- - [1, 2]
+  - [2, 3]
+  - [3, 4]
+  - [4, 5]
+  - [5, 6]
+  - [6, 7]
+  - [7, 8]
+  - [8, 9]
+  - [9, 10]
 ...
 s:drop() -- test_on_repl_ddl
 ---
diff --git a/test/box/on_replace.test.lua b/test/box/on_replace.test.lua
index 7fffc1e0..79c828da 100644
--- a/test/box/on_replace.test.lua
+++ b/test/box/on_replace.test.lua
@@ -181,19 +181,16 @@ second:drop()
 
 s = box.schema.space.create('test_on_repl_ddl')
 _ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {2, 'unsigned'}})
 t = s:on_replace(function () box.schema.space.create('some_space') end)
 s:replace({1, 2})
-t = s:on_replace(function () s:create_index('sec') end, t)
+t = s:on_replace(function () s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) end, t)
 s:replace({2, 3})
-t = s:on_replace(function () box.schema.user.create('newu') end, t)
+t = s:on_replace(function () box.schema.user.create('newu') box.schema.role.create('newr') end, t)
 s:replace({3, 4})
-t = s:on_replace(function () box.schema.role.create('newr') end, t)
+t = s:on_replace(function () box.schema.user.drop('newu') box.schema.role.drop('newr') end, t)
 s:replace({4, 5})
-t = s:on_replace(function () box.space._user:delete{box.schema.GUEST_ID} end, t)
-s:replace({4, 5})
-t = s:on_replace(function () box.space._user:delete{box.schema.SUPER_ROLE_ID} end, t)
-s:replace({4, 5})
-t = s:on_replace(function () s:drop() end, t)
+t = s:on_replace(function () s.index.sk:drop() end, t)
 s:replace({5, 6})
 t = s:on_replace(function () box.schema.func.create('newf') end, t)
 s:replace({6, 7})
diff --git a/test/box/transaction.result b/test/box/transaction.result
index ad2d650c..9a48f2f7 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -84,22 +84,12 @@ while f:status() ~= 'dead' do fiber.sleep(0) end;
 ---
 ...
 -- transactions and system spaces
--- some operation involves more than one ddl spaces, so they should fail
-box.begin() box.schema.space.create('test');
+box.begin() box.schema.space.create('test') box.rollback();
 ---
-- error: Space _space does not support multi-statement transactions
 ...
-box.rollback();
+box.begin() box.schema.user.create('test') box.rollback();
 ---
 ...
-box.begin() box.schema.user.create('test');
----
-- error: Space _priv does not support multi-statement transactions
-...
-box.rollback();
----
-...
--- but this is Ok now
 box.begin() box.schema.func.create('test') box.rollback();
 ---
 ...
@@ -657,21 +647,14 @@ box.space.vinyl:select{};
 -- Two DDL satements in a row
 box.begin()
 box.space.test:truncate()
-box.space.test:truncate();
----
-- error: Space _truncate does not support multi-statement transactions
-...
--- A transaction is left open due to an exception in the above fragment
+box.space.test:truncate()
 box.rollback();
 ---
 ...
 -- Two DDL stateemnts on different engines
 box.begin()
 box.space.memtx:truncate()
-box.space.vinyl:truncate();
----
-- error: Space _truncate does not support multi-statement transactions
-...
+box.space.vinyl:truncate()
 box.rollback();
 ---
 ...
@@ -738,3 +721,60 @@ box.commit() -- error
 s:drop()
 ---
 ...
+--
+-- Multiple DDL statements in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function create()
+    box.schema.role.create('my_role')
+    box.schema.user.create('my_user')
+    box.schema.user.grant('my_user', 'my_role')
+    box.schema.space.create('memtx_space', {engine = 'memtx'})
+    box.schema.space.create('vinyl_space', {engine = 'vinyl'})
+    box.schema.role.grant('my_role', 'read,write', 'space', 'vinyl_space')
+    box.schema.user.grant('my_user', 'read,write', 'space', 'memtx_space')
+    box.space.memtx_space:create_index('pk', {sequence = true})
+    box.space.memtx_space:create_index('sk', {parts = {2, 'unsigned'}})
+    box.space.vinyl_space:create_index('pk', {sequence = true})
+    box.space.vinyl_space:create_index('sk', {parts = {2, 'unsigned'}})
+    box.space.memtx_space:truncate()
+    box.space.vinyl_space:truncate()
+    box.space.memtx_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+    box.space.vinyl_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+    box.schema.func.create('my_func')
+end;
+---
+...
+function drop()
+    box.schema.func.drop('my_func')
+    box.space.memtx_space:truncate()
+    box.space.vinyl_space:truncate()
+    box.schema.user.revoke('my_user', 'read,write', 'space', 'memtx_space')
+    box.schema.role.revoke('my_role', 'read,write', 'space', 'vinyl_space')
+    box.space.memtx_space:drop()
+    box.space.vinyl_space:drop()
+    box.schema.user.revoke('my_user', 'my_role')
+    box.schema.user.drop('my_user')
+    box.schema.role.drop('my_role')
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+box.begin() create() box.rollback()
+---
+...
+box.begin() create() box.commit()
+---
+...
+box.begin() drop() box.rollback()
+---
+...
+box.begin() drop() box.commit()
+---
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 8cd3e8ba..0792cc3c 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -41,12 +41,8 @@ f = fiber.create(sloppy);
 -- ensure it's rolled back automatically
 while f:status() ~= 'dead' do fiber.sleep(0) end;
 -- transactions and system spaces
--- some operation involves more than one ddl spaces, so they should fail
-box.begin() box.schema.space.create('test');
-box.rollback();
-box.begin() box.schema.user.create('test');
-box.rollback();
--- but this is Ok now
+box.begin() box.schema.space.create('test') box.rollback();
+box.begin() box.schema.user.create('test') box.rollback();
 box.begin() box.schema.func.create('test') box.rollback();
 box.begin() box.schema.user.grant('guest', 'read', 'space', '_priv') box.rollback();
 space = box.schema.space.create('test');
@@ -341,16 +337,13 @@ box.space.vinyl:select{};
 -- Two DDL satements in a row
 box.begin()
 box.space.test:truncate()
-box.space.test:truncate();
-
--- A transaction is left open due to an exception in the above fragment
+box.space.test:truncate()
 box.rollback();
 
 -- Two DDL stateemnts on different engines
 box.begin()
 box.space.memtx:truncate()
-box.space.vinyl:truncate();
-
+box.space.vinyl:truncate()
 box.rollback();
 
 box.space.memtx:select{};
@@ -381,3 +374,44 @@ s = box.schema.space.create('test')
 box.begin() s:create_index('pk') fiber.sleep(0)
 box.commit() -- error
 s:drop()
+
+--
+-- Multiple DDL statements in the same transaction.
+--
+test_run:cmd("setopt delimiter ';'")
+function create()
+    box.schema.role.create('my_role')
+    box.schema.user.create('my_user')
+    box.schema.user.grant('my_user', 'my_role')
+    box.schema.space.create('memtx_space', {engine = 'memtx'})
+    box.schema.space.create('vinyl_space', {engine = 'vinyl'})
+    box.schema.role.grant('my_role', 'read,write', 'space', 'vinyl_space')
+    box.schema.user.grant('my_user', 'read,write', 'space', 'memtx_space')
+    box.space.memtx_space:create_index('pk', {sequence = true})
+    box.space.memtx_space:create_index('sk', {parts = {2, 'unsigned'}})
+    box.space.vinyl_space:create_index('pk', {sequence = true})
+    box.space.vinyl_space:create_index('sk', {parts = {2, 'unsigned'}})
+    box.space.memtx_space:truncate()
+    box.space.vinyl_space:truncate()
+    box.space.memtx_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+    box.space.vinyl_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+    box.schema.func.create('my_func')
+end;
+function drop()
+    box.schema.func.drop('my_func')
+    box.space.memtx_space:truncate()
+    box.space.vinyl_space:truncate()
+    box.schema.user.revoke('my_user', 'read,write', 'space', 'memtx_space')
+    box.schema.role.revoke('my_role', 'read,write', 'space', 'vinyl_space')
+    box.space.memtx_space:drop()
+    box.space.vinyl_space:drop()
+    box.schema.user.revoke('my_user', 'my_role')
+    box.schema.user.drop('my_user')
+    box.schema.role.drop('my_role')
+end;
+test_run:cmd("setopt delimiter ''");
+
+box.begin() create() box.rollback()
+box.begin() create() box.commit()
+box.begin() drop() box.rollback()
+box.begin() drop() box.commit()
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index 30f305e9..c59c85f1 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -2215,8 +2215,62 @@ box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
 box.commit();
 ---
 ...
--- index and space drop are not currently supported (because of truncate)
-s.index.pk:drop();
+box.begin()
+s.index.pk:drop()
+s:drop()
+box.commit();
+---
+...
+--
+-- Only the first statement in a transaction is allowed to be
+-- a yielding DDL statement (index build, space format check).
+--
+s = box.schema.space.create('test', {engine = engine});
+---
+...
+_ = s:create_index('pk');
+---
+...
+s:insert{1, 1};
+---
+- [1, 1]
+...
+-- ok
+box.begin()
+s:create_index('sk', {parts = {2, 'unsigned'}})
+box.commit();
+---
+...
+s.index.sk:drop();
+---
+...
+-- ok
+box.begin()
+s:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+box.commit();
+---
+...
+s:format({});
+---
+...
+-- error
+box.begin()
+s.index.pk:alter{sequence = true}
+s:create_index('sk', {parts = {2, 'unsigned'}});
+---
+- error: Can not perform index build in a multi-statement transaction
+...
+box.rollback();
+---
+...
+-- error
+box.begin()
+s.index.pk:alter{sequence = true}
+s:format({{'a', 'unsigned'}, {'b', 'unsigned'}});
+---
+- error: Can not perform space format check in a multi-statement transaction
+...
+box.rollback();
 ---
 ...
 s:drop();
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 25ce8ee6..1670b548 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -827,8 +827,43 @@ box.begin()
 box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
 box.commit();
 
--- index and space drop are not currently supported (because of truncate)
-s.index.pk:drop();
+box.begin()
+s.index.pk:drop()
+s:drop()
+box.commit();
+
+--
+-- Only the first statement in a transaction is allowed to be
+-- a yielding DDL statement (index build, space format check).
+--
+s = box.schema.space.create('test', {engine = engine});
+_ = s:create_index('pk');
+s:insert{1, 1};
+
+-- ok
+box.begin()
+s:create_index('sk', {parts = {2, 'unsigned'}})
+box.commit();
+s.index.sk:drop();
+
+-- ok
+box.begin()
+s:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
+box.commit();
+s:format({});
+
+-- error
+box.begin()
+s.index.pk:alter{sequence = true}
+s:create_index('sk', {parts = {2, 'unsigned'}});
+box.rollback();
+
+-- error
+box.begin()
+s.index.pk:alter{sequence = true}
+s:format({{'a', 'unsigned'}, {'b', 'unsigned'}});
+box.rollback();
+
 s:drop();
 
 --
-- 
2.11.0

Re: [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:
>  static int
>  memtx_engine_begin(struct engine *engine, struct txn *txn)
>  {
>  	(void)engine;
> +	txn_disable_yield(txn);

This name is rather obscure.

txn_prohibit_yield()
txn_rollback_on_yield()
txn_disallow_yield()
txn_set_yield_trigger()
txn_set_rollback_on_yield_trigger()
txn_can_yield(false);

> -	/* .prepare = */ memtx_engine_prepare,
> +	/* .begin_statement = */ generic_engine_begin_statement,
> +	/* .prepare = */ generic_engine_prepare,

If some of these callbacks are now empty in all engines, they
should be removed from vtab.

> +	txn_enable_yield_for_ddl();
> +

Same here:

txn_allow_yield()

txn_yield_exception();
txn_can_yield(true);

>  	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
>  	struct memtx_ddl_state state;
>  	state.format = format;
> @@ -930,6 +932,7 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
>  	iterator_delete(it);
>  	diag_destroy(&state.diag);
>  	trigger_clear(&on_replace);
> +	txn_disable_yield_after_ddl();

txn_can_yield(false);

> +
>  static inline void
>  fiber_set_txn(struct fiber *fiber, struct txn *txn)
>  {
> @@ -194,6 +200,7 @@ txn_begin()
>  	txn->has_triggers  = false;
>  	txn->is_done = false;
>  	txn->is_aborted = false;
> +	txn->is_yield_disabled = false;

It is hight time to convert these to bit fields.

txn->can_yield

>  	txn->in_sub_stmt = 0;
>  	txn->id = ++tsn;
>  	txn->signature = -1;
> @@ -201,8 +208,8 @@ txn_begin()
>  	txn->engine_tx = NULL;
>  	txn->psql_txn = NULL;
>  	txn->fiber = NULL;
> -	/* fiber_on_yield/fiber_on_stop initialized by engine on demand */
>  	fiber_set_txn(fiber(), txn);
> +	/* fiber_on_yield is initialized by engine on demand */
>  	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
>  	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
>  	return txn;
> @@ -463,6 +470,12 @@ txn_write_to_wal(struct txn *txn)
>  static int
>  txn_prepare(struct txn *txn)
>  {
> +	if (txn->is_aborted) {
> +		assert(txn->is_yield_disabled);

To anyone who doesn't know that is_aborted is only set by the
yield trigger, this is rather obscure. How are these variables
connected? 
is_aborted is a very general name, perhaps the context has changed
a bit but now when I read this code I don't immediately understadn
why is_aborted == ER_TRANACTION_YIELD. 

Perhaps is_aborted should be is_aborted_by_yield.

Overall I agree the approach is much better than before, at least
easier to track since the logic is not hidden in the engine.

Please let's make the names more evident now, and the api simple.
I think txn_can_yield(true/false) is good, and I also think that
we need to collapse all txn flags into a bit field now that we
have a critical mass.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:

Please write a follow up patch which changes box/lua/schema.lua
operations to use transactions wherever possible.

> The patch is pretty straightforward - all it does is moves checks for
> single statement transactions from alter.cc to txn_enable_yield_for_ddl
> so that now any DDL request may be executed in a transaction unless it
> builds an index or checks the format of a non-empty space (those are the
> only two operations that may yield).
> 
> There's two things that must be noted explicitly. The first is removal
> of an assertion from priv_grant. The assertion ensured that a revoked
> privilege was in the cache. The problem is the cache is built from the
> contents of the space, see user_reload_privs. On rollback, we first
> revert the content of the space to the original state, and only then
> start invoking rollback triggers, which call priv_grant. As a result, we
> will revert the cache to the original state right after the first
> trigger is invoked and the following triggers will have no effect on it.
> Thus we have to remove this assertion.
> 
> The second subtlety lays in vinyl_index_commit_modify. Before the commit
> we assumed that if statement lsn is <= vy_lsm::commit_lsn, then it must
> be local recovery from WAL. Now it's not true, because there may be
> several operations for the same index in a transaction, and they all
> will receive the same signature in on_commit trigger. We could, of
> course, try to assign different signatures to them, but that would look
> cumbersome - better simply allow lsn <= vy_lsm::commit_lsn after local
> recovery, there's actually nothing wrong about that.
> 
> Closes #4083
> 
> @TarantoolBot document
> Title: Transactional DDL
> 
> Now it's possible to group non-yielding DDL statements into
> transactions, e.g.
> 
> ```Lua
> box.begin()
> box.schema.space.create('my_space')
> box.space.my_space:create_index('primary')
> box.commit() -- or box.rollback()
> ```
> 
> Most DDL statements don't yield and hence can be run from transactions.
> There are just two exceptions: creation of a new index and changing the
> format of a non-empty space. Those are long operations that may yield
> so as not to block the event loop for too long. Those statements can't
> be executed from transactions (to be more exact, such a statement must
> go first in any transaction).
> 
> Also, just like in case of DML transactions in memtx, it's forbidden to
> explicitly yield in a DDL transaction by calling fiber.sleep or any
> other yielding function. If this happens, the transaction will be
> aborted and an attempt to commit it will fail.
> ---
>  src/box/alter.cc              | 14 --------
>  src/box/errcode.h             |  2 +-
>  src/box/memtx_space.c         |  8 +++--
>  src/box/txn.c                 | 27 +++++++-------
>  src/box/txn.h                 | 25 ++++---------
>  src/box/user.cc               |  1 -
>  src/box/vinyl.c               | 23 ++++++++----
>  test/box/misc.result          |  1 +
>  test/box/on_replace.result    | 53 +++++++++++++---------------
>  test/box/on_replace.test.lua  | 13 +++----
>  test/box/transaction.result   | 82 ++++++++++++++++++++++++++++++++-----------
>  test/box/transaction.test.lua | 56 +++++++++++++++++++++++------
>  test/engine/ddl.result        | 58 ++++++++++++++++++++++++++++--
>  test/engine/ddl.test.lua      | 39 ++++++++++++++++++--
>  14 files changed, 273 insertions(+), 129 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 612bcd89..c7be6b77 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1816,7 +1816,6 @@ static void
>  on_replace_dd_space(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _space");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -2113,7 +2112,6 @@ static void
>  on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _index");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -2305,7 +2303,6 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
> -	txn_check_singlestatement_xc(txn, "Space _truncate");
>  	struct tuple *new_tuple = stmt->new_tuple;
>  
>  	if (new_tuple == NULL) {
> @@ -2535,7 +2532,6 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
> -	txn_check_singlestatement_xc(txn, "Space _user");
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
>  
> @@ -2686,7 +2682,6 @@ static void
>  on_replace_dd_func(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _func");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -2880,7 +2875,6 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> -	txn_check_singlestatement_xc(txn, "Space _collation");
>  	if (new_tuple == NULL && old_tuple != NULL) {
>  		/* DELETE */
>  		struct trigger *on_commit =
> @@ -3178,7 +3172,6 @@ static void
>  on_replace_dd_priv(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _priv");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -3227,7 +3220,6 @@ static void
>  on_replace_dd_schema(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _schema");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -3305,7 +3297,6 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
>  {
>  	(void) trigger;
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _cluster");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -3458,7 +3449,6 @@ static void
>  on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _sequence");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -3661,7 +3651,6 @@ static void
>  on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _space_sequence");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *tuple = stmt->new_tuple ? stmt->new_tuple : stmt->old_tuple;
>  
> @@ -3806,7 +3795,6 @@ static void
>  on_replace_dd_trigger(struct trigger * /* trigger */, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _trigger");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -4189,7 +4177,6 @@ static void
>  on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _fk_constraint");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> @@ -4480,7 +4467,6 @@ static void
>  on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
>  {
>  	struct txn *txn = (struct txn *) event;
> -	txn_check_singlestatement_xc(txn, "Space _ck_constraint");
>  	struct txn_stmt *stmt = txn_current_stmt(txn);
>  	struct tuple *old_tuple = stmt->old_tuple;
>  	struct tuple *new_tuple = stmt->new_tuple;
> diff --git a/src/box/errcode.h b/src/box/errcode.h
> index be8dab27..97fc9675 100644
> --- a/src/box/errcode.h
> +++ b/src/box/errcode.h
> @@ -106,7 +106,7 @@ struct errcode_record {
>  	/* 51 */_(ER_NO_SUCH_FUNCTION,		"Function '%s' does not exist") \
>  	/* 52 */_(ER_FUNCTION_EXISTS,		"Function '%s' already exists") \
>  	/* 53 */_(ER_BEFORE_REPLACE_RET,	"Invalid return value of space:before_replace trigger: expected tuple or nil, got %s") \
> -	/* 54 */_(ER_UNUSED2,			"") \
> +	/* 54 */_(ER_MULTISTATEMENT_DDL,	"Can not perform %s in a multi-statement transaction") \
>  	/* 55 */_(ER_TRIGGER_EXISTS,		"Trigger '%s' already exists") \
>  	/* 56 */_(ER_USER_MAX,			"A limit on the total number of users has been reached: %u") \
>  	/* 57 */_(ER_NO_SUCH_ENGINE,		"Space engine '%s' does not exist") \
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 921dbcbf..47369994 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -888,7 +888,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
>  	if (it == NULL)
>  		return -1;
>  
> -	txn_enable_yield_for_ddl();
> +	if (txn_enable_yield_for_ddl("space format check") != 0)
> +		return -1;
>  
>  	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
>  	struct memtx_ddl_state state;
> @@ -1018,6 +1019,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  	struct index *pk = index_find(src_space, 0);
>  	if (pk == NULL)
>  		return -1;
> +	if (index_size(pk) == 0)
> +		return 0;
>  
>  	struct errinj *inj = errinj(ERRINJ_BUILD_INDEX, ERRINJ_INT);
>  	if (inj != NULL && inj->iparam == (int)new_index->def->iid) {
> @@ -1030,7 +1033,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  	if (it == NULL)
>  		return -1;
>  
> -	txn_enable_yield_for_ddl();
> +	if (txn_enable_yield_for_ddl("index build") != 0)
> +		return -1;
>  
>  	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
>  	struct memtx_ddl_state state;
> diff --git a/src/box/txn.c b/src/box/txn.c
> index bab98d7e..9ae11aa7 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -592,17 +592,6 @@ txn_abort(struct txn *txn)
>  	txn->is_aborted = true;
>  }
>  
> -int
> -txn_check_singlestatement(struct txn *txn, const char *where)
> -{
> -	if (!txn_is_first_statement(txn)) {
> -		diag_set(ClientError, ER_UNSUPPORTED,
> -			 where, "multi-statement transactions");
> -		return -1;
> -	}
> -	return 0;
> -}
> -
>  void
>  txn_disable_yield(struct txn *txn)
>  {
> @@ -612,12 +601,24 @@ txn_disable_yield(struct txn *txn)
>  	trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
>  }
>  
> -void
> -txn_enable_yield_for_ddl(void)
> +int
> +txn_enable_yield_for_ddl(const char *what)
>  {
>  	struct txn *txn = in_txn();
>  	assert(txn != NULL && txn->is_yield_disabled);
> +	/*
> +	 * It's okay to yield while executing the first DDL statement
> +	 * in a transaction, because the schema hasn't been updated
> +	 * yet and so other transactions can't see uncommitted objects.
> +	 * Yielding in subsequent statements is not safe, as there
> +	 * may be uncommitted objects in the schema cache.
> +	 */
> +	if (!txn_is_first_statement(txn)) {
> +		diag_set(ClientError, ER_MULTISTATEMENT_DDL, what);
> +		return -1;
> +	}
>  	trigger_clear(&txn->fiber_on_yield);
> +	return 0;
>  }
>  
>  void
> diff --git a/src/box/txn.h b/src/box/txn.h
> index 258a8db7..92366038 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -358,13 +358,6 @@ void
>  txn_rollback_stmt(struct txn *txn);
>  
>  /**
> - * Raise an error if this is a multi-statement transaction: DDL
> - * can not be part of a multi-statement transaction.
> - */
> -int
> -txn_check_singlestatement(struct txn *txn, const char *where);
> -
> -/**
>   * Installs yield-in-transaction trigger: roll back the effects
>   * of the transaction and mark the transaction as aborted.
>   *
> @@ -384,9 +377,13 @@ txn_disable_yield(struct txn *txn);
>   * This function temporarily disables the trigger for this purpose.
>   * One must call txn_disable_yield_after_ddl() once the DDL request
>   * has been complete.
> + *
> + * Before enabling yields, this function checks if it doesn't
> + * violate transaction isolation. If it does, it returns -1 and
> + * sets diag. The 'what' message is used for error reporting.
>   */
> -void
> -txn_enable_yield_for_ddl(void);
> +int
> +txn_enable_yield_for_ddl(const char *what);
>  
>  /** See txn_enable_yield_for_ddl(). */
>  void
> @@ -525,16 +522,6 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *savepoint);
>  
>  #if defined(__cplusplus)
>  } /* extern "C" */
> -
> -#include "diag.h"
> -
> -static inline void
> -txn_check_singlestatement_xc(struct txn *txn, const char *where)
> -{
> -	if (txn_check_singlestatement(txn, where) != 0)
> -		diag_raise();
> -}
> -
>  #endif /* defined(__cplusplus) */
>  
>  #endif /* TARANTOOL_BOX_TXN_H_INCLUDED */
> diff --git a/src/box/user.cc b/src/box/user.cc
> index 48bdf18e..c46ff67d 100644
> --- a/src/box/user.cc
> +++ b/src/box/user.cc
> @@ -709,7 +709,6 @@ priv_grant(struct user *grantee, struct priv_def *priv)
>  	if (object == NULL)
>  		return;
>  	struct access *access = &object[grantee->auth_token];
> -	assert(privset_search(&grantee->privs, priv) || access->granted == 0);
>  	access->granted = priv->access;
>  	rebuild_effective_grants(grantee);
>  }
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index 8629aa8e..af044f0c 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -919,16 +919,17 @@ vinyl_index_commit_modify(struct index *index, int64_t lsn)
>  	       env->status == VINYL_FINAL_RECOVERY_LOCAL ||
>  	       env->status == VINYL_FINAL_RECOVERY_REMOTE);
>  
> -	if (lsn <= lsm->commit_lsn) {
> +	if (env->status == VINYL_FINAL_RECOVERY_LOCAL &&
> +	    lsn <= lsm->commit_lsn) {
>  		/*
> -		 * This must be local recovery from WAL, when
> -		 * the operation has already been committed to
> -		 * vylog.
> +		 * The statement we are recovering from WAL has
> +		 * been successfully written to vylog so we must
> +		 * not replay it.
>  		 */
> -		assert(env->status == VINYL_FINAL_RECOVERY_LOCAL);
>  		return;
>  	}
>  
> +	assert(lsm->commit_lsn <= lsn);
>  	lsm->commit_lsn = lsn;
>  
>  	vy_log_tx_begin();
> @@ -1121,7 +1122,11 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
>  	bool need_wal_sync;
>  	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
>  
> -	txn_enable_yield_for_ddl();
> +	if (!need_wal_sync && vy_lsm_is_empty(pk))
> +		return 0; /* space is empty, nothing to do */
> +
> +	if (txn_enable_yield_for_ddl("space format check") != 0)
> +		return -1;
>  
>  	struct trigger on_replace;
>  	struct vy_check_format_ctx ctx;
> @@ -4348,7 +4353,11 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
>  	bool need_wal_sync;
>  	tx_manager_abort_writers_for_ddl(env->xm, src_space, &need_wal_sync);
>  
> -	txn_enable_yield_for_ddl();
> +	if (!need_wal_sync && vy_lsm_is_empty(pk))
> +		return 0; /* space is empty, nothing to do */
> +
> +	if (txn_enable_yield_for_ddl("index build") != 0)
> +		return -1;
>  
>  	/*
>  	 * Iterate over all tuples stored in the space and insert
> diff --git a/test/box/misc.result b/test/box/misc.result
> index dab8549b..3c8e6994 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -385,6 +385,7 @@ t;
>    51: box.error.NO_SUCH_FUNCTION
>    52: box.error.FUNCTION_EXISTS
>    53: box.error.BEFORE_REPLACE_RET
> +  54: box.error.MULTISTATEMENT_DDL
>    55: box.error.TRIGGER_EXISTS
>    56: box.error.USER_MAX
>    57: box.error.NO_SUCH_ENGINE
> diff --git a/test/box/on_replace.result b/test/box/on_replace.result
> index b71c9878..6334d9a2 100644
> --- a/test/box/on_replace.result
> +++ b/test/box/on_replace.result
> @@ -465,86 +465,83 @@ s = box.schema.space.create('test_on_repl_ddl')
>  _ = s:create_index('pk')
>  ---
>  ...
> +_ = s:create_index('sk', {parts = {2, 'unsigned'}})
> +---
> +...
>  t = s:on_replace(function () box.schema.space.create('some_space') end)
>  ---
>  ...
>  s:replace({1, 2})
>  ---
> -- error: Space _schema does not support multi-statement transactions
> +- [1, 2]
>  ...
> -t = s:on_replace(function () s:create_index('sec') end, t)
> +t = s:on_replace(function () s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) end, t)
>  ---
>  ...
>  s:replace({2, 3})
>  ---
> -- error: Space _index does not support multi-statement transactions
> +- [2, 3]
>  ...
> -t = s:on_replace(function () box.schema.user.create('newu') end, t)
> +t = s:on_replace(function () box.schema.user.create('newu') box.schema.role.create('newr') end, t)
>  ---
>  ...
>  s:replace({3, 4})
>  ---
> -- error: Space _user does not support multi-statement transactions
> +- [3, 4]
>  ...
> -t = s:on_replace(function () box.schema.role.create('newr') end, t)
> +t = s:on_replace(function () box.schema.user.drop('newu') box.schema.role.drop('newr') end, t)
>  ---
>  ...
>  s:replace({4, 5})
>  ---
> -- error: Space _user does not support multi-statement transactions
> +- [4, 5]
>  ...
> -t = s:on_replace(function () box.space._user:delete{box.schema.GUEST_ID} end, t)
> ----
> -...
> -s:replace({4, 5})
> ----
> -- error: Space _user does not support multi-statement transactions
> -...
> -t = s:on_replace(function () box.space._user:delete{box.schema.SUPER_ROLE_ID} end, t)
> ----
> -...
> -s:replace({4, 5})
> ----
> -- error: Space _user does not support multi-statement transactions
> -...
> -t = s:on_replace(function () s:drop() end, t)
> +t = s:on_replace(function () s.index.sk:drop() end, t)
>  ---
>  ...
>  s:replace({5, 6})
>  ---
> -- error: Space _index does not support multi-statement transactions
> +- [5, 6]
>  ...
>  t = s:on_replace(function () box.schema.func.create('newf') end, t)
>  ---
>  ...
>  s:replace({6, 7})
>  ---
> -- error: Space _func does not support multi-statement transactions
> +- [6, 7]
>  ...
>  t = s:on_replace(function () box.schema.user.grant('guest', 'read,write', 'space', 'test_on_repl_ddl') end, t)
>  ---
>  ...
>  s:replace({7, 8})
>  ---
> -- error: Space _priv does not support multi-statement transactions
> +- [7, 8]
>  ...
>  t = s:on_replace(function () s:rename('newname') end, t)
>  ---
>  ...
>  s:replace({8, 9})
>  ---
> -- error: Space _space does not support multi-statement transactions
> +- [8, 9]
>  ...
>  t = s:on_replace(function () s.index.pk:rename('newname') end, t)
>  ---
>  ...
>  s:replace({9, 10})
>  ---
> -- error: Space _index does not support multi-statement transactions
> +- [9, 10]
>  ...
>  s:select()
>  ---
> -- []
> +- - [1, 2]
> +  - [2, 3]
> +  - [3, 4]
> +  - [4, 5]
> +  - [5, 6]
> +  - [6, 7]
> +  - [7, 8]
> +  - [8, 9]
> +  - [9, 10]
>  ...
>  s:drop() -- test_on_repl_ddl
>  ---
> diff --git a/test/box/on_replace.test.lua b/test/box/on_replace.test.lua
> index 7fffc1e0..79c828da 100644
> --- a/test/box/on_replace.test.lua
> +++ b/test/box/on_replace.test.lua
> @@ -181,19 +181,16 @@ second:drop()
>  
>  s = box.schema.space.create('test_on_repl_ddl')
>  _ = s:create_index('pk')
> +_ = s:create_index('sk', {parts = {2, 'unsigned'}})
>  t = s:on_replace(function () box.schema.space.create('some_space') end)
>  s:replace({1, 2})
> -t = s:on_replace(function () s:create_index('sec') end, t)
> +t = s:on_replace(function () s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) end, t)
>  s:replace({2, 3})
> -t = s:on_replace(function () box.schema.user.create('newu') end, t)
> +t = s:on_replace(function () box.schema.user.create('newu') box.schema.role.create('newr') end, t)
>  s:replace({3, 4})
> -t = s:on_replace(function () box.schema.role.create('newr') end, t)
> +t = s:on_replace(function () box.schema.user.drop('newu') box.schema.role.drop('newr') end, t)
>  s:replace({4, 5})
> -t = s:on_replace(function () box.space._user:delete{box.schema.GUEST_ID} end, t)
> -s:replace({4, 5})
> -t = s:on_replace(function () box.space._user:delete{box.schema.SUPER_ROLE_ID} end, t)
> -s:replace({4, 5})
> -t = s:on_replace(function () s:drop() end, t)
> +t = s:on_replace(function () s.index.sk:drop() end, t)
>  s:replace({5, 6})
>  t = s:on_replace(function () box.schema.func.create('newf') end, t)
>  s:replace({6, 7})
> diff --git a/test/box/transaction.result b/test/box/transaction.result
> index ad2d650c..9a48f2f7 100644
> --- a/test/box/transaction.result
> +++ b/test/box/transaction.result
> @@ -84,22 +84,12 @@ while f:status() ~= 'dead' do fiber.sleep(0) end;
>  ---
>  ...
>  -- transactions and system spaces
> --- some operation involves more than one ddl spaces, so they should fail
> -box.begin() box.schema.space.create('test');
> +box.begin() box.schema.space.create('test') box.rollback();
>  ---
> -- error: Space _space does not support multi-statement transactions
>  ...
> -box.rollback();
> +box.begin() box.schema.user.create('test') box.rollback();
>  ---
>  ...
> -box.begin() box.schema.user.create('test');
> ----
> -- error: Space _priv does not support multi-statement transactions
> -...
> -box.rollback();
> ----
> -...
> --- but this is Ok now
>  box.begin() box.schema.func.create('test') box.rollback();
>  ---
>  ...
> @@ -657,21 +647,14 @@ box.space.vinyl:select{};
>  -- Two DDL satements in a row
>  box.begin()
>  box.space.test:truncate()
> -box.space.test:truncate();
> ----
> -- error: Space _truncate does not support multi-statement transactions
> -...
> --- A transaction is left open due to an exception in the above fragment
> +box.space.test:truncate()
>  box.rollback();
>  ---
>  ...
>  -- Two DDL stateemnts on different engines
>  box.begin()
>  box.space.memtx:truncate()
> -box.space.vinyl:truncate();
> ----
> -- error: Space _truncate does not support multi-statement transactions
> -...
> +box.space.vinyl:truncate()
>  box.rollback();
>  ---
>  ...
> @@ -738,3 +721,60 @@ box.commit() -- error
>  s:drop()
>  ---
>  ...
> +--
> +-- Multiple DDL statements in the same transaction.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +function create()
> +    box.schema.role.create('my_role')
> +    box.schema.user.create('my_user')
> +    box.schema.user.grant('my_user', 'my_role')
> +    box.schema.space.create('memtx_space', {engine = 'memtx'})
> +    box.schema.space.create('vinyl_space', {engine = 'vinyl'})
> +    box.schema.role.grant('my_role', 'read,write', 'space', 'vinyl_space')
> +    box.schema.user.grant('my_user', 'read,write', 'space', 'memtx_space')
> +    box.space.memtx_space:create_index('pk', {sequence = true})
> +    box.space.memtx_space:create_index('sk', {parts = {2, 'unsigned'}})
> +    box.space.vinyl_space:create_index('pk', {sequence = true})
> +    box.space.vinyl_space:create_index('sk', {parts = {2, 'unsigned'}})
> +    box.space.memtx_space:truncate()
> +    box.space.vinyl_space:truncate()
> +    box.space.memtx_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +    box.space.vinyl_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +    box.schema.func.create('my_func')
> +end;
> +---
> +...
> +function drop()
> +    box.schema.func.drop('my_func')
> +    box.space.memtx_space:truncate()
> +    box.space.vinyl_space:truncate()
> +    box.schema.user.revoke('my_user', 'read,write', 'space', 'memtx_space')
> +    box.schema.role.revoke('my_role', 'read,write', 'space', 'vinyl_space')
> +    box.space.memtx_space:drop()
> +    box.space.vinyl_space:drop()
> +    box.schema.user.revoke('my_user', 'my_role')
> +    box.schema.user.drop('my_user')
> +    box.schema.role.drop('my_role')
> +end;
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +box.begin() create() box.rollback()
> +---
> +...
> +box.begin() create() box.commit()
> +---
> +...
> +box.begin() drop() box.rollback()
> +---
> +...
> +box.begin() drop() box.commit()
> +---
> +...
> diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
> index 8cd3e8ba..0792cc3c 100644
> --- a/test/box/transaction.test.lua
> +++ b/test/box/transaction.test.lua
> @@ -41,12 +41,8 @@ f = fiber.create(sloppy);
>  -- ensure it's rolled back automatically
>  while f:status() ~= 'dead' do fiber.sleep(0) end;
>  -- transactions and system spaces
> --- some operation involves more than one ddl spaces, so they should fail
> -box.begin() box.schema.space.create('test');
> -box.rollback();
> -box.begin() box.schema.user.create('test');
> -box.rollback();
> --- but this is Ok now
> +box.begin() box.schema.space.create('test') box.rollback();
> +box.begin() box.schema.user.create('test') box.rollback();
>  box.begin() box.schema.func.create('test') box.rollback();
>  box.begin() box.schema.user.grant('guest', 'read', 'space', '_priv') box.rollback();
>  space = box.schema.space.create('test');
> @@ -341,16 +337,13 @@ box.space.vinyl:select{};
>  -- Two DDL satements in a row
>  box.begin()
>  box.space.test:truncate()
> -box.space.test:truncate();
> -
> --- A transaction is left open due to an exception in the above fragment
> +box.space.test:truncate()
>  box.rollback();
>  
>  -- Two DDL stateemnts on different engines
>  box.begin()
>  box.space.memtx:truncate()
> -box.space.vinyl:truncate();
> -
> +box.space.vinyl:truncate()
>  box.rollback();
>  
>  box.space.memtx:select{};
> @@ -381,3 +374,44 @@ s = box.schema.space.create('test')
>  box.begin() s:create_index('pk') fiber.sleep(0)
>  box.commit() -- error
>  s:drop()
> +
> +--
> +-- Multiple DDL statements in the same transaction.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +function create()
> +    box.schema.role.create('my_role')
> +    box.schema.user.create('my_user')
> +    box.schema.user.grant('my_user', 'my_role')
> +    box.schema.space.create('memtx_space', {engine = 'memtx'})
> +    box.schema.space.create('vinyl_space', {engine = 'vinyl'})
> +    box.schema.role.grant('my_role', 'read,write', 'space', 'vinyl_space')
> +    box.schema.user.grant('my_user', 'read,write', 'space', 'memtx_space')
> +    box.space.memtx_space:create_index('pk', {sequence = true})
> +    box.space.memtx_space:create_index('sk', {parts = {2, 'unsigned'}})
> +    box.space.vinyl_space:create_index('pk', {sequence = true})
> +    box.space.vinyl_space:create_index('sk', {parts = {2, 'unsigned'}})
> +    box.space.memtx_space:truncate()
> +    box.space.vinyl_space:truncate()
> +    box.space.memtx_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +    box.space.vinyl_space:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +    box.schema.func.create('my_func')
> +end;
> +function drop()
> +    box.schema.func.drop('my_func')
> +    box.space.memtx_space:truncate()
> +    box.space.vinyl_space:truncate()
> +    box.schema.user.revoke('my_user', 'read,write', 'space', 'memtx_space')
> +    box.schema.role.revoke('my_role', 'read,write', 'space', 'vinyl_space')
> +    box.space.memtx_space:drop()
> +    box.space.vinyl_space:drop()
> +    box.schema.user.revoke('my_user', 'my_role')
> +    box.schema.user.drop('my_user')
> +    box.schema.role.drop('my_role')
> +end;
> +test_run:cmd("setopt delimiter ''");
> +
> +box.begin() create() box.rollback()
> +box.begin() create() box.commit()
> +box.begin() drop() box.rollback()
> +box.begin() drop() box.commit()
> diff --git a/test/engine/ddl.result b/test/engine/ddl.result
> index 30f305e9..c59c85f1 100644
> --- a/test/engine/ddl.result
> +++ b/test/engine/ddl.result
> @@ -2215,8 +2215,62 @@ box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
>  box.commit();
>  ---
>  ...
> --- index and space drop are not currently supported (because of truncate)
> -s.index.pk:drop();
> +box.begin()
> +s.index.pk:drop()
> +s:drop()
> +box.commit();
> +---
> +...
> +--
> +-- Only the first statement in a transaction is allowed to be
> +-- a yielding DDL statement (index build, space format check).
> +--
> +s = box.schema.space.create('test', {engine = engine});
> +---
> +...
> +_ = s:create_index('pk');
> +---
> +...
> +s:insert{1, 1};
> +---
> +- [1, 1]
> +...
> +-- ok
> +box.begin()
> +s:create_index('sk', {parts = {2, 'unsigned'}})
> +box.commit();
> +---
> +...
> +s.index.sk:drop();
> +---
> +...
> +-- ok
> +box.begin()
> +s:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +box.commit();
> +---
> +...
> +s:format({});
> +---
> +...
> +-- error
> +box.begin()
> +s.index.pk:alter{sequence = true}
> +s:create_index('sk', {parts = {2, 'unsigned'}});
> +---
> +- error: Can not perform index build in a multi-statement transaction
> +...
> +box.rollback();
> +---
> +...
> +-- error
> +box.begin()
> +s.index.pk:alter{sequence = true}
> +s:format({{'a', 'unsigned'}, {'b', 'unsigned'}});
> +---
> +- error: Can not perform space format check in a multi-statement transaction
> +...
> +box.rollback();
>  ---
>  ...
>  s:drop();
> diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
> index 25ce8ee6..1670b548 100644
> --- a/test/engine/ddl.test.lua
> +++ b/test/engine/ddl.test.lua
> @@ -827,8 +827,43 @@ box.begin()
>  box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
>  box.commit();
>  
> --- index and space drop are not currently supported (because of truncate)
> -s.index.pk:drop();
> +box.begin()
> +s.index.pk:drop()
> +s:drop()
> +box.commit();
> +
> +--
> +-- Only the first statement in a transaction is allowed to be
> +-- a yielding DDL statement (index build, space format check).
> +--
> +s = box.schema.space.create('test', {engine = engine});
> +_ = s:create_index('pk');
> +s:insert{1, 1};
> +
> +-- ok
> +box.begin()
> +s:create_index('sk', {parts = {2, 'unsigned'}})
> +box.commit();
> +s.index.sk:drop();
> +
> +-- ok
> +box.begin()
> +s:format({{'a', 'unsigned'}, {'b', 'unsigned'}})
> +box.commit();
> +s:format({});
> +
> +-- error
> +box.begin()
> +s.index.pk:alter{sequence = true}
> +s:create_index('sk', {parts = {2, 'unsigned'}});
> +box.rollback();
> +
> +-- error
> +box.begin()
> +s.index.pk:alter{sequence = true}
> +s:format({{'a', 'unsigned'}, {'b', 'unsigned'}});
> +box.rollback();
> +
>  s:drop();
>  
>  --
> -- 
> 2.11.0
> 

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:

A test plan for this feature:

1. A transaction which swaps two spaces by name.

name = foo.name;
foo.name = bar.name
bar.name = name

2. A transaction which creates a space, formats it to have an
   integer column, changes the format to 'any', changes the values
   to 'string', changes the format to 'string'

3. A transaction which creates a space, a user, and grants all privileges
   on this space to the user.Then sudoes to the user and tries to
   use the space.

4. A transaction, which revokes all privileges from a user and drops
   the space.

5. A transaction which creates a space and secondary indexes

6. A transaction which creates a space with a sequence.

7. Transactional drop a) empty b) nonempty space with all its belongings:
   secondary indexes, check constraints, sequences.

5. A transaction which creates a space and sets space triggers, as
   well as on_rollback trigger, then inserts a bunch of data and then
   a) commits b) rolls back.

6. An error injection crash test which starts 10 fibers which perform
   a random combination of transactional DDL in a loop, then,
   after 0.1 second ER_WAL_IO is enabled, fibers are joined,
   ER_WAL_IO is disabled. The DDL is using objects which names
   clash,  so that it does something useful, e.g:

   function name(object_type)
        return object_type_name ..  math.random(1, NUM_NAMES)
    end

    function ddl(object_type, object_name) 
        -- this function actually checks if the name exists 
        --  at the moment and only returns valid statements
        if object_type is user or role then
            return random("grant", "revoke", "create", "drop")
        end
        if object_type is table then 
            return random("create ", "format", "rename", "add
            index", "drop index", "rename index", "change index
            keys", "change unique -> non_unique", "add
            constraint", "drop constraint", "drop")
        end


    function gen_test()
        for each object type:
            eval = "box.begin()"
            eval = eval .. ddl(object_type, name(object_type))
            eval = eval .. "box.commit()"
        end
        return eval
    end
    function fiber()
        while true do
            eval(gen_test)
        end
    end

    for i in 1.. NUM_FIBERS do
        fibers[i] = fiber.create(fiber)
    end
    fiber.sleep(0.1)
    errinj.enable(er_wal_io)
    for i in 1.. NUM_FIBERS do
        fiber.join(fibers[i])
    end

    (cleanup all objects matching the name..$i pattern)


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

On Wed, Jul 10, 2019 at 11:34:39PM +0300, Konstantin Osipov wrote:
> > -	/* .prepare = */ memtx_engine_prepare,
> > +	/* .begin_statement = */ generic_engine_begin_statement,
> > +	/* .prepare = */ generic_engine_prepare,
> 
> If some of these callbacks are now empty in all engines, they
> should be removed from vtab.

Those methods are still used in Vinyl so we can't remove them :-(

> >  	txn->has_triggers  = false;
> >  	txn->is_done = false;
> >  	txn->is_aborted = false;
> > +	txn->is_yield_disabled = false;
> 
> It is hight time to convert these to bit fields.

NP but let's please do it in a separate patch if we have to.

> Overall I agree the approach is much better than before, at least
> easier to track since the logic is not hidden in the engine.
> 
> Please let's make the names more evident now, and the api simple.
> I think txn_can_yield(true/false) is good, and I also think that
> we need to collapse all txn flags into a bit field now that we
> have a critical mass.

Agree, the current API does look kinda crooked :-/ Reworked according to
your review. Please see the updated patch below and on the branch.
---

From 73cb8a22680bf362a5dce925b0aebd2e7508d334 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 5 Jul 2019 17:40:56 +0300
Subject: [PATCH] memtx: fix txn_on_yield for DDL transactions

Memtx engine doesn't allow yielding inside a transaction. To achieve
that, it installs fiber->on_yield trigger that aborts the current
transaction (rolls it back, but leaves it be so that commit fails).

There's an exception though - DDL statements are allowed to yield.
This is required so as not to block the event loop while a new index
is built or a space format is checked. Currently, we handle this
exception by checking space id and omitting installation of the
trigger for system spaces. This isn't entirely correct, because we
may yield after a DDL statement is complete, in which case the
transaction won't be aborted though it should:

  box.begin()
  box.space.my_space:create_index('my_index')
  fiber.sleep(0) -- doesn't abort the transaction!

This patch fixes the problem by making the memtx engine install the
on_yield trigger unconditionally, for all kinds of transactions, and
instead explicitly disabling the trigger for yielding DDL operations.

In order not to spread the yield-in-transaction logic between memtx
and txn code, let's move all fiber_on_yield related stuff to txn,
export a method to disable yields, and use the method in memtx.

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index c8376110..869cd343 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -55,58 +55,6 @@
 static void
 checkpoint_cancel(struct checkpoint *ckpt);
 
-/*
- * Memtx yield-in-transaction trigger: roll back the effects
- * of the transaction and mark the transaction as aborted.
- */
-static void
-txn_on_yield(struct trigger *trigger, void *event)
-{
-	(void) trigger;
-	(void) event;
-
-	struct txn *txn = in_txn();
-	assert(txn && txn->engine_tx);
-	if (txn == NULL || txn->engine_tx == NULL)
-		return;
-	txn_abort(txn);                 /* doesn't yield or fail */
-}
-
-/**
- * Initialize context for yield triggers.
- * In case of a yield inside memtx multi-statement transaction
- * we must, first of all, roll back the effects of the transaction
- * so that concurrent transactions won't see dirty, uncommitted
- * data.
- * Second, we must abort the transaction, since it has been rolled
- * back implicitly. The transaction can not be rolled back
- * completely from within a yield trigger, since a yield trigger
- * can't fail. Instead, we mark the transaction as aborted and
- * raise an error on attempt to commit it.
- *
- * So much hassle to be user-friendly until we have a true
- * interactive transaction support in memtx.
- */
-void
-memtx_init_txn(struct txn *txn)
-{
-	struct fiber *fiber = fiber();
-
-	trigger_create(&txn->fiber_on_yield, txn_on_yield,
-		       NULL, NULL);
-	/*
-	 * Memtx doesn't allow yields between statements of
-	 * a transaction. Set a trigger which would roll
-	 * back the transaction if there is a yield.
-	 */
-	trigger_add(&fiber->on_yield, &txn->fiber_on_yield);
-	/*
-	 * This serves as a marker that the triggers are
-	 * initialized.
-	 */
-	txn->engine_tx = txn;
-}
-
 struct PACKED memtx_tuple {
 	/*
 	 * sic: the header of the tuple is used
@@ -372,45 +320,10 @@ memtx_engine_create_space(struct engine *engine, struct space_def *def,
 }
 
 static int
-memtx_engine_prepare(struct engine *engine, struct txn *txn)
-{
-	(void)engine;
-	if (txn->engine_tx == 0)
-		return 0;
-	/*
-	 * These triggers are only used for memtx and only
-	 * when autocommit == false, so we are saving
-	 * on calls to trigger_create/trigger_clear.
-	 */
-	trigger_clear(&txn->fiber_on_yield);
-	if (txn->is_aborted) {
-		diag_set(ClientError, ER_TRANSACTION_YIELD);
-		diag_log();
-		return -1;
-	}
-	return 0;
-}
-
-static int
 memtx_engine_begin(struct engine *engine, struct txn *txn)
 {
 	(void)engine;
-	(void)txn;
-	return 0;
-}
-
-static int
-memtx_engine_begin_statement(struct engine *engine, struct txn *txn)
-{
-	(void)engine;
-	(void)txn;
-	if (txn->engine_tx == NULL) {
-		struct space *space = txn_last_stmt(txn)->space;
-
-		if (space->def->id > BOX_SYSTEM_ID_MAX)
-			/* Setup triggers for non-ddl transactions. */
-			memtx_init_txn(txn);
-	}
+	txn_can_yield(txn, false);
 	return 0;
 }
 
@@ -459,9 +372,6 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn,
 static void
 memtx_engine_rollback(struct engine *engine, struct txn *txn)
 {
-	if (txn->engine_tx != NULL) {
-		trigger_clear(&txn->fiber_on_yield);
-	}
 	struct txn_stmt *stmt;
 	stailq_reverse(&txn->stmts);
 	stailq_foreach_entry(stmt, &txn->stmts, next)
@@ -948,8 +858,8 @@ static const struct engine_vtab memtx_engine_vtab = {
 	/* .create_space = */ memtx_engine_create_space,
 	/* .join = */ memtx_engine_join,
 	/* .begin = */ memtx_engine_begin,
-	/* .begin_statement = */ memtx_engine_begin_statement,
-	/* .prepare = */ memtx_engine_prepare,
+	/* .begin_statement = */ generic_engine_begin_statement,
+	/* .prepare = */ generic_engine_prepare,
 	/* .commit = */ generic_engine_commit,
 	/* .rollback_statement = */ memtx_engine_rollback_statement,
 	/* .rollback = */ memtx_engine_rollback,
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index f0e1cfd2..926b3f18 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -878,6 +878,8 @@ memtx_check_on_replace(struct trigger *trigger, void *event)
 static int
 memtx_space_check_format(struct space *space, struct tuple_format *format)
 {
+	struct txn *txn = in_txn();
+
 	if (space->index_count == 0)
 		return 0;
 	struct index *pk = space->index[0];
@@ -888,6 +890,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format)
 	if (it == NULL)
 		return -1;
 
+	txn_can_yield(txn, true);
+
 	struct memtx_engine *memtx = (struct memtx_engine *)space->engine;
 	struct memtx_ddl_state state;
 	state.format = format;
@@ -930,6 +934,7 @@ 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, false);
 	return rc;
 }
 
@@ -1002,6 +1007,7 @@ static int
 memtx_space_build_index(struct space *src_space, struct index *new_index,
 			struct tuple_format *new_format)
 {
+	struct txn *txn = in_txn();
 	/**
 	 * If it's a secondary key, and we're not building them
 	 * yet (i.e. it's snapshot recovery for memtx), do nothing.
@@ -1027,6 +1033,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
 	if (it == NULL)
 		return -1;
 
+	txn_can_yield(txn, true);
+
 	struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine;
 	struct memtx_ddl_state state;
 	state.index = new_index;
@@ -1103,6 +1111,7 @@ 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, false);
 	return rc;
 }
 
diff --git a/src/box/txn.c b/src/box/txn.c
index c605345d..5193b49c 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -40,6 +40,12 @@ double too_long_threshold;
 /* Txn cache. */
 static struct stailq txn_cache = {NULL, &txn_cache.first};
 
+static void
+txn_on_stop(struct trigger *trigger, void *event);
+
+static void
+txn_on_yield(struct trigger *trigger, void *event);
+
 static inline void
 fiber_set_txn(struct fiber *fiber, struct txn *txn)
 {
@@ -193,7 +199,8 @@ txn_begin()
 	txn->n_applier_rows = 0;
 	txn->has_triggers  = false;
 	txn->is_done = false;
-	txn->is_aborted = false;
+	txn->can_yield = true;
+	txn->is_aborted_by_yield = false;
 	txn->in_sub_stmt = 0;
 	txn->id = ++tsn;
 	txn->signature = -1;
@@ -201,8 +208,8 @@ txn_begin()
 	txn->engine_tx = NULL;
 	txn->psql_txn = NULL;
 	txn->fiber = NULL;
-	/* fiber_on_yield/fiber_on_stop initialized by engine on demand */
 	fiber_set_txn(fiber(), txn);
+	/* fiber_on_yield is initialized by engine on demand */
 	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
 	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
 	return txn;
@@ -463,6 +470,12 @@ txn_write_to_wal(struct txn *txn)
 static int
 txn_prepare(struct txn *txn)
 {
+	if (txn->is_aborted_by_yield) {
+		assert(!txn->can_yield);
+		diag_set(ClientError, ER_TRANSACTION_YIELD);
+		diag_log();
+		return -1;
+	}
 	/*
 	 * If transaction has been started in SQL, deferred
 	 * foreign key constraints must not be violated.
@@ -484,6 +497,8 @@ txn_prepare(struct txn *txn)
 			return -1;
 	}
 	trigger_clear(&txn->fiber_on_stop);
+	if (!txn->can_yield)
+		trigger_clear(&txn->fiber_on_yield);
 	return 0;
 }
 
@@ -550,23 +565,13 @@ txn_rollback(struct txn *txn)
 {
 	assert(txn == in_txn());
 	trigger_clear(&txn->fiber_on_stop);
+	if (!txn->can_yield)
+		trigger_clear(&txn->fiber_on_yield);
 	txn->signature = -1;
 	txn_complete(txn);
 	fiber_set_txn(fiber(), NULL);
 }
 
-void
-txn_abort(struct txn *txn)
-{
-	assert(in_txn() == txn);
-	txn_rollback_to_svp(txn, NULL);
-	if (txn->has_triggers) {
-		txn_run_triggers(txn, &txn->on_rollback);
-		txn->has_triggers = false;
-	}
-	txn->is_aborted = true;
-}
-
 int
 txn_check_singlestatement(struct txn *txn, const char *where)
 {
@@ -578,6 +583,20 @@ txn_check_singlestatement(struct txn *txn, const char *where)
 	return 0;
 }
 
+void
+txn_can_yield(struct txn *txn, bool set)
+{
+	assert(txn == in_txn());
+	assert(txn->can_yield != set);
+	txn->can_yield = set;
+	if (set) {
+		trigger_clear(&txn->fiber_on_yield);
+	} else {
+		trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
+		trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
+	}
+}
+
 int64_t
 box_txn_id(void)
 {
@@ -711,7 +730,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
 	return 0;
 }
 
-void
+static void
 txn_on_stop(struct trigger *trigger, void *event)
 {
 	(void) trigger;
@@ -719,3 +738,34 @@ txn_on_stop(struct trigger *trigger, void *event)
 	txn_rollback(in_txn());                 /* doesn't yield or fail */
 }
 
+/**
+ * Memtx yield-in-transaction trigger callback.
+ *
+ * In case of a yield inside memtx multi-statement transaction
+ * we must, first of all, roll back the effects of the transaction
+ * so that concurrent transactions won't see dirty, uncommitted
+ * data.
+ *
+ * Second, we must abort the transaction, since it has been rolled
+ * back implicitly. The transaction can not be rolled back
+ * completely from within a yield trigger, since a yield trigger
+ * can't fail. Instead, we mark the transaction as aborted and
+ * raise an error on attempt to commit it.
+ *
+ * So much hassle to be user-friendly until we have a true
+ * interactive transaction support in memtx.
+ */
+static void
+txn_on_yield(struct trigger *trigger, void *event)
+{
+	(void) trigger;
+	(void) event;
+	struct txn *txn = in_txn();
+	assert(txn != NULL && !txn->can_yield);
+	txn_rollback_to_svp(txn, NULL);
+	if (txn->has_triggers) {
+		txn_run_triggers(txn, &txn->on_rollback);
+		txn->has_triggers = false;
+	}
+	txn->is_aborted_by_yield = true;
+}
diff --git a/src/box/txn.h b/src/box/txn.h
index d1ef220a..a6d6878b 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -168,7 +168,12 @@ struct txn {
 	 * True if the transaction was aborted so should be
 	 * rolled back at commit.
 	 */
-	bool is_aborted;
+	bool is_aborted_by_yield;
+	/**
+	 * True if yields are allowed inside a transaction,
+	 * see txn_can_yield().
+	 */
+	bool can_yield;
 	/** True if on_commit and on_rollback lists are non-empty. */
 	bool has_triggers;
 	/** The number of active nested statement-level transactions. */
@@ -259,17 +264,6 @@ int
 txn_write(struct txn *txn);
 
 /**
- * Roll back the transaction but keep the object around.
- * A special case for memtx transaction abort on yield. In this
- * case we need to abort the transaction to avoid dirty reads but
- * need to keep it around to ensure a new one is not implicitly
- * started and committed by the user program. Later, at
- * transaction commit we will raise an exception.
- */
-void
-txn_abort(struct txn *txn);
-
-/**
  * Most txns don't have triggers, and txn objects
  * are created on every access to data, so txns
  * are partially initialized.
@@ -371,6 +365,21 @@ int
 txn_check_singlestatement(struct txn *txn, const char *where);
 
 /**
+ * Enables or disables fiber yields inside the current transaction
+ * depending on the value of the given flag. Yields are disabled
+ * by installing a fiber-on-yield trigger that marks the transaction
+ * as aborted, which results in rolling back the transaction on
+ * commit.
+ *
+ * This function is used by the memtx engine, because it doesn't
+ * support yields inside transactions. It is also used to temporarily
+ * enable yields for long DDL operations such as building an index
+ * or checking a space format.
+ */
+void
+txn_can_yield(struct txn *txn, bool set);
+
+/**
  * Returns true if the transaction has a single statement.
  * Supposed to be used from a space on_replace trigger to
  * detect transaction boundaries.
@@ -400,12 +409,6 @@ txn_last_stmt(struct txn *txn)
 }
 
 /**
- * Fiber-stop trigger: roll back the transaction.
- */
-void
-txn_on_stop(struct trigger *trigger, void *event);
-
-/**
  * Return VDBE that is being currently executed.
  *
  * @retval VDBE that is being currently executed.
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index e0de65d0..7635c84c 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1093,6 +1093,7 @@ static int
 vinyl_space_check_format(struct space *space, struct tuple_format *format)
 {
 	struct vy_env *env = vy_env(space->engine);
+	struct txn *txn = in_txn();
 
 	/*
 	 * If this is local recovery, the space was checked before
@@ -1121,6 +1122,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
 
+	txn_can_yield(txn, true);
+
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
 	ctx.format = format;
@@ -1168,6 +1171,7 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
+	txn_can_yield(txn, false);
 	return rc;
 }
 
@@ -4314,6 +4318,7 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 {
 	struct vy_env *env = vy_env(src_space->engine);
 	struct vy_lsm *pk = vy_lsm(src_space->index[0]);
+	struct txn *txn = in_txn();
 
 	if (new_index->def->iid == 0 && !vy_lsm_is_empty(pk)) {
 		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
@@ -4345,6 +4350,8 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	bool need_wal_sync;
 	tx_manager_abort_writers_for_ddl(env->xm, src_space, &need_wal_sync);
 
+	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
@@ -4438,6 +4445,7 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 
 	diag_destroy(&ctx.diag);
 	trigger_clear(&on_replace);
+	txn_can_yield(txn, false);
 	return rc;
 }
 
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 857314b7..ad2d650c 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -722,3 +722,19 @@ box.commit() -- error
 s:drop()
 ---
 ...
+--
+-- Check that a DDL transaction is rolled back on fiber yield.
+--
+s = box.schema.space.create('test')
+---
+...
+box.begin() s:create_index('pk') fiber.sleep(0)
+---
+...
+box.commit() -- error
+---
+- error: Transaction has been aborted by a fiber yield
+...
+s:drop()
+---
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 8ffae2fe..8cd3e8ba 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -373,3 +373,11 @@ fiber.sleep(0)
 s.index.pk == nil
 box.commit() -- error
 s:drop()
+
+--
+-- Check that a DDL transaction is rolled back on fiber yield.
+--
+s = box.schema.space.create('test')
+box.begin() s:create_index('pk') fiber.sleep(0)
+box.commit() -- error
+s:drop()
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index 277e7bda..d65a1df1 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -8,7 +8,7 @@ fiber =  require('fiber')
 ---
 ...
 --
--- Check that space truncation is forbidden in a transaction.
+-- Check that space truncation works fine in a transaction.
 --
 s = box.schema.create_space('test', {engine = engine})
 ---
@@ -19,13 +19,7 @@ _ = s:create_index('pk')
 _ = s:insert{123}
 ---
 ...
-box.begin()
----
-...
-s:truncate()
----
-...
-box.commit()
+box.begin() s:truncate() box.commit()
 ---
 ...
 s:select()
diff --git a/test/engine/truncate.test.lua b/test/engine/truncate.test.lua
index 74fdd52b..fe897fb6 100644
--- a/test/engine/truncate.test.lua
+++ b/test/engine/truncate.test.lua
@@ -4,14 +4,12 @@ engine = test_run:get_cfg('engine')
 fiber =  require('fiber')
 
 --
--- Check that space truncation is forbidden in a transaction.
+-- Check that space truncation works fine in a transaction.
 --
 s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
 _ = s:insert{123}
-box.begin()
-s:truncate()
-box.commit()
+box.begin() s:truncate() box.commit()
 s:select()
 s:drop()
 

Re: [PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Wed, Jul 10, 2019 at 11:43:58PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:
> 
> Please write a follow up patch which changes box/lua/schema.lua
> operations to use transactions wherever possible.

Sure. I was actually planning to do that anyway once the patch set has
been committed.

Re: [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/12 17:57]:

This is a very nice patch. Thank you for addressing my comments.
The bit fields is going to be trivial, so go ahead and don't
postpone it - it doesn't need a review.

One more comment below.

> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index e0de65d0..7635c84c 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -1093,6 +1093,7 @@ static int
>  vinyl_space_check_format(struct space *space, struct tuple_format *format)
>  {
>  	struct vy_env *env = vy_env(space->engine);
> +	struct txn *txn = in_txn();
>  
>  	/*
>  	 * If this is local recovery, the space was checked before
> @@ -1121,6 +1122,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
>  	bool need_wal_sync;
>  	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
>  
> +	txn_can_yield(txn, true);

I keep forgetting how this can happen, and then remember it's a
*memtx* transaction.

Please add this comment:

 /*
  * Tarantool doesn't allow 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 either, 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.
  */

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 3/3] ddl: allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:

LGTM.

lgtm


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 2/3] ddl: don't use space_index from AlterSpaceOp::commit,rollback

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/10 16:13]:
> If there are multiple DDL operations in the same transactions, which is
> impossible now, but will be implemented soon, AlterSpaceOp::commit and
> rollback methods must not access space index map. To understand that,
> consider the following example:
> 
>   - on_replace: AlterSpaceOp1 creates index I1 for space S1
>   - on_replace: AlterSpaceOp2 moves index I1 from space S1 to space S2
>   - on_commit:  AlterSpaceOp1 commits creation of index I1
> 
> AlterSpaceOp1 can't lookup I1 in S1 by id, because the index was moved
> from S1 to S2 by AlterSpaceOp2. If AlterSpaceOp1 attempts to look it up,
> it will access a wrong index.
> 
> Fix that by storing pointers to old and new indexes in AlterSpaceOp if
> required.

Please make this comment more clear.

LGTM.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2 0/3] Transactional DDL

[Vladimir Davydov]

On Wed, Jul 10, 2019 at 04:09:26PM +0300, Vladimir Davydov wrote:
> Vladimir Davydov (3):
>   memtx: fix txn_on_yield for DDL transactions
>   ddl: don't use space_index from AlterSpaceOp::commit,rollback
>   ddl: allow to execute non-yielding DDL statements in transactions

Pushed to master after addressing comments by Kostja.