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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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()