fix a bug with symlinks detection

programs/install/Install.cpp

@@ -1,6 +1,7 @@
 #include <iostream>
 #include <filesystem>
 #include <boost/program_options.hpp>
+#include <Common/filesystemHelpers.h>
 
 #include <sys/stat.h>
 #include <pwd.h>
@@ -378,10 +379,10 @@ int mainEntryClickHouseInstall(int argc, char ** argv)
 
             if (fs::exists(symlink_path))
             {
-                bool is_symlink = fs::is_symlink(symlink_path);
+                bool is_symlink = FS::isSymlink(symlink_path);
                 fs::path points_to;
                 if (is_symlink)
-                    points_to = fs::weakly_canonical(fs::read_symlink(symlink_path));
+                    points_to = fs::weakly_canonical(FS::readSymlink(symlink_path));
 
                 if (is_symlink && points_to == main_bin_path)
                 {

src/Common/DateLUT.cpp

@@ -3,6 +3,7 @@
 #include <Poco/DigestStream.h>
 #include <Poco/Exception.h>
 #include <Poco/SHA1Engine.h>
+#include <Common/filesystemHelpers.h>
 
 #include <filesystem>
 #include <fstream>
@@ -64,9 +65,9 @@ std::string determineDefaultTimeZone()
         ///  /etc/localtime -> /usr/share/zoneinfo//UTC
         ///  /usr/share/zoneinfo//UTC -> UCT
         /// But the preferred time zone name is pointed by the first link (UTC), and the second link is just an internal detail.
-        if (fs::is_symlink(tz_file_path))
+        if (FS::isSymlink(tz_file_path))
         {
-            tz_file_path = fs::read_symlink(tz_file_path);
+            tz_file_path = FS::readSymlink(tz_file_path);
             /// If it's relative - make it absolute.
             if (tz_file_path.is_relative())
                 tz_file_path = (fs::path("/etc/") / tz_file_path).lexically_normal();

src/Common/filesystemHelpers.cpp

@@ -351,4 +351,24 @@ void setModificationTime(const std::string & path, time_t time)
     if (utime(path.c_str(), &tb) != 0)
         DB::throwFromErrnoWithPath("Cannot set modification time for file: " + path, path, DB::ErrorCodes::PATH_ACCESS_DENIED);
 }
+
+bool isSymlink(const fs::path & path)
+{
+    /// Remove trailing stash before checking if file is symlink.
+    /// Let /path/to/link is a symlink to /path/to/target/dir/ directory.
+    /// In this case is_symlink("/path/to/link") is true,
+    /// but is_symlink("/path/to/link/") is false (it's a directory)
+    if (path.filename().empty())
+        return fs::is_symlink(path.parent_path());      /// STYLE_CHECK_ALLOW_STD_FS_SYMLINK
+    return fs::is_symlink(path);        /// STYLE_CHECK_ALLOW_STD_FS_SYMLINK
+}
+
+fs::path readSymlink(const fs::path & path)
+{
+    /// See the comment for isSymlink
+    if (path.filename().empty())
+        return fs::read_symlink(path.parent_path());        /// STYLE_CHECK_ALLOW_STD_FS_SYMLINK
+    return fs::read_symlink(path);      /// STYLE_CHECK_ALLOW_STD_FS_SYMLINK
+}
+
 }

src/Common/filesystemHelpers.h

@@ -9,6 +9,7 @@
 #include <sys/statvfs.h>
 #include <Poco/TemporaryFile.h>
 
+namespace fs = std::filesystem;
 
 namespace DB
 {
@@ -89,4 +90,8 @@ Poco::Timestamp getModificationTimestamp(const std::string & path);
 void setModificationTime(const std::string & path, time_t time);
 /// st_ctime
 time_t getChangeTime(const std::string & path);
+
+bool isSymlink(const fs::path & path);
+fs::path readSymlink(const fs::path & path);
+
 }

src/Databases/DatabaseAtomic.cpp

@@ -6,6 +6,7 @@
 #include <IO/ReadBufferFromFile.h>
 #include <Parsers/formatAST.h>
 #include <Common/atomicRename.h>
+#include <Common/filesystemHelpers.h>
 #include <Storages/StorageMaterializedView.h>
 #include <Interpreters/Context.h>
 #include <Interpreters/ExternalDictionariesLoader.h>
@@ -424,7 +425,7 @@ void DatabaseAtomic::beforeLoadingMetadata(ContextMutablePtr /*context*/, bool f
     /// Recreate symlinks to table data dirs in case of force restore, because some of them may be broken
     for (const auto & table_path : fs::directory_iterator(path_to_table_symlinks))
     {
-        if (!fs::is_symlink(table_path))
+        if (!FS::isSymlink(table_path))
         {
             throw Exception(ErrorCodes::ABORTED,
                 "'{}' is not a symlink. Atomic database should contains only symlinks.", std::string(table_path.path()));
@@ -495,7 +496,7 @@ void DatabaseAtomic::tryCreateMetadataSymlink()
     fs::path metadata_symlink(path_to_metadata_symlink);
     if (fs::exists(metadata_symlink))
     {
-        if (!fs::is_symlink(metadata_symlink))
+        if (!FS::isSymlink(metadata_symlink))
             throw Exception(ErrorCodes::FILE_ALREADY_EXISTS, "Directory {} exists", path_to_metadata_symlink);
     }
     else

src/Databases/DatabaseFactory.cpp

@@ -16,6 +16,7 @@
 #include <Storages/ExternalDataSourceConfiguration.h>
 #include <Common/logger_useful.h>
 #include <Common/Macros.h>
+#include <Common/filesystemHelpers.h>
 
 #include "config_core.h"
 
@@ -60,8 +61,43 @@ namespace ErrorCodes
     extern const int NOT_IMPLEMENTED;
 }
 
+void cckMetadataPathForOrdinary(const ASTCreateQuery & create, const String & metadata_path)
+{
+    const String & engine_name = create.storage->engine->name;
+    const String & database_name = create.getDatabase();
+
+    if (engine_name != "Ordinary")
+        return;
+
+    if (!FS::isSymlink(metadata_path))
+        return;
+
+    String target_path = FS::readSymlink(metadata_path).string();
+    fs::path path_to_remove = metadata_path;
+    if (path_to_remove.filename().empty())
+        path_to_remove = path_to_remove.parent_path();
+
+    /// Before 20.7 metadata/db_name.sql file might absent and Ordinary database was attached if there's metadata/db_name/ dir.
+    /// Between 20.7 and 22.7 metadata/db_name.sql was created in this case as well.
+    /// Since 20.7 `default` database is created with Atomic engine on the very first server run.
+    /// The problem is that if server crashed during the very first run and metadata/db_name/ -> store/whatever symlink was created
+    /// then it's considered as Ordinary database. And it even works somehow
+    /// until background task tries to remove unused dir from store/...
+    throw Exception(ErrorCodes::CANNOT_CREATE_DATABASE,
+                    "Metadata directory {} for Ordinary database {} is a symbolic link to {}. "
+                    "It may be a result of manual intervention, crash on very first server start or a bug. "
+                    "Database cannot be attached (it's kind of protection from potential data loss). "
+                    "Metadata directory must not be a symlink and must contain tables metadata files itself. "
+                    "You have to resolve this manually. It can be done like this: rm {}; sudo -u clickhouse mv {} {};",
+                    metadata_path, database_name, target_path,
+                    quoteString(path_to_remove.string()), quoteString(target_path), quoteString(path_to_remove.string()));
+
+}
+
 DatabasePtr DatabaseFactory::get(const ASTCreateQuery & create, const String & metadata_path, ContextPtr context)
 {
+    cckMetadataPathForOrdinary(create, metadata_path);
+
     /// Creates store/xxx/ for Atomic
     fs::create_directories(fs::path(metadata_path).parent_path());
 
@@ -127,19 +163,6 @@ DatabasePtr DatabaseFactory::getImpl(const ASTCreateQuery & create, const String
             throw Exception(ErrorCodes::UNKNOWN_DATABASE_ENGINE,
                             "Ordinary database engine is deprecated (see also allow_deprecated_database_ordinary setting)");
 
-        /// Before 20.7 metadata/db_name.sql file might absent and Ordinary database was attached if there's metadata/db_name/ dir.
-        /// Between 20.7 and 22.7 metadata/db_name.sql was created in this case as well.
-        /// Since 20.7 `default` database is created with Atomic engine on the very first server run.
-        /// The problem is that if server crashed during the very first run and metadata/db_name/ -> store/whatever symlink was created
-        /// then it's considered as Ordinary database. And it even works somehow
-        /// until background task tries to remove onused dir from store/...
-        if (fs::is_symlink(metadata_path))
-            throw Exception(ErrorCodes::CANNOT_CREATE_DATABASE, "Metadata directory {} for Ordinary database {} is a symbolic link to {}. "
-                            "It may be a result of manual intervention, crash on very first server start or a bug. "
-                            "Database cannot be attached (it's kind of protection from potential data loss). "
-                            "Metadata directory must not be a symlink and must contain tables metadata files itself. "
-                            "You have to resolve this manually.",
-                            metadata_path, database_name, fs::read_symlink(metadata_path).string());
         return std::make_shared<DatabaseOrdinary>(database_name, metadata_path, context);
     }
 

utils/check-style/check-style

@@ -346,3 +346,8 @@ fi
 
 # Forbid files that differ only by character case
 find $ROOT_PATH | sort -f | uniq -i -c | awk '{ if ($1 > 1) print }'
+
+# Forbid std::filesystem::is_symlink and std::filesystem::read_symlink, because it's easy to use them incorrectly
+find $ROOT_PATH/{src,programs,utils} -name '*.h' -or -name '*.cpp' |
+    grep -vP $EXCLUDE_DIRS |
+    xargs grep -P '::(is|read)_symlink' | grep -v "STYLE_CHECK_ALLOW_STD_FS_SYMLINK" && echo "Use DB::FS::isSymlink and DB::FS::readSymlink instead"