From 0eb351044c00def206cd39abe9453a320b21da67 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Mon, 30 Mar 2020 17:53:05 +0300 Subject: [PATCH] revert some changes around DDLGuard --- dbms/src/Interpreters/DatabaseCatalog.cpp | 30 ++++--------------- dbms/src/Interpreters/DatabaseCatalog.h | 1 - .../Interpreters/InterpreterCreateQuery.cpp | 18 ++--------- .../src/Interpreters/InterpreterDropQuery.cpp | 10 ++----- 4 files changed, 10 insertions(+), 49 deletions(-) diff --git a/dbms/src/Interpreters/DatabaseCatalog.cpp b/dbms/src/Interpreters/DatabaseCatalog.cpp index db87462f952..81656464957 100644 --- a/dbms/src/Interpreters/DatabaseCatalog.cpp +++ b/dbms/src/Interpreters/DatabaseCatalog.cpp @@ -139,13 +139,6 @@ void DatabaseCatalog::shutdown() for (auto & database : current_databases) database.second->shutdown(); - { - std::lock_guard lock(tables_marked_droped_mutex); - for (auto & elem : tables_marked_droped) - if (elem.need_shutdown) - elem.table->shutdown(); - } - std::lock_guard lock(databases_mutex); assert(std::find_if_not(uuid_map.begin(), uuid_map.end(), [](const auto & elem) { return elem.map.empty(); }) == uuid_map.end()); databases.clear(); @@ -482,7 +475,7 @@ void DatabaseCatalog::loadMarkedAsDroppedTables() String full_path = path + it.name(); Strings name_parts; - boost::split(name_parts, it.name(), boost::is_any_of(".")); + boost::split(name_parts, it.name(), boost::is_any_of(".")); // NOLINT: LLVM Bug 41141 if (name_parts.size() != 4) /// Unexpected file continue; @@ -520,7 +513,6 @@ void DatabaseCatalog::enqueueDroppedTableCleanup(StorageID table_id, StoragePtr assert(dropped_metadata_path == getPathForDroppedMetadata(table_id)); time_t drop_time; - bool need_shutdown = true; if (table) drop_time = std::chrono::system_clock::to_time_t(std::chrono::system_clock::now()); else @@ -557,14 +549,13 @@ void DatabaseCatalog::enqueueDroppedTableCleanup(StorageID table_id, StoragePtr } drop_time = Poco::File(dropped_metadata_path).getLastModified().epochTime(); - need_shutdown = false; } std::lock_guard lock(tables_marked_droped_mutex); if (ignore_delay) - tables_marked_droped.push_front({table_id, table, dropped_metadata_path, 0, need_shutdown}); + tables_marked_droped.push_front({table_id, table, dropped_metadata_path, 0}); else - tables_marked_droped.push_back({table_id, table, dropped_metadata_path, drop_time, need_shutdown}); + tables_marked_droped.push_back({table_id, table, dropped_metadata_path, drop_time}); } void DatabaseCatalog::dropTableDataTask() @@ -583,19 +574,14 @@ void DatabaseCatalog::dropTableDataTask() // "time elapsed = " + std::to_string(current_time - elem.drop_time)); bool not_in_use = !elem.table || elem.table.unique(); bool old_enough = elem.drop_time + drop_delay_s < current_time; - return (not_in_use && old_enough) || elem.need_shutdown; + return not_in_use && old_enough; }); - if (it != tables_marked_droped.end() && !it->need_shutdown) + if (it != tables_marked_droped.end()) { table = std::move(*it); LOG_INFO(log, "Will try drop " + table.table_id.getNameForLogs()); tables_marked_droped.erase(it); } - else if (it->need_shutdown) - { - table = *it; - it->need_shutdown = false; - } } catch (...) { @@ -604,12 +590,6 @@ void DatabaseCatalog::dropTableDataTask() if (table.table_id) { - if (table.need_shutdown) - { - table.table->shutdown(); - (*drop_task)->scheduleAfter(0); - return; - } try { diff --git a/dbms/src/Interpreters/DatabaseCatalog.h b/dbms/src/Interpreters/DatabaseCatalog.h index 1e09a23fb40..8d800faaebc 100644 --- a/dbms/src/Interpreters/DatabaseCatalog.h +++ b/dbms/src/Interpreters/DatabaseCatalog.h @@ -185,7 +185,6 @@ private: StoragePtr table; String metadata_path; time_t drop_time; - bool need_shutdown{true}; }; using TablesMarkedAsDropped = std::list; diff --git a/dbms/src/Interpreters/InterpreterCreateQuery.cpp b/dbms/src/Interpreters/InterpreterCreateQuery.cpp index beca15e6ca0..d81fff61f83 100644 --- a/dbms/src/Interpreters/InterpreterCreateQuery.cpp +++ b/dbms/src/Interpreters/InterpreterCreateQuery.cpp @@ -539,26 +539,13 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) String current_database = context.getCurrentDatabase(); - String metadata_path_tmp; - bool created_tmp = false; - SCOPE_EXIT({ if (created_tmp) Poco::File(metadata_path_tmp).remove(); }); - // If this is a stub ATTACH query, read the query definition from the database if (create.attach && !create.storage && !create.columns_list) { auto database_name = create.database.empty() ? current_database : create.database; auto database = DatabaseCatalog::instance().getDatabase(database_name); - - //TODO do we really need it? refactor it if we do - if (database->getEngineName() == "Atomic") - { - metadata_path_tmp = database->getObjectMetadataPath(create.table) + ".tmp"; - if (!Poco::File(metadata_path_tmp).createFile()) - throw Exception("Cannot attach table because of concurrent query.", ErrorCodes::TABLE_ALREADY_EXISTS); - created_tmp = true; - } - bool if_not_exists = create.if_not_exists; + // Table SQL definition is available even if the table is detached auto query = database->getCreateTableQuery(context, create.table); create = query->as(); // Copy the saved create query, but use ATTACH instead of CREATE @@ -622,8 +609,7 @@ bool InterpreterCreateQuery::doCreateTable(/*const*/ ASTCreateQuery & create, /** If the request specifies IF NOT EXISTS, we allow concurrent CREATE queries (which do nothing). * If table doesnt exist, one thread is creating table, while others wait in DDLGuard. */ - if (database->getEngineName() != "Atomic") - guard = DatabaseCatalog::instance().getDDLGuard(create.database, table_name); + guard = DatabaseCatalog::instance().getDDLGuard(create.database, table_name); /// Table can be created before or it can be created concurrently in another thread, while we were waiting in DDLGuard. if (database->isTableExist(context, table_name)) diff --git a/dbms/src/Interpreters/InterpreterDropQuery.cpp b/dbms/src/Interpreters/InterpreterDropQuery.cpp index 7c0c4488446..e6279de818c 100644 --- a/dbms/src/Interpreters/InterpreterDropQuery.cpp +++ b/dbms/src/Interpreters/InterpreterDropQuery.cpp @@ -75,10 +75,7 @@ BlockIO InterpreterDropQuery::executeToTable( auto table_id = context.resolveStorageID(table_id_, Context::ResolveOrdinary); - std::unique_ptr ddl_guard; - if (auto db = DatabaseCatalog::instance().tryGetDatabase(table_id.database_name)) //FIXME - if (db->getEngineName() != "Atomic") - ddl_guard = (!query.no_ddl_lock ? DatabaseCatalog::instance().getDDLGuard(table_id.database_name, table_id.table_name) : nullptr); + auto ddl_guard = (!query.no_ddl_lock ? DatabaseCatalog::instance().getDDLGuard(table_id.database_name, table_id.table_name) : nullptr); auto [database, table] = tryGetDatabaseAndTable(table_id.database_name, table_id.table_name, query.if_exists); @@ -115,12 +112,11 @@ BlockIO InterpreterDropQuery::executeToTable( context.checkAccess(table->isView() ? AccessType::DROP_VIEW : AccessType::DROP_TABLE, table_id); table->checkTableCanBeDropped(); + table->shutdown(); + TableStructureWriteLockHolder table_lock; if (database->getEngineName() != "Atomic") - { - table->shutdown(); table_lock = table->lockExclusively(context.getCurrentQueryId()); - } database->dropTable(context, table_id.table_name, query.no_delay); }