[PATCH 0/5] Transactional DDL

[PATCH 0/5] 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

Vladimir Davydov (5):
  vinyl: don't sync WAL on space alter if not necessary
  txn: run on_rollback triggers on txn_abort
  txn: fix execution order of commit triggers
  memtx: fix txn_on_yield for DDL transactions
  Allow to execute non-yielding DDL statements in transactions

 src/box/alter.cc              |  14 ----
 src/box/memtx_engine.c        |  10 +--
 src/box/memtx_space.c         |  10 +++
 src/box/txn.c                 |  33 +++++++-
 src/box/txn.h                 |  44 +++++++----
 src/box/user.cc               |   1 -
 src/box/vinyl.c               |  65 ++++++++++------
 src/box/vy_tx.c               |  11 ++-
 src/box/vy_tx.h               |   7 +-
 src/lib/core/trigger.h        |  11 +++
 test/box/on_replace.result    |  53 ++++++-------
 test/box/on_replace.test.lua  |  13 ++--
 test/box/transaction.result   | 171 ++++++++++++++++++++++++++++++++++++------
 test/box/transaction.test.lua |  92 ++++++++++++++++++++---
 test/engine/truncate.result   |  10 +--
 test/engine/truncate.test.lua |   6 +-
 16 files changed, 406 insertions(+), 145 deletions(-)

-- 
2.11.0

[PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary

[Vladimir Davydov]

Changes done to an altered space while a new index is being built or
the format is being checked are propagated via an on_replace trigger.
The problem is there may be transactions that started before the alter
request. Their working set can't be checked so we simply abort them.
We can't abort transactions that have reached WAL so we also call
wal_sync() to flush all pending WAL requests. This is a yielding
operation and we call it even if there's no transactions that need
to be flushed. As a result, vinyl space alter yields unconditionally,
even if the space is empty and there is no pending transactions
affecting it. This prevents us from implementing transactional DDL.
Let's call wal_sync() only if there's actually at least one pending
transaction affecting the altered space and waiting for WAL.
---
 src/box/vinyl.c | 51 ++++++++++++++++++++++++++++++---------------------
 src/box/vy_tx.c | 11 ++++++++++-
 src/box/vy_tx.h |  7 ++++++-
 3 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 128b1199..e0de65d0 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1052,25 +1052,8 @@ vinyl_space_invalidate(struct space *space)
 	 * soon as it's done reading disk, which will make the DML
 	 * request bail out early, without dereferencing the space.
 	 */
-	tx_manager_abort_writers_for_ddl(env->xm, space);
-}
-
-/**
- * This function is called after installing on_replace trigger
- * used for propagating changes done during DDL. It aborts all
- * rw transactions affecting the given space that began
- * before the trigger was installed so that DDL doesn't miss
- * their working set.
- */
-static void
-vy_abort_writers_for_ddl(struct vy_env *env, struct space *space)
-{
-	tx_manager_abort_writers_for_ddl(env->xm, space);
-	/*
-	 * Wait for prepared transactions to complete
-	 * (we can't abort them as they reached WAL).
-	 */
-	wal_sync();
+	bool unused;
+	tx_manager_abort_writers_for_ddl(env->xm, space, &unused);
 }
 
 /** Argument passed to vy_check_format_on_replace(). */
@@ -1131,6 +1114,13 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	 */
 	struct vy_lsm *pk = vy_lsm(space->index[0]);
 
+	/*
+	 * Transactions started before the space alter request can't
+	 * be checked with on_replace trigger so we abort them.
+	 */
+	bool need_wal_sync;
+	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
+
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
 	ctx.format = format;
@@ -1139,7 +1129,13 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
 	trigger_create(&on_replace, vy_check_format_on_replace, &ctx, NULL);
 	trigger_add(&space->on_replace, &on_replace);
 
-	vy_abort_writers_for_ddl(env, space);
+	/*
+	 * Flush transactions waiting on WAL after installing on_replace
+	 * trigger so that changes made by newer transactions are checked
+	 * by the trigger callback.
+	 */
+	if (need_wal_sync)
+		wal_sync();
 
 	struct vy_read_iterator itr;
 	vy_read_iterator_open(&itr, pk, NULL, ITER_ALL, pk->env->empty_key,
@@ -4343,6 +4339,13 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 		return vy_build_recover(env, new_lsm, pk);
 
 	/*
+	 * Transactions started before the space alter request can't
+	 * be checked with on_replace trigger so we abort them.
+	 */
+	bool need_wal_sync;
+	tx_manager_abort_writers_for_ddl(env->xm, src_space, &need_wal_sync);
+
+	/*
 	 * Iterate over all tuples stored in the space and insert
 	 * each of them into the new LSM tree. Since read iterator
 	 * may yield, we install an on_replace trigger to forward
@@ -4359,7 +4362,13 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index,
 	trigger_create(&on_replace, vy_build_on_replace, &ctx, NULL);
 	trigger_add(&src_space->on_replace, &on_replace);
 
-	vy_abort_writers_for_ddl(env, src_space);
+	/*
+	 * Flush transactions waiting on WAL after installing on_replace
+	 * trigger so that changes made by newer transactions are checked
+	 * by the trigger callback.
+	 */
+	if (need_wal_sync)
+		wal_sync();
 
 	struct vy_read_iterator itr;
 	vy_read_iterator_open(&itr, pk, NULL, ITER_ALL, pk->env->empty_key,
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 56b9f292..ca8c57ad 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -1144,13 +1144,22 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt)
 }
 
 void
-tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space)
+tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space,
+				 bool *need_wal_sync)
 {
+	*need_wal_sync = false;
 	if (space->index_count == 0)
 		return; /* no indexes, no conflicts */
 	struct vy_lsm *lsm = vy_lsm(space->index[0]);
 	struct vy_tx *tx;
 	rlist_foreach_entry(tx, &xm->writers, in_writers) {
+		/*
+		 * We can't abort prepared transactions as they have
+		 * already reached WAL. The caller needs to sync WAL
+		 * to make sure they are gone.
+		 */
+		if (tx->state == VINYL_TX_COMMIT)
+			*need_wal_sync = true;
 		if (tx->state != VINYL_TX_READY)
 			continue;
 		if (tx->last_stmt_space == space ||
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index 9ee10755..376f4330 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -293,9 +293,14 @@ tx_manager_mem_used(struct tx_manager *xm);
  * Abort all rw transactions that affect the given space
  * and haven't reached WAL yet. Called before executing a DDL
  * operation.
+ *
+ * @need_wal_sync is set if at least one transaction can't be
+ * aborted, because it has reached WAL. The caller is supposed
+ * to call wal_sync() to flush them.
  */
 void
-tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space);
+tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct space *space,
+				 bool *need_wal_sync);
 
 /**
  * Abort all local rw transactions that haven't reached WAL yet.
-- 
2.11.0

Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> Changes done to an altered space while a new index is being built or
> the format is being checked are propagated via an on_replace trigger.
> The problem is there may be transactions that started before the alter
> request. Their working set can't be checked so we simply abort them.
> We can't abort transactions that have reached WAL so we also call
> wal_sync() to flush all pending WAL requests. This is a yielding
> operation and we call it even if there's no transactions that need
> to be flushed. As a result, vinyl space alter yields unconditionally,
> even if the space is empty and there is no pending transactions
> affecting it. This prevents us from implementing transactional DDL.
> Let's call wal_sync() only if there's actually at least one pending
> transaction affecting the altered space and waiting for WAL.
> ---
>  src/box/vinyl.c | 51 ++++++++++++++++++++++++++++++---------------------
>  src/box/vy_tx.c | 11 ++++++++++-
>  src/box/vy_tx.h |  7 ++++++-
>  3 files changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index 128b1199..e0de65d0 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -1052,25 +1052,8 @@ vinyl_space_invalidate(struct space *space)
>  	 * soon as it's done reading disk, which will make the DML
>  	 * request bail out early, without dereferencing the space.
>  	 */
> -	tx_manager_abort_writers_for_ddl(env->xm, space);
> -}

Please add a comment for vinyl_space_invalidate, explaining what
it is used for and who's using it, just like in the comment that
you removed:
> -
> -/**
> - * This function is called after installing on_replace trigger
> - * used for propagating changes done during DDL. It aborts all
> - * rw transactions affecting the given space that began
> - * before the trigger was installed so that DDL doesn't miss
> - * their working set.
> - */
> -static void
> -vy_abort_writers_for_ddl(struct vy_env *env, struct space *space)

Otherwise LGTM.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 12:29:05PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > Changes done to an altered space while a new index is being built or
> > the format is being checked are propagated via an on_replace trigger.
> > The problem is there may be transactions that started before the alter
> > request. Their working set can't be checked so we simply abort them.
> > We can't abort transactions that have reached WAL so we also call
> > wal_sync() to flush all pending WAL requests. This is a yielding
> > operation and we call it even if there's no transactions that need
> > to be flushed. As a result, vinyl space alter yields unconditionally,
> > even if the space is empty and there is no pending transactions
> > affecting it. This prevents us from implementing transactional DDL.
> > Let's call wal_sync() only if there's actually at least one pending
> > transaction affecting the altered space and waiting for WAL.
> > ---
> >  src/box/vinyl.c | 51 ++++++++++++++++++++++++++++++---------------------
> >  src/box/vy_tx.c | 11 ++++++++++-
> >  src/box/vy_tx.h |  7 ++++++-
> >  3 files changed, 46 insertions(+), 23 deletions(-)
> > 
> > diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> > index 128b1199..e0de65d0 100644
> > --- a/src/box/vinyl.c
> > +++ b/src/box/vinyl.c
> > @@ -1052,25 +1052,8 @@ vinyl_space_invalidate(struct space *space)
> >  	 * soon as it's done reading disk, which will make the DML
> >  	 * request bail out early, without dereferencing the space.
> >  	 */
> > -	tx_manager_abort_writers_for_ddl(env->xm, space);
> > -}
> 
> Please add a comment for vinyl_space_invalidate, explaining what
> it is used for and who's using it, just like in the comment that
> you removed:

I didn't remove any comments. I just moved a comment from
vy_abort_writers_for_ddl() to vinyl_space_build_index() and
vy_space_check_format().

Regarding vinyl_space_invalidate - I didn't touch it. And it seems to be
pretty well commented as it is, see comments to vinyl_space_invalidate()
and space_vtab.

Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary

[Vladimir Davydov]

Pushed to master.

[PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Vladimir Davydov]

When a memtx transaction is aborted on yield, it isn't enough to
rollback individual statements - we must also run on_rollback triggers,
otherwise changes done to the schema by an aborted DDL transaction will
be visible to other fibers until an attempt to commit it is made.
---
 src/box/txn.c                 |  7 ++++++-
 test/box/transaction.result   | 24 ++++++++++++++++++++++++
 test/box/transaction.test.lua | 10 ++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/src/box/txn.c b/src/box/txn.c
index 818f405b..5d80833a 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -346,6 +346,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
 	 * Some triggers require for in_txn variable to be set so
 	 * restore it for the time triggers are in progress.
 	 */
+	struct txn *old_txn = in_txn();
 	fiber_set_txn(fiber(), txn);
 	/* Rollback triggers must not throw. */
 	if (trigger_run(trigger, txn) != 0) {
@@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
 		unreachable();
 		panic("commit/rollback trigger failed");
 	}
-	fiber_set_txn(fiber(), NULL);
+	fiber_set_txn(fiber(), old_txn);
 }
 
 /**
@@ -556,6 +557,10 @@ void
 txn_abort(struct 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;
 }
 
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 9da53e5b..857314b7 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -698,3 +698,27 @@ box.space.memtx:drop()
 box.space.vinyl:drop()
 ---
 ...
+--
+-- Check that changes done to the schema by a DDL statement are
+-- rolled back when the transaction is aborted on fiber yield.
+--
+s = box.schema.space.create('test')
+---
+...
+box.begin() s:create_index('pk') s:insert{1}
+---
+...
+fiber.sleep(0)
+---
+...
+s.index.pk == nil
+---
+- true
+...
+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 a6789316..8ffae2fe 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -363,3 +363,13 @@ if box.space.test then box.space.test:drop() end
 box.space.memtx:drop()
 box.space.vinyl:drop()
 
+--
+-- Check that changes done to the schema by a DDL statement are
+-- rolled back when the transaction is aborted on fiber yield.
+--
+s = box.schema.space.create('test')
+box.begin() s:create_index('pk') s:insert{1}
+fiber.sleep(0)
+s.index.pk == nil
+box.commit() -- error
+s:drop()
-- 
2.11.0

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> When a memtx transaction is aborted on yield, it isn't enough to
> rollback individual statements - we must also run on_rollback triggers,
> otherwise changes done to the schema by an aborted DDL transaction will
> be visible to other fibers until an attempt to commit it is made.
> ---
>  src/box/txn.c                 |  7 ++++++-
>  test/box/transaction.result   | 24 ++++++++++++++++++++++++
>  test/box/transaction.test.lua | 10 ++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 818f405b..5d80833a 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -346,6 +346,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
>  	 * Some triggers require for in_txn variable to be set so
>  	 * restore it for the time triggers are in progress.
>  	 */
> +	struct txn *old_txn = in_txn();

This manipulation should be in txn_abort(), not in
txn_run_triggers(). It's txn_abort().
>  	fiber_set_txn(fiber(), txn);
>  	/* Rollback triggers must not throw. */
>  	if (trigger_run(trigger, txn) != 0) {
> @@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
>  		unreachable();
>  		panic("commit/rollback trigger failed");
>  	}
> -	fiber_set_txn(fiber(), NULL);
> +	fiber_set_txn(fiber(), old_txn);

Ideally we should never need to restore old_txn. All transaction
statements, like txn_begin() or txn_abort() should set the txn, 
and whenever the transaction yields, the txn should be cleared.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 12:32:01PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > When a memtx transaction is aborted on yield, it isn't enough to
> > rollback individual statements - we must also run on_rollback triggers,
> > otherwise changes done to the schema by an aborted DDL transaction will
> > be visible to other fibers until an attempt to commit it is made.
> > ---
> >  src/box/txn.c                 |  7 ++++++-
> >  test/box/transaction.result   | 24 ++++++++++++++++++++++++
> >  test/box/transaction.test.lua | 10 ++++++++++
> >  3 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/box/txn.c b/src/box/txn.c
> > index 818f405b..5d80833a 100644
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -346,6 +346,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
> >  	 * Some triggers require for in_txn variable to be set so
> >  	 * restore it for the time triggers are in progress.
> >  	 */
> > +	struct txn *old_txn = in_txn();
> 
> This manipulation should be in txn_abort(), not in
> txn_run_triggers(). It's txn_abort().

But this function is also called from completion callback, where it has
to set the txn as well. That's why I put it there, otherwise we would
have to set/restore txn context in txn_complete as well. I'm not really
against it - just pointint it out. I'll prepare a patch that does that,
see how it looks.

> >  	fiber_set_txn(fiber(), txn);
> >  	/* Rollback triggers must not throw. */
> >  	if (trigger_run(trigger, txn) != 0) {
> > @@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
> >  		unreachable();
> >  		panic("commit/rollback trigger failed");
> >  	}
> > -	fiber_set_txn(fiber(), NULL);
> > +	fiber_set_txn(fiber(), old_txn);
> 
> Ideally we should never need to restore old_txn. All transaction
> statements, like txn_begin() or txn_abort() should set the txn, 
> and whenever the transaction yields, the txn should be cleared.

But we do want the transaction to remain attached to the fiber once
it resumes its execution so that we can raise an error on 'commit'.
Actually, we used to clear txn on yield, but then it was reworked
to make 'commit' more user-friendly.

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 13:02]:
> > This manipulation should be in txn_abort(), not in
> > txn_run_triggers(). It's txn_abort().
> 
> But this function is also called from completion callback, where it has
> to set the txn as well. That's why I put it there, otherwise we would
> have to set/restore txn context in txn_complete as well. I'm not really
> against it - just pointint it out. I'll prepare a patch that does that,
> see how it looks.

Yes, so, basically, txn_begin(), txn_abort(), txn_complete()
should manage the fiber key.

> 
> > >  	fiber_set_txn(fiber(), txn);
> > >  	/* Rollback triggers must not throw. */
> > >  	if (trigger_run(trigger, txn) != 0) {
> > > @@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
> > >  		unreachable();
> > >  		panic("commit/rollback trigger failed");
> > >  	}
> > > -	fiber_set_txn(fiber(), NULL);
> > > +	fiber_set_txn(fiber(), old_txn);
> > 
> > Ideally we should never need to restore old_txn. All transaction
> > statements, like txn_begin() or txn_abort() should set the txn, 
> > and whenever the transaction yields, the txn should be cleared.
> 
> But we do want the transaction to remain attached to the fiber once
> it resumes its execution so that we can raise an error on 'commit'.
> Actually, we used to clear txn on yield, but then it was reworked
> to make 'commit' more user-friendly.

I don't get it. When a transaction is resumed after a yield, it
has to set its key again anyway. Basically, transaction == fiber
is no longer true, and each time a transaction starts running it
should update its key. Better yet, let's kill the transaction key
in the fiber altogether and pass the txn around by value.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 03:14:08PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 13:02]:
> > > This manipulation should be in txn_abort(), not in
> > > txn_run_triggers(). It's txn_abort().
> > 
> > But this function is also called from completion callback, where it has
> > to set the txn as well. That's why I put it there, otherwise we would
> > have to set/restore txn context in txn_complete as well. I'm not really
> > against it - just pointint it out. I'll prepare a patch that does that,
> > see how it looks.
> 
> Yes, so, basically, txn_begin(), txn_abort(), txn_complete()
> should manage the fiber key.

Okay, please take a look at the patch below.

> 
> > 
> > > >  	fiber_set_txn(fiber(), txn);
> > > >  	/* Rollback triggers must not throw. */
> > > >  	if (trigger_run(trigger, txn) != 0) {
> > > > @@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
> > > >  		unreachable();
> > > >  		panic("commit/rollback trigger failed");
> > > >  	}
> > > > -	fiber_set_txn(fiber(), NULL);
> > > > +	fiber_set_txn(fiber(), old_txn);
> > > 
> > > Ideally we should never need to restore old_txn. All transaction
> > > statements, like txn_begin() or txn_abort() should set the txn, 
> > > and whenever the transaction yields, the txn should be cleared.
> > 
> > But we do want the transaction to remain attached to the fiber once
> > it resumes its execution so that we can raise an error on 'commit'.
> > Actually, we used to clear txn on yield, but then it was reworked
> > to make 'commit' more user-friendly.
> 
> I don't get it. When a transaction is resumed after a yield, it
> has to set its key again anyway.

It's not a transaction that is resumed, it's fiber. So we do need to
know which transaction is assigned to a fiber.

> Basically, transaction == fiber
> is no longer true, and each time a transaction starts running it
> should update its key. Better yet, let's kill the transaction key
> in the fiber altogether and pass the txn around by value.

We can't kill the transaction key, because when transactions are used
from Lua it's the only way to get txn.

---
From 21f26c3437b3733ebd3acfb643886fce755db263 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 5 Jul 2019 17:08:45 +0300
Subject: [PATCH] txn: run on_rollback triggers on txn_abort

When a memtx transaction is aborted on yield, it isn't enough to
rollback individual statements - we must also run on_rollback triggers,
otherwise changes done to the schema by an aborted DDL transaction will
be visible to other fibers until an attempt to commit it is made.

diff --git a/src/box/txn.c b/src/box/txn.c
index 818f405b..c605345d 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -342,11 +342,6 @@ fail:
 static inline void
 txn_run_triggers(struct txn *txn, struct rlist *trigger)
 {
-	/*
-	 * Some triggers require for in_txn variable to be set so
-	 * restore it for the time triggers are in progress.
-	 */
-	fiber_set_txn(fiber(), txn);
 	/* Rollback triggers must not throw. */
 	if (trigger_run(trigger, txn) != 0) {
 		/*
@@ -357,7 +352,6 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger)
 		unreachable();
 		panic("commit/rollback trigger failed");
 	}
-	fiber_set_txn(fiber(), NULL);
 }
 
 /**
@@ -412,7 +406,15 @@ txn_entry_done_cb(struct journal_entry *entry, void *data)
 {
 	struct txn *txn = data;
 	txn->signature = entry->res;
+	/*
+	 * Some commit/rollback triggers require for in_txn fiber
+	 * variable to be set so restore it for the time triggers
+	 * are in progress.
+	 */
+	assert(in_txn() == NULL);
+	fiber_set_txn(fiber(), txn);
 	txn_complete(txn);
+	fiber_set_txn(fiber(), NULL);
 }
 
 static int64_t
@@ -497,14 +499,15 @@ txn_write(struct txn *txn)
 	 * After this point the transaction must not be used
 	 * so reset the corresponding key in the fiber storage.
 	 */
-	fiber_set_txn(fiber(), NULL);
 	txn->start_tm = ev_monotonic_now(loop());
 	if (txn->n_new_rows + txn->n_applier_rows == 0) {
 		/* Nothing to do. */
 		txn->signature = 0;
 		txn_complete(txn);
+		fiber_set_txn(fiber(), NULL);
 		return 0;
 	}
+	fiber_set_txn(fiber(), NULL);
 	return txn_write_to_wal(txn);
 }
 
@@ -555,7 +558,12 @@ txn_rollback(struct txn *txn)
 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;
 }
 
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 9da53e5b..857314b7 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -698,3 +698,27 @@ box.space.memtx:drop()
 box.space.vinyl:drop()
 ---
 ...
+--
+-- Check that changes done to the schema by a DDL statement are
+-- rolled back when the transaction is aborted on fiber yield.
+--
+s = box.schema.space.create('test')
+---
+...
+box.begin() s:create_index('pk') s:insert{1}
+---
+...
+fiber.sleep(0)
+---
+...
+s.index.pk == nil
+---
+- true
+...
+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 a6789316..8ffae2fe 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -363,3 +363,13 @@ if box.space.test then box.space.test:drop() end
 box.space.memtx:drop()
 box.space.vinyl:drop()
 
+--
+-- Check that changes done to the schema by a DDL statement are
+-- rolled back when the transaction is aborted on fiber yield.
+--
+s = box.schema.space.create('test')
+box.begin() s:create_index('pk') s:insert{1}
+fiber.sleep(0)
+s.index.pk == nil
+box.commit() -- error
+s:drop()

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> On Mon, Jul 08, 2019 at 03:14:08PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 13:02]:
> > > > This manipulation should be in txn_abort(), not in
> > > > txn_run_triggers(). It's txn_abort().
> > > 
> > > But this function is also called from completion callback, where it has
> > > to set the txn as well. That's why I put it there, otherwise we would
> > > have to set/restore txn context in txn_complete as well. I'm not really
> > > against it - just pointint it out. I'll prepare a patch that does that,
> > > see how it looks.
> > 
> > Yes, so, basically, txn_begin(), txn_abort(), txn_complete()
> > should manage the fiber key.
> 
> Okay, please take a look at the patch below.

Much safer. Thanks. LGTM.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Vladimir Davydov]

On Tue, Jul 09, 2019 at 12:56:18AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> > On Mon, Jul 08, 2019 at 03:14:08PM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 13:02]:
> > > > > This manipulation should be in txn_abort(), not in
> > > > > txn_run_triggers(). It's txn_abort().
> > > > 
> > > > But this function is also called from completion callback, where it has
> > > > to set the txn as well. That's why I put it there, otherwise we would
> > > > have to set/restore txn context in txn_complete as well. I'm not really
> > > > against it - just pointint it out. I'll prepare a patch that does that,
> > > > see how it looks.
> > > 
> > > Yes, so, basically, txn_begin(), txn_abort(), txn_complete()
> > > should manage the fiber key.
> > 
> > Okay, please take a look at the patch below.
> 
> Much safer. Thanks. LGTM.

Pushed to master.

[PATCH 3/5] txn: fix execution order of commit triggers

[Vladimir Davydov]

Both commit and rollback triggers are currently added to the list head.
As a result, they are both run in the reverse order. This is correct for
rollback triggers, because this matches the order in which statements
that added the triggers are rolled back, but this is wrong for commit
triggers. For example, suppose we create a space and then create an
index for it in the same transaction. We expect that on success we first
run the trigger that commits the space and only then the trigger that
commits the index, not vice versa. That said, reverse the order of
commit triggers in the scope of preparations for transactional DDL.
---
 src/box/txn.h          | 13 +++++++++++--
 src/lib/core/trigger.h | 11 +++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/box/txn.h b/src/box/txn.h
index a19becce..d1ef220a 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -198,7 +198,16 @@ struct txn {
 	 * in case a fiber stops (all engines).
 	 */
 	struct trigger fiber_on_stop;
-	 /** Commit and rollback triggers */
+	/**
+	 * Commit and rollback triggers.
+	 *
+	 * Note, we commit triggers are added to the tail of
+	 * the list while rollback triggers are added to the
+	 * head, see txn_on_commit() and txn_on_rollback().
+	 * This ensures that the triggers are fired in the
+	 * same order as statements that added them, both on
+	 * commit and on rollback.
+	 */
 	struct rlist on_commit, on_rollback;
 	struct sql_txn *psql_txn;
 };
@@ -279,7 +288,7 @@ static inline void
 txn_on_commit(struct txn *txn, struct trigger *trigger)
 {
 	txn_init_triggers(txn);
-	trigger_add(&txn->on_commit, trigger);
+	trigger_add_tail(&txn->on_commit, trigger);
 }
 
 static inline void
diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
index d3ffb9a8..4bc4da1a 100644
--- a/src/lib/core/trigger.h
+++ b/src/lib/core/trigger.h
@@ -84,6 +84,17 @@ trigger_add(struct rlist *list, struct trigger *trigger)
 	rlist_add_entry(list, trigger, link);
 }
 
+/**
+ * Same as trigger_add(), but adds a trigger to the tail of the list
+ * rather than to the head. Use it when you need to ensure a certain
+ * execution order among triggers.
+ */
+static inline void
+trigger_add_tail(struct rlist *list, struct trigger *trigger)
+{
+	rlist_add_tail_entry(list, trigger, link);
+}
+
 static inline void
 trigger_add_unique(struct rlist *list, struct trigger *trigger)
 {
-- 
2.11.0

Re: [PATCH 3/5] txn: fix execution order of commit triggers

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> Both commit and rollback triggers are currently added to the list head.
> As a result, they are both run in the reverse order. This is correct for
> rollback triggers, because this matches the order in which statements
> that added the triggers are rolled back, but this is wrong for commit
> triggers. For example, suppose we create a space and then create an
> index for it in the same transaction. We expect that on success we first
> run the trigger that commits the space and only then the trigger that
> commits the index, not vice versa. That said, reverse the order of
> commit triggers in the scope of preparations for transactional DDL.
> ---

lgtm.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 3/5] txn: fix execution order of commit triggers

[Vladimir Davydov]

Pushed to master.

[PATCH 4/5] 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.
---
 src/box/memtx_engine.c        | 10 ++--------
 src/box/memtx_space.c         |  6 ++++++
 src/box/txn.c                 | 18 ++++++++++++++++++
 src/box/txn.h                 | 18 ++++++++++++++++++
 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, 78 insertions(+), 20 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index c8376110..79cd26ad 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -403,14 +403,8 @@ 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);
-	}
+	if (txn->engine_tx == NULL)
+		memtx_init_txn(txn);
 	return 0;
 }
 
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 5d80833a..a70e50cc 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -575,6 +575,24 @@ txn_check_singlestatement(struct txn *txn, const char *where)
 	return 0;
 }
 
+void
+txn_enable_yield_for_ddl(void)
+{
+	struct txn *txn = in_txn();
+	/* See memtx_init_txn(). */
+	assert(txn != NULL && txn->engine_tx == txn);
+	trigger_clear(&txn->fiber_on_yield);
+}
+
+void
+txn_disable_yield_after_ddl(void)
+{
+	struct txn *txn = in_txn();
+	/* See memtx_init_txn(). */
+	assert(txn != NULL && txn->engine_tx == txn);
+	trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
+}
+
 int64_t
 box_txn_id(void)
 {
diff --git a/src/box/txn.h b/src/box/txn.h
index d1ef220a..5cbddc6f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -371,6 +371,24 @@ int
 txn_check_singlestatement(struct txn *txn, const char *where);
 
 /**
+ * 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.
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

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> 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.

Nit: I think since we set up triggers in memtx_* methods, we
should clear them inside memtx_* subsystem as well.

so it's not txn_*, it's memtx_{enable|disable}_yields

another way to do it is to clear fiber->txn key whenever
check_format /build_index functions yield. Basically these
functions run in the background, in a transaction that is
temporarily detached from the main fiber so should not pollute the
caller fiber key.

this is perhaps an over engineering at this stage, but I still
would want you to consider this.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 03:22:48PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > 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.
> 
> Nit: I think since we set up triggers in memtx_* methods, we
> should clear them inside memtx_* subsystem as well.
> 
> so it's not txn_*, it's memtx_{enable|disable}_yields

Calling a memtx_* method from vinyl would look rather weird IMO.

> 
> another way to do it is to clear fiber->txn key whenever
> check_format /build_index functions yield. Basically these
> functions run in the background, in a transaction that is
> temporarily detached from the main fiber so should not pollute the
> caller fiber key.

I don't get it. Need to discuss f2f.

> 
> this is perhaps an over engineering at this stage, but I still
> would want you to consider this.

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 07:41:41PM +0300, Vladimir Davydov wrote:
> On Mon, Jul 08, 2019 at 03:22:48PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > > 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.
> > 
> > Nit: I think since we set up triggers in memtx_* methods, we
> > should clear them inside memtx_* subsystem as well.
> > 
> > so it's not txn_*, it's memtx_{enable|disable}_yields
> 
> Calling a memtx_* method from vinyl would look rather weird IMO.

May be, instead we should move memtx stuff dealing with yields to
txn? Something like, txn_disable_yield() that would disable yields
altogether. Memtx would call this function while vinyl wouldn't.
Then everything related to yields in transactions would live in
txn.c. What do you think?

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 07:58:10PM +0300, Vladimir Davydov wrote:
> On Mon, Jul 08, 2019 at 07:41:41PM +0300, Vladimir Davydov wrote:
> > On Mon, Jul 08, 2019 at 03:22:48PM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > > > 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.
> > > 
> > > Nit: I think since we set up triggers in memtx_* methods, we
> > > should clear them inside memtx_* subsystem as well.
> > > 
> > > so it's not txn_*, it's memtx_{enable|disable}_yields
> > 
> > Calling a memtx_* method from vinyl would look rather weird IMO.
> 
> May be, instead we should move memtx stuff dealing with yields to
> txn? Something like, txn_disable_yield() that would disable yields
> altogether. Memtx would call this function while vinyl wouldn't.
> Then everything related to yields in transactions would live in
> txn.c. What do you think?

The patch below illustrates what I mean. IMHO it looks much better
than the original one.
---

From 67e36c5a99065a17c391716972039f7e8fbb7691 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..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()
 

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> > another way to do it is to clear fiber->txn key whenever
> > check_format /build_index functions yield. Basically these
> > functions run in the background, in a transaction that is
> > temporarily detached from the main fiber so should not pollute the
> > caller fiber key.
> 
> I don't get it. Need to discuss f2f.

I don't get why you need to call this from vinyl.

If both engines use the api, it should be txn_* api of course. but
I don't get why both engines need to use it, vinyl doesn't set
yield triggers to my knowledge.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov]

On Tue, Jul 09, 2019 at 12:57:50AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> > > another way to do it is to clear fiber->txn key whenever
> > > check_format /build_index functions yield. Basically these
> > > functions run in the background, in a transaction that is
> > > temporarily detached from the main fiber so should not pollute the
> > > caller fiber key.
> > 
> > I don't get it. Need to discuss f2f.
> 
> I don't get why you need to call this from vinyl.
> 
> If both engines use the api, it should be txn_* api of course. but
> I don't get why both engines need to use it, vinyl doesn't set
> yield triggers to my knowledge.

DDL is a memtx operation, but actual index creation is done in
vinyl_space_build_index.

[PATCH 5/5] 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 one thing that must be noted explicitly - it's 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.

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/memtx_space.c         |   8 ++-
 src/box/txn.c                 |  24 ++++----
 src/box/txn.h                 |  23 ++------
 src/box/user.cc               |   1 -
 src/box/vinyl.c               |  12 +++-
 test/box/on_replace.result    |  53 ++++++++---------
 test/box/on_replace.test.lua  |  13 ++---
 test/box/transaction.result   | 131 +++++++++++++++++++++++++++++++++++-------
 test/box/transaction.test.lua |  74 ++++++++++++++++++++----
 10 files changed, 237 insertions(+), 116 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 80fd50d1..1fbffbae 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1798,7 +1798,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;
@@ -2095,7 +2094,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;
@@ -2288,7 +2286,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) {
@@ -2518,7 +2515,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;
 
@@ -2669,7 +2665,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;
@@ -2863,7 +2858,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 =
@@ -3161,7 +3155,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;
@@ -3210,7 +3203,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;
@@ -3288,7 +3280,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;
@@ -3441,7 +3432,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;
@@ -3644,7 +3634,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;
 
@@ -3789,7 +3778,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;
@@ -4172,7 +4160,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;
@@ -4463,7 +4450,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/memtx_space.c b/src/box/memtx_space.c
index 921dbcbf..14d440cf 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() != 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() != 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 a70e50cc..3f55e0fc 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -565,23 +565,25 @@ txn_abort(struct txn *txn)
 }
 
 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_enable_yield_for_ddl(void)
 {
 	struct txn *txn = in_txn();
 	/* See memtx_init_txn(). */
 	assert(txn != NULL && txn->engine_tx == txn);
+	/*
+	 * 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_UNSUPPORTED, "DDL",
+			 "yielding statements in transactions");
+		return -1;
+	}
 	trigger_clear(&txn->fiber_on_yield);
+	return 0;
 }
 
 void
diff --git a/src/box/txn.h b/src/box/txn.h
index 5cbddc6f..6f4ae005 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -364,13 +364,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);
-
-/**
  * 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
@@ -380,8 +373,12 @@ txn_check_singlestatement(struct txn *txn, const char *where);
  * 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.
  */
-void
+int
 txn_enable_yield_for_ddl(void);
 
 /** See txn_enable_yield_for_ddl(). */
@@ -527,16 +524,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..4e69d964 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1121,7 +1121,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() != 0)
+		return -1;
 
 	struct trigger on_replace;
 	struct vy_check_format_ctx ctx;
@@ -4348,7 +4352,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() != 0)
+		return -1;
 
 	/*
 	 * Iterate over all tuples stored in the space and insert
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..96c75f98 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,109 @@ 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()
+---
+...
+--
+-- Only the first statement in a transaction is allowed to be
+-- a yielding DDL statement (index build, format check).
+--
+s = box.schema.space.create('test')
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:insert{1, 1}
+---
+- [1, 1]
+...
+box.begin() s:create_index('sk', {parts = {2, 'unsigned'}}) s:insert{2, 2} box.commit()
+---
+...
+s.index.sk:drop()
+---
+...
+box.begin() s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) s:insert{3, 3} box.commit()
+---
+...
+s:format({})
+---
+...
+box.begin() s:insert{4, 4} s:create_index('sk', {parts = {2, 'unsigned'}}) -- error
+---
+- error: DDL does not support yielding statements in transactions
+...
+box.rollback()
+---
+...
+box.begin() s:insert{5, 5} s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) -- error
+---
+- error: DDL does not support yielding statements in transactions
+...
+box.rollback()
+---
+...
+s:select()
+---
+- - [1, 1]
+  - [2, 2]
+  - [3, 3]
+...
+s:drop()
+---
+...
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index 8cd3e8ba..9b360aed 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,62 @@ 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()
+
+--
+-- Only the first statement in a transaction is allowed to be
+-- a yielding DDL statement (index build, format check).
+--
+s = box.schema.space.create('test')
+_ = s:create_index('pk')
+s:insert{1, 1}
+box.begin() s:create_index('sk', {parts = {2, 'unsigned'}}) s:insert{2, 2} box.commit()
+s.index.sk:drop()
+box.begin() s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) s:insert{3, 3} box.commit()
+s:format({})
+box.begin() s:insert{4, 4} s:create_index('sk', {parts = {2, 'unsigned'}}) -- error
+box.rollback()
+box.begin() s:insert{5, 5} s:format({{'a', 'unsigned'}, {'b', 'unsigned'}}) -- error
+box.rollback()
+s:select()
+s:drop()
-- 
2.11.0

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:

Please work on a test case which not only ensures the statements
are allowed, but also produce desired results - as well as leave
no traces/partial effects on rollback.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Sat, Jul 06, 2019 at 01:56:35AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> 
> Please work on a test case which not only ensures the statements
> are allowed, but also produce desired results - as well as leave
> no traces/partial effects on rollback.

As a matter of fact, this simple test does check that rollback doesn't
leave any traces - if it did, an attempt to commit the same statements
would fail:

  box.begin() create() box.rollback() -- suppose it leaved some traces
  box.begin() create() box.commit() -- then this would fail

It also checks that the DDL operations produce the desired results,
otherwise an attempt to drop created objects would fail:

  box.begin() create() box.commit() -- suppose it didn't create some objects
  box.begin() drop() box.commit() -- then this would fail

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 11:14]:
> On Sat, Jul 06, 2019 at 01:56:35AM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > 
> > Please work on a test case which not only ensures the statements
> > are allowed, but also produce desired results - as well as leave
> > no traces/partial effects on rollback.
> 
> As a matter of fact, this simple test does check that rollback doesn't
> leave any traces - if it did, an attempt to commit the same statements
> would fail:
> 
>   box.begin() create() box.rollback() -- suppose it leaved some traces
>   box.begin() create() box.commit() -- then this would fail
> 
> It also checks that the DDL operations produce the desired results,
> otherwise an attempt to drop created objects would fail:
> 
>   box.begin() create() box.commit() -- suppose it didn't create some objects
>   box.begin() drop() box.commit() -- then this would fail

In any case you test a fixed number of combinations, and should
come up with a randomized test. This feature encompasses all DDL
operators, including:

- roles
- users
- functions
- SQL triggers
- SQL foreign key constraints
- SQL views
- SQL check constraints

Every object should be involved in the test, and the generated
transactions should be random.

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 11:21:16AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 11:14]:
> > On Sat, Jul 06, 2019 at 01:56:35AM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > > 
> > > Please work on a test case which not only ensures the statements
> > > are allowed, but also produce desired results - as well as leave
> > > no traces/partial effects on rollback.
> > 
> > As a matter of fact, this simple test does check that rollback doesn't
> > leave any traces - if it did, an attempt to commit the same statements
> > would fail:
> > 
> >   box.begin() create() box.rollback() -- suppose it leaved some traces
> >   box.begin() create() box.commit() -- then this would fail
> > 
> > It also checks that the DDL operations produce the desired results,
> > otherwise an attempt to drop created objects would fail:
> > 
> >   box.begin() create() box.commit() -- suppose it didn't create some objects
> >   box.begin() drop() box.commit() -- then this would fail
> 
> In any case you test a fixed number of combinations, and should
> come up with a randomized test. This feature encompasses all DDL
> operators, including:
> 
> - roles
> - users
> - functions
> - SQL triggers
> - SQL foreign key constraints
> - SQL views
> - SQL check constraints
> 
> Every object should be involved in the test, and the generated
> transactions should be random.

Sounds tricky :-/ May be, hand this over to Q&A?

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 11:45]:
> > In any case you test a fixed number of combinations, and should
> > come up with a randomized test. This feature encompasses all DDL
> > operators, including:
> > 
> > - roles
> > - users
> > - functions
> > - SQL triggers
> > - SQL foreign key constraints
> > - SQL views
> > - SQL check constraints
> > 
> > Every object should be involved in the test, and the generated
> > transactions should be random.
> 
> Sounds tricky :-/ May be, hand this over to Q&A?

Nobody is going to write the test better than you. You may even
hand this over to mermaids or some other mythical creature, if you
don't want this to happen.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 12:25:06PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 11:45]:
> > > In any case you test a fixed number of combinations, and should
> > > come up with a randomized test. This feature encompasses all DDL
> > > operators, including:
> > > 
> > > - roles
> > > - users
> > > - functions
> > > - SQL triggers
> > > - SQL foreign key constraints
> > > - SQL views
> > > - SQL check constraints
> > > 
> > > Every object should be involved in the test, and the generated
> > > transactions should be random.
> > 
> > Sounds tricky :-/ May be, hand this over to Q&A?
> 
> Nobody is going to write the test better than you. You may even
> hand this over to mermaids or some other mythical creature, if you
> don't want this to happen.

Mermaids are cool ;-)

The problem is such a test is really difficult to come up with, because
we'd have to implement something like a dependency graph describing
which operations can be run after which. IMHO it isn't worth doing just
for the sake of a functional test.

Regarding SQL, the SQL team already has tests dealing with SQL related
objects (like constraints or triggers). Once they introduce support of
transactional DDL, we will get those tests for free.

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> The problem is such a test is really difficult to come up with, because
> we'd have to implement something like a dependency graph describing
> which operations can be run after which. IMHO it isn't worth doing just
> for the sake of a functional test.

I volunteer to help :), and it is very much worth doing according
to my experience. 


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> +box.begin() s:insert{4, 4} s:create_index('sk', {parts = {2, 'unsigned'}}) -- error
> +---
> +- error: DDL does not support yielding statements in transactions

this message is rather obscure. Let's try to explain it in more
plain English. "Even if your engine allows a yield in transaction,
yielding in a transaction that has a DDL statement in it is not
uspported unless it's the first statement".

Is it what you wanted to say?


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 03:26:26PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > +box.begin() s:insert{4, 4} s:create_index('sk', {parts = {2, 'unsigned'}}) -- error
> > +---
> > +- error: DDL does not support yielding statements in transactions
> 
> this message is rather obscure. Let's try to explain it in more
> plain English. "Even if your engine allows a yield in transaction,
> yielding in a transaction that has a DDL statement in it is not
> uspported unless it's the first statement".
> 
> Is it what you wanted to say?

Basically, yes, but I'd prefer to keep the error message as brief as
possible. Besides we can't put such a text in ER_UNSUPPORTED error which
has the following template: "%s does not support %s". We can, of course,
introduce a separate error code, but I failed to come up with a good
name for it :-/

What about

  DDL does not support long operations, such as building an index or checking a space format, in a transaction

?

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> What about
> 
>   DDL does not support long operations, such as building an index or checking a space format, in a transaction

It's not DDL, it's Tarantool :)

I think a separate error code is OK, a clear message has more
value than sticking to the same error code.

Can not perform %s in a multi-statement transaction. 

Is there a workaround? If yes, let's add it to the message.

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Tue, Jul 09, 2019 at 01:02:27AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> > What about
> > 
> >   DDL does not support long operations, such as building an index or checking a space format, in a transaction
> 
> It's not DDL, it's Tarantool :)

Well, all error codes are about Tarantool. However, this particular one
is about limitations of our data dictionarly language (DDL).

> 
> I think a separate error code is OK, a clear message has more
> value than sticking to the same error code.
> 
> Can not perform %s in a multi-statement transaction. 

Okay. Sounds good to me.

> 
> Is there a workaround? If yes, let's add it to the message.

No, there's no workaround. The only way to run index build or space
format check in a transaction is to execute it first, i.e. in fact in
a single-statement transaction.

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Tue, Jul 09, 2019 at 11:11:50AM +0300, Vladimir Davydov wrote:
> On Tue, Jul 09, 2019 at 01:02:27AM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 22:14]:
> > > What about
> > > 
> > >   DDL does not support long operations, such as building an index or checking a space format, in a transaction
> > 
> > It's not DDL, it's Tarantool :)
> 
> Well, all error codes are about Tarantool. However, this particular one
> is about limitations of our data dictionarly language (DDL).
> 
> > 
> > I think a separate error code is OK, a clear message has more
> > value than sticking to the same error code.
> > 
> > Can not perform %s in a multi-statement transaction. 
> 
> Okay. Sounds good to me.
> 
> > 
> > Is there a workaround? If yes, let's add it to the message.
> 
> No, there's no workaround. The only way to run index build or space
> format check in a transaction is to execute it first, i.e. in fact in
> a single-statement transaction.

Here's the patch with the fixed error message. Please take a look once
time permits. Both remaining patches are available on the branch:

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

I can resend them in a separate thread if you wish.
---

From 19e11c407c95499a757c247903825d8265fec355 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 5 Jul 2019 20:13:16 +0300
Subject: [PATCH] Allow to execute non-yielding DDL statements in transactions

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 one thing that must be noted explicitly - it's 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.

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.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index ce0cf2d9..2054ff6d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1828,7 +1828,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;
@@ -2125,7 +2124,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;
@@ -2318,7 +2316,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) {
@@ -2548,7 +2545,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;
 
@@ -2699,7 +2695,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;
@@ -2893,7 +2888,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 =
@@ -3191,7 +3185,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;
@@ -3240,7 +3233,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;
@@ -3318,7 +3310,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;
@@ -3471,7 +3462,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;
@@ -3674,7 +3664,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;
 
@@ -3819,7 +3808,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;
@@ -4202,7 +4190,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;
@@ -4493,7 +4480,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..dadaa230 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1121,7 +1121,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 +4352,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();
 
 --

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> 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 one thing that must be noted explicitly - it's 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.
> 
> Closes #4083

also I would collapse this patch with the previous one, it doesn't
seem the previous one is worth it and fully clear without this
patch.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 5/5] Allow to execute non-yielding DDL statements in transactions

[Vladimir Davydov]

On Mon, Jul 08, 2019 at 03:31:25PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > 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 one thing that must be noted explicitly - it's 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.
> > 
> > Closes #4083
> 
> also I would collapse this patch with the previous one, it doesn't
> seem the previous one is worth it and fully clear without this
> patch.

I beg to disagree. The previous patch fixes certain bugs in the code
disabling yields in memtx transactions. This is covered by the new test.
Better keep it separated in case we want to cherry-pick it.