From 23f4e6f6ad3a8f1020aac55d5246b9001adf1b89 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov@tarantool.org>
Date: Tue, 10 Jan 2023 17:12:11 +0300
Subject: [PATCH] txn: fail ro stmt if transaction is aborted

We fail write statements if the current transaction was aborted by yield
or timeout. We should fail read-only statements in this case, as well.
Note, we already fail read-only statements if the current transaction
was aborted by conflict.

Closes #8123

NO_DOC=bug fix

(cherry picked from commit 5f1500f4a83c3c91c429b5193c25b7f18b120dd3)
---
 .../gh-8123-txn-fail-ro-stmt-if-aborted.md    |   5 +
 src/box/txn.c                                 |  25 ----
 src/box/txn.h                                 |  37 +++++-
 ..._8123_txn_fail_ro_stmt_if_aborted_test.lua | 107 ++++++++++++++++++
 ...ox_iproto_transactions_over_streams.result |  23 ++--
 ..._iproto_transactions_over_streams.test.lua |  10 +-
 test/box/net.box_tx_timeout.result            |  16 +--
 test/box/net.box_tx_timeout.test.lua          |   8 +-
 test/box/transaction.result                   |  11 +-
 test/box/transaction.test.lua                 |   6 +-
 test/box/tx_timeout.result                    |  16 +--
 test/box/tx_timeout.test.lua                  |   8 +-
 test/sql/triggers.result                      |   6 +-
 test/sql/triggers.test.lua                    |   6 +-
 14 files changed, 197 insertions(+), 87 deletions(-)
 create mode 100644 changelogs/unreleased/gh-8123-txn-fail-ro-stmt-if-aborted.md
 create mode 100644 test/box-luatest/gh_8123_txn_fail_ro_stmt_if_aborted_test.lua

diff --git a/changelogs/unreleased/gh-8123-txn-fail-ro-stmt-if-aborted.md b/changelogs/unreleased/gh-8123-txn-fail-ro-stmt-if-aborted.md
new file mode 100644
index 0000000000..7b9d5d5533
--- /dev/null
+++ b/changelogs/unreleased/gh-8123-txn-fail-ro-stmt-if-aborted.md
@@ -0,0 +1,5 @@
+## bugfix/core
+
+* Fixed a bug because of which read-only statements executed in a transaction
+  aborted by yield or timeout completed successfully. Now, read-only statements
+  fail in this case, just like write statements (gh-8123).
diff --git a/src/box/txn.c b/src/box/txn.c
index 784ca73cb8..b074f2d7fb 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -234,31 +234,6 @@ tx_region_release(struct txn *txn, enum tx_alloc_type alloc_type)
 		tx_track_allocation(txn, new_alloc_size, alloc_type);
 }
 
-static inline enum box_error_code
-txn_flags_to_error_code(struct txn *txn)
-{
-	if (txn_has_flag(txn, TXN_IS_CONFLICTED))
-		return ER_TRANSACTION_CONFLICT;
-	else if (txn_has_flag(txn, TXN_IS_ABORTED_BY_YIELD))
-		return ER_TRANSACTION_YIELD;
-	else if (txn_has_flag(txn, TXN_IS_ABORTED_BY_TIMEOUT))
-		return ER_TRANSACTION_TIMEOUT;
-	return ER_UNKNOWN;
-}
-
-static inline int
-txn_check_can_continue(struct txn *txn)
-{
-	enum txn_flag flags =
-		TXN_IS_CONFLICTED | TXN_IS_ABORTED_BY_YIELD |
-		TXN_IS_ABORTED_BY_TIMEOUT;
-	if (txn_has_any_of_flags(txn, flags)) {
-		diag_set(ClientError, txn_flags_to_error_code(txn));
-		return -1;
-	}
-	return 0;
-}
-
 static inline void
 txn_set_timeout(struct txn *txn, double timeout)
 {
diff --git a/src/box/txn.h b/src/box/txn.h
index 0ffffb39c4..848b463c27 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -567,7 +567,7 @@ txn_has_flag(struct txn *txn, enum txn_flag flag)
 }
 
 static inline bool
-txn_has_any_of_flags(struct txn *txn, enum txn_flag flags)
+txn_has_any_of_flags(struct txn *txn, unsigned int flags)
 {
 	return (txn->flags & flags) != 0;
 }
@@ -584,6 +584,37 @@ txn_clear_flags(struct txn *txn, unsigned int flags)
 	txn->flags &= ~flags;
 }
 
+/**
+ * Returns the code of the error that caused abort of the given transaction.
+ */
+static inline enum box_error_code
+txn_flags_to_error_code(struct txn *txn)
+{
+	if (txn_has_flag(txn, TXN_IS_CONFLICTED))
+		return ER_TRANSACTION_CONFLICT;
+	else if (txn_has_flag(txn, TXN_IS_ABORTED_BY_YIELD))
+		return ER_TRANSACTION_YIELD;
+	else if (txn_has_flag(txn, TXN_IS_ABORTED_BY_TIMEOUT))
+		return ER_TRANSACTION_TIMEOUT;
+	return ER_UNKNOWN;
+}
+
+/**
+ * Checks if new statements can be executed in the given transaction.
+ * Returns 0 if true. Otherwise, sets diag and returns -1.
+ */
+static inline int
+txn_check_can_continue(struct txn *txn)
+{
+	unsigned int flags = TXN_IS_CONFLICTED | TXN_IS_ABORTED_BY_YIELD |
+			     TXN_IS_ABORTED_BY_TIMEOUT;
+	if (txn_has_any_of_flags(txn, flags)) {
+		diag_set(ClientError, txn_flags_to_error_code(txn));
+		return -1;
+	}
+	return 0;
+}
+
 /* Pointer to the current transaction (if any) */
 static inline struct txn *
 in_txn(void)
@@ -792,10 +823,8 @@ txn_begin_ro_stmt(struct space *space, struct txn **txn,
 	svp->region_used = region_used(svp->region);
 	*txn = in_txn();
 	if (*txn != NULL) {
-		if ((*txn)->status == TXN_CONFLICTED) {
-			diag_set(ClientError, ER_TRANSACTION_CONFLICT);
+		if (txn_check_can_continue(*txn) != 0)
 			return -1;
-		}
 		struct engine *engine = space->engine;
 		return txn_begin_in_engine(engine, *txn);
 	}
diff --git a/test/box-luatest/gh_8123_txn_fail_ro_stmt_if_aborted_test.lua b/test/box-luatest/gh_8123_txn_fail_ro_stmt_if_aborted_test.lua
new file mode 100644
index 0000000000..ea993fef65
--- /dev/null
+++ b/test/box-luatest/gh_8123_txn_fail_ro_stmt_if_aborted_test.lua
@@ -0,0 +1,107 @@
+local server = require('luatest.server')
+local t = require('luatest')
+
+local g_mvcc_off = t.group('gh_8123.mvcc_off')
+
+g_mvcc_off.before_all(function(cg)
+    cg.server = server:new({
+        alias = 'default',
+        box_cfg = {memtx_use_mvcc_engine = false},
+    })
+    cg.server:start()
+end)
+
+g_mvcc_off.after_all(function(cg)
+    cg.server:drop()
+end)
+
+g_mvcc_off.after_each(function(cg)
+    cg.server:exec(function()
+        if box.space.test ~= nil then
+            box.space.test:drop()
+        end
+    end)
+end)
+
+g_mvcc_off.test_yield = function(cg)
+    cg.server:exec(function()
+        local fiber = require('fiber')
+        local t = require('luatest')
+        local s = box.schema.create_space('test')
+        s:create_index('primary')
+        box.begin()
+        s:insert({1})
+        fiber.yield()
+        local errmsg = 'Transaction has been aborted by a fiber yield'
+        t.assert_error_msg_equals(errmsg, s.get, s, {1})
+        t.assert_error_msg_equals(errmsg, s.select, s, {1})
+        t.assert_error_msg_equals(errmsg, s.insert, s, {2})
+        t.assert_error_msg_equals(errmsg, s.get, s, {2})
+        t.assert_error_msg_equals(errmsg, s.select, s, {2})
+        t.assert_error_msg_equals(errmsg, box.commit)
+    end)
+end
+
+local g_mvcc_on = t.group('gh_8123.mvcc_on', t.helpers.matrix({
+    engine = {'memtx', 'vinyl'},
+}))
+
+g_mvcc_on.before_all(function(cg)
+    cg.server = server:new({
+        alias = 'default',
+        box_cfg = {memtx_use_mvcc_engine = true},
+    })
+    cg.server:start()
+end)
+
+g_mvcc_on.after_all(function(cg)
+    cg.server:drop()
+end)
+
+g_mvcc_on.after_each(function(cg)
+    cg.server:exec(function()
+        if box.space.test ~= nil then
+            box.space.test:drop()
+        end
+    end)
+end)
+
+g_mvcc_on.test_timeout = function(cg)
+    cg.server:exec(function(engine)
+        local fiber = require('fiber')
+        local t = require('luatest')
+        local s = box.schema.create_space('test', {engine = engine})
+        s:create_index('primary')
+        box.begin({timeout = 0.01})
+        s:insert({1})
+        fiber.sleep(0.1)
+        local errmsg = 'Transaction has been aborted by timeout'
+        t.assert_error_msg_equals(errmsg, s.get, s, {1})
+        t.assert_error_msg_equals(errmsg, s.select, s, {1})
+        t.assert_error_msg_equals(errmsg, s.insert, s, {2})
+        t.assert_error_msg_equals(errmsg, s.get, s, {2})
+        t.assert_error_msg_equals(errmsg, s.select, s, {2})
+        t.assert_error_msg_equals(errmsg, box.commit)
+    end, {cg.params.engine})
+end
+
+g_mvcc_on.test_conflict = function(cg)
+    cg.server:exec(function(engine)
+        local fiber = require('fiber')
+        local t = require('luatest')
+        local s = box.schema.create_space('test', {engine = engine})
+        s:create_index('primary')
+        box.begin()
+        s:insert({1})
+        local f = fiber.new(s.insert, s, {1, 1})
+        f:set_joinable(true)
+        t.assert_equals({f:join()}, {true, {1, 1}})
+        local errmsg = 'Transaction has been aborted by conflict'
+        t.assert_error_msg_equals(errmsg, s.get, s, {1})
+        t.assert_error_msg_equals(errmsg, s.select, s, {1})
+        t.assert_error_msg_equals(errmsg, s.insert, s, {2})
+        t.assert_error_msg_equals(errmsg, s.get, s, {2})
+        t.assert_error_msg_equals(errmsg, s.select, s, {2})
+        t.assert_error_msg_equals(errmsg, box.commit)
+    end, {cg.params.engine})
+end
diff --git a/test/box/net.box_iproto_transactions_over_streams.result b/test/box/net.box_iproto_transactions_over_streams.result
index f875b952dd..1b3bc49fcb 100644
--- a/test/box/net.box_iproto_transactions_over_streams.result
+++ b/test/box/net.box_iproto_transactions_over_streams.result
@@ -273,17 +273,10 @@ space:replace({1})
 ---
 - [1]
 ...
--- Empty select, transaction was not commited and
--- is not visible from requests not belonging to the
--- transaction.
-space:select{}
----
-- []
-...
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 ---
-- []
+- error: Transaction has been aborted by a fiber yield
 ...
 test_run:switch("test")
 ---
@@ -329,14 +322,14 @@ stream:call('s:replace', {{1}})
 ---
 - [1]
 ...
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 ---
-- []
+- error: Transaction has been aborted by a fiber yield
 ...
 stream:call('s:select', {})
 ---
-- []
+- error: Transaction has been aborted by a fiber yield
 ...
 test_run:switch("test")
 ---
@@ -371,14 +364,14 @@ stream:call('s:replace', {{1}})
 ---
 - [1]
 ...
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 ---
-- []
+- error: Transaction has been aborted by a fiber yield
 ...
 stream:call('s:select', {})
 ---
-- []
+- error: Transaction has been aborted by a fiber yield
 ...
 test_run:switch("test")
 ---
diff --git a/test/box/net.box_iproto_transactions_over_streams.test.lua b/test/box/net.box_iproto_transactions_over_streams.test.lua
index f77d4c9389..0f9ea0a17a 100644
--- a/test/box/net.box_iproto_transactions_over_streams.test.lua
+++ b/test/box/net.box_iproto_transactions_over_streams.test.lua
@@ -144,11 +144,7 @@ conn, stream, space = start_server_and_init(false)
 stream:begin()
 test_run:wait_cond(function () return get_current_stream_count() == 1 end)
 space:replace({1})
--- Empty select, transaction was not commited and
--- is not visible from requests not belonging to the
--- transaction.
-space:select{}
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 test_run:switch("test")
 -- Select is empty, transaction was not commited
@@ -167,7 +163,7 @@ stream:commit()
 -- Same checks for `call` end `eval` functions.
 stream:call('box.begin')
 stream:call('s:replace', {{1}})
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 stream:call('s:select', {})
 test_run:switch("test")
@@ -183,7 +179,7 @@ space:select{}
 -- begin and commit transaction.
 stream:execute('START TRANSACTION')
 stream:call('s:replace', {{1}})
--- Select is empty, because memtx_use_mvcc_engine is false
+-- Select fails, because memtx_use_mvcc_engine is false
 space:select({})
 stream:call('s:select', {})
 test_run:switch("test")
diff --git a/test/box/net.box_tx_timeout.result b/test/box/net.box_tx_timeout.result
index 1bf5129c77..261cc9d237 100644
--- a/test/box/net.box_tx_timeout.result
+++ b/test/box/net.box_tx_timeout.result
@@ -102,9 +102,9 @@ space:select({}) -- [1]
 _ = test_run:eval("test", string.format("sleep_with_timeout(%f)", txn_timeout + 0.1))
 ---
 ...
-space:select({}) -- []
+space:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 space:replace({2})
 ---
@@ -113,9 +113,9 @@ space:replace({2})
 fiber.yield()
 ---
 ...
-space:select({}) -- []
+space:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 stream:commit() -- transaction was aborted by timeout
 ---
@@ -137,9 +137,9 @@ space:select({}) -- [1]
 _= test_run:eval("test", string.format("sleep_with_timeout(%f)", txn_timeout + 0.1))
 ---
 ...
-space:select({}) -- []
+space:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 space:replace({2})
 ---
@@ -148,9 +148,9 @@ space:replace({2})
 fiber.yield()
 ---
 ...
-space:select({}) -- []
+space:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 stream:commit() -- transaction was aborted by timeout
 ---
diff --git a/test/box/net.box_tx_timeout.test.lua b/test/box/net.box_tx_timeout.test.lua
index 3280568d61..b3f502422a 100644
--- a/test/box/net.box_tx_timeout.test.lua
+++ b/test/box/net.box_tx_timeout.test.lua
@@ -38,10 +38,10 @@ stream:begin()
 space:replace({1})
 space:select({}) -- [1]
 _ = test_run:eval("test", string.format("sleep_with_timeout(%f)", txn_timeout + 0.1))
-space:select({}) -- []
+space:select({})
 space:replace({2})
 fiber.yield()
-space:select({}) -- []
+space:select({})
 stream:commit() -- transaction was aborted by timeout
 
 -- Check that transaction aborted by timeout, which
@@ -50,10 +50,10 @@ stream:begin({timeout = txn_timeout})
 space:replace({1})
 space:select({}) -- [1]
 _= test_run:eval("test", string.format("sleep_with_timeout(%f)", txn_timeout + 0.1))
-space:select({}) -- []
+space:select({})
 space:replace({2})
 fiber.yield()
-space:select({}) -- []
+space:select({})
 stream:commit() -- transaction was aborted by timeout
 
 -- Check that transaction is not rollback until timeout expired.
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 2a34e2e5a3..d4cc01712f 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -211,9 +211,14 @@ box.commit();
 box.begin() s:insert{1, 'Must be rolled back'};
 ---
 ...
--- nothing - the transaction was rolled back
--- nothing to commit because of yield
-while s:get{1} ~= nil do fiber.sleep(0) end
+fiber.yield();
+---
+...
+-- error - the transaction was rolled back by yield
+s:get{1};
+---
+- error: Transaction has been aborted by a fiber yield
+...
 box.commit();
 ---
 - error: Transaction has been aborted by a fiber yield
diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
index a1e49f5ac2..6bd039f65a 100644
--- a/test/box/transaction.test.lua
+++ b/test/box/transaction.test.lua
@@ -99,9 +99,9 @@ box.begin();
 -- back a transction with no statements.
 box.commit();
 box.begin() s:insert{1, 'Must be rolled back'};
--- nothing - the transaction was rolled back
-while s:get{1} ~= nil do fiber.sleep(0) end
--- nothing to commit because of yield
+fiber.yield();
+-- error - the transaction was rolled back by yield
+s:get{1};
 box.commit();
 -- Test background fiber
 --
diff --git a/test/box/tx_timeout.result b/test/box/tx_timeout.result
index a64da87e61..7f7eea153a 100644
--- a/test/box/tx_timeout.result
+++ b/test/box/tx_timeout.result
@@ -117,9 +117,9 @@ s:select({}) -- [1]
 fiber.sleep(txn_timeout + 0.1)
 ---
 ...
-s:select({}) --[]
+s:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 s:replace({2})
 ---
@@ -128,9 +128,9 @@ s:replace({2})
 fiber.yield()
 ---
 ...
-s:select({}) -- []
+s:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 box.commit() -- Transaction has been aborted by timeout
 ---
@@ -152,9 +152,9 @@ s:select({}) -- [1]
 fiber.sleep(txn_timeout  / 2 + 0.1)
 ---
 ...
-s:select({}) --[]
+s:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 s:replace({2})
 ---
@@ -163,9 +163,9 @@ s:replace({2})
 fiber.yield()
 ---
 ...
-s:select({}) -- []
+s:select({})
 ---
-- []
+- error: Transaction has been aborted by timeout
 ...
 box.commit() -- Transaction has been aborted by timeout
 ---
diff --git a/test/box/tx_timeout.test.lua b/test/box/tx_timeout.test.lua
index cbd6bc4de6..83a98aed25 100644
--- a/test/box/tx_timeout.test.lua
+++ b/test/box/tx_timeout.test.lua
@@ -46,10 +46,10 @@ box.begin()
 s:replace({1})
 s:select({}) -- [1]
 fiber.sleep(txn_timeout + 0.1)
-s:select({}) --[]
+s:select({})
 s:replace({2})
 fiber.yield()
-s:select({}) -- []
+s:select({})
 box.commit() -- Transaction has been aborted by timeout
 
 -- Check that transaction aborted by timeout, which
@@ -58,10 +58,10 @@ box.begin({timeout = txn_timeout})
 s:replace({1})
 s:select({}) -- [1]
 fiber.sleep(txn_timeout  / 2 + 0.1)
-s:select({}) --[]
+s:select({})
 s:replace({2})
 fiber.yield()
-s:select({}) -- []
+s:select({})
 box.commit() -- Transaction has been aborted by timeout
 
 -- Check that transaction is not rollback until timeout expired.
diff --git a/test/sql/triggers.result b/test/sql/triggers.result
index ceecb8ef2f..0a98f08bf2 100644
--- a/test/sql/triggers.result
+++ b/test/sql/triggers.result
@@ -395,7 +395,7 @@ box.execute("CREATE TABLE test2 (id INT PRIMARY KEY)")
 ---
 - row_count: 1
 ...
-box.execute("INSERT INTO test2 VALUES (2)")
+box.execute("INSERT INTO test VALUES (2)")
 ---
 - row_count: 1
 ...
@@ -403,11 +403,11 @@ box.execute("START TRANSACTION")
 ---
 - row_count: 0
 ...
-box.execute("INSERT INTO test VALUES (1)")
+box.execute("INSERT INTO test2 VALUES (1)")
 ---
 - row_count: 1
 ...
-box.execute("SELECT * FROM test2")
+box.execute("SELECT * FROM test")
 ---
 - null
 - A multi-statement transaction can not use multiple storage engines
diff --git a/test/sql/triggers.test.lua b/test/sql/triggers.test.lua
index f5c8a3961d..6d0960a3ba 100644
--- a/test/sql/triggers.test.lua
+++ b/test/sql/triggers.test.lua
@@ -135,10 +135,10 @@ box.space._session_settings:update('sql_default_engine', {{'=', 2, 'memtx'}})
 box.execute("CREATE TABLE test (id INT PRIMARY KEY)")
 box.space._session_settings:update('sql_default_engine', {{'=', 2, 'vinyl'}})
 box.execute("CREATE TABLE test2 (id INT PRIMARY KEY)")
-box.execute("INSERT INTO test2 VALUES (2)")
+box.execute("INSERT INTO test VALUES (2)")
 box.execute("START TRANSACTION")
-box.execute("INSERT INTO test VALUES (1)")
-box.execute("SELECT * FROM test2")
+box.execute("INSERT INTO test2 VALUES (1)")
+box.execute("SELECT * FROM test")
 box.execute("ROLLBACK;")
 box.execute("DROP TABLE test;")
 box.execute("DROP TABLE test2;")
-- 
GitLab