Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, gorcunov@gmail.com,
	sergepetrenko@tarantool.org
Subject: [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override
Date: Fri, 11 Jun 2021 23:56:09 +0200
Message-ID: <6ef0d84375576f94283e2bf1fea56a55daf9523e.1623448465.git.v.shpilevoy@tarantool.org> (raw)
In-Reply-To: <cover.1623448465.git.v.shpilevoy@tarantool.org>

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.
 		 */
-		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
-- 
2.24.3 (Apple Git-128)


  parent reply	other threads:[~2021-06-11 21:59 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 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-06-15 13:44   ` [Tarantool-patches] [PATCH 13/13] txn: stop TXN_SIGNATURE_ABORT override Serge Petrenko via Tarantool-patches
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=6ef0d84375576f94283e2bf1fea56a55daf9523e.1623448465.git.v.shpilevoy@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=sergepetrenko@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git