From d879bcb010fdf27f5d467e2f658daeed2fd67b1e Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 05:32:42 +0300 Subject: [PATCH 1/3] Added a test for deadlock in RENAME TABLE --- .../01004_rename_deadlock.reference | 0 .../0_stateless/01004_rename_deadlock.sh | 60 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/01004_rename_deadlock.reference create mode 100755 dbms/tests/queries/0_stateless/01004_rename_deadlock.sh diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.reference b/dbms/tests/queries/0_stateless/01004_rename_deadlock.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh new file mode 100755 index 00000000000..b09fabe3d9e --- /dev/null +++ b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test1"; +$CLICKHOUSE_CLIENT --query "DROP TABLE IF EXISTS test2"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test1 (x UInt8) ENGINE = MergeTree ORDER BY x"; +$CLICKHOUSE_CLIENT --query "CREATE TABLE test2 (x UInt8) ENGINE = MergeTree ORDER BY x"; + +function thread1() +{ + while true; do + $CLICKHOUSE_CLIENT --query "RENAME TABLE test1 TO test_tmp, test2 TO test1, test_tmp TO test2" + done +} + +function thread2() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT * FROM test1 UNION ALL SELECT * FROM test2" --format Null + done +} + +function thread3() +{ + while true; do + $CLICKHOUSE_CLIENT --query "SELECT * FROM system.tables" --format Null + done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; +export -f thread3; + +TIMEOUT=1000 + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +timeout $TIMEOUT bash -c thread1 2> /dev/null & +timeout $TIMEOUT bash -c thread2 2> /dev/null & +timeout $TIMEOUT bash -c thread3 2> /dev/null & + +wait + +$CLICKHOUSE_CLIENT -q "DROP TABLE test1" +$CLICKHOUSE_CLIENT -q "DROP TABLE test2" From 202673e3bd40c57dd738bb4a2adf19f973bd53e7 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 13:37:28 +0300 Subject: [PATCH 2/3] Avoid deadlock in multiple tables RENAME --- .../Interpreters/InterpreterRenameQuery.cpp | 38 ++++++------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/dbms/src/Interpreters/InterpreterRenameQuery.cpp b/dbms/src/Interpreters/InterpreterRenameQuery.cpp index e763c002209..3e8bc3a88fb 100644 --- a/dbms/src/Interpreters/InterpreterRenameQuery.cpp +++ b/dbms/src/Interpreters/InterpreterRenameQuery.cpp @@ -26,8 +26,6 @@ struct RenameDescription to_table_name(elem.to.table) {} - TableStructureWriteLockHolder from_table_lock; - String from_database_name; String from_table_name; @@ -77,8 +75,6 @@ BlockIO InterpreterRenameQuery::execute() } }; - std::map tables_from_locks; - /// Don't allow to drop tables (that we are renaming); don't allow to create tables in places where tables will be renamed. std::map> table_guards; @@ -89,36 +85,26 @@ BlockIO InterpreterRenameQuery::execute() UniqueTableName from(descriptions.back().from_database_name, descriptions.back().from_table_name); UniqueTableName to(descriptions.back().to_database_name, descriptions.back().to_table_name); - if (!tables_from_locks.count(from)) - if (auto table = context.tryGetTable(from.database_name, from.table_name)) - tables_from_locks.emplace(from, table->lockExclusively(context.getCurrentQueryId())); - - descriptions.back().from_table_lock = tables_from_locks[from]; - - if (!table_guards.count(from)) - table_guards.emplace(from, context.getDDLGuard(from.database_name, from.table_name)); - - if (!table_guards.count(to)) - table_guards.emplace(to, context.getDDLGuard(to.database_name, to.table_name)); + table_guards[from]; + table_guards[to]; } - /** All tables are locked. If there are more than one rename in chain, - * we need to hold global lock while doing all renames. Order matters to avoid deadlocks. - * It provides atomicity of all RENAME chain as a whole, from the point of view of DBMS client, - * but only in cases when there was no exceptions during this process and server does not fall. - */ - - decltype(context.getLock()) lock; - - if (descriptions.size() > 1) - lock = context.getLock(); + /// Must do it in consistent order. + for (auto & table_guard : table_guards) + table_guard.second = context.getDDLGuard(table_guard.first.database_name, table_guard.first.table_name); for (auto & elem : descriptions) { context.assertTableDoesntExist(elem.to_database_name, elem.to_table_name); + auto from_table = context.getTable(elem.from_database_name, elem.from_table_name); + auto from_table_lock = from_table->lockExclusively(context.getCurrentQueryId()); context.getDatabase(elem.from_database_name)->renameTable( - context, elem.from_table_name, *context.getDatabase(elem.to_database_name), elem.to_table_name, elem.from_table_lock); + context, + elem.from_table_name, + *context.getDatabase(elem.to_database_name), + elem.to_table_name, + from_table_lock); } return {}; From 3568d3d890c6d3e7f657e397b6882d7aad1985ba Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sat, 31 Aug 2019 13:38:20 +0300 Subject: [PATCH 3/3] Updated test --- dbms/tests/queries/0_stateless/01004_rename_deadlock.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh index b09fabe3d9e..5d5726bb001 100755 --- a/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh +++ b/dbms/tests/queries/0_stateless/01004_rename_deadlock.sh @@ -36,7 +36,7 @@ export -f thread1; export -f thread2; export -f thread3; -TIMEOUT=1000 +TIMEOUT=10 timeout $TIMEOUT bash -c thread1 2> /dev/null & timeout $TIMEOUT bash -c thread2 2> /dev/null &