Merge pull request #54731 from ClickHouse/revert-54536-backport/23.3/…

…54430

Revert "Backport #54430 to 23.3: reproduce and fix the bug in removeSharedRecursive"

src/Disks/ObjectStorages/DiskObjectStorageTransaction.cpp

@@ -6,7 +6,6 @@
 #include <Common/Exception.h>
 
 #include <Disks/ObjectStorages/MetadataStorageFromDisk.h>
-#include <boost/algorithm/string/join.hpp>
 
 namespace DB
 {
@@ -156,13 +155,14 @@ struct RemoveObjectStorageOperation final : public IDiskObjectStorageOperation
 
 struct RemoveManyObjectStorageOperation final : public IDiskObjectStorageOperation
 {
-    const RemoveBatchRequest remove_paths;
-    const bool keep_all_batch_data;
-    const NameSet file_names_remove_metadata_only;
+    RemoveBatchRequest remove_paths;
+    bool keep_all_batch_data;
+    NameSet file_names_remove_metadata_only;
 
-    std::vector<String> paths_removed_with_objects;
     std::vector<ObjectsToRemove> objects_to_remove;
 
+    bool remove_from_cache = false;
+
     RemoveManyObjectStorageOperation(
         IObjectStorage & object_storage_,
         IMetadataStorage & metadata_storage_,
@@ -202,7 +202,6 @@ struct RemoveManyObjectStorageOperation final : public IDiskObjectStorageOperati
                 if (unlink_outcome && !keep_all_batch_data && !file_names_remove_metadata_only.contains(fs::path(path).filename()))
                 {
                     objects_to_remove.emplace_back(ObjectsToRemove{std::move(objects), std::move(unlink_outcome)});
-                    paths_removed_with_objects.push_back(path);
                 }
             }
             catch (const Exception & e)
@@ -213,12 +212,6 @@ struct RemoveManyObjectStorageOperation final : public IDiskObjectStorageOperati
                     || e.code() == ErrorCodes::CANNOT_READ_ALL_DATA
                     || e.code() == ErrorCodes::CANNOT_OPEN_FILE)
                 {
-                    LOG_DEBUG(
-                        &Poco::Logger::get("RemoveManyObjectStorageOperation"),
-                        "Can't read metadata because of an exception. Just remove it from the filesystem. Path: {}, exception: {}",
-                        metadata_storage.getPath() + path,
-                        e.message());
-
                     tx->unlinkFile(path);
                 }
                 else
@@ -244,31 +237,16 @@ struct RemoveManyObjectStorageOperation final : public IDiskObjectStorageOperati
         /// TL;DR Don't pay any attention to 404 status code
         if (!remove_from_remote.empty())
             object_storage.removeObjectsIfExist(remove_from_remote);
-
-        if (!keep_all_batch_data)
-        {
-            LOG_DEBUG(
-                &Poco::Logger::get("RemoveManyObjectStorageOperation"),
-                "metadata and objects were removed for [{}], "
-                "only metadata were removed for [{}].",
-                boost::algorithm::join(paths_removed_with_objects, ", "),
-                boost::algorithm::join(file_names_remove_metadata_only, ", "));
-        }
     }
 };
 
 
 struct RemoveRecursiveObjectStorageOperation final : public IDiskObjectStorageOperation
 {
-    /// path inside disk with metadata
-    const std::string path;
-    const bool keep_all_batch_data;
-    /// paths inside the 'this->path'
-    const NameSet file_names_remove_metadata_only;
-
-    /// map from local_path to its remote objects with hardlinks counter
-    /// local_path is the path inside 'this->path'
+    std::string path;
     std::unordered_map<std::string, ObjectsToRemove> objects_to_remove_by_path;
+    bool keep_all_batch_data;
+    NameSet file_names_remove_metadata_only;
 
     RemoveRecursiveObjectStorageOperation(
         IObjectStorage & object_storage_,
@@ -295,16 +273,11 @@ struct RemoveRecursiveObjectStorageOperation final : public IDiskObjectStorageOp
         {
             try
             {
-                chassert(path_to_remove.starts_with(path));
-                auto rel_path = String(fs::relative(fs::path(path_to_remove), fs::path(path)));
-
                 auto objects_paths = metadata_storage.getStorageObjects(path_to_remove);
                 auto unlink_outcome = tx->unlinkMetadata(path_to_remove);
-
-                if (unlink_outcome && !file_names_remove_metadata_only.contains(rel_path))
+                if (unlink_outcome)
                 {
-                    objects_to_remove_by_path[std::move(rel_path)]
-                        = ObjectsToRemove{std::move(objects_paths), std::move(unlink_outcome)};
+                    objects_to_remove_by_path[path_to_remove] = ObjectsToRemove{std::move(objects_paths), std::move(unlink_outcome)};
                 }
             }
             catch (const Exception & e)
@@ -346,38 +319,25 @@ struct RemoveRecursiveObjectStorageOperation final : public IDiskObjectStorageOp
 
     void undo() override
     {
+
     }
 
     void finalize() override
     {
         if (!keep_all_batch_data)
         {
-            std::vector<String> total_removed_paths;
-            total_removed_paths.reserve(objects_to_remove_by_path.size());
-
             StoredObjects remove_from_remote;
             for (auto && [local_path, objects_to_remove] : objects_to_remove_by_path)
             {
-                chassert(!file_names_remove_metadata_only.contains(local_path));
-                if (objects_to_remove.unlink_outcome->num_hardlinks == 0)
+                if (!file_names_remove_metadata_only.contains(fs::path(local_path).filename()))
                 {
-                    std::move(objects_to_remove.objects.begin(), objects_to_remove.objects.end(), std::back_inserter(remove_from_remote));
-                    total_removed_paths.push_back(local_path);
+                    if (objects_to_remove.unlink_outcome->num_hardlinks == 0)
+                        std::move(objects_to_remove.objects.begin(), objects_to_remove.objects.end(), std::back_inserter(remove_from_remote));
                 }
             }
-
             /// Read comment inside RemoveObjectStorageOperation class
             /// TL;DR Don't pay any attention to 404 status code
             object_storage.removeObjectsIfExist(remove_from_remote);
-
-            LOG_DEBUG(
-                &Poco::Logger::get("RemoveRecursiveObjectStorageOperation"),
-                "Recursively remove path {}: "
-                "metadata and objects were removed for [{}], "
-                "only metadata were removed for [{}].",
-                path,
-                boost::algorithm::join(total_removed_paths, ", "),
-                boost::algorithm::join(file_names_remove_metadata_only, ", "));
         }
     }
 };

src/Disks/ObjectStorages/S3/S3ObjectStorage.cpp

@@ -306,7 +306,7 @@ void S3ObjectStorage::removeObjectImpl(const StoredObject & object, bool if_exis
 
     throwIfUnexpectedError(outcome, if_exists);
 
-    LOG_DEBUG(log, "Object with path {} was removed from S3", object.absolute_path);
+    LOG_TRACE(log, "Object with path {} was removed from S3", object.absolute_path);
 }
 
 void S3ObjectStorage::removeObjectsImpl(const StoredObjects & objects, bool if_exists)
@@ -354,7 +354,7 @@ void S3ObjectStorage::removeObjectsImpl(const StoredObjects & objects, bool if_e
 
             throwIfUnexpectedError(outcome, if_exists);
 
-            LOG_DEBUG(log, "Objects with paths [{}] were removed from S3", keys);
+            LOG_TRACE(log, "Objects with paths [{}] were removed from S3", keys);
         }
     }
 }

src/Storages/StorageReplicatedMergeTree.cpp

@@ -1466,8 +1466,6 @@ MergeTreeData::DataPartsVector StorageReplicatedMergeTree::checkPartChecksumsAnd
 
     while (true)
     {
-        LOG_DEBUG(log, "Committing part {} to zookeeper", part->name);
-
         Coordination::Requests ops;
         NameSet absent_part_paths_on_replicas;
 
@@ -8177,14 +8175,6 @@ void StorageReplicatedMergeTree::lockSharedData(
 
         LOG_TRACE(log, "Set zookeeper persistent lock {}", zookeeper_node);
 
-        if (!path_to_set_hardlinked_files.empty() && !hardlinks.empty())
-        {
-            LOG_DEBUG(log, "Locking shared node {} with hardlinks from the other shared node {}, "
-                           "hardlinks: [{}]",
-                      zookeeper_node, path_to_set_hardlinked_files,
-                      boost::algorithm::join(hardlinks, ","));
-        }
-
         createZeroCopyLockNode(
             zookeeper, zookeeper_node, zkutil::CreateMode::Persistent,
             replace_existing_lock, path_to_set_hardlinked_files, hardlinks);