* [PATCH 2/2] vinyl: fail aborted transactions early
2019-03-22 18:12 [PATCH 1/2] vinyl: drop useless vy_read_view->is_aborted flag Vladimir Davydov
@ 2019-03-22 18:12 ` Vladimir Davydov
2019-03-28 14:05 ` [tarantool-patches] " Konstantin Osipov
2019-03-28 12:50 ` [tarantool-patches] Re: [PATCH 1/2] vinyl: drop useless vy_read_view->is_aborted flag Konstantin Osipov
1 sibling, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-03-22 18:12 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread