SYSTEM UNFREEZE query that deletes the whole backup #36424

PolyProgrammist · 2022-04-19T14:09:24Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added SYSTEM UNFREEZE query that deletes the whole backup regardless if the corresponding table is deleted or not.

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

#34794

Code to be deduplicated, tests and docs to be added later in this PR.

CLAassistant · 2022-04-19T14:09:30Z

All committers have signed the CLA.

ianton-ru · 2022-04-20T08:37:13Z

src/Interpreters/InterpreterSystemQuery.cpp

+                auto store_path = backup_path / "store";
+
+                for (auto disk: disks) {
+                    if (!disk->exists(store_path))


Also check and remove backup_path if exists.

ianton-ru · 2022-04-20T08:38:51Z

src/Interpreters/InterpreterSystemQuery.cpp

+                        continue;
+                    for (auto it = disk->iterateDirectory(store_path); it->isValid(); it->next()) {
+                        const auto & prefix_directory = it->name();
+                        auto absolute_prefix_directory = store_path / prefix_directory;


Name absolute_... may confuse here, it's relative to disk root.

ianton-ru · 2022-04-20T08:42:09Z

src/Interpreters/InterpreterSystemQuery.cpp

+                    disk->removeRecursive(backup_path);
+                }
+
+            LOG_ERROR(&Poco::Logger::get("AwesomeClass"), "Unfreezing{}", "b");


InterpreterSystemQuery has a log member, use it - LOG_ERROR(log, ...)
In other case, use class name, like LOG_ERROR(&Poco::Logger::get("InterpreterSystemQuery"), ...)

And what is last "b"?

ianton-ru · 2022-04-20T08:54:45Z

src/Interpreters/InterpreterSystemQuery.cpp

+                        for (auto it = disk->iterateDirectory(absolute_prefix_directory); it->isValid(); it->next()) {
+                            const auto & table_directory = it->name();
+                            auto absolute_table_directory = absolute_prefix_directory / table_directory;
+                            MergeTreeData::unfreezePartitionsFromTableDirectory([] (const String &) { return true; }, query.backup_name, disks, absolute_table_directory, &Poco::Logger::get("AwesomeClass"), getContext());


MergeTreeData has own log member, not necessary to send log object from here (and in common case do not send it from class to class, use own log in each class).

ianton-ru · 2022-04-20T08:55:46Z

src/Interpreters/InterpreterSystemQuery.cpp

+        case Type::UNFREEZE: 
+        {
+            LOG_ERROR(&Poco::Logger::get("AwesomeClass"), "getrequiredlalal{}", "b");
+            exit(0);


ianton-ru · 2022-04-20T09:02:48Z

src/Storages/MergeTree/MergeTreeData.cpp

-PartitionCommandsResultInfo MergeTreeData::unfreezePartitionsByMatcher(MatcherFn matcher, const String & backup_name, ContextPtr)
+// copy and some edits from StorageReplicatedMergeTree::removeSharedDetachedPart
+bool removeSharedDetachedPart(DiskPtr disk, const String & path, const String & part_name, const String & table_uuid,
+    const String &, const String & detached_replica_name, const String & detached_zookeeper_path, Poco::Logger * log, ContextPtr local_context)


This code (removeSharedDetachedPart, removeDetachedPart and may be unfreezePartitionsFromTableDirectory) should be moved in some new separate class, I think. FreezeMetaData struct too.
Non-replicated MergeTreeData should know nothing about ZooKeeper.

ianton-ru · 2022-04-20T09:06:18Z

src/Storages/StorageReplicatedMergeTree.h

+struct FreezeMetaData
+{
+public:
+    void fill(const StorageReplicatedMergeTree & storage)


Leave methods definitions in cpp file.

ianton-ru · 2022-04-20T09:09:15Z

src/Parsers/ParserSystemQuery.cpp

@@ -5,6 +5,7 @@
 #include <Parsers/ASTIdentifier.h>
 #include <Parsers/ASTLiteral.h>
 #include <Parsers/parseDatabaseAndTableName.h>
+#include <base/logger_useful.h>


Is log used somewhere here?

GrigoryPervakov · 2022-04-22T12:51:25Z

src/Access/Common/AccessType.h

@@ -162,6 +162,7 @@ enum class AccessType
    M(SYSTEM_FLUSH_LOGS, "FLUSH LOGS", GLOBAL, SYSTEM_FLUSH) \
    M(SYSTEM_FLUSH, "", GROUP, SYSTEM) \
    M(SYSTEM_THREAD_FUZZER, "SYSTEM START THREAD FUZZER, SYSTEM STOP THREAD FUZZER, START THREAD FUZZER, STOP THREAD FUZZER", GLOBAL, SYSTEM) \
+    M(SYSTEM_UNFREEZE, "RESTART REPLICA", GLOBAL, SYSTEM) \


Strange permission

ianton-ru · 2022-04-25T18:07:31Z

src/Interpreters/InterpreterSystemQuery.cpp

+        case Type::UNFREEZE:
+        {
+            getContext()->checkAccess(AccessType::SYSTEM_UNFREEZE);
+            result = Unfreezer().unfreeze(query.backup_name, getContext());


This is the only one place in this method where some result returned. I thing here should be a comment with explanation, why.

PolyProgrammist · 2022-05-04T12:44:43Z

Comments from GrigoryPervakov and ianton-ru are fixed

antonio2368

Please merge the latest master

src/Storages/Freeze.h

src/Storages/Freeze.cpp

tests/queries/0_stateless/01417_freeze_partition_verbose.sh

PolyProgrammist · 2022-05-27T18:54:12Z

@antonio2368 all comments have been addressed

src/Storages/Freeze.cpp

src/Storages/Freeze.h

PolyProgrammist · 2022-05-31T11:23:35Z

@antonio2368 thank you, all comments have been addressed

antonio2368

Very nice tests

src/Storages/Freeze.cpp

antonio2368 · 2022-06-01T13:37:36Z

One of the freeze tests failed
https://s3.amazonaws.com/clickhouse-test-reports/36424/a530b47769e2ba6712b6475bd39941acac0b7976/stateless_tests__release__s3_storage__actions_.html

tavplubix · 2022-06-15T09:57:53Z

The new test 01417_freeze_partition_verbose fails in check ClickHouse Stateless Tests (release, Databaseordinary, Actions) (we run this check only in master, so checks in this PR were OK). I disabled this test for Ordinary database (dcfe4ee), please fix it if it was supposed to work with Ordinary database as well (and ignore this comment if it was not).

PolyProgrammist · 2022-06-15T10:01:28Z

Ok, I will fix the test, thank you @tavplubix

PolyProgrammist · 2022-06-21T09:26:04Z

@tavplubix #38262 (review)

src/Parsers/ParserSystemQuery.cpp

src/Storages/Freeze.cpp

src/Storages/MergeTree/MergeTreeData.cpp

src/Storages/Freeze.cpp

tavplubix · 2022-06-21T17:41:00Z

src/Storages/Freeze.cpp

+            const auto & partition_directory = it->name();
+
+            /// Partition ID is prefix of part directory name: <partition id>_<rest of part directory name>
+            auto found = partition_directory.find('_');
+            if (found == std::string::npos)
+                continue;
+            auto partition_id = partition_directory.substr(0, found);
+
+            if (!matcher(partition_id))
+                continue;


It does not work correctly if table was created with old syntax. It's ok to drop support for UNFREEZE in this case, but we should throw an exception at least.

forgot about that

Do you mean to match old syntax and if there is an old syntax, throw an exception? How do I match old syntax? What if user wants to unfreeze just something but instead gets an exception and no unfreezed data?

Do you mean to match old syntax and if there is an old syntax, throw an exception?

Yep, it's enough (but you can add support for old syntax if you want).

How do I match old syntax?

When unfreezePartitionsFromTableDirectory is called from MergeTreeData you can pass format_version as an argument. And when it's called Unfreezer::unfreeze you don't need to parse partition id at all.

src/Storages/Freeze.h

src/Storages/StorageReplicatedMergeTree.cpp

tavplubix · 2022-06-24T16:28:32Z

src/Storages/StorageReplicatedMergeTree.cpp

 {
    bool keep_shared = false;

-    zkutil::ZooKeeperPtr zookeeper = getZooKeeper();
+    zkutil::ZooKeeperPtr zookeeper = local_context->getZooKeeper();


This looks absolutely incorrect. Usually all operations with zk during a query/task execution must be done in single session.

PolyProgrammist · 2022-06-24T17:56:12Z

@tavplubix fixed everything except zookeeper here #38262 (review)

alexey-milovidov · 2022-07-21T02:32:16Z

@PolyProgrammist

The query sounds dangerous. What protection do we have to disallow it by default?
Maybe add a server configuration option?

PolyProgrammist · 2022-07-21T10:00:47Z

@PolyProgrammist

The query sounds dangerous. What protection do we have to disallow it by default? Maybe add a server configuration option?

Sound like a good idea

PolyProgrammist · 2022-07-21T10:31:08Z

@PolyProgrammist

The query sounds dangerous. What protection do we have to disallow it by default? Maybe add a server configuration option?

Why does that sound dangerous? There is already a query to unfreeze partition or a table. Is it because of recursive deletion?

PolyProgrammist · 2022-07-22T10:25:35Z

Zookeeper and server config fixed here #38262

robot-ch-test-poll added the pr-feature Pull request with new product feature label Apr 19, 2022

PolyProgrammist changed the title ~~R1unfreeze~~ SYSTEM UNFREEZE query that deletes the whole backup Apr 19, 2022

ianton-ru suggested changes Apr 20, 2022

View reviewed changes

GrigoryPervakov reviewed Apr 22, 2022

View reviewed changes

ianton-ru reviewed Apr 25, 2022

View reviewed changes

PolyProgrammist force-pushed the r1unfreeze branch from 300ad63 to 4b581dc Compare April 27, 2022 16:25

antonio2368 self-assigned this May 20, 2022

antonio2368 reviewed May 20, 2022

View reviewed changes

PolyProgrammist force-pushed the r1unfreeze branch 2 times, most recently from 81c00f2 to bc45ebc Compare May 27, 2022 18:49

PolyProgrammist force-pushed the r1unfreeze branch from bc45ebc to 0e40c5f Compare May 30, 2022 10:25

antonio2368 reviewed May 31, 2022

View reviewed changes

src/Storages/Freeze.cpp Outdated Show resolved Hide resolved

src/Storages/Freeze.cpp Outdated Show resolved Hide resolved

src/Storages/Freeze.h Outdated Show resolved Hide resolved

PolyProgrammist force-pushed the r1unfreeze branch from 3dff44e to 781acbd Compare May 31, 2022 11:23

antonio2368 approved these changes Jun 1, 2022

View reviewed changes

src/Storages/Freeze.cpp Show resolved Hide resolved

src/Storages/Freeze.cpp Outdated Show resolved Hide resolved

Add SYSTEM UNFREEZE query

637d293

PolyProgrammist force-pushed the r1unfreeze branch from a530b47 to 637d293 Compare June 8, 2022 12:29

antonio2368 added 3 commits June 13, 2022 09:53

Fix trailing whitespace

e67b6c3

Fix revision write for Freeze

879e162

Merge branch 'master' into pp-system-unfreeze

b9cf6fe

antonio2368 merged commit dded528 into ClickHouse:master Jun 14, 2022

PolyProgrammist mentioned this pull request Jun 21, 2022

Fix SYSTEM UNFREEZE for ordinary database #38262

Merged