[PATCH 2/2] vinyl: fail aborted transactions early

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

We abort transactions when switching to read-only mode, building a new
index, or reverting a statement after a failed WAL write. It's no use
to allow an aborted transaction to proceed as usual until commit - we
should fail it as early as we can to avoid wasting resources. Currently,
we do it in a rather abrupt way - by an assertion :-) This patch makes
any DML/DQL operation fail gracefully instead.

Closes #4070
---
https://github.com/tarantool/tarantool/issues/4070
https://github.com/tarantool/tarantool/commits/dv/gh-4070-vy-fail-aborted-transactions-early

 src/box/vinyl.c                | 16 +++++++++++----
 src/box/vy_tx.c                | 40 ++++++++++++++++++++++++++++---------
 src/box/vy_tx.h                |  4 ++--
 test/vinyl/errinj_ddl.result   | 45 ++++++++++++++++++++++++++++++++++++++++++
 test/vinyl/errinj_ddl.test.lua | 20 +++++++++++++++++++
 test/vinyl/errinj_tx.result    | 29 +++++++++++++++++++++++++++
 test/vinyl/errinj_tx.test.lua  | 10 ++++++++++
 test/vinyl/misc.result         | 20 +++++++++++++++++++
 test/vinyl/misc.test.lua       | 11 +++++++++++
 9 files changed, 180 insertions(+), 15 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index bf27f5fb..3ef43e18 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -2434,8 +2434,7 @@ vinyl_engine_begin_statement(struct engine *engine, struct txn *txn)
 	struct vy_tx *tx = txn->engine_tx;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
 	assert(tx != NULL);
-	stmt->engine_savepoint = vy_tx_begin_statement(tx);
-	return 0;
+	return vy_tx_begin_statement(tx, &stmt->engine_savepoint);
 }
 
 static void
@@ -3817,6 +3816,12 @@ vinyl_index_create_iterator(struct index *base, enum iterator_type type,
 		return NULL;
 	}
 
+	struct vy_tx *tx = in_txn() ? in_txn()->engine_tx : NULL;
+	if (tx != NULL && tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return NULL;
+	}
+
 	struct vinyl_iterator *it = mempool_alloc(&env->iterator_pool);
 	if (it == NULL) {
 	        diag_set(OutOfMemory, sizeof(struct vinyl_iterator),
@@ -3840,8 +3845,6 @@ vinyl_index_create_iterator(struct index *base, enum iterator_type type,
 	it->lsm = lsm;
 	vy_lsm_ref(lsm);
 
-	struct vy_tx *tx = in_txn() ? in_txn()->engine_tx : NULL;
-	assert(tx == NULL || tx->state == VINYL_TX_READY);
 	if (tx != NULL) {
 		/*
 		 * Register a trigger that will abort this iterator
@@ -3874,6 +3877,11 @@ vinyl_index_get(struct index *index, const char *key,
 	const struct vy_read_view **rv = (tx != NULL ? vy_tx_read_view(tx) :
 					  &env->xm->p_global_read_view);
 
+	if (tx != NULL && tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
+
 	if (vy_get_by_raw_key(lsm, tx, rv, key, part_count, ret) != 0)
 		return -1;
 	if (*ret != NULL) {
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index ab0bf071..ae660dd8 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -335,6 +335,15 @@ vy_tx_destroy(struct vy_tx *tx)
 	rlist_del_entry(tx, in_writers);
 }
 
+/** Mark a transaction as aborted and account it in stats. */
+static void
+vy_tx_abort(struct vy_tx *tx)
+{
+	assert(tx->state == VINYL_TX_READY);
+	tx->state = VINYL_TX_ABORT;
+	tx->xm->stat.conflict++;
+}
+
 /** Return true if the transaction is read-only. */
 static bool
 vy_tx_is_ro(struct vy_tx *tx)
@@ -394,7 +403,7 @@ vy_tx_abort_readers(struct vy_tx *tx, struct txv *v)
 		/* Abort only active TXs */
 		if (abort->state != VINYL_TX_READY)
 			continue;
-		abort->state = VINYL_TX_ABORT;
+		vy_tx_abort(abort);
 	}
 }
 
@@ -614,13 +623,18 @@ vy_tx_prepare(struct vy_tx *tx)
 {
 	struct tx_manager *xm = tx->xm;
 
+	if (tx->state == VINYL_TX_ABORT) {
+		/* Conflict is already accounted - see vy_tx_abort(). */
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
+
+	assert(tx->state == VINYL_TX_READY);
 	if (vy_tx_is_ro(tx)) {
-		assert(tx->state == VINYL_TX_READY);
 		tx->state = VINYL_TX_COMMIT;
 		return 0;
 	}
-
-	if (vy_tx_is_in_read_view(tx) || tx->state == VINYL_TX_ABORT) {
+	if (vy_tx_is_in_read_view(tx)) {
 		xm->stat.conflict++;
 		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
 		return -1;
@@ -834,18 +848,26 @@ vy_tx_rollback(struct vy_tx *tx)
 	mempool_free(&xm->tx_mempool, tx);
 }
 
-void *
-vy_tx_begin_statement(struct vy_tx *tx)
+int
+vy_tx_begin_statement(struct vy_tx *tx, void **savepoint)
 {
+	if (tx->state == VINYL_TX_ABORT) {
+		diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		return -1;
+	}
 	assert(tx->state == VINYL_TX_READY);
 	if (stailq_empty(&tx->log))
 		rlist_add_entry(&tx->xm->writers, tx, in_writers);
-	return stailq_last(&tx->log);
+	*savepoint = stailq_last(&tx->log);
+	return 0;
 }
 
 void
 vy_tx_rollback_statement(struct vy_tx *tx, void *svp)
 {
+	if (tx->state == VINYL_TX_ABORT)
+		return;
+
 	assert(tx->state == VINYL_TX_READY);
 	struct stailq_entry *last = svp;
 	struct stailq tail;
@@ -1071,7 +1093,7 @@ tx_manager_abort_writers_for_ddl(struct tx_manager *xm, struct vy_lsm *lsm)
 		if (tx->state == VINYL_TX_READY &&
 		    write_set_search_key(&tx->write_set, lsm,
 					 lsm->env->empty_key) != NULL)
-			tx->state = VINYL_TX_ABORT;
+			vy_tx_abort(tx);
 	}
 }
 
@@ -1082,7 +1104,7 @@ tx_manager_abort_writers_for_ro(struct tx_manager *xm)
 	rlist_foreach_entry(tx, &xm->writers, in_writers) {
 		/* Applier ignores ro flag. */
 		if (tx->state == VINYL_TX_READY && !tx->is_applier_session)
-			tx->state = VINYL_TX_ABORT;
+			vy_tx_abort(tx);
 	}
 }
 
diff --git a/src/box/vy_tx.h b/src/box/vy_tx.h
index 79fb9ac2..aaa31bee 100644
--- a/src/box/vy_tx.h
+++ b/src/box/vy_tx.h
@@ -326,8 +326,8 @@ vy_tx_rollback(struct vy_tx *tx);
  * transaction state. The transaction can be rolled back
  * to a save point with vy_tx_rollback_statement().
  */
-void *
-vy_tx_begin_statement(struct vy_tx *tx);
+int
+vy_tx_begin_statement(struct vy_tx *tx, void **savepoint);
 
 /**
  * Rollback a transaction statement.
diff --git a/test/vinyl/errinj_ddl.result b/test/vinyl/errinj_ddl.result
index 932132db..04cb581f 100644
--- a/test/vinyl/errinj_ddl.result
+++ b/test/vinyl/errinj_ddl.result
@@ -637,3 +637,48 @@ c:get()
 s:drop()
 ---
 ...
+--
+-- gh-4070: a transaction aborted by DDL must fail any DML/DQL request.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+---
+...
+_ = s:create_index('pk')
+---
+...
+s:replace{1, 1}
+---
+- [1, 1]
+...
+box.begin()
+---
+...
+s:replace{1, 2}
+---
+- [1, 2]
+...
+ch = fiber.channel(1)
+---
+...
+fiber = fiber.create(function() s:create_index('sk', {parts = {2, 'unsigned'}}) ch:put(true) end)
+---
+...
+ch:get()
+---
+- true
+...
+s:get(1)
+---
+- error: Transaction has been aborted by conflict
+...
+s:replace{1, 3}
+---
+- error: Transaction has been aborted by conflict
+...
+box.commit()
+---
+- error: Transaction has been aborted by conflict
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/errinj_ddl.test.lua b/test/vinyl/errinj_ddl.test.lua
index 95e0ad3c..50dc4aa1 100644
--- a/test/vinyl/errinj_ddl.test.lua
+++ b/test/vinyl/errinj_ddl.test.lua
@@ -278,3 +278,23 @@ errinj.set('ERRINJ_VY_READ_PAGE_TIMEOUT', 0)
 c:get()
 
 s:drop()
+
+--
+-- gh-4070: a transaction aborted by DDL must fail any DML/DQL request.
+--
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk')
+s:replace{1, 1}
+
+box.begin()
+s:replace{1, 2}
+
+ch = fiber.channel(1)
+fiber = fiber.create(function() s:create_index('sk', {parts = {2, 'unsigned'}}) ch:put(true) end)
+ch:get()
+
+s:get(1)
+s:replace{1, 3}
+box.commit()
+
+s:drop()
diff --git a/test/vinyl/errinj_tx.result b/test/vinyl/errinj_tx.result
index e2e4916e..7d59d156 100644
--- a/test/vinyl/errinj_tx.result
+++ b/test/vinyl/errinj_tx.result
@@ -436,6 +436,8 @@ space:drop()
 --
 -- Check that dependent transaction is aborted on WAL write.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 ---
 ...
@@ -465,6 +467,17 @@ itr.next()
 ---
 - [1]
 ...
+c = txn_proxy.new()
+---
+...
+c:begin()
+---
+- 
+...
+c('s:get(1)')
+---
+- - [1]
+...
 -- Resume the first transaction and let it fail on WAL write.
 errinj.set('ERRINJ_WAL_WRITE', true)
 ---
@@ -487,6 +500,22 @@ itr.next()
 ---
 - error: The read view is aborted
 ...
+c('s:get(1)')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c('s:select()')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c('s:replace{1}')
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
+c:commit()
+---
+- - {'error': 'Transaction has been aborted by conflict'}
+...
 itr = nil
 ---
 ...
diff --git a/test/vinyl/errinj_tx.test.lua b/test/vinyl/errinj_tx.test.lua
index d17c1abb..c434d92f 100644
--- a/test/vinyl/errinj_tx.test.lua
+++ b/test/vinyl/errinj_tx.test.lua
@@ -196,6 +196,8 @@ space:drop()
 --
 -- Check that dependent transaction is aborted on WAL write.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 _ = s:create_index('pk')
 s:replace{10}
@@ -209,6 +211,10 @@ _ = fiber.create(function() pcall(s.replace, s, {1}) ch:put(true) end)
 itr = create_iterator(s)
 itr.next()
 
+c = txn_proxy.new()
+c:begin()
+c('s:get(1)')
+
 -- Resume the first transaction and let it fail on WAL write.
 errinj.set('ERRINJ_WAL_WRITE', true)
 errinj.set('ERRINJ_WAL_DELAY', false)
@@ -217,6 +223,10 @@ errinj.set('ERRINJ_WAL_WRITE', false)
 
 -- Must fail.
 itr.next()
+c('s:get(1)')
+c('s:select()')
+c('s:replace{1}')
+c:commit()
 
 itr = nil
 s:drop()
diff --git a/test/vinyl/misc.result b/test/vinyl/misc.result
index 685fd776..4f613cb0 100644
--- a/test/vinyl/misc.result
+++ b/test/vinyl/misc.result
@@ -286,6 +286,8 @@ test_run:cmd('cleanup server test')
 -- gh-4016: local rw transactions are aborted when the instance
 -- switches to read-only mode.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 ---
 ...
@@ -308,6 +310,10 @@ _ = fiber.create(function()
     box.begin()
     s:replace{1, 2}
     ch1:get()
+    local status, err = pcall(s.replace, s, {3, 4})
+    ch1:put(status or err)
+    local status, err = pcall(s.select, s)
+    ch1:put(status or err)
     local status, err = pcall(box.commit)
     ch1:put(status or err)
 end);
@@ -321,6 +327,8 @@ _ = fiber.create(function()
     box.begin()
     s:select()
     ch2:get()
+    local status, err = pcall(s.select, s)
+    ch2:put(status or err)
     local status, err = pcall(box.commit)
     ch2:put(status or err)
 end);
@@ -345,8 +353,20 @@ ch2:put(true)
 ...
 ch1:get()
 ---
+- Can't modify data because this instance is in read-only mode.
+...
+ch1:get()
+---
 - Transaction has been aborted by conflict
 ...
+ch1:get()
+---
+- Transaction has been aborted by conflict
+...
+ch2:get()
+---
+- true
+...
 ch2:get()
 ---
 - true
diff --git a/test/vinyl/misc.test.lua b/test/vinyl/misc.test.lua
index cdc22774..bffaaf16 100644
--- a/test/vinyl/misc.test.lua
+++ b/test/vinyl/misc.test.lua
@@ -119,6 +119,8 @@ test_run:cmd('cleanup server test')
 -- gh-4016: local rw transactions are aborted when the instance
 -- switches to read-only mode.
 --
+-- gh-4070: an aborted transaction must fail any DML/DQL request.
+--
 s = box.schema.space.create('test', {engine = 'vinyl'})
 _ = s:create_index('pk')
 s:replace({1, 1})
@@ -129,6 +131,10 @@ _ = fiber.create(function()
     box.begin()
     s:replace{1, 2}
     ch1:get()
+    local status, err = pcall(s.replace, s, {3, 4})
+    ch1:put(status or err)
+    local status, err = pcall(s.select, s)
+    ch1:put(status or err)
     local status, err = pcall(box.commit)
     ch1:put(status or err)
 end);
@@ -138,6 +144,8 @@ _ = fiber.create(function()
     box.begin()
     s:select()
     ch2:get()
+    local status, err = pcall(s.select, s)
+    ch2:put(status or err)
     local status, err = pcall(box.commit)
     ch2:put(status or err)
 end);
@@ -148,6 +156,9 @@ box.cfg{read_only = true}
 ch1:put(true)
 ch2:put(true)
 ch1:get()
+ch1:get()
+ch1:get()
+ch2:get()
 ch2:get()
 -- Cleanup.
 box.cfg{read_only = false}
-- 
2.11.0