Repository: ClickHouse/ClickHouse
File: src/Databases/DatabaseReplicated.cpp (C++)

Code before:
    add_ops(buildDatabaseNodesInZooKeeper());

    UInt64 tables_digest = 0;

    for (auto existing_tables_it = getTablesIterator(local_context, {}, /*skip_not_loaded=*/false); existing_tables_it->isValid();
         existing_tables_it->next())
    {
        const String table_name = existing_tables_it->name();
        LOG_TEST(log, "Restoring metadata in Keeper of table {}", table_name);

        const String statement = getObjectDefinitionFromCreateQuery(getCreateTableQuery(table_name, local_context));
        const String table_metadata_zk_path = zookeeper_path + "/metadata/" + escapeForFileName(table_name);
        add_ops({zkutil::makeCreateRequest(table_metadata_zk_path, statement, zkutil::CreateMode::Persistent)});

        tables_digest += DB::getMetadataHash(table_name, statement);
    }

    {
        std::lock_guard lock{metadata_mutex};
        tables_metadata_digest = tables_digest;
        if (!checkDigestValid(local_context))
            throw Exception(ErrorCodes::LOGICAL_ERROR, "Digest does not match");
    }
    Coordination::Responses responses;
    auto code = zookeeper->tryMulti(ops, responses);

    if (code == Coordination::Error::ZOK)
        return;

    zkutil::KeeperMultiException::check(code, ops, responses);
    UNREACHABLE();
}

void DatabaseReplicated::reinitializeDDLWorker()
{
    std::lock_guard lock{ddl_worker_mutex};

    if (is_probably_dropped.exchange(false, std::memory_order_relaxed))
    {
        LOG_TRACE(log, "Flag is_probably_dropped is being reset to restore database metadata in keeper.");
    }

    if (ddl_worker)
    {
        LOG_TRACE(log, "Resetting DDL worker.");
        ddl_worker->shutdown();
        {
            ddl_worker_initialized = false;
            ddl_worker = nullptr;
        }
    }

Review comment (bug):
I think this was somewhat true even before the refactoring of this (`restoreDatabaseNodesInKeeper`) function, but it looks like we mutate the in memory digest before doing `tryMulti`. If for some reason `tryMulti` fails, it looks like we don't clean up the mutated digest. Is this ok? Perhaps we can handle this better eg. revert the digest when `tryMulti` fails?

Code after:

    add_ops(buildDatabaseNodesInZooKeeper());

    UInt64 tables_digest = 0;

    for (auto existing_tables_it = getTablesIterator(local_context, {}, /*skip_not_loaded=*/false); existing_tables_it->isValid();
         existing_tables_it->next())
    {
        const String table_name = existing_tables_it->name();
        LOG_TEST(log, "Restoring metadata in Keeper of table {}", table_name);

        const String statement = getObjectDefinitionFromCreateQuery(getCreateTableQuery(table_name, local_context));
        const String table_metadata_zk_path = zookeeper_path + "/metadata/" + escapeForFileName(table_name);
        add_ops({zkutil::makeCreateRequest(table_metadata_zk_path, statement, zkutil::CreateMode::Persistent)});

        tables_digest += DB::getMetadataHash(table_name, statement);
    }

    {
        std::lock_guard lock{metadata_mutex};
        tables_metadata_digest = tables_digest;
        if (!checkDigestValid(local_context))
            throw Exception(ErrorCodes::LOGICAL_ERROR, "Digest does not match");
    }
    Coordination::Responses responses;
    auto code = zookeeper->tryMulti(ops, responses);

    if (code == Coordination::Error::ZOK)
        return;

    zkutil::KeeperMultiException::check(code, ops, responses);
    UNREACHABLE();
}

void DatabaseReplicated::reinitializeDDLWorker()
{
    std::lock_guard lock{ddl_worker_mutex};

    if (is_probably_dropped.exchange(false, std::memory_order_relaxed))
    {
        LOG_TRACE(log, "Flag is_probably_dropped is being reset to restore database metadata in keeper.");
    }

    if (ddl_worker)
    {
        LOG_TRACE(log, "Resetting DDL worker.");
        ddl_worker->shutdown();
        {
            ddl_worker_initialized = false;
            ddl_worker = nullptr;
        }