Merge pull request ClickHouse#78741 from azat/rdb-digest-fix-2

Fix Digest does not match (from Replicated database) on CI

Author: azat

src/Databases/DatabaseReplicated.cpp

@@ -810,35 +810,12 @@ void DatabaseReplicated::stopLoading()
     DatabaseAtomic::stopLoading();
 }
 
-void DatabaseReplicated::dumpLocalTablesForDebugOnly(const ContextPtr & local_context) const
+ASTPtr DatabaseReplicated::tryGetCreateOrAttachTableQuery(const String & name, ContextPtr local_context) const
 {
-    auto table_names = getAllTableNames(context.lock());
-    for (const auto & table_name : table_names)
-    {
-        auto ast_ptr = tryGetCreateTableQuery(table_name, local_context);
-        if (ast_ptr)
-            LOG_DEBUG(log, "[local] Table {} create query is {}", table_name, ast_ptr->formatForLogging());
-        else
-            LOG_DEBUG(log, "[local] Table {} has no create query", table_name);
-    }
-}
-
-void DatabaseReplicated::dumpTablesInZooKeeperForDebugOnly() const
-{
-    UInt32 max_log_ptr{};
-    auto table_name_to_metadata = tryGetConsistentMetadataSnapshot(getZooKeeper(), max_log_ptr);
-    for (const auto & [table_name, create_table_query] : table_name_to_metadata)
-    {
-        auto query_ast = parseQueryFromMetadataInZooKeeper(table_name, create_table_query);
-        if (query_ast)
-        {
-            LOG_DEBUG(log, "[zookeeper] Table {} create query is {}", table_name, query_ast->formatForLogging());
-        }
-        else
-        {
-            LOG_DEBUG(log, "[zookeeper] Table {} has no create query", table_name);
-        }
-    }
+    auto res = tryGetCreateTableQuery(name, local_context);
+    auto & create = res->as<ASTCreateQuery &>();
+    create.attach = create.is_materialized_view_with_inner_table();
+    return res;
 }
 
 void DatabaseReplicated::tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnly(const ContextPtr & local_context) const
@@ -848,57 +825,58 @@ void DatabaseReplicated::tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnl
     auto table_names_local = getAllTableNames(local_context);
 
     if (table_name_to_metadata_in_zk.size() != table_names_local.size())
-        LOG_DEBUG(log, "Amount of tables in zk {} locally {}", table_name_to_metadata_in_zk.size(), table_names_local.size());
+    {
+        LOG_ERROR(log, "Amount of tables in coordinator {} differs from number of tables locally {}",
+            table_name_to_metadata_in_zk.size(), table_names_local.size());
+    }
 
     std::unordered_set<std::string> checked_tables;
 
     for (const auto & table_name : table_names_local)
     {
-        auto local_ast_ptr = tryGetCreateTableQuery(table_name, local_context);
-        if (table_name_to_metadata_in_zk.contains(table_name))
-        {
-            checked_tables.insert(table_name);
-            auto create_table_query_in_zk = table_name_to_metadata_in_zk[table_name];
-            auto zk_ast_ptr = parseQueryFromMetadataInZooKeeper(table_name, create_table_query_in_zk);
+        checked_tables.insert(table_name);
 
-            if (local_ast_ptr == nullptr && zk_ast_ptr == nullptr)
-            {
-                LOG_DEBUG(log, "AST for table {} is the same (nullptr) in local and ZK", table_name);
-            }
-            else if (local_ast_ptr != nullptr && zk_ast_ptr != nullptr && local_ast_ptr->formatWithSecretsOneLine() != zk_ast_ptr->formatWithSecretsOneLine())
-            {
-                LOG_ERROR(log, "AST differs for table {}, local {}, in zookeeper {}", table_name, local_ast_ptr->formatForLogging(), zk_ast_ptr->formatForLogging());
-            }
-            else if (local_ast_ptr == nullptr)
-            {
-                LOG_ERROR(log, "AST differs for table {}, local nullptr, in zookeeper {}", table_name, zk_ast_ptr->formatForLogging());
-            }
-            else if (zk_ast_ptr == nullptr)
-            {
-                LOG_ERROR(log, "AST differs for table {}, local {}, in zookeeper nullptr", table_name, local_ast_ptr->formatForLogging());
-            }
-            else
-            {
-                LOG_DEBUG(log, "AST for table {} is the same in local and ZK", table_name);
-            }
+        auto local_ast_ptr = tryGetCreateOrAttachTableQuery(table_name, local_context);
+        auto zk_metadata_it = table_name_to_metadata_in_zk.find(table_name);
+
+        auto on_disk_ast_ptr = parseQueryFromMetadataOnDisk(table_name);
+
+        ASTPtr zk_ast_ptr;
+        if (zk_metadata_it != table_name_to_metadata_in_zk.end())
+            zk_ast_ptr = parseQueryFromMetadataInZooKeeper(table_name, zk_metadata_it->second);
+
+        auto local_query_with_secrets = local_ast_ptr ? local_ast_ptr->formatWithSecretsOneLine() : "";
+        auto zookeeper_query_with_secrets = zk_ast_ptr ? zk_ast_ptr->formatWithSecretsOneLine() : "";
+        auto on_disk_query_with_secrets = on_disk_ast_ptr ? on_disk_ast_ptr->formatWithSecretsOneLine() : "";
+
+        if (local_query_with_secrets != zookeeper_query_with_secrets || local_query_with_secrets != on_disk_query_with_secrets)
+        {
+            /// NOTE: due to transaction will be committed **before**
+            /// tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnly()
+            /// runs, you will almost never enter this code path, since it will
+            /// update on disk metadata. But the checkDigestValid() will still
+            /// throw LOGICAL_ERROR since database relies on the on-disk data
+            /// (for tracking tables_metadata_digest)
+            LOG_ERROR(log, "AST differs for table {}", table_name);
+            LOG_ERROR(log, "\t  in memory: {}", local_ast_ptr ? local_ast_ptr->formatForLogging() : "nullptr");
+            LOG_ERROR(log, "\tcoordinator: {}", zk_ast_ptr ? zk_ast_ptr->formatForLogging() : "nullptr");
+            LOG_ERROR(log, "\t    on disk: {}", on_disk_ast_ptr ? on_disk_ast_ptr->formatForLogging() : "nullptr");
         }
         else
         {
-            if (local_ast_ptr == nullptr)
-                LOG_ERROR(log, "Table {} exists locally, but missing in ZK", table_name);
-            else
-                LOG_ERROR(log, "Table {} exists locally with AST {}, but missing in ZK", table_name, local_ast_ptr->formatForLogging());
+            LOG_DEBUG(log, "AST for table {} is the same", table_name);
+            LOG_DEBUG(log, "\t  in memory: {}", local_ast_ptr ? local_ast_ptr->formatForLogging() : "nullptr");
+            LOG_DEBUG(log, "\tcoordinator: {}", zk_ast_ptr ? zk_ast_ptr->formatForLogging() : "nullptr");
+            LOG_DEBUG(log, "\t    on disk: {}", on_disk_ast_ptr ? on_disk_ast_ptr->formatForLogging() : "nullptr");
         }
     }
     for (const auto & [table_name, table_metadata] : table_name_to_metadata_in_zk)
     {
         if (!checked_tables.contains(table_name))
         {
             auto zk_ast_ptr = parseQueryFromMetadataInZooKeeper(table_name, table_metadata);
-            if (zk_ast_ptr == nullptr)
-                LOG_ERROR(log, "Table {} exists in ZK with AST {}, but missing locally", table_name, zk_ast_ptr->formatForLogging());
-            else
-                LOG_ERROR(log, "Table {} exists in ZK, but missing locally", table_name);
+            auto zookeeper_query = zk_ast_ptr ? zk_ast_ptr->formatForLogging() : "nullptr";
+            LOG_ERROR(log, "Table {} exists in ZK, but missing locally: {}", table_name, zookeeper_query);
         }
     }
 }
@@ -1037,7 +1015,7 @@ bool DatabaseReplicated::checkDigestValid(const ContextPtr & local_context) cons
     UInt64 local_digest = 0;
     {
         std::lock_guard lock{mutex};
-        for (const auto & table : TSA_SUPPRESS_WARNING_FOR_READ(tables))
+        for (const auto & table : tables)
             local_digest += getMetadataHash(table.first);
     }
 
@@ -1046,8 +1024,6 @@ bool DatabaseReplicated::checkDigestValid(const ContextPtr & local_context) cons
         LOG_ERROR(log, "Digest of local metadata ({}) is not equal to in-memory digest ({})", local_digest, tables_metadata_digest);
 
 #ifndef NDEBUG
-        dumpLocalTablesForDebugOnly(local_context);
-        dumpTablesInZooKeeperForDebugOnly();
         tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnly(local_context);
 #endif
 
@@ -1068,8 +1044,6 @@ bool DatabaseReplicated::checkDigestValid(const ContextPtr & local_context) cons
     {
         LOG_ERROR(log, "Digest of local metadata ({}) is not equal to digest in Keeper ({})", local_digest_str, zk_digest);
 #ifndef NDEBUG
-        dumpLocalTablesForDebugOnly(local_context);
-        dumpTablesInZooKeeperForDebugOnly();
         tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnly(local_context);
 #endif
         return false;
@@ -1601,10 +1575,9 @@ std::map<String, String> DatabaseReplicated::getConsistentMetadataSnapshotImpl(
     return table_name_to_metadata;
 }
 
-ASTPtr DatabaseReplicated::parseQueryFromMetadataInZooKeeper(const String & node_name, const String & query) const
+ASTPtr DatabaseReplicated::parseQueryFromMetadata(const String & table_name, const String & query, const String & description) const
 {
     ParserCreateQuery parser;
-    String description = "in ZooKeeper " + zookeeper_path + "/metadata/" + node_name;
     auto ast = parseQuery(
         parser,
         query,
@@ -1615,14 +1588,26 @@ ASTPtr DatabaseReplicated::parseQueryFromMetadataInZooKeeper(const String & node
 
     auto & create = ast->as<ASTCreateQuery &>();
     if (create.uuid == UUIDHelpers::Nil || create.getTable() != TABLE_WITH_UUID_NAME_PLACEHOLDER || create.database)
-        throw Exception(ErrorCodes::LOGICAL_ERROR, "Got unexpected query from {}: {}", node_name, query);
+        throw Exception(ErrorCodes::LOGICAL_ERROR, "Got unexpected query from {}: {}", table_name, query);
 
     create.setDatabase(getDatabaseName());
-    create.setTable(unescapeForFileName(node_name));
+    create.setTable(unescapeForFileName(table_name));
     create.attach = create.is_materialized_view_with_inner_table();
 
     return ast;
 }
+ASTPtr DatabaseReplicated::parseQueryFromMetadataInZooKeeper(const String & node_name, const String & query) const
+{
+    String description = fmt::format("in ZooKeeper {}/metadata/{}", zookeeper_path, node_name);
+    return parseQueryFromMetadata(node_name, query, description);
+}
+ASTPtr DatabaseReplicated::parseQueryFromMetadataOnDisk(const String & table_name) const
+{
+    auto file_path = getObjectMetadataPath(table_name);
+    String description = fmt::format("in metadata {}", file_path);
+    String query = DB::readMetadataFile(db_disk, file_path);
+    return parseQueryFromMetadata(table_name, query, description);
+}
 
 void DatabaseReplicated::dropReplica(
     DatabaseReplicated * database, const String & database_zookeeper_path, const String & shard, const String & replica, bool throw_if_noop)

src/Databases/DatabaseReplicated.h

@@ -105,6 +105,8 @@ class DatabaseReplicated : public DatabaseAtomic
     bool createDatabaseNodesInZooKeeper(const ZooKeeperPtr & current_zookeeper);
     static bool looksLikeReplicatedDatabasePath(const ZooKeeperPtr & current_zookeeper, const String & path);
     void createReplicaNodesInZooKeeper(const ZooKeeperPtr & current_zookeeper);
+    /// For Replicated database will return ATTACH for MVs with inner table
+    ASTPtr tryGetCreateOrAttachTableQuery(const String & name, ContextPtr context) const;
 
     struct
     {
@@ -127,7 +129,9 @@ class DatabaseReplicated : public DatabaseAtomic
     std::map<String, String> getConsistentMetadataSnapshotImpl(const ZooKeeperPtr & zookeeper, const FilterByNameFunction & filter_by_table_name,
                                                                size_t max_retries, UInt32 & max_log_ptr) const;
 
+    ASTPtr parseQueryFromMetadata(const String & table_name, const String & query, const String & description) const;
     ASTPtr parseQueryFromMetadataInZooKeeper(const String & node_name, const String & query) const;
+    ASTPtr parseQueryFromMetadataOnDisk(const String & table_name) const;
     String readMetadataFile(const String & table_name) const;
 
     ClusterPtr getClusterImpl(bool all_groups = false) const;
@@ -151,8 +155,6 @@ class DatabaseReplicated : public DatabaseAtomic
     void assertDigestInTransactionOrInline(const ContextPtr & local_context, const ZooKeeperMetadataTransactionPtr & txn) TSA_REQUIRES(metadata_mutex);
 
     /// For debug purposes only, don't use in production code
-    void dumpLocalTablesForDebugOnly(const ContextPtr & local_context) const;
-    void dumpTablesInZooKeeperForDebugOnly() const;
     void tryCompareLocalAndZooKeeperTablesAndDumpDiffForDebugOnly(const ContextPtr & local_context) const;
 
     void waitDatabaseStarted() const override;

tests/queries/0_stateless/03001_matview_columns_after_modify_query.sh

@@ -1,4 +1,7 @@
 #!/usr/bin/env bash
+# Tags: no-replicated-database, no-shared-catalog
+# Tag no-replicated-database -- modify on-disk metadata that may lead to "Digest does not match" in case Replicated database
+# Tag no-shared-catalog -- same
 
 CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 # shellcheck source=../shell_config.sh

tests/queries/0_stateless/03350_alter_table_fetch_partition_thread_pool.sql

@@ -1,6 +1,6 @@
 -- Tags: no-parallel, no-replicated-database, no-shared-merge-tree
 -- Tag: no-parallel - to avoid polluting FETCH PARTITION thread pool with other fetches
--- Tag: no-database-replicated - replica_path is different
+-- Tag: no-replicated-database - replica_path is different
 
 drop table if exists data1;
 drop table if exists data2;