Merge pull request ClickHouse#107583 from groeneai/fix-timeseries-rename-digest-mismatch

Support renaming a TimeSeries table and fix Digest does not match crash in a Replicated database

Author: vitlibar

src/Databases/DatabaseAtomic.cpp

@@ -13,6 +13,7 @@
 #include <Interpreters/ExternalDictionariesLoader.h>
 #include <Interpreters/Context.h>
 #include <Storages/StorageMaterializedView.h>
+#include <Storages/StorageTimeSeries.h>
 #include <base/isSharedPtrUnique.h>
 #include <Common/PoolId.h>
 #include <Common/ZooKeeper/ZooKeeperCommon.h>
@@ -306,6 +307,9 @@ void DatabaseAtomic::renameTable(ContextPtr local_context, const String & table_
         if (const auto * mv = dynamic_cast<const StorageMaterializedView *>(table_.get()))
             if (mv->hasInnerTable())
                 throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Cannot move MaterializedView with inner table to other database");
+        if (const auto * ts = dynamic_cast<const StorageTimeSeries *>(table_.get()))
+            if (ts->hasInnerTables())
+                throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Cannot move TimeSeries table with inner tables to other database");
     };
 
     String table_data_path;

src/Databases/DatabaseOrdinary.cpp

@@ -427,6 +427,11 @@ bool DatabaseOrdinary::shouldLazyLoad(const ASTCreateQuery & query, LoadingStric
         || query.isParameterizedView() || query.is_window_view)
         return false;
 
+    /// A lazy proxy would hide the TimeSeries type from the cross-database rename guard, so its
+    /// inner tables could be orphaned by a cross-database move. Load it eagerly, as for views.
+    if (query.is_time_series_table)
+        return false;
+
     /// Already handled by `StorageTableFunctionProxy`.
     if (query.as_table_function)
         return false;

src/Storages/StorageTimeSeries.cpp

@@ -8,9 +8,11 @@
 #include <Interpreters/DatabaseCatalog.h>
 #include <Interpreters/InterpreterCreateQuery.h>
 #include <Interpreters/InterpreterDropQuery.h>
+#include <Interpreters/InterpreterRenameQuery.h>
 #include <Parsers/ASTDropQuery.h>
 #include <Parsers/ASTCreateQuery.h>
 #include <Parsers/ASTInsertQuery.h>
+#include <Parsers/ASTRenameQuery.h>
 #include <Backups/BackupEntriesCollector.h>
 #include <Backups/IBackup.h>
 #include <Backups/RestorerFromBackup.h>
@@ -39,6 +41,7 @@ namespace ErrorCodes
     extern const int LOGICAL_ERROR;
     extern const int NOT_IMPLEMENTED;
     extern const int SUPPORT_IS_DISABLED;
+    extern const int TABLE_ALREADY_EXISTS;
     extern const int UNEXPECTED_TABLE_ENGINE;
     extern const int UNKNOWN_TABLE;
 }
@@ -482,9 +485,52 @@ void StorageTimeSeries::alter(const AlterCommands & params, ContextPtr local_con
 }
 
 
-void StorageTimeSeries::renameInMemory(const StorageID & /* new_table_id */)
+void StorageTimeSeries::renameInMemory(const StorageID & new_table_id)
 {
-    throw Exception(ErrorCodes::NOT_IMPLEMENTED, "Renaming is not supported by storage {} yet", getName());
+    auto old_table_id = getStorageID();
+
+    /// In an Atomic/Replicated database both ids carry a UUID; inner tables are addressed by the
+    /// outer table's UUID (the `.inner_id.<kind>.<uuid>` name), which is preserved by the rename, so
+    /// only the outer table id changes. In an Ordinary database the inner table names embed the old
+    /// outer table name, so each inner table has to be renamed too (same as StorageMaterializedView).
+    bool from_atomic_to_atomic_database = old_table_id.hasUUID() && new_table_id.hasUUID();
+
+    if (!from_atomic_to_atomic_database && hasInnerTables())
+    {
+        /// Collect every inner table rename first so that all destination names can be checked
+        /// before any rename is executed. The inner renames are not transactional, so renaming
+        /// them one by one would leave the table half-renamed (some inner tables moved, the rest
+        /// not) if a later destination name happened to be occupied.
+        std::vector<std::pair<StorageID, String>> inner_renames;
+        for (auto target_kind : getTargetKinds())
+        {
+            if (!isInnerTable(target_kind))
+                continue;
+
+            auto inner_table = tryGetTargetTable(target_kind, getContext());
+            if (!inner_table)
+                continue;
+
+            auto inner_table_id = inner_table->getStorageID();
+            auto new_inner_table_name = getTimeSeriesInnerTableName(target_kind, new_table_id);
+
+            if (DatabaseCatalog::instance().isTableExist(StorageID{new_table_id.database_name, new_inner_table_name}, getContext()))
+                throw Exception(ErrorCodes::TABLE_ALREADY_EXISTS, "Table {} already exists",
+                                StorageID{new_table_id.database_name, new_inner_table_name}.getNameForLogs());
+
+            inner_renames.emplace_back(std::move(inner_table_id), std::move(new_inner_table_name));
+        }
+
+        for (const auto & [inner_table_id, new_inner_table_name] : inner_renames)
+        {
+            auto rename = make_intrusive<ASTRenameQuery>();
+            rename->addElement(inner_table_id.database_name, inner_table_id.table_name,
+                               new_table_id.database_name, new_inner_table_name);
+            InterpreterRenameQuery(rename, getContext()).execute();
+        }
+    }
+
+    IStorage::renameInMemory(new_table_id);
 }
 
 

tests/queries/0_stateless/04340_timeseries_rename_digest_mismatch.reference

@@ -0,0 +1,16 @@
+0
+1
+0
+1
+1
+1
+1
+rejected
+1
+1
+1
+1
+rejected
+3
+0
+1

tests/queries/0_stateless/04340_timeseries_rename_digest_mismatch.sh

@@ -0,0 +1,98 @@
+#!/usr/bin/env bash
+# Tags: zookeeper, no-parallel, no-replicated-database
+# Tag no-parallel: enables a REGULAR failpoint that affects the whole server process.
+# Tag no-replicated-database: the test creates its own Replicated database and the
+#                             failpoint is enabled only on one server node.
+
+CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CURDIR"/../shell_config.sh
+
+DB="${CLICKHOUSE_DATABASE}_repl"
+ZK_PATH="/test/timeseries_rename_digest/${CLICKHOUSE_TEST_UNIQUE_NAME}"
+
+${CLICKHOUSE_CLIENT} -q "
+    CREATE DATABASE ${DB}
+    ENGINE = Replicated('${ZK_PATH}', 's1', 'r1')
+"
+
+${CLICKHOUSE_CLIENT} --distributed_ddl_output_mode=none --allow_experimental_time_series_table=1 \
+    -q "CREATE TABLE ${DB}.ts ENGINE = TimeSeries"
+
+# Force the metadata digest assertion to always run (skip the 1/16 probability gate), so any
+# divergence between the in-memory tables map and ZooKeeper is caught immediately. In release
+# builds assertDigestWithProbability is compiled out, so this is a no-op there.
+${CLICKHOUSE_CLIENT} -q "SYSTEM ENABLE FAILPOINT database_replicated_force_metadata_digest_check"
+
+# RENAME TABLE must succeed and the table must keep working. Before the fix it was rejected from
+# renameInMemory() only after DatabaseAtomic::renameTable() had detached the table and committed
+# the ZooKeeper transaction, leaving it counted in tables_metadata_digest but missing locally, so
+# the next forced digest assertion aborted the server with "Digest does not match".
+${CLICKHOUSE_CLIENT} --distributed_ddl_output_mode=none -q "RENAME TABLE ${DB}.ts TO ${DB}.ts2"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${DB}.ts"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${DB}.ts2"
+
+# RENAME DATABASE must succeed too (it renames every contained table in place).
+${CLICKHOUSE_CLIENT} --distributed_ddl_output_mode=none -q "RENAME DATABASE ${DB} TO ${DB}_renamed"
+${CLICKHOUSE_CLIENT} -q "EXISTS DATABASE ${DB}"
+${CLICKHOUSE_CLIENT} -q "EXISTS DATABASE ${DB}_renamed"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${DB}_renamed.ts2"
+
+# CREATE OR REPLACE creates a temporary table and renames it onto the target name; that internal
+# rename is the path the original report tripped over. With the target absent it must create the
+# table cleanly.
+${CLICKHOUSE_CLIENT} --distributed_ddl_output_mode=none --allow_experimental_time_series_table=1 \
+    -q "CREATE OR REPLACE TABLE ${DB}_renamed.cor ENGINE = TimeSeries"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${DB}_renamed.cor"
+
+# With the forced digest check still on, an unrelated DDL must SUCCEED, proving the digest stayed
+# consistent through every rename above. Run it without grep so a crash, lost connection, or any
+# other failure fails the test instead of being masked.
+${CLICKHOUSE_CLIENT} --distributed_ddl_output_mode=none \
+    -q "CREATE TABLE ${DB}_renamed.probe (id UInt64) ENGINE = ReplicatedMergeTree ORDER BY id"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${DB}_renamed.probe"
+
+${CLICKHOUSE_CLIENT} -q "SYSTEM DISABLE FAILPOINT database_replicated_force_metadata_digest_check"
+
+${CLICKHOUSE_CLIENT} -q "DROP DATABASE ${DB}_renamed SYNC" 2>/dev/null || true
+
+# In a non-UUID (Ordinary) database the inner tables embed the outer table name, so renaming a
+# TimeSeries table renames each inner table too. Those inner renames are not transactional, so if
+# one destination name is already taken the rename must be rejected up front, before any inner
+# table is moved -- otherwise the table would be left half-renamed with an orphaned inner table.
+ORD="${CLICKHOUSE_DATABASE}_ord"
+${CLICKHOUSE_CLIENT} -q "DROP DATABASE IF EXISTS ${ORD} SYNC"
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal --allow_deprecated_database_ordinary=1 -q "CREATE DATABASE ${ORD} ENGINE = Ordinary"
+${CLICKHOUSE_CLIENT} --allow_experimental_time_series_table=1 -q "CREATE TABLE ${ORD}.ts ENGINE = TimeSeries"
+# Occupy one of the destination inner-table names so the rename cannot complete.
+${CLICKHOUSE_CLIENT} -q "CREATE TABLE ${ORD}.\`.inner.tags.ts2\` (x UInt8) ENGINE = MergeTree ORDER BY x"
+# The rename is rejected before any inner table is moved.
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "RENAME TABLE ${ORD}.ts TO ${ORD}.ts2" 2>&1 | grep -q -F "TABLE_ALREADY_EXISTS" && echo "rejected"
+# Every source inner table is still present (none orphaned), so the table keeps working.
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${ORD}.\`.inner.samples.ts\`"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${ORD}.\`.inner.tags.ts\`"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${ORD}.\`.inner.metrics.ts\`"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${ORD}.ts"
+
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "DROP DATABASE ${ORD} SYNC" 2>/dev/null || true
+
+# With lazy_load_tables=1 a reloaded TimeSeries table is materialized as a StorageTableProxy. The
+# proxy must not hide the TimeSeries type from the cross-database move guard -- otherwise a
+# cross-database RENAME would move only the outer table and orphan its inner tables in the old
+# database. The outer TimeSeries table is loaded eagerly so the guard sees it.
+LZ="${CLICKHOUSE_DATABASE}_lazy"
+LZ2="${CLICKHOUSE_DATABASE}_lazy2"
+${CLICKHOUSE_CLIENT} -q "DROP DATABASE IF EXISTS ${LZ} SYNC; DROP DATABASE IF EXISTS ${LZ2} SYNC"
+${CLICKHOUSE_CLIENT} -q "CREATE DATABASE ${LZ} ENGINE = Atomic SETTINGS lazy_load_tables = 1"
+${CLICKHOUSE_CLIENT} -q "CREATE DATABASE ${LZ2} ENGINE = Atomic"
+${CLICKHOUSE_CLIENT} --allow_experimental_time_series_table=1 -q "CREATE TABLE ${LZ}.ts ENGINE = TimeSeries"
+# Reattach so the table is recreated as a lazy proxy.
+${CLICKHOUSE_CLIENT} -q "DETACH DATABASE ${LZ}; ATTACH DATABASE ${LZ}"
+# The cross-database move is rejected before any inner table is moved.
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "RENAME TABLE ${LZ}.ts TO ${LZ2}.ts" 2>&1 | grep -q -F "Cannot move TimeSeries table with inner tables" && echo "rejected"
+# Every inner table stayed in the source database (none orphaned in the target).
+${CLICKHOUSE_CLIENT} -q "SELECT count() FROM system.tables WHERE database = '${LZ}' AND name LIKE '.inner_id.%'"
+${CLICKHOUSE_CLIENT} -q "SELECT count() FROM system.tables WHERE database = '${LZ2}' AND name LIKE '.inner_id.%'"
+${CLICKHOUSE_CLIENT} -q "EXISTS TABLE ${LZ}.ts"
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "DROP DATABASE ${LZ} SYNC" 2>/dev/null || true
+${CLICKHOUSE_CLIENT} --send_logs_level=fatal -q "DROP DATABASE ${LZ2} SYNC" 2>/dev/null || true