From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 2/2] vinyl: fail aborted transactions early Date: Fri, 22 Mar 2019 21:12:50 +0300 Message-Id: In-Reply-To: <6a374b13b7dc52ce14d8d23433efd46f3e2161cf.1553277782.git.vdavydov.dev@gmail.com> References: <6a374b13b7dc52ce14d8d23433efd46f3e2161cf.1553277782.git.vdavydov.dev@gmail.com> In-Reply-To: <6a374b13b7dc52ce14d8d23433efd46f3e2161cf.1553277782.git.vdavydov.dev@gmail.com> References: <6a374b13b7dc52ce14d8d23433efd46f3e2161cf.1553277782.git.vdavydov.dev@gmail.com> To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: 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