From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions Date: Fri, 5 Jul 2019 23:25:30 +0300 Message-Id: <9fb58e54aee22309ba87fd2d6bed6bd658ab2e6d.1562357452.git.vdavydov.dev@gmail.com> In-Reply-To: References: In-Reply-To: References: To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: 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