[PATCH 4/5] memtx: fix txn_on_yield for DDL transactions
Vladimir Davydov
vdavydov.dev at gmail.com
Fri Jul 5 23:25:30 MSK 2019
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
More information about the Tarantool-patches
mailing list