Reimplement FREEZE command.

.gitignore

@@ -11,6 +11,7 @@
 
 /build
 /build_*
+/build-*
 /docs/build
 /docs/edit
 /docs/tools/venv/

dbms/src/Interpreters/InterpreterAlterQuery.cpp

@@ -85,10 +85,6 @@ BlockIO InterpreterAlterQuery::execute()
                 table->freezePartition(command.partition, command.with_name, context);
                 break;
 
-            case PartitionCommand::FREEZE_ALL:
-                table->freezeAll(command.with_name, context);
-                break;
-
             case PartitionCommand::CLEAR_COLUMN:
                 table->clearColumnInPartition(command.partition, command.column_name, context);
                 break;

dbms/src/Storages/AlterCommands.cpp

@@ -91,6 +91,13 @@ std::optional<AlterCommand> AlterCommand::parse(const ASTAlterCommand * command_
         command.primary_key = command_ast->primary_key;
         return command;
     }
+    else if (command_ast->type == ASTAlterCommand::FREEZE_ALL)
+    {
+        AlterCommand command;
+        command.type = AlterCommand::FREEZE_ALL;
+        command.with_name = command_ast->with_name;
+        return command;
+    }
     else
         return {};
 }
@@ -238,6 +245,10 @@ void AlterCommand::apply(ColumnsDescription & columns_description) const
         /// This have no relation to changing the list of columns.
         /// TODO Check that all columns exist, that only columns with constant defaults are added.
     }
+    else if (type == FREEZE_ALL)
+    {
+        /// Do nothing with columns.
+    }
     else
         throw Exception("Wrong parameter type in ALTER query", ErrorCodes::LOGICAL_ERROR);
 }

dbms/src/Storages/AlterCommands.h

@@ -12,13 +12,19 @@ namespace DB
 
 class ASTAlterCommand;
 
-/// Operation from the ALTER query (except for manipulation with PART/PARTITION). Adding Nested columns is not expanded to add individual columns.
+/// Operation from the ALTER query (except for manipulation with PART/PARTITION).
+/// Adding Nested columns is not expanded to add individual columns.
 struct AlterCommand
 {
     enum Type
     {
         ADD_COLUMN,
         DROP_COLUMN,
+
+        FREEZE_ALL,
+        // Even though this command operates on partitions, it needs global locks to prevent table alteration.
+        // It's vulnerable to the column modification commands.
+
         MODIFY_COLUMN,
         MODIFY_PRIMARY_KEY,
     };
@@ -30,6 +36,9 @@ struct AlterCommand
     /// For DROP COLUMN ... FROM PARTITION
     String partition_name;
 
+    /// For FREEZE of all partitions
+    String with_name;
+
     /// For ADD and MODIFY, a new column type.
     DataTypePtr data_type;
 

dbms/src/Storages/IStorage.h

@@ -278,13 +278,6 @@ public:
         throw Exception("Method freezePartition is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED);
     }
 
-    /** Run the FREEZE request. That is, create a local backup (snapshot) of data using the `localBackup` function (see localBackup.h)
-      */
-    virtual void freezeAll(const String & /*with_name*/, const Context & /*context*/)
-    {
-        throw Exception("Method freezeAll is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED);
-    }
-
     /** Perform any background work. For example, combining parts in a MergeTree type table.
       * Returns whether any work has been done.
       */

dbms/src/Storages/MergeTree/MergeTreeData.cpp

@@ -1279,6 +1279,40 @@ MergeTreeData::AlterDataPartTransactionPtr MergeTreeData::alterDataPart(
     return transaction;
 }
 
+void MergeTreeData::freezeAll(const String & with_name, const Context & context)
+{
+    String clickhouse_path = Poco::Path(context.getPath()).makeAbsolute().toString();
+    String shadow_path = clickhouse_path + "shadow/";
+    Poco::File(shadow_path).createDirectories();
+    String backup_path = shadow_path
+        + (!with_name.empty()
+            ? escapeForFileName(with_name)
+            : toString(Increment(shadow_path + "increment.txt").get(true)))
+        + "/";
+
+    LOG_DEBUG(log, "Snapshot will be placed at " + backup_path);
+
+    /// Acquire a snapshot of active data parts to prevent removing while doing backup.
+    const auto data_parts = getDataParts();
+
+    size_t parts_processed = 0;
+    for (const auto & part : data_parts)
+    {
+        LOG_DEBUG(log, "Freezing part " << part->name);
+
+        String part_absolute_path = Poco::Path(part->getFullPath()).absolute().toString();
+        if (!startsWith(part_absolute_path, clickhouse_path))
+            throw Exception("Part path " + part_absolute_path + " is not inside " + clickhouse_path, ErrorCodes::LOGICAL_ERROR);
+
+        String backup_part_absolute_path = part_absolute_path;
+        backup_part_absolute_path.replace(0, clickhouse_path.size(), backup_path);
+        localBackup(part_absolute_path, backup_part_absolute_path);
+        ++parts_processed;
+    }
+
+    LOG_DEBUG(log, "Freezed " << parts_processed << " parts");
+}
+
 void MergeTreeData::AlterDataPartTransaction::commit()
 {
     if (!data_part)
@@ -2104,42 +2138,6 @@ void MergeTreeData::freezePartition(const ASTPtr & partition_ast, const String &
     LOG_DEBUG(log, "Freezed " << parts_processed << " parts");
 }
 
-void MergeTreeData::freezeAll(const String & with_name, const Context & context)
-{
-    String clickhouse_path = Poco::Path(context.getPath()).makeAbsolute().toString();
-    String shadow_path = clickhouse_path + "shadow/";
-    Poco::File(shadow_path).createDirectories();
-    String backup_path = shadow_path
-        + (!with_name.empty()
-            ? escapeForFileName(with_name)
-            : toString(Increment(shadow_path + "increment.txt").get(true)))
-        + "/";
-
-    LOG_DEBUG(log, "Snapshot will be placed at " + backup_path);
-
-    auto lock = lockStructureForAlter(__PRETTY_FUNCTION__);
-
-    /// Acquire a snapshot of active data parts to prevent removing while doing backup.
-    const auto data_parts = getDataParts();
-
-    size_t parts_processed = 0;
-    for (const auto & part : data_parts)
-    {
-        LOG_DEBUG(log, "Freezing part " << part->name);
-
-        String part_absolute_path = Poco::Path(part->getFullPath()).absolute().toString();
-        if (!startsWith(part_absolute_path, clickhouse_path))
-            throw Exception("Part path " + part_absolute_path + " is not inside " + clickhouse_path, ErrorCodes::LOGICAL_ERROR);
-
-        String backup_part_absolute_path = part_absolute_path;
-        backup_part_absolute_path.replace(0, clickhouse_path.size(), backup_path);
-        localBackup(part_absolute_path, backup_part_absolute_path);
-        ++parts_processed;
-    }
-
-    LOG_DEBUG(log, "Freezed " << parts_processed << " parts");
-}
-
 size_t MergeTreeData::getPartitionSize(const std::string & partition_id) const
 {
     size_t size = 0;

dbms/src/Storages/MergeTree/MergeTreeData.h

@@ -480,6 +480,9 @@ public:
         const ASTPtr & new_primary_key,
         bool skip_sanity_checks);
 
+    /// Freezes all parts.
+    void freezeAll(const String & with_name, const Context & context);
+
     /// Should be called if part data is suspected to be corrupted.
     void reportBrokenPart(const String & name) const
     {
@@ -501,12 +504,6 @@ public:
       */
     void freezePartition(const ASTPtr & partition, const String & with_name, const Context & context);
 
-    /** Create local backup (snapshot) for all parts.
-      * Backup is created in directory clickhouse_dir/shadow/i/, where i - incremental number,
-      *  or if 'with_name' is specified - backup is created in directory with specified name.
-      */
-    void freezeAll(const String & with_name, const Context & context);
-
     /// Returns the size of partition in bytes.
     size_t getPartitionSize(const std::string & partition_id) const;
 

dbms/src/Storages/PartitionCommands.cpp

@@ -58,13 +58,6 @@ std::optional<PartitionCommand> PartitionCommand::parse(const ASTAlterCommand *
         res.with_name = command_ast->with_name;
         return res;
     }
-    else if (command_ast->type == ASTAlterCommand::FREEZE_ALL)
-    {
-        PartitionCommand res;
-        res.type = FREEZE_ALL;
-        res.with_name = command_ast->with_name;
-        return res;
-    }
     else if (command_ast->type == ASTAlterCommand::DROP_COLUMN && command_ast->partition)
     {
         if (!command_ast->clear_column)

dbms/src/Storages/PartitionCommands.h

@@ -17,13 +17,12 @@ struct PartitionCommand
 {
     enum Type
     {
-        DROP_PARTITION,
         ATTACH_PARTITION,
-        REPLACE_PARTITION,
+        CLEAR_COLUMN,
+        DROP_PARTITION,
         FETCH_PARTITION,
         FREEZE_PARTITION,
-        FREEZE_ALL,
+        REPLACE_PARTITION,
-        CLEAR_COLUMN,
     };
 
     Type type;

dbms/src/Storages/StorageMaterializedView.cpp

@@ -263,12 +263,6 @@ void StorageMaterializedView::freezePartition(const ASTPtr & partition, const St
     getTargetTable()->freezePartition(partition, with_name, context);
 }
 
-void StorageMaterializedView::freezeAll(const String & with_name, const Context & context)
-{
-    checkStatementCanBeForwarded();
-    getTargetTable()->freezeAll(with_name, context);
-}
-
 void StorageMaterializedView::mutate(const MutationCommands & commands, const Context & context)
 {
     checkStatementCanBeForwarded();

dbms/src/Storages/StorageMaterializedView.h

@@ -39,7 +39,6 @@ public:
     void clearColumnInPartition(const ASTPtr & partition, const Field & column_name, const Context & context) override;
     void attachPartition(const ASTPtr & partition, bool part, const Context & context) override;
     void freezePartition(const ASTPtr & partition, const String & with_name, const Context & context) override;
-    void freezeAll(const String & with_name, const Context & context) override;
     void mutate(const MutationCommands & commands, const Context & context) override;
 
     void shutdown() override;

dbms/src/Storages/StorageMergeTree.cpp

@@ -261,6 +261,15 @@ void StorageMergeTree::alter(
 
     if (primary_key_is_modified)
         data.loadDataParts(false);
+
+    /// Do freeze of all parts after all other operations.
+    for (const AlterCommand & param : params)
+    {
+        if (param.type == AlterCommand::FREEZE_ALL)
+        {
+            data.freezeAll(param.with_name, context);
+        }
+    }
 }
 
 
@@ -863,11 +872,6 @@ void StorageMergeTree::freezePartition(const ASTPtr & partition, const String &
     data.freezePartition(partition, with_name, context);
 }
 
-void StorageMergeTree::freezeAll(const String & with_name, const Context & context)
-{
-    data.freezeAll(with_name, context);
-}
-
 void StorageMergeTree::replacePartitionFrom(const StoragePtr & source_table, const ASTPtr & partition, bool replace, const Context & context)
 {
     auto lock1 = lockStructure(false, __PRETTY_FUNCTION__);

dbms/src/Storages/StorageMergeTree.h

@@ -71,7 +71,6 @@ public:
     void attachPartition(const ASTPtr & partition, bool part, const Context & context) override;
     void replacePartitionFrom(const StoragePtr & source_table, const ASTPtr & partition, bool replace, const Context & context) override;
     void freezePartition(const ASTPtr & partition, const String & with_name, const Context & context) override;
-    void freezeAll(const String & with_name, const Context & context) override;
 
     void mutate(const MutationCommands & commands, const Context & context) override;
 

dbms/src/Storages/StorageReplicatedMergeTree.cpp

@@ -4160,12 +4160,6 @@ void StorageReplicatedMergeTree::freezePartition(const ASTPtr & partition, const
 }
 
 
-void StorageReplicatedMergeTree::freezeAll(const String & with_name, const Context & context)
-{
-    data.freezeAll(with_name, context);
-}
-
-
 void StorageReplicatedMergeTree::mutate(const MutationCommands & commands, const Context &)
 {
     /// Overview of the mutation algorithm.

dbms/src/Storages/StorageReplicatedMergeTree.h

@@ -122,7 +122,6 @@ public:
     void replacePartitionFrom(const StoragePtr & source_table, const ASTPtr & partition, bool replace, const Context & context) override;
     void fetchPartition(const ASTPtr & partition, const String & from, const Context & context) override;
     void freezePartition(const ASTPtr & partition, const String & with_name, const Context & context) override;
-    void freezeAll(const String & with_name, const Context & context) override;
 
     void mutate(const MutationCommands & commands, const Context & context) override;
 

dbms/tests/clickhouse-test

@@ -277,7 +277,7 @@ def main(args):
                                         result_is_different = subprocess.call(['cmp', '-s', reference_file, stdout_file], stdout = PIPE)
 
                                         if result_is_different:
-                                            (diff, _) = Popen(['diff', '--side-by-side', reference_file, stdout_file], stdout = PIPE).communicate()
+                                            (diff, _) = Popen(['diff', '-u', reference_file, stdout_file], stdout = PIPE).communicate()
                                             diff = unicode(diff, errors='replace', encoding='utf-8')
 
                                             failure = et.Element("failure", attrib = {"message": "result differs with reference"})

dbms/tests/queries/0_stateless/00428_partition.reference

@@ -1,6 +1,44 @@
 5
 5
+55a54008ad1ba589aa210d2629c1df41  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/primary.idx
+88cdc31ded355e7572d68d8cde525d3a  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/p.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/p.mrk
+e2af3bef1fd129aea73a890ede1e7a30  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/k.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/k.mrk
+082814b5aa5109160d5c0c5aff10d4df  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/v1.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/v1.mrk
+77d5af402ada101574f4da114f242e02  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/columns.txt
+b01e3d4df40467db3f1c2d029f59f6a2  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/checksums.txt
+9e688c58a5487b8eaf69c9e1005ad0bf  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/primary.idx
+cfcb770c3ecd0990dcceb1bde129e6c6  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/p.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/p.mrk
+082814b5aa5109160d5c0c5aff10d4df  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/k.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/k.mrk
+38e62ff37e1e5064e9a3f605dfe09d13  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/v1.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/v1.mrk
+77d5af402ada101574f4da114f242e02  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/columns.txt
+e6654eba9e88b001280d3bdd21ccc417  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/checksums.txt
+b026324c6904b2a9cb4b88d6d61c81d1  shadow/increment.txt
 5
 5
+55a54008ad1ba589aa210d2629c1df41  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/primary.idx
+88cdc31ded355e7572d68d8cde525d3a  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/p.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/p.mrk
+e2af3bef1fd129aea73a890ede1e7a30  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/k.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/k.mrk
+082814b5aa5109160d5c0c5aff10d4df  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/v1.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/v1.mrk
+77d5af402ada101574f4da114f242e02  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/columns.txt
+b01e3d4df40467db3f1c2d029f59f6a2  shadow/1/data/test/partition_428/19700201_19700201_1_1_0/checksums.txt
+9e688c58a5487b8eaf69c9e1005ad0bf  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/primary.idx
+cfcb770c3ecd0990dcceb1bde129e6c6  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/p.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/p.mrk
+082814b5aa5109160d5c0c5aff10d4df  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/k.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/k.mrk
+38e62ff37e1e5064e9a3f605dfe09d13  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/v1.bin
+4ae71336e44bf9bf79d2752e234818a5  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/v1.mrk
+77d5af402ada101574f4da114f242e02  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/columns.txt
+e6654eba9e88b001280d3bdd21ccc417  shadow/1/data/test/partition_428/19700102_19700102_2_2_0/checksums.txt
+b026324c6904b2a9cb4b88d6d61c81d1  shadow/increment.txt
 31,1,2
 1,2,3

dbms/tests/queries/0_stateless/00428_partition.sh

@@ -25,9 +25,8 @@ done
 
 $chl "ALTER TABLE test.partition_428 FREEZE"
 
-pushd
+# Do `cd` for consistent output for reference
-cd $chdir && find shadow -type f -exec md5sum {} \;
+cd $ch_dir && find shadow -type f -exec md5sum {} \;
-popd
 
 $chl "ALTER TABLE test.partition_428 DETACH PARTITION 197001"
 $chl "ALTER TABLE test.partition_428 ATTACH PARTITION 197001"
@@ -40,10 +39,8 @@ done
 
 $chl "ALTER TABLE test.partition_428 MODIFY COLUMN v1 Int8"
 
-# Check that backup hasn't changed
+# Check the backup hasn't changed
-pushd
+cd $ch_dir && find shadow -type f -exec md5sum {} \;
-cd $chdir && find shadow -type f -exec md5sum {} \;
-popd
 
 $chl "OPTIMIZE TABLE test.partition_428"