Revert "Support for refresh in readonly MergeTree tables"

Author: tavplubix

src/Core/SettingsChangesHistory.cpp

@@ -703,7 +703,6 @@ const VersionToSettingsChangesMap & getMergeTreeSettingsChangesHistory()
     {
         addSettingsChanges(merge_tree_settings_changes_history, "25.4",
         {
-            {"refresh_parts_interval", 0, 0, "A new setting"},
             {"max_merge_delayed_streams_for_parallel_write", 1000, 100, "New setting"},
             {"max_postpone_time_for_failed_replicated_fetches_ms", 1ULL * 60 * 1000, 1ULL * 60 * 1000, "Added new setting to enable postponing fetch tasks in the replication queue."},
             {"max_postpone_time_for_failed_replicated_merges_ms", 1ULL * 60 * 1000, 1ULL * 60 * 1000, "Added new setting to enable postponing merge tasks in the replication queue."},

src/Databases/DatabaseLazy.cpp

@@ -326,7 +326,7 @@ try
 
         if (!it->second.table || isSharedPtrUnique(it->second.table))
         {
-            LOG_DEBUG(log, "Removing table {} from cache.", backQuote(it->first));
+            LOG_DEBUG(log, "Drop table {} from cache.", backQuote(it->first));
             it->second.table.reset();
             expired_tables.erase(it->second.expiration_iterator);
             it->second.expiration_iterator = cache_expiration_queue.end();

src/Disks/IDisk.h

@@ -461,10 +461,9 @@ class IDisk : public Space
     virtual void startupImpl(ContextPtr) {}
 
     /// If the state can be changed under the hood and become outdated in memory, perform a reload if necessary.
-    /// but don't do it more frequently than the specified parameter.
     /// Note: for performance reasons, it's allowed to assume that only some subset of changes are possible
     /// (those that MergeTree tables can make).
-    virtual void refresh(UInt64 /* not_sooner_than_milliseconds */)
+    virtual void refresh()
     {
         /// The default no-op implementation when the state in memory cannot be out of sync of the actual state.
     }

src/Disks/ObjectStorages/DiskObjectStorage.h

@@ -146,9 +146,9 @@ friend class DiskObjectStorageRemoteMetadataRestoreHelper;
 
     void startupImpl(ContextPtr context) override;
 
-    void refresh(UInt64 not_sooner_than_milliseconds) override
+    void refresh() override
     {
-        metadata_storage->refresh(not_sooner_than_milliseconds);
+        metadata_storage->refresh();
     }
 
     ReservationPtr reserve(UInt64 bytes) override;

src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp

@@ -766,11 +766,11 @@ std::unique_ptr<WriteBufferFromFileBase> DiskObjectStorageTransaction::writeFile
     if (metadata_helper)
     {
         if (!object_key.hasPrefix())
-            throw Exception(ErrorCodes::LOGICAL_ERROR, "Metadata helper is not supported with absolute paths");
+            throw Exception(ErrorCodes::LOGICAL_ERROR, "metadata helper is not supported with absolute paths");
 
-        auto revision = ++metadata_helper->revision_counter;
-        object_attributes =
-        {
+        auto revision = metadata_helper->revision_counter + 1;
+        metadata_helper->revision_counter++;
+        object_attributes = {
             {"path", path}
         };
 
@@ -782,7 +782,7 @@ std::unique_ptr<WriteBufferFromFileBase> DiskObjectStorageTransaction::writeFile
     /// Does metadata_storage support empty files without actual blobs in the object_storage?
     const bool do_not_write_empty_blob = metadata_storage.supportsEmptyFilesWithoutBlobs();
 
-    /// Seems ok
+    /// seems ok
     auto object = StoredObject(object_key.serialize(), path);
     std::function<void(size_t count)> create_metadata_callback;
 
@@ -862,6 +862,7 @@ std::unique_ptr<WriteBufferFromFileBase> DiskObjectStorageTransaction::writeFile
         operations_to_execute.emplace_back(std::move(write_operation));
     }
 
+
     auto impl = object_storage.writeObject(
         object,
         /// We always use mode Rewrite because we simulate append using metadata and different files

src/Disks/ObjectStorages/FlatDirectoryStructureKeyGenerator.cpp

@@ -19,15 +19,15 @@ FlatDirectoryStructureKeyGenerator::FlatDirectoryStructureKeyGenerator(
 ObjectStorageKey FlatDirectoryStructureKeyGenerator::generate(const String & path, bool is_directory, const std::optional<String> & key_prefix) const
 {
     if (is_directory)
-        chassert(path.empty() || path.ends_with('/'));
+        chassert(path.ends_with('/'));
 
-    const auto fs_path = std::filesystem::path(path);
-    std::filesystem::path directory = fs_path.parent_path();
+    const auto p = std::filesystem::path(path);
+    auto directory = p.parent_path();
 
     std::optional<std::filesystem::path> remote_path;
     {
         const auto ptr = path_map.lock();
-        auto res = ptr->getRemotePathInfoIfExists(fs_path);
+        auto res = ptr->getRemotePathInfoIfExists(p);
         if (res)
             return ObjectStorageKey::createAsRelative(key_prefix.has_value() ? *key_prefix : storage_key_prefix, res->path);
 
@@ -41,7 +41,7 @@ ObjectStorageKey FlatDirectoryStructureKeyGenerator::generate(const String & pat
                                                         : directory;
 
     if (!is_directory)
-        key /= fs_path.filename();
+        key /= p.filename();
 
     return ObjectStorageKey::createAsRelative(key_prefix.has_value() ? *key_prefix : storage_key_prefix, key);
 }

src/Disks/ObjectStorages/IMetadataStorage.h

@@ -256,11 +256,10 @@ class IMetadataStorage : private boost::noncopyable
         /// This method is overridden for specific metadata implementations in ClickHouse Cloud.
     }
 
-    /// If the state can be changed under the hood and become outdated in memory, perform a reload if necessary,
-    /// but don't do it more frequently than the specified parameter.
+    /// If the state can be changed under the hood and become outdated in memory, perform a reload if necessary.
     /// Note: for performance reasons, it's allowed to assume that only some subset of changes are possible
     /// (those that MergeTree tables can make).
-    virtual void refresh(UInt64 /* not_sooner_than_milliseconds */)
+    virtual void refresh()
     {
         /// The default no-op implementation when the state in memory cannot be out of sync of the actual state.
     }

src/Disks/ObjectStorages/InMemoryDirectoryPathMap.h

@@ -39,7 +39,6 @@ class InMemoryDirectoryPathMap
     struct RemotePathInfo
     {
         std::string path;
-        std::string etag;
         time_t last_modified = 0;
         FileNames files;
     };
@@ -56,49 +55,28 @@ class InMemoryDirectoryPathMap
         return remote_directories.contains(remote_path);
     }
 
-    bool existsRemotePathUnchanged(const std::string & remote_path, const std::string & etag) const
-    {
-        std::lock_guard lock(mutex);
-        auto it = remote_directories.find(remote_path);
-        return it != remote_directories.end() && it->second->second.etag == etag;
-    }
-
     bool existsLocalPath(const std::string & local_path) const
     {
         std::lock_guard lock(mutex);
         return map.contains(local_path);
     }
 
-    void addOrReplacePath(std::string path, RemotePathInfo info)
+    auto addPathIfNotExists(std::string path, RemotePathInfo info)
     {
         std::string remote_path = info.path;
         std::lock_guard lock(mutex);
 
         size_t num_files = info.files.size();
+        auto res = map.emplace(std::move(path), std::move(info));
 
-        /// If the logical path already exists, skip it.
-        if (map.contains(path))
-            return;
-
-        /// If the path was differently named before.
-        auto old_it = remote_directories.find(info.path);
-        if (old_it != remote_directories.end())
+        if (res.second)
         {
-            metric_files.sub(old_it->second->second.files.size());
-            metric_directories.sub(1);
-
-            map.erase(old_it->second->first);
-            remote_directories.erase(old_it);
+            remote_directories.emplace(remote_path);
+            metric_directories.add(1);
+            metric_files.add(num_files);
         }
 
-        auto res = map.emplace(std::move(path), std::move(info));
-
-        if (!res.second)
-            return;
-
-        remote_directories.emplace(remote_path, &*res.first);
-        metric_directories.add(1);
-        metric_files.add(num_files);
+        return res;
     }
 
     bool existsFile(const std::string & local_path) const
@@ -241,14 +219,11 @@ class InMemoryDirectoryPathMap
     mutable std::mutex mutex;
 
     /// A mapping from logical filesystem path to the storage path.
-    using LogicalToPhysicalMap = std::map<std::filesystem::path, RemotePathInfo, PathComparator>;
-    LogicalToPhysicalMap TSA_GUARDED_BY(mutex) map;
-
-    /// A mapping from the storage path to info. Note: std::map has pointers to its nodes stable.
-    using PhysicalPaths = std::map<std::string, LogicalToPhysicalMap::const_pointer>;
+    using Map = std::map<std::filesystem::path, RemotePathInfo, PathComparator>;
+    Map TSA_GUARDED_BY(mutex) map;
 
     /// A set of known storage paths (randomly-assigned names).
-    PhysicalPaths TSA_GUARDED_BY(mutex) remote_directories;
+    FileNames TSA_GUARDED_BY(mutex) remote_directories;
 
     CurrentMetrics::Increment metric_directories;
     CurrentMetrics::Increment metric_files;

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp

@@ -21,21 +21,18 @@ namespace ErrorCodes
     extern const int NOT_IMPLEMENTED;
     extern const int BAD_ARGUMENTS;
     extern const int CANNOT_UNLINK;
-    extern const int CANNOT_RMDIR;
-    extern const int READONLY;
 }
 
-LocalObjectStorage::LocalObjectStorage(LocalObjectStorageSettings settings_)
-    : settings(std::move(settings_))
+LocalObjectStorage::LocalObjectStorage(String key_prefix_)
+    : key_prefix(std::move(key_prefix_))
     , log(getLogger("LocalObjectStorage"))
 {
     if (auto block_device_id = tryGetBlockDeviceId("/"); block_device_id.has_value())
         description = *block_device_id;
     else
         description = "/";
 
-    if (!settings.read_only)
-        fs::create_directories(settings.key_prefix);
+    fs::create_directories(key_prefix);
 }
 
 bool LocalObjectStorage::exists(const StoredObject & object) const
@@ -72,8 +69,6 @@ std::unique_ptr<WriteBufferFromFileBase> LocalObjectStorage::writeObject( /// NO
     size_t buf_size,
     const WriteSettings & /* write_settings */)
 {
-    throwIfReadonly();
-
     if (mode != WriteMode::Rewrite)
         throw Exception(ErrorCodes::BAD_ARGUMENTS, "LocalObjectStorage doesn't support append to files");
 
@@ -88,52 +83,28 @@ std::unique_ptr<WriteBufferFromFileBase> LocalObjectStorage::writeObject( /// NO
 
 void LocalObjectStorage::removeObject(const StoredObject & object) const
 {
-    throwIfReadonly();
-
     /// For local object storage files are actually removed when "metadata" is removed.
     if (!exists(object))
         return;
 
     if (0 != unlink(object.remote_path.data()))
         ErrnoException::throwFromPath(ErrorCodes::CANNOT_UNLINK, object.remote_path, "Cannot unlink file {}", object.remote_path);
-
-    /// Remove empty directories.
-    fs::path dir = fs::path(object.remote_path).parent_path();
-    fs::path root = fs::weakly_canonical(settings.key_prefix);
-    while (dir.has_parent_path() && dir.has_relative_path() && dir != root && pathStartsWith(dir, root))
-    {
-        LOG_TEST(log, "Removing empty directory {}, has_parent_path: {}, has_relative_path: {}, root: {}, starts with root: {}",
-            std::string(dir), dir.has_parent_path(), dir.has_relative_path(), std::string(root), pathStartsWith(dir, root));
-
-        std::string dir_str = dir;
-        if (0 != rmdir(dir_str.data()))
-        {
-            if (errno == ENOTDIR || errno == ENOTEMPTY)
-                break;
-            ErrnoException::throwFromPath(ErrorCodes::CANNOT_RMDIR, dir_str, "Cannot remove directory {}", dir_str);
-        }
-
-        dir = dir.parent_path();
-    }
 }
 
 void LocalObjectStorage::removeObjects(const StoredObjects & objects) const
 {
-    throwIfReadonly();
     for (const auto & object : objects)
         removeObject(object);
 }
 
 void LocalObjectStorage::removeObjectIfExists(const StoredObject & object)
 {
-    throwIfReadonly();
     if (exists(object))
         removeObject(object);
 }
 
 void LocalObjectStorage::removeObjectsIfExist(const StoredObjects & objects)
 {
-    throwIfReadonly();
     for (const auto & object : objects)
         removeObjectIfExists(object);
 }
@@ -142,21 +113,14 @@ ObjectMetadata LocalObjectStorage::getObjectMetadata(const std::string & path) c
 {
     ObjectMetadata object_metadata;
     LOG_TEST(log, "Getting metadata for path: {}", path);
-
-    auto time = fs::last_write_time(path);
-
     object_metadata.size_bytes = fs::file_size(path);
-    object_metadata.etag = std::to_string(std::chrono::duration_cast<std::chrono::nanoseconds>(time.time_since_epoch()).count());
     object_metadata.last_modified = Poco::Timestamp::fromEpochTime(
-        std::chrono::duration_cast<std::chrono::seconds>(time.time_since_epoch()).count());
+        std::chrono::duration_cast<std::chrono::seconds>(fs::last_write_time(path).time_since_epoch()).count());
     return object_metadata;
 }
 
 void LocalObjectStorage::listObjects(const std::string & path, RelativePathsWithMetadata & children, size_t/* max_keys */) const
 {
-    if (!fs::is_directory(path))
-        return;
-
     for (const auto & entry : fs::directory_iterator(path))
     {
         if (entry.is_directory())
@@ -183,7 +147,6 @@ void LocalObjectStorage::copyObject( // NOLINT
     const WriteSettings & write_settings,
     std::optional<ObjectAttributes> /* object_to_attributes */)
 {
-    throwIfReadonly();
     auto in = readObject(object_from, read_settings);
     auto out = writeObject(object_to, WriteMode::Rewrite, /* attributes= */ {}, /* buf_size= */ DBMS_DEFAULT_BUFFER_SIZE, write_settings);
     copyData(*in, *out);
@@ -198,12 +161,6 @@ void LocalObjectStorage::startup()
 {
 }
 
-void LocalObjectStorage::throwIfReadonly() const
-{
-    if (settings.read_only)
-        throw Exception(ErrorCodes::READONLY, "Local object storage `{}` is readonly", getName());
-}
-
 std::unique_ptr<IObjectStorage> LocalObjectStorage::cloneObjectStorage(
     const std::string & /* new_namespace */,
     const Poco::Util::AbstractConfiguration & /* config */,
@@ -216,7 +173,7 @@ ObjectStorageKey
 LocalObjectStorage::generateObjectKeyForPath(const std::string & /* path */, const std::optional<std::string> & /* key_prefix */) const
 {
     constexpr size_t key_name_total_size = 32;
-    return ObjectStorageKey::createAsRelative(settings.key_prefix, getRandomASCIIString(key_name_total_size));
+    return ObjectStorageKey::createAsRelative(key_prefix, getRandomASCIIString(key_name_total_size));
 }
 
 }

src/Disks/ObjectStorages/Local/LocalObjectStorage.h

@@ -4,42 +4,28 @@
 
 #include <Disks/ObjectStorages/IObjectStorage.h>
 
-
 namespace Poco
 {
-    class Logger;
+class Logger;
 }
 
 namespace DB
 {
 
-struct LocalObjectStorageSettings
-{
-    LocalObjectStorageSettings(String key_prefix_, bool read_only_)
-        : key_prefix(key_prefix_), read_only(read_only_)
-    {
-    }
-
-    String key_prefix;
-    bool read_only = false;
-};
-
 /// Treat local disk as an object storage (for interface compatibility).
 class LocalObjectStorage : public IObjectStorage
 {
 public:
-    explicit LocalObjectStorage(LocalObjectStorageSettings settings_);
+    explicit LocalObjectStorage(String key_prefix_);
 
     std::string getName() const override { return "LocalObjectStorage"; }
 
     ObjectStorageType getType() const override { return ObjectStorageType::Local; }
 
-    std::string getCommonKeyPrefix() const override { return settings.key_prefix; }
+    std::string getCommonKeyPrefix() const override { return key_prefix; }
 
     std::string getDescription() const override { return description; }
 
-    bool isReadOnly() const override { return settings.read_only; }
-
     bool exists(const StoredObject & object) const override;
 
     std::unique_ptr<ReadBufferFromFileBase> readObject( /// NOLINT
@@ -95,11 +81,10 @@ class LocalObjectStorage : public IObjectStorage
 
 private:
     void removeObject(const StoredObject & object) const;
-    void removeObjects(const StoredObjects &  objects) const;
 
-    void throwIfReadonly() const;
+    void removeObjects(const StoredObjects &  objects) const;
 
-    LocalObjectStorageSettings settings;
+    String key_prefix;
     LoggerPtr log;
     std::string description;
 };