From c701253403889fddcec70eebfd315767d210b6ba Mon Sep 17 00:00:00 2001
From: Georgy Kirichenko <georgy@tarantool.org>
Date: Thu, 1 Mar 2018 17:02:17 +0300
Subject: [PATCH] Don't try to lock a ddl latch in a multistatement tx
Any ddl is prohibited in a multistatement transaction, there is no
reason to try to lock a ddl latch in tis case. Locking for already
locked latch will cause an yield and a silent transaction rollback, and
this will crash or assert tarantool server.
Fixes #2783
---
src/box/alter.cc | 9 +++++++
test/box/ddl.result | 47 +++++++++++++++++++++++++++++++++++++
test/box/ddl.test.lua | 28 ++++++++++++++++++++++
test/box/on_replace.result | 2 +-
test/box/transaction.result | 2 +-
test/engine/truncate.result | 2 +-
6 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 0e611400c1..5dec2519da 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3153,6 +3153,15 @@ lock_before_dd(struct trigger *trigger, void *event)
if (fiber() == latch_owner(&schema_lock))
return;
struct txn *txn = (struct txn *)event;
+ /*
+ * This trigger is executed before any check and may yield
+ * on the latch lock. But a yield in a non-autocommit
+ * memtx transaction will roll it back silently, rather
+ * than produce an error, which is very confusing.
+ * So don't try to lock a latch if there is
+ * a multistatement transaction.
+ */
+ txn_check_singlestatement_xc(txn, "DDL");
latch_lock(&schema_lock);
struct trigger *on_commit =
txn_alter_trigger_new(unlock_after_dd, NULL);
diff --git a/test/box/ddl.result b/test/box/ddl.result
index 0eef379928..f6991840f7 100644
--- a/test/box/ddl.result
+++ b/test/box/ddl.result
@@ -514,3 +514,50 @@ s:format()[3].custom_field
s:drop()
---
...
+--
+-- gh-2783
+-- A ddl operation shoud fail before trying to lock a ddl latch
+-- in a multi-statement transaction.
+-- If operation tries to lock already an locked latch then the
+-- current transaction will be silently rolled back under our feet.
+-- This is confusing. So check for multi-statement transaction
+-- before locking the latch.
+--
+test_latch = box.schema.space.create('test_latch')
+---
+...
+_ = test_latch:create_index('primary', {unique = true, parts = {1, 'unsigned'}})
+---
+...
+fiber = require('fiber')
+---
+...
+c = fiber.channel(1)
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+_ = fiber.create(function()
+ test_latch:create_index("sec", {unique = true, parts = {2, 'unsigned'}})
+ c:put(true)
+end);
+---
+...
+box.begin()
+test_latch:create_index("sec2", {unique = true, parts = {2, 'unsigned'}})
+box.commit();
+---
+- error: DDL does not support multi-statement transactions
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+_ = c:get()
+---
+...
+test_latch:drop() -- this is where everything stops
+---
+...
diff --git a/test/box/ddl.test.lua b/test/box/ddl.test.lua
index 820fe7d4dc..6184ccb1a3 100644
--- a/test/box/ddl.test.lua
+++ b/test/box/ddl.test.lua
@@ -198,3 +198,31 @@ format[3] = {'field3', 'unsigned', custom_field = 'custom_value'}
s = box.schema.create_space('test', {format = format})
s:format()[3].custom_field
s:drop()
+
+--
+-- gh-2783
+-- A ddl operation shoud fail before trying to lock a ddl latch
+-- in a multi-statement transaction.
+-- If operation tries to lock already an locked latch then the
+-- current transaction will be silently rolled back under our feet.
+-- This is confusing. So check for multi-statement transaction
+-- before locking the latch.
+--
+test_latch = box.schema.space.create('test_latch')
+_ = test_latch:create_index('primary', {unique = true, parts = {1, 'unsigned'}})
+fiber = require('fiber')
+c = fiber.channel(1)
+test_run:cmd("setopt delimiter ';'")
+_ = fiber.create(function()
+ test_latch:create_index("sec", {unique = true, parts = {2, 'unsigned'}})
+ c:put(true)
+end);
+
+box.begin()
+test_latch:create_index("sec2", {unique = true, parts = {2, 'unsigned'}})
+box.commit();
+
+test_run:cmd("setopt delimiter ''");
+
+_ = c:get()
+test_latch:drop() -- this is where everything stops
diff --git a/test/box/on_replace.result b/test/box/on_replace.result
index d6158dc5a1..26d074a946 100644
--- a/test/box/on_replace.result
+++ b/test/box/on_replace.result
@@ -492,7 +492,7 @@ t = s:on_replace(function () s:drop() end, t)
...
s:replace({5, 6})
---
-- error: Space _index does not support multi-statement transactions
+- error: DDL does not support multi-statement transactions
...
t = s:on_replace(function () box.schema.func.create('newf') end, t)
---
diff --git a/test/box/transaction.result b/test/box/transaction.result
index 2a4b3b289b..f60846e75a 100644
--- a/test/box/transaction.result
+++ b/test/box/transaction.result
@@ -107,7 +107,7 @@ s = box.schema.space.create('test');
...
box.begin() index = s:create_index('primary');
---
-- error: Space _index does not support multi-statement transactions
+- error: DDL does not support multi-statement transactions
...
box.rollback();
---
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index b6e1a99a28..7f294031ff 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -24,7 +24,7 @@ box.begin()
...
s:truncate()
---
-- error: Space _truncate does not support multi-statement transactions
+- error: DDL does not support multi-statement transactions
...
box.commit()
---
--
GitLab