From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 9 Jul 2019 13:12:07 +0300 From: Vladimir Davydov Subject: Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions Message-ID: <20190709101207.jqvhyl32oxexneea@esperanza> References: <9fb58e54aee22309ba87fd2d6bed6bd658ab2e6d.1562357452.git.vdavydov.dev@gmail.com> <20190708122248.GC11062@atlas> <20190708164141.35f667dw7537syax@esperanza> <20190708165810.p3ur3rhysjwwmcmk@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190708165810.p3ur3rhysjwwmcmk@esperanza> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Mon, Jul 08, 2019 at 07:58:10PM +0300, Vladimir Davydov wrote: > On Mon, Jul 08, 2019 at 07:41:41PM +0300, Vladimir Davydov wrote: > > On Mon, Jul 08, 2019 at 03:22:48PM +0300, Konstantin Osipov wrote: > > > * Vladimir Davydov [19/07/05 23:27]: > > > > 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. > > > > > > Nit: I think since we set up triggers in memtx_* methods, we > > > should clear them inside memtx_* subsystem as well. > > > > > > so it's not txn_*, it's memtx_{enable|disable}_yields > > > > Calling a memtx_* method from vinyl would look rather weird IMO. > > May be, instead we should move memtx stuff dealing with yields to > txn? Something like, txn_disable_yield() that would disable yields > altogether. Memtx would call this function while vinyl wouldn't. > Then everything related to yields in transactions would live in > txn.c. What do you think? The patch below illustrates what I mean. IMHO it looks much better than the original one. --- >From 67e36c5a99065a17c391716972039f7e8fbb7691 Mon Sep 17 00:00:00 2001 From: Vladimir Davydov Date: Fri, 5 Jul 2019 17:40:56 +0300 Subject: [PATCH] memtx: fix txn_on_yield for DDL transactions 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. In order not to spread the yield-in-transaction logic between memtx and txn code, let's move all fiber_on_yield related stuff to txn, export a method to disable yields, and use the method in memtx. diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index c8376110..6ed38a91 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -55,58 +55,6 @@ static void checkpoint_cancel(struct checkpoint *ckpt); -/* - * Memtx yield-in-transaction trigger: roll back the effects - * of the transaction and mark the transaction as aborted. - */ -static void -txn_on_yield(struct trigger *trigger, void *event) -{ - (void) trigger; - (void) event; - - struct txn *txn = in_txn(); - assert(txn && txn->engine_tx); - if (txn == NULL || txn->engine_tx == NULL) - return; - txn_abort(txn); /* doesn't yield or fail */ -} - -/** - * Initialize context for yield triggers. - * In case of a yield inside memtx multi-statement transaction - * we must, first of all, roll back the effects of the transaction - * so that concurrent transactions won't see dirty, uncommitted - * data. - * Second, we must abort the transaction, since it has been rolled - * back implicitly. The transaction can not be rolled back - * completely from within a yield trigger, since a yield trigger - * can't fail. Instead, we mark the transaction as aborted and - * raise an error on attempt to commit it. - * - * So much hassle to be user-friendly until we have a true - * interactive transaction support in memtx. - */ -void -memtx_init_txn(struct txn *txn) -{ - struct fiber *fiber = fiber(); - - trigger_create(&txn->fiber_on_yield, txn_on_yield, - NULL, NULL); - /* - * Memtx doesn't allow yields between statements of - * a transaction. Set a trigger which would roll - * back the transaction if there is a yield. - */ - trigger_add(&fiber->on_yield, &txn->fiber_on_yield); - /* - * This serves as a marker that the triggers are - * initialized. - */ - txn->engine_tx = txn; -} - struct PACKED memtx_tuple { /* * sic: the header of the tuple is used @@ -372,45 +320,10 @@ memtx_engine_create_space(struct engine *engine, struct space_def *def, } static int -memtx_engine_prepare(struct engine *engine, struct txn *txn) -{ - (void)engine; - if (txn->engine_tx == 0) - return 0; - /* - * These triggers are only used for memtx and only - * when autocommit == false, so we are saving - * on calls to trigger_create/trigger_clear. - */ - trigger_clear(&txn->fiber_on_yield); - if (txn->is_aborted) { - diag_set(ClientError, ER_TRANSACTION_YIELD); - diag_log(); - return -1; - } - return 0; -} - -static int memtx_engine_begin(struct engine *engine, struct txn *txn) { (void)engine; - (void)txn; - return 0; -} - -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); - } + txn_disable_yield(txn); return 0; } @@ -459,9 +372,6 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, static void memtx_engine_rollback(struct engine *engine, struct txn *txn) { - if (txn->engine_tx != NULL) { - trigger_clear(&txn->fiber_on_yield); - } struct txn_stmt *stmt; stailq_reverse(&txn->stmts); stailq_foreach_entry(stmt, &txn->stmts, next) @@ -948,8 +858,8 @@ static const struct engine_vtab memtx_engine_vtab = { /* .create_space = */ memtx_engine_create_space, /* .join = */ memtx_engine_join, /* .begin = */ memtx_engine_begin, - /* .begin_statement = */ memtx_engine_begin_statement, - /* .prepare = */ memtx_engine_prepare, + /* .begin_statement = */ generic_engine_begin_statement, + /* .prepare = */ generic_engine_prepare, /* .commit = */ generic_engine_commit, /* .rollback_statement = */ memtx_engine_rollback_statement, /* .rollback = */ memtx_engine_rollback, 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 c605345d..bab98d7e 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -40,6 +40,12 @@ double too_long_threshold; /* Txn cache. */ static struct stailq txn_cache = {NULL, &txn_cache.first}; +static void +txn_on_stop(struct trigger *trigger, void *event); + +static void +txn_on_yield(struct trigger *trigger, void *event); + static inline void fiber_set_txn(struct fiber *fiber, struct txn *txn) { @@ -194,6 +200,7 @@ txn_begin() txn->has_triggers = false; txn->is_done = false; txn->is_aborted = false; + txn->is_yield_disabled = false; txn->in_sub_stmt = 0; txn->id = ++tsn; txn->signature = -1; @@ -201,8 +208,8 @@ txn_begin() txn->engine_tx = NULL; txn->psql_txn = NULL; txn->fiber = NULL; - /* fiber_on_yield/fiber_on_stop initialized by engine on demand */ fiber_set_txn(fiber(), txn); + /* fiber_on_yield is initialized by engine on demand */ trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL); trigger_add(&fiber()->on_stop, &txn->fiber_on_stop); return txn; @@ -463,6 +470,12 @@ txn_write_to_wal(struct txn *txn) static int txn_prepare(struct txn *txn) { + if (txn->is_aborted) { + assert(txn->is_yield_disabled); + diag_set(ClientError, ER_TRANSACTION_YIELD); + diag_log(); + return -1; + } /* * If transaction has been started in SQL, deferred * foreign key constraints must not be violated. @@ -484,6 +497,8 @@ txn_prepare(struct txn *txn) return -1; } trigger_clear(&txn->fiber_on_stop); + if (txn->is_yield_disabled) + trigger_clear(&txn->fiber_on_yield); return 0; } @@ -550,12 +565,22 @@ txn_rollback(struct txn *txn) { assert(txn == in_txn()); trigger_clear(&txn->fiber_on_stop); + if (txn->is_yield_disabled) + trigger_clear(&txn->fiber_on_yield); txn->signature = -1; txn_complete(txn); fiber_set_txn(fiber(), NULL); } -void +/** + * Roll back the transaction but keep the object around. + * A special case for memtx transaction abort on yield. In this + * case we need to abort the transaction to avoid dirty reads but + * need to keep it around to ensure a new one is not implicitly + * started and committed by the user program. Later, at + * transaction commit we will raise an exception. + */ +static void txn_abort(struct txn *txn) { assert(in_txn() == txn); @@ -578,6 +603,31 @@ txn_check_singlestatement(struct txn *txn, const char *where) return 0; } +void +txn_disable_yield(struct txn *txn) +{ + assert(!txn->is_yield_disabled); + txn->is_yield_disabled = true; + trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL); + trigger_add(&fiber()->on_yield, &txn->fiber_on_yield); +} + +void +txn_enable_yield_for_ddl(void) +{ + struct txn *txn = in_txn(); + assert(txn != NULL && txn->is_yield_disabled); + trigger_clear(&txn->fiber_on_yield); +} + +void +txn_disable_yield_after_ddl(void) +{ + struct txn *txn = in_txn(); + assert(txn != NULL && txn->is_yield_disabled); + trigger_add(&fiber()->on_yield, &txn->fiber_on_yield); +} + int64_t box_txn_id(void) { @@ -711,7 +761,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp) return 0; } -void +static void txn_on_stop(struct trigger *trigger, void *event) { (void) trigger; @@ -719,3 +769,29 @@ txn_on_stop(struct trigger *trigger, void *event) txn_rollback(in_txn()); /* doesn't yield or fail */ } +/** + * Memtx yield-in-transaction trigger callback. + * + * In case of a yield inside memtx multi-statement transaction + * we must, first of all, roll back the effects of the transaction + * so that concurrent transactions won't see dirty, uncommitted + * data. + * + * Second, we must abort the transaction, since it has been rolled + * back implicitly. The transaction can not be rolled back + * completely from within a yield trigger, since a yield trigger + * can't fail. Instead, we mark the transaction as aborted and + * raise an error on attempt to commit it. + * + * So much hassle to be user-friendly until we have a true + * interactive transaction support in memtx. + */ +static void +txn_on_yield(struct trigger *trigger, void *event) +{ + (void) trigger; + (void) event; + struct txn *txn = in_txn(); + assert(txn != NULL && txn->is_yield_disabled); + txn_abort(txn); /* doesn't yield or fail */ +} diff --git a/src/box/txn.h b/src/box/txn.h index d1ef220a..258a8db7 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -169,6 +169,11 @@ struct txn { * rolled back at commit. */ bool is_aborted; + /** + * True if the engine doesn't support yields inside + * transactions, see txn_disable_yield(). + */ + bool is_yield_disabled; /** True if on_commit and on_rollback lists are non-empty. */ bool has_triggers; /** The number of active nested statement-level transactions. */ @@ -259,17 +264,6 @@ int txn_write(struct txn *txn); /** - * Roll back the transaction but keep the object around. - * A special case for memtx transaction abort on yield. In this - * case we need to abort the transaction to avoid dirty reads but - * need to keep it around to ensure a new one is not implicitly - * started and committed by the user program. Later, at - * transaction commit we will raise an exception. - */ -void -txn_abort(struct txn *txn); - -/** * Most txns don't have triggers, and txn objects * are created on every access to data, so txns * are partially initialized. @@ -371,6 +365,34 @@ int txn_check_singlestatement(struct txn *txn, const char *where); /** + * Installs yield-in-transaction trigger: roll back the effects + * of the transaction and mark the transaction as aborted. + * + * Used by the memtx engine which doesn't support yields in + * transactions. + */ +void +txn_disable_yield(struct txn *txn); + +/** + * 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. @@ -400,12 +422,6 @@ txn_last_stmt(struct txn *txn) } /** - * Fiber-stop trigger: roll back the transaction. - */ -void -txn_on_stop(struct trigger *trigger, void *event); - -/** * Return VDBE that is being currently executed. * * @retval VDBE that is being currently executed. 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()