use a designated mutex for path_map

src/Disks/ObjectStorages/CommonPathPrefixKeyGenerator.cpp

@@ -1,4 +1,5 @@
 #include "CommonPathPrefixKeyGenerator.h"
+#include "Disks/ObjectStorages/PathComparator.h"
 
 #include <Common/getRandomASCIIString.h>
 
@@ -9,9 +10,8 @@
 namespace DB
 {
 
-CommonPathPrefixKeyGenerator::CommonPathPrefixKeyGenerator(
-    String key_prefix_, SharedMutex & shared_mutex_, std::weak_ptr<PathMap> path_map_)
-    : storage_key_prefix(key_prefix_), shared_mutex(shared_mutex_), path_map(std::move(path_map_))
+CommonPathPrefixKeyGenerator::CommonPathPrefixKeyGenerator(String key_prefix_, std::weak_ptr<InMemoryPathMap> path_map_)
+    : storage_key_prefix(key_prefix_), path_map(std::move(path_map_))
 {
 }
 
@@ -49,14 +49,13 @@ std::tuple<std::string, std::vector<std::string>> CommonPathPrefixKeyGenerator::
     std::filesystem::path p(path);
     std::deque<std::string> dq;
 
-    std::shared_lock lock(shared_mutex);
-
     auto ptr = path_map.lock();
+    std::shared_lock lock(ptr->mutex);
 
     while (p != p.root_path())
     {
-        auto it = ptr->find(p);
-        if (it != ptr->end())
+        auto it = ptr->map.find(p);
+        if (it != ptr->map.end())
         {
             std::vector<std::string> vec(std::make_move_iterator(dq.begin()), std::make_move_iterator(dq.end()));
             return std::make_tuple(it->second, std::move(vec));

src/Disks/ObjectStorages/CommonPathPrefixKeyGenerator.h

@@ -25,9 +25,8 @@ class CommonPathPrefixKeyGenerator : public IObjectStorageKeysGenerator
 {
 public:
     /// Local to remote path map. Leverages filesystem::path comparator for paths.
-    using PathMap = std::map<std::filesystem::path, std::string, PathComparator>;
 
-    explicit CommonPathPrefixKeyGenerator(String key_prefix_, SharedMutex & shared_mutex_, std::weak_ptr<PathMap> path_map_);
+    explicit CommonPathPrefixKeyGenerator(String key_prefix_, std::weak_ptr<InMemoryPathMap> path_map_);
 
     ObjectStorageKey generate(const String & path, bool is_directory, const std::optional<String> & key_prefix) const override;
 
@@ -37,8 +36,7 @@ private:
 
     const String storage_key_prefix;
 
-    SharedMutex & shared_mutex;
-    std::weak_ptr<PathMap> path_map;
+    std::weak_ptr<InMemoryPathMap> path_map;
 };
 
 }

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorage.h

@@ -28,10 +28,6 @@ using UnlinkMetadataFileOperationOutcomePtr = std::shared_ptr<UnlinkMetadataFile
 /// structure as on disk MergeTree, and does not require metadata from a local disk to restore.
 class MetadataStorageFromPlainObjectStorage : public IMetadataStorage
 {
-public:
-    /// Local path prefixes mapped to storage key prefixes.
-    using PathMap = std::map<std::filesystem::path, std::string, PathComparator>;
-
 private:
     friend class MetadataStorageFromPlainObjectStorageTransaction;
 
@@ -86,7 +82,7 @@ protected:
     virtual std::string getMetadataKeyPrefix() const { return object_storage->getCommonKeyPrefix(); }
 
     /// Returns a map of local paths to paths in object storage.
-    virtual std::shared_ptr<PathMap> getPathMap() const { throwNotImplemented(); }
+    virtual std::shared_ptr<InMemoryPathMap> getPathMap() const { throwNotImplemented(); }
 };
 
 class MetadataStorageFromPlainObjectStorageTransaction final : public IMetadataTransaction, private MetadataOperationsHolder

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.cpp

@@ -1,4 +1,5 @@
 #include "MetadataStorageFromPlainObjectStorageOperations.h"
+#include "Disks/ObjectStorages/PathComparator.h"
 
 #include <IO/ReadHelpers.h>
 #include <IO/WriteHelpers.h>
@@ -30,7 +31,7 @@ ObjectStorageKey createMetadataObjectKey(const std::string & key_prefix, const s
 MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::MetadataStorageFromPlainObjectStorageCreateDirectoryOperation(
     std::filesystem::path && path_,
     std::string && key_prefix_,
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
+    InMemoryPathMap & path_map_,
     ObjectStoragePtr object_storage_,
     const std::string & metadata_key_prefix_)
     : path(std::move(path_))
@@ -43,8 +44,13 @@ MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::MetadataStorageFr
 
 void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::execute(std::unique_lock<SharedMutex> &)
 {
-    if (path_map.contains(path.parent_path()))
-        return;
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
+    {
+        std::shared_lock lock(mutex);
+        if (map.contains(path.parent_path()))
+            return;
+    }
 
     auto metadata_object_key = createMetadataObjectKey(key_prefix, metadata_key_prefix);
 
@@ -64,8 +70,11 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::execute(std:
 
     write_created = true;
 
-    [[maybe_unused]] auto result = path_map.emplace(path.parent_path(), std::move(key_prefix));
-    chassert(result.second);
+    {
+        std::unique_lock lock(mutex);
+        [[maybe_unused]] auto result = map.emplace(path.parent_path(), std::move(key_prefix));
+        chassert(result.second);
+    }
     auto metric = object_storage->getMetadataStorageMetrics().directory_map_size;
     CurrentMetrics::add(metric, 1);
 
@@ -80,11 +89,17 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::execute(std:
 
 void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::undo(std::unique_lock<SharedMutex> &)
 {
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
+
     auto metadata_object_key = createMetadataObjectKey(key_prefix, metadata_key_prefix);
 
     if (write_finalized)
     {
-        path_map.erase(path.parent_path());
+        {
+            std::unique_lock lock(mutex);
+            map.erase(path.parent_path());
+        }
         auto metric = object_storage->getMetadataStorageMetrics().directory_map_size;
         CurrentMetrics::sub(metric, 1);
 
@@ -97,7 +112,7 @@ void MetadataStorageFromPlainObjectStorageCreateDirectoryOperation::undo(std::un
 MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::MetadataStorageFromPlainObjectStorageMoveDirectoryOperation(
     std::filesystem::path && path_from_,
     std::filesystem::path && path_to_,
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
+    InMemoryPathMap & path_map_,
     ObjectStoragePtr object_storage_,
     const std::string & metadata_key_prefix_)
     : path_from(std::move(path_from_))
@@ -111,14 +126,25 @@ MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::MetadataStorageFrom
 std::unique_ptr<WriteBufferFromFileBase> MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::createWriteBuf(
     const std::filesystem::path & expected_path, const std::filesystem::path & new_path, bool validate_content)
 {
-    auto expected_it = path_map.find(expected_path.parent_path());
-    if (expected_it == path_map.end())
-        throw Exception(ErrorCodes::FILE_DOESNT_EXIST, "Metadata object for the expected (source) path '{}' does not exist", expected_path);
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
 
-    if (path_map.contains(new_path.parent_path()))
-        throw Exception(ErrorCodes::FILE_ALREADY_EXISTS, "Metadata object for the new (destination) path '{}' already exists", new_path);
+    std::filesystem::path remote_path;
+    {
+        std::shared_lock lock(mutex);
+        auto expected_it = map.find(expected_path.parent_path());
+        if (expected_it == map.end())
+            throw Exception(
+                ErrorCodes::FILE_DOESNT_EXIST, "Metadata object for the expected (source) path '{}' does not exist", expected_path);
 
-    auto metadata_object_key = createMetadataObjectKey(expected_it->second, metadata_key_prefix);
+        if (map.contains(new_path.parent_path()))
+            throw Exception(
+                ErrorCodes::FILE_ALREADY_EXISTS, "Metadata object for the new (destination) path '{}' already exists", new_path);
+
+        remote_path = expected_it->second;
+    }
+
+    auto metadata_object_key = createMetadataObjectKey(remote_path, metadata_key_prefix);
 
     auto metadata_object = StoredObject(metadata_object_key.serialize(), expected_path / PREFIX_PATH_FILE_NAME);
 
@@ -156,8 +182,13 @@ void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::execute(std::u
     writeString(path_to.string(), *write_buf);
     write_buf->finalize();
 
-    [[maybe_unused]] auto result = path_map.emplace(path_to.parent_path(), path_map.extract(path_from.parent_path()).mapped());
-    chassert(result.second);
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
+    {
+        std::unique_lock lock(mutex);
+        [[maybe_unused]] auto result = map.emplace(path_to.parent_path(), map.extract(path_from.parent_path()).mapped());
+        chassert(result.second);
+    }
 
     write_finalized = true;
 }
@@ -165,7 +196,12 @@ void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::execute(std::u
 void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::undo(std::unique_lock<SharedMutex> &)
 {
     if (write_finalized)
-        path_map.emplace(path_from.parent_path(), path_map.extract(path_to.parent_path()).mapped());
+    {
+        auto & map = path_map.map;
+        auto & mutex = path_map.mutex;
+        std::unique_lock lock(mutex);
+        map.emplace(path_from.parent_path(), map.extract(path_to.parent_path()).mapped());
+    }
 
     if (write_created)
     {
@@ -176,28 +212,34 @@ void MetadataStorageFromPlainObjectStorageMoveDirectoryOperation::undo(std::uniq
 }
 
 MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation::MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation(
-    std::filesystem::path && path_,
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
-    ObjectStoragePtr object_storage_,
-    const std::string & metadata_key_prefix_)
+    std::filesystem::path && path_, InMemoryPathMap & path_map_, ObjectStoragePtr object_storage_, const std::string & metadata_key_prefix_)
     : path(std::move(path_)), path_map(path_map_), object_storage(object_storage_), metadata_key_prefix(metadata_key_prefix_)
 {
 }
 
 void MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation::execute(std::unique_lock<SharedMutex> & /* metadata_lock */)
 {
-    auto path_it = path_map.find(path.parent_path());
-    if (path_it == path_map.end())
-        return;
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
+    {
+        std::shared_lock lock(mutex);
+        auto path_it = map.find(path.parent_path());
+        if (path_it == map.end())
+            return;
+        key_prefix = path_it->second;
+    }
 
     LOG_TRACE(getLogger("MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation"), "Removing directory '{}'", path);
 
-    key_prefix = path_it->second;
     auto metadata_object_key = createMetadataObjectKey(key_prefix, metadata_key_prefix);
     auto metadata_object = StoredObject(metadata_object_key.serialize(), path / PREFIX_PATH_FILE_NAME);
     object_storage->removeObject(metadata_object);
 
-    path_map.erase(path_it);
+    {
+        std::unique_lock lock(mutex);
+        map.erase(path.parent_path());
+    }
+
     auto metric = object_storage->getMetadataStorageMetrics().directory_map_size;
     CurrentMetrics::sub(metric, 1);
 
@@ -223,7 +265,12 @@ void MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation::undo(std::un
     writeString(path.string(), *buf);
     buf->finalize();
 
-    path_map.emplace(path.parent_path(), std::move(key_prefix));
+    auto & map = path_map.map;
+    auto & mutex = path_map.mutex;
+    {
+        std::unique_lock lock(mutex);
+        map.emplace(path.parent_path(), std::move(key_prefix));
+    }
     auto metric = object_storage->getMetadataStorageMetrics().directory_map_size;
     CurrentMetrics::add(metric, 1);
 }

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorageOperations.h

@@ -2,6 +2,7 @@
 
 #include <Disks/ObjectStorages/IMetadataOperation.h>
 #include <Disks/ObjectStorages/MetadataStorageFromPlainObjectStorage.h>
+#include "Disks/ObjectStorages/PathComparator.h"
 
 #include <filesystem>
 #include <map>
@@ -14,7 +15,7 @@ class MetadataStorageFromPlainObjectStorageCreateDirectoryOperation final : publ
 private:
     std::filesystem::path path;
     std::string key_prefix;
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map;
+    InMemoryPathMap & path_map;
     ObjectStoragePtr object_storage;
     const std::string metadata_key_prefix;
 
@@ -26,7 +27,7 @@ public:
     MetadataStorageFromPlainObjectStorageCreateDirectoryOperation(
         std::filesystem::path && path_,
         std::string && key_prefix_,
-        MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
+        InMemoryPathMap & path_map_,
         ObjectStoragePtr object_storage_,
         const std::string & metadata_key_prefix_);
 
@@ -39,7 +40,7 @@ class MetadataStorageFromPlainObjectStorageMoveDirectoryOperation final : public
 private:
     std::filesystem::path path_from;
     std::filesystem::path path_to;
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map;
+    InMemoryPathMap & path_map;
     ObjectStoragePtr object_storage;
     const std::string metadata_key_prefix;
 
@@ -53,7 +54,7 @@ public:
     MetadataStorageFromPlainObjectStorageMoveDirectoryOperation(
         std::filesystem::path && path_from_,
         std::filesystem::path && path_to_,
-        MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
+        InMemoryPathMap & path_map_,
         ObjectStoragePtr object_storage_,
         const std::string & metadata_key_prefix_);
 
@@ -67,7 +68,7 @@ class MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation final : publ
 private:
     std::filesystem::path path;
 
-    MetadataStorageFromPlainObjectStorage::PathMap & path_map;
+    InMemoryPathMap & path_map;
     ObjectStoragePtr object_storage;
     const std::string metadata_key_prefix;
 
@@ -77,7 +78,7 @@ private:
 public:
     MetadataStorageFromPlainObjectStorageRemoveDirectoryOperation(
         std::filesystem::path && path_,
-        MetadataStorageFromPlainObjectStorage::PathMap & path_map_,
+        InMemoryPathMap & path_map_,
         ObjectStoragePtr object_storage_,
         const std::string & metadata_key_prefix_);
 

src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.cpp

@@ -3,11 +3,12 @@
 
 #include <unordered_set>
 #include <IO/ReadHelpers.h>
-#include <IO/SharedThreadPools.h>
 #include <IO/S3Common.h>
+#include <IO/SharedThreadPools.h>
 #include <Common/ErrorCodes.h>
 #include <Common/logger_useful.h>
 #include "CommonPathPrefixKeyGenerator.h"
+#include "Disks/ObjectStorages/PathComparator.h"
 
 
 namespace DB
@@ -37,9 +38,10 @@ std::string getMetadataKeyPrefix(ObjectStoragePtr object_storage)
         : metadata_key_prefix;
 }
 
-MetadataStorageFromPlainObjectStorage::PathMap loadPathPrefixMap(const std::string & metadata_key_prefix, ObjectStoragePtr object_storage)
+InMemoryPathMap::Map loadPathPrefixMap(const std::string & metadata_key_prefix, ObjectStoragePtr object_storage)
 {
-    MetadataStorageFromPlainObjectStorage::PathMap result;
+    using Map = InMemoryPathMap::Map;
+    Map result;
 
     ThreadPool & pool = getIOThreadPool().get();
     ThreadPoolCallbackRunnerLocal<void> runner(pool, "PlainRWMetaLoad");
@@ -94,7 +96,7 @@ MetadataStorageFromPlainObjectStorage::PathMap loadPathPrefixMap(const std::stri
             chassert(remote_metadata_path.string().starts_with(metadata_key_prefix));
             auto suffix = remote_metadata_path.string().substr(metadata_key_prefix.size());
             auto remote_path = std::filesystem::path(std::move(suffix));
-            std::pair<MetadataStorageFromPlainObjectStorage::PathMap::iterator, bool> res;
+            std::pair<Map::iterator, bool> res;
             {
                 std::lock_guard lock(mutex);
                 res = result.emplace(std::filesystem::path(local_path).parent_path(), remote_path.parent_path());
@@ -126,14 +128,13 @@ void getDirectChildrenOnDiskImpl(
     const std::string & storage_key_perfix,
     const RelativePathsWithMetadata & remote_paths,
     const std::string & local_path,
-    const MetadataStorageFromPlainObjectStorage::PathMap & local_path_prefixes,
+    const InMemoryPathMap::Map & local_path_prefixes,
     SharedMutex & shared_mutex,
     std::unordered_set<std::string> & result)
 {
-    using PathMap = MetadataStorageFromPlainObjectStorage::PathMap;
-
     /// Map remote paths into local subdirectories.
-    std::unordered_map<PathMap::mapped_type, PathMap::key_type> remote_to_local_subdir;
+    using Map = InMemoryPathMap::Map;
+    std::unordered_map<Map::mapped_type, Map::key_type> remote_to_local_subdir;
 
     {
         std::shared_lock lock(shared_mutex);
@@ -189,7 +190,7 @@ MetadataStorageFromPlainRewritableObjectStorage::MetadataStorageFromPlainRewrita
     ObjectStoragePtr object_storage_, String storage_path_prefix_)
     : MetadataStorageFromPlainObjectStorage(object_storage_, storage_path_prefix_)
     , metadata_key_prefix(DB::getMetadataKeyPrefix(object_storage))
-    , path_map(std::make_shared<PathMap>(loadPathPrefixMap(metadata_key_prefix, object_storage)))
+    , path_map(std::make_shared<InMemoryPathMap>(loadPathPrefixMap(metadata_key_prefix, object_storage)))
 {
     if (object_storage->isWriteOnce())
         throw Exception(
@@ -197,14 +198,14 @@ MetadataStorageFromPlainRewritableObjectStorage::MetadataStorageFromPlainRewrita
             "MetadataStorageFromPlainRewritableObjectStorage is not compatible with write-once storage '{}'",
             object_storage->getName());
 
-    auto keys_gen = std::make_shared<CommonPathPrefixKeyGenerator>(object_storage->getCommonKeyPrefix(), metadata_mutex, path_map);
+    auto keys_gen = std::make_shared<CommonPathPrefixKeyGenerator>(object_storage->getCommonKeyPrefix(), path_map);
     object_storage->setKeysGenerator(keys_gen);
 }
 
 MetadataStorageFromPlainRewritableObjectStorage::~MetadataStorageFromPlainRewritableObjectStorage()
 {
     auto metric = object_storage->getMetadataStorageMetrics().directory_map_size;
-    CurrentMetrics::sub(metric, path_map->size());
+    CurrentMetrics::sub(metric, path_map->map.size());
 }
 
 bool MetadataStorageFromPlainRewritableObjectStorage::exists(const std::string & path) const
@@ -263,7 +264,7 @@ void MetadataStorageFromPlainRewritableObjectStorage::getDirectChildrenOnDisk(
     const std::string & local_path,
     std::unordered_set<std::string> & result) const
 {
-    getDirectChildrenOnDiskImpl(storage_key, storage_key_perfix, remote_paths, local_path, *getPathMap(), metadata_mutex, result);
+    getDirectChildrenOnDiskImpl(storage_key, storage_key_perfix, remote_paths, local_path, getPathMap()->map, getPathMap()->mutex, result);
 }
 
 }

src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.h

@@ -13,7 +13,7 @@ class MetadataStorageFromPlainRewritableObjectStorage final : public MetadataSto
 {
 private:
     const std::string metadata_key_prefix;
-    std::shared_ptr<PathMap> path_map;
+    std::shared_ptr<InMemoryPathMap> path_map;
 
 public:
     MetadataStorageFromPlainRewritableObjectStorage(ObjectStoragePtr object_storage_, String storage_path_prefix_);
@@ -27,7 +27,7 @@ public:
 
 protected:
     std::string getMetadataKeyPrefix() const override { return metadata_key_prefix; }
-    std::shared_ptr<PathMap> getPathMap() const override { return path_map; }
+    std::shared_ptr<InMemoryPathMap> getPathMap() const override { return path_map; }
     void getDirectChildrenOnDisk(
         const std::string & storage_key,
         const std::string & storage_key_perfix,

src/Disks/ObjectStorages/PathComparator.h

@@ -1,20 +1,29 @@
 #pragma once
 
 #include <filesystem>
+#include <map>
+#include "Common/SharedMutex.h"
 
 namespace DB
 {
-// TODO: rename
-struct PathComparator
+
+
+struct InMemoryPathMap
 {
-    bool operator()(const std::filesystem::path & path1, const std::filesystem::path & path2) const
+    struct PathComparator
     {
-        auto d1 = std::distance(path1.begin(), path1.end());
-        auto d2 = std::distance(path2.begin(), path2.end());
-        if (d1 != d2)
-            return d1 < d2;
-        return path1 < path2;
-    }
+        bool operator()(const std::filesystem::path & path1, const std::filesystem::path & path2) const
+        {
+            auto d1 = std::distance(path1.begin(), path1.end());
+            auto d2 = std::distance(path2.begin(), path2.end());
+            if (d1 != d2)
+                return d1 < d2;
+            return path1 < path2;
+        }
+    };
+    using Map = std::map<std::filesystem::path, std::string, PathComparator>;
+    Map map;
+    SharedMutex mutex;
 };
 
 }