From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>, tarantool-patches@dev.tarantool.org, gorcunov@gmail.com Subject: Re: [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override Date: Tue, 15 Jun 2021 16:44:07 +0300 [thread overview] Message-ID: <a94008ed-0a2e-7991-e0ab-8afb5c7b98aa@tarantool.org> (raw) In-Reply-To: <6ef0d84375576f94283e2bf1fea56a55daf9523e.1623448465.git.v.shpilevoy@tarantool.org> 12.06.2021 00:56, Vladislav Shpilevoy пишет: > When txn_commit/try_async() failed before going to WAL thread, > they installed TXN_SIGNATURE_ABORT signature meaning that the > caller and the rollback triggers must look at the global diag. > > But they called txn_rollback() before doing return and calling the > triggers, which overrode the signature with TXN_SIGNATURE_ROLLBACK > leading to the original error loss. > > The patch makes TXN_SIGNATURE_ROLLBACK installed only when a real > rollback happens (via box_txn_rollback()). > > This makes the original commit errors like a conflict in the > transaction manager and OOM not lost. > > Besides, ERRINJ_TXN_COMMIT_ASYNC does not need its own diag_log() > anymore. Because since this commit the applier logs the correct > error instead of ER_WAL_IO/ER_TXN_ROLLBACK. > > Closes #6027 > --- > .../unreleased/gh-6027-applier-lost-error.md | 7 ++ > src/box/txn.c | 10 ++- > .../gh-6027-applier-error-show.result | 82 +++++++++++++++++++ > .../gh-6027-applier-error-show.test.lua | 31 +++++++ > test/replication/suite.cfg | 1 + > test/replication/suite.ini | 2 +- > 6 files changed, 129 insertions(+), 4 deletions(-) > create mode 100644 changelogs/unreleased/gh-6027-applier-lost-error.md > create mode 100644 test/replication/gh-6027-applier-error-show.result > create mode 100644 test/replication/gh-6027-applier-error-show.test.lua > > diff --git a/changelogs/unreleased/gh-6027-applier-lost-error.md b/changelogs/unreleased/gh-6027-applier-lost-error.md > new file mode 100644 > index 000000000..9c765b8e2 > --- /dev/null > +++ b/changelogs/unreleased/gh-6027-applier-lost-error.md > @@ -0,0 +1,7 @@ > +## bugfix/replication > + > +* When an error happened during appliance of a transaction received from a > + remote instance via replication, it was always reported as "Failed to write > + to disk" regardless of what really happened. Now the correct error is shown. > + For example, "Out of memory", or "Transaction has been aborted by conflict", > + and so on (gh-6027). > diff --git a/src/box/txn.c b/src/box/txn.c > index 5cae7b41d..c2734c237 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -805,7 +805,6 @@ txn_commit_try_async(struct txn *txn) > * Log it for the testing sake: we grep > * output to mark this event. > */ The comment became rather misleading now. We log the error in applier, not here. I would drop it altogether, but up to you. > - diag_log(); > goto rollback; > }); > > @@ -983,11 +982,11 @@ void > txn_rollback(struct txn *txn) > { > assert(txn == in_txn()); > + assert(txn->signature != TXN_SIGNATURE_UNKNOWN); > txn->status = TXN_ABORTED; > trigger_clear(&txn->fiber_on_stop); > if (!txn_has_flag(txn, TXN_CAN_YIELD)) > trigger_clear(&txn->fiber_on_yield); > - txn->signature = TXN_SIGNATURE_ROLLBACK; > txn_complete_fail(txn); > fiber_set_txn(fiber(), NULL); > } > @@ -1086,6 +1085,8 @@ box_txn_rollback(void) > diag_set(ClientError, ER_ROLLBACK_IN_SUB_STMT); > return -1; > } > + assert(txn->signature == TXN_SIGNATURE_UNKNOWN); > + txn->signature = TXN_SIGNATURE_ROLLBACK; > txn_rollback(txn); /* doesn't throw */ > fiber_gc(); > return 0; > @@ -1221,7 +1222,10 @@ txn_on_stop(struct trigger *trigger, void *event) > { > (void) trigger; > (void) event; > - txn_rollback(in_txn()); /* doesn't yield or fail */ > + struct txn *txn = in_txn(); > + assert(txn->signature == TXN_SIGNATURE_UNKNOWN); > + txn->signature = TXN_SIGNATURE_ROLLBACK; > + txn_rollback(txn); > fiber_gc(); > return 0; > } > diff --git a/test/replication/gh-6027-applier-error-show.result b/test/replication/gh-6027-applier-error-show.result > new file mode 100644 > index 000000000..c3a01ab50 > --- /dev/null > +++ b/test/replication/gh-6027-applier-error-show.result > @@ -0,0 +1,82 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > + > +-- > +-- gh-6027: on attempt to a commit transaction its original error was lost. > +-- > + > +box.schema.user.grant('guest', 'super') > + | --- > + | ... > +s = box.schema.create_space('test') > + | --- > + | ... > +_ = s:create_index('pk') > + | --- > + | ... > + > +test_run:cmd('create server replica with rpl_master=default, '.. \ > + 'script="replication/replica.lua"') > + | --- > + | - true > + | ... > +test_run:cmd('start server replica') > + | --- > + | - true > + | ... > + > +test_run:switch('replica') > + | --- > + | - true > + | ... > +box.error.injection.set('ERRINJ_TXN_COMMIT_ASYNC', true) > + | --- > + | - ok > + | ... > + > +test_run:switch('default') > + | --- > + | - true > + | ... > +_ = s:replace{1} > + | --- > + | ... > + > +test_run:switch('replica') > + | --- > + | - true > + | ... > +test_run:wait_upstream(1, {status = 'stopped'}) > + | --- > + | - true > + | ... > +-- Should be something about error injection. > +box.info.replication[1].upstream.message > + | --- > + | - Error injection 'txn commit async injection' > + | ... > + > +test_run:switch('default') > + | --- > + | - true > + | ... > +test_run:cmd('stop server replica') > + | --- > + | - true > + | ... > +test_run:cmd('delete server replica') > + | --- > + | - true > + | ... > +box.error.injection.set('ERRINJ_TXN_COMMIT_ASYNC', false) > + | --- > + | - ok > + | ... > +s:drop() > + | --- > + | ... > +box.schema.user.revoke('guest', 'super') > + | --- > + | ... > diff --git a/test/replication/gh-6027-applier-error-show.test.lua b/test/replication/gh-6027-applier-error-show.test.lua > new file mode 100644 > index 000000000..8e17cdfa9 > --- /dev/null > +++ b/test/replication/gh-6027-applier-error-show.test.lua > @@ -0,0 +1,31 @@ > +test_run = require('test_run').new() > + > +-- > +-- gh-6027: on attempt to a commit transaction its original error was lost. > +-- > + > +box.schema.user.grant('guest', 'super') > +s = box.schema.create_space('test') > +_ = s:create_index('pk') > + > +test_run:cmd('create server replica with rpl_master=default, '.. \ > + 'script="replication/replica.lua"') > +test_run:cmd('start server replica') > + > +test_run:switch('replica') > +box.error.injection.set('ERRINJ_TXN_COMMIT_ASYNC', true) > + > +test_run:switch('default') > +_ = s:replace{1} > + > +test_run:switch('replica') > +test_run:wait_upstream(1, {status = 'stopped'}) > +-- Should be something about error injection. > +box.info.replication[1].upstream.message > + > +test_run:switch('default') > +test_run:cmd('stop server replica') > +test_run:cmd('delete server replica') > +box.error.injection.set('ERRINJ_TXN_COMMIT_ASYNC', false) > +s:drop() > +box.schema.user.revoke('guest', 'super') > diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg > index 9eccf9e19..3a0a8649f 100644 > --- a/test/replication/suite.cfg > +++ b/test/replication/suite.cfg > @@ -45,6 +45,7 @@ > "gh-5536-wal-limit.test.lua": {}, > "gh-5566-final-join-synchro.test.lua": {}, > "gh-5613-bootstrap-prefer-booted.test.lua": {}, > + "gh-6027-applier-error-show.test.lua": {}, > "gh-6032-promote-wal-write.test.lua": {}, > "gh-6057-qsync-confirm-async-no-wal.test.lua": {}, > "gh-6094-rs-uuid-mismatch.test.lua": {}, > diff --git a/test/replication/suite.ini b/test/replication/suite.ini > index 80e968d56..ccf3559df 100644 > --- a/test/replication/suite.ini > +++ b/test/replication/suite.ini > @@ -3,7 +3,7 @@ core = tarantool > script = master.lua > description = tarantool/box, replication > disabled = consistent.test.lua > -release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua > +release_disabled = catch.test.lua errinj.test.lua gc.test.lua gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua hang_on_synchro_fail.test.lua anon_register_gap.test.lua gh-5213-qsync-applier-order.test.lua gh-5213-qsync-applier-order-3.test.lua gh-6027-applier-error-show.test.lua gh-6032-promote-wal-write.test.lua gh-6057-qsync-confirm-async-no-wal.test.lua > config = suite.cfg > lua_libs = lua/fast_replica.lua lua/rlimit.lua > use_unix_sockets = True -- Serge Petrenko
next prev parent reply other threads:[~2021-06-15 13:44 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-11 21:56 [Tarantool-patches] [PATCH 00/13] Applier rollback reason Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 01/13] error: introduce ER_CASCADE_ROLLBACK Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 10/13] txn: install proper diag errors on txn fail Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 11/13] wal: introduce JOURNAL_ENTRY_ERR_CASCADE Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 12/13] txn: introduce TXN_SIGNATURE_ABORT Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override Vladislav Shpilevoy via Tarantool-patches 2021-06-15 13:44 ` Serge Petrenko via Tarantool-patches [this message] 2021-06-15 19:34 ` Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 02/13] test: remove replica-applier-rollback.lua Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 03/13] journal: make journal_write() set diag on error Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 04/13] wal: refactor wal_write_to_disk() Vladislav Shpilevoy via Tarantool-patches 2021-06-15 20:46 ` Cyrill Gorcunov via Tarantool-patches 2021-06-16 6:22 ` Vladislav Shpilevoy via Tarantool-patches 2021-06-16 8:02 ` Cyrill Gorcunov via Tarantool-patches 2021-06-16 23:32 ` Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 05/13] diag: introduce diag_set_detailed() Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 06/13] wal: encapsulate ER_WAL_IO Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 07/13] txn: change limbo rollback check in the trigger Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 08/13] journal: introduce proper error codes Vladislav Shpilevoy via Tarantool-patches 2021-06-11 21:56 ` [Tarantool-patches] [PATCH 09/13] txn: assert after WAL write that txn is not done Vladislav Shpilevoy via Tarantool-patches 2021-06-15 13:43 ` [Tarantool-patches] [PATCH 00/13] Applier rollback reason Serge Petrenko via Tarantool-patches 2021-06-16 23:32 ` Vladislav Shpilevoy via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a94008ed-0a2e-7991-e0ab-8afb5c7b98aa@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox