Merge pull request #6756 from yandex/add-test-deadlock-rename-table

Added a test for deadlock in RENAME TABLE

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<UniqueTableName, TableStructureWriteLockHolder> 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<UniqueTableName, std::unique_ptr<DDLGuard>> 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 {};

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=10
+
+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"