Fix SYSTEM UNFREEZE for ordinary database #38262

PolyProgrammist · 2022-06-21T09:20:56Z

Changelog category (leave one):

Bug Fix (user-visible misbehavior in official stable or prestable release)

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

Fix SYSTEM UNFREEZE query for Ordinary database. Fix for #36424

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

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

src/Storages/Freeze.cpp

-        if (!disk->exists(store_path))
-            continue;
-        for (auto prefix_it = disk->iterateDirectory(store_path); prefix_it->isValid(); prefix_it->next())
+        for (auto store_path: store_paths)


Just for loop added

tavplubix · 2022-06-21T17:53:18Z

I looked through the changes in previous PR (just to understand changes in this one) and left some comments in #36424, please take a look. You can address the comments in this PR.

tavplubix · 2022-06-24T18:12:36Z

src/Storages/Freeze.cpp

+    if (version == 1) {
+        /// is_replicated and is_remote are not used
+        bool is_replicated = true;
+        writeBoolText(is_replicated, buffer);
+        buffer.write("\n", 1);
+        bool is_remote = true;
+        writeBoolText(is_remote, buffer);
+        buffer.write("\n", 1);


I would prefer to completely remove these flags without changing version. It will break compatibility with 22.6.1, but I think it's not a problem, because 22.6 was just released recently and I doubt that someone has already started to use 22.6.1 with this freeze-metadata-stuff in production. And previous serialization format was invalid anyway (I mean issues with escaping).

The thing is, flags were introduced before this PR. See history of removed code for details

tavplubix · 2022-06-24T18:13:44Z

src/Storages/Freeze.cpp

+        writeBoolText(is_remote, buffer);
+        buffer.write("\n", 1);
+    }
+    writeString(escapeForFileName(replica_name), buffer);


Please also check that other strings cannot contain unexpected chars

tavplubix · 2022-06-24T18:17:33Z

src/Storages/Freeze.cpp

            }
        }
        if (disk->exists(backup_path))
        {
+            /// After unfreezing we need to clear revision.txt file and empty directories


Still not clear why we cannot just remove everything recursively. Why do we need complex "unfreeze" logic?

/// After unfreezing we need to clear revision.txt file and empty directories.
/// revision.txt file shouldn't be unfreezed, it should just be deleted

Is that clear?

No. I mean that FREEZE PARTITION only creates hardlinks. So UNFREEZE should only remove hardlinks. It's not clear why do we need some complex logic for "unfreezing", it was supposed to do just rm -rf. I understand that all this stuff is needed for "s3 zero copy replication", but I do not understand how "s3 zero copy replication" (and especially "unfreezing" with "s3 zero copy replication") work because of lack of comments. A comment that describes how all this stuff works is required.

tavplubix · 2022-06-24T18:18:54Z

See also #36424 (comment)

tavplubix · 2022-06-24T18:19:38Z

Issue with zk session should be fixed as well

PolyProgrammist · 2022-06-24T18:29:46Z

Also: 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.

Freeze.cpp: 178

alexey-milovidov · 2022-07-21T02:30:42Z

Ordinary databases are deprecated in 22.7.
Is this PR worth merging?

tavplubix · 2022-07-21T09:54:11Z

Ordinary databases are deprecated in 22.7. Is this PR worth merging?

Yes, because it also addresses review comments from #36424. Previous PR was merged by mistake and it has to be fixed. We can drop support for Ordinary, but we need the rest fixes.

PolyProgrammist · 2022-07-21T13:58:34Z

@tavplubix please check zookeeper and partition matching. do you mean something like that? 85bf022

tavplubix · 2022-07-21T14:18:33Z

src/Storages/Freeze.cpp

@@ -179,8 +184,13 @@ PartitionCommandsResultInfo Unfreezer::unfreezePartitionsFromTableDirectory(Merg
        for (auto it = disk->iterateDirectory(table_directory); it->isValid(); it->next())
        {
            const auto & partition_directory = it->name();
+
+            int count_underscores = std::count_if(partition_directory.begin(), partition_directory.end(), []( char c ){return c =='_';});
+            if ((format_version.has_value() && format_version == 0) || count_underscores == 4) {


Number of underscores can be 4 with new syntax as well (if mutation version is not 0)

Also we don't need to parse partition_id at all if matcher always returns true (when it called from Unfreezer::unfreeze or similar place). Currently it can parse some garbage, but simply does not throw exception in this case.

I don't understand. Parsing partition_id is needed in order to output it. If I remove check with underscores,

ALTER TABLE UNFREEZE will delete data only in case if format_version is more than zero.

SYSTEM UNFREEZE will delete any data despite of format_version

So not checking this leads to different behaviour of these queries. Am I right?

Sorry, I missed that SYSTEM UNFREEZE prints partition ids in query result, I though ids are needed only for filtering. So currently it prints invalid ids if table was created with old syntax. It's not clear how to fix it, I see the following options:

Do not return list of partitions for SYSTEM UNFREEZE

Autodetect format_version somehow for SYSTEM UNFREEZE (not sure if it's possible, but maybe checking some files inside frozen data part can help)

Ok, I think not returning list is easier and for sure possible. But still what about deleting data? Different data will be deleted for alter and system queries if not doing autodetect

Because if format_version is 0, there will be an exception and since then the partition will not be removed with ALTER query. But as soon as there is no exception when called from Unfreezer::unfreeze, the partition will be removed with SYSTEM query

I think it's ok, SYSTEM UNFREEZE will simply remove everything, because it's impossible to distinguish partitions in this case.

Hm, it seems like throwing an exception may break some user queries since this logic was introduced for alter unfreeze. May it? Is it critical?

ClickHouse/src/Storages/MergeTree/MergeTreeData.cpp

Line 5250 in f60f99d

if (!matcher(partition_id))

I would rather mark it as TODO and / or create a new issue about this. If I touch this code, the only solution that doesn't break any query I see is to autodetect format version. Otherwise, it may break query and behave differently for alter and system queries. I would create a separate issue for autodetection as soon as it is not about just SYSTEM UNFREEZE feature. It is about fixing small bug for both ALTER UNFREEZE and SYSTEM UNFREEZE which may introduce bigger bug

src/Storages/Freeze.cpp

src/Storages/Freeze.h

…to enable system unfreeze

tavplubix

LGTM, but a few things left:

fix tests (see tests/config/install.sh)
add a comment describing how all this stuff with hardlinks and unfreezing works

ianton-ru · 2022-08-18T11:06:43Z

@tavplubix Can you see now? TestsBugfixCheck failed with "nothing to run" for intergation tests - is this mean that integration test required too?

tavplubix · 2022-08-18T11:11:37Z

TestsBugfixCheck failed with "nothing to run" for intergation tests - is this mean that integration test required too?

No, it means that TestsBugfixCheck works incorrectly, cc: @vdimir

vdimir · 2022-08-18T11:50:01Z

Notice: bugfix check: stateless - error: fail: 0, passed: 2 Report url: https://s3.amazonaws.com/clickhouse-test-reports/38262/46de6997dd7ef0da596a6c2dfa07197ca7355b65/stateless_tests_bugfix_validate_check.html
Notice: bugfix check: integration - error: Nothing to run Report url: https://s3.amazonaws.com/clickhouse-test-reports/38262/46de6997dd7ef0da596a6c2dfa07197ca7355b65/integration_tests_bugfix_validate_check.html

It expects that added test would fail on the previous stable release (either functional or integration), but it's passed. In some cases, if the test is not stably reproduced, it can be fine.

But "error: Nothing to run" looks confusing, I'll fix the message.

tavplubix · 2022-08-18T11:52:46Z

Also it should post check status with link to the report (as other checks do) and actions task should not fail

tavplubix · 2022-08-19T11:23:28Z

Integration tests failures look suspicious

ianton-ru · 2022-08-20T08:42:10Z

Yes, we will fix it.

tavplubix · 2022-08-26T18:29:25Z

Integration tests still fail

alexey-milovidov · 2022-09-06T02:13:12Z

Integration tests have failed.

…mist/ClickHouse into fix-ordinary-system-unfreeze

src/Storages/Freeze.cpp

…-system-unfreeze Fix SYSTEM UNFREEZE for ordinary database

…tem-unfreeze-for-ordinary-database Merge pull request #38262 from PolyProgrammist/fix-ordinary-system-un…

PolyProgrammist commented Jun 21, 2022

View reviewed changes

robot-ch-test-poll added the pr-bugfix Pull request with bugfix, not backported by default label Jun 21, 2022

PolyProgrammist mentioned this pull request Jun 21, 2022

SYSTEM UNFREEZE query that deletes the whole backup #36424

Merged

antonio2368 added the can be tested Allows running workflows for external contributors label Jun 21, 2022

tavplubix self-assigned this Jun 21, 2022

Fix SYSTEM UNFREEZE for ordinary database

3296ba2

PolyProgrammist force-pushed the fix-ordinary-system-unfreeze branch from 639b84f to 3296ba2 Compare June 24, 2022 17:50

tavplubix reviewed Jun 24, 2022

View reviewed changes

review zookeeper and partition matching

85bf022

tavplubix reviewed Jul 21, 2022

View reviewed changes

remove exception for old partitions, add server configuration option …

8f14c60

…to enable system unfreeze

tavplubix approved these changes Jul 27, 2022

View reviewed changes

PolyProgrammist mentioned this pull request Aug 16, 2022

Unfreeze queries don't work correctly if table was created with old syntax #40256

Open

ianton-ru and others added 5 commits August 17, 2022 11:04

Add comment with explain for unfreeze with zero-copy replication

dd59612

Fix tests

7627f83

Add option enable_system_unfreeze to unfreeze test config

5aca68a

Fix style tests

b548979

fix style more

46de699

vdimir mentioned this pull request Aug 18, 2022

Improve TestsBugfixCheck #40349

Closed

3 tasks

PolyProgrammist added 2 commits August 19, 2022 08:59

Move config

4c60921

fix xml

f140ced

PolyProgrammist added 3 commits August 23, 2022 13:53

fix enable_unfreeze handler and sanitizer hints

9c371d5

fix underscore

4d08154

fix style

c2ba9ea

tavplubix marked this pull request as draft September 5, 2022 12:04

ianton-ru and others added 2 commits September 5, 2022 17:01

Fix FreezeMetaData::load

15b0b8f

Merge branch 'master' into fix-ordinary-system-unfreeze

39e1fc7

ianton-ru added 2 commits September 6, 2022 12:14

Fix test_merge_tree_s3/test.py::test_freeze_system_unfreeze[node]

5e7cf1f

Merge branch 'fix-ordinary-system-unfreeze' of github.com:PolyProgram…

e91d22a

…mist/ClickHouse into fix-ordinary-system-unfreeze

tavplubix marked this pull request as ready for review September 9, 2022 13:09

tavplubix reviewed Sep 9, 2022

View reviewed changes

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

Update src/Storages/Freeze.cpp

98509e6

tavplubix merged commit 4d146b0 into ClickHouse:master Sep 13, 2022

alesapin added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Sep 20, 2022

alesapin removed the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Sep 20, 2022

aalexfvk pushed a commit to aalexfvk/ClickHouse that referenced this pull request Jan 20, 2023

Merge pull request ClickHouse#38262 from PolyProgrammist/fix-ordinary…

e0f9f20

…-system-unfreeze Fix SYSTEM UNFREEZE for ordinary database

aalexfvk mentioned this pull request Jan 20, 2023

Backport #38262 to 22.8: Fix SYSTEM UNFREEZE for ordinary database #45471

Closed

alesapin added a commit that referenced this pull request Jan 27, 2023

Merge pull request #45650 from ClickHouse/backport/22.8/38262-fix-sys…

e481794

…tem-unfreeze-for-ordinary-database Merge pull request #38262 from PolyProgrammist/fix-ordinary-system-un…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SYSTEM UNFREEZE for ordinary database #38262

Fix SYSTEM UNFREEZE for ordinary database #38262

PolyProgrammist commented Jun 21, 2022 •

edited

PolyProgrammist Jun 21, 2022

tavplubix commented Jun 21, 2022

tavplubix Jun 24, 2022

PolyProgrammist Jun 24, 2022

tavplubix Jun 24, 2022

tavplubix Jun 24, 2022

PolyProgrammist Jun 24, 2022

PolyProgrammist Jun 24, 2022

tavplubix Jun 24, 2022

tavplubix commented Jun 24, 2022

tavplubix commented Jun 24, 2022

PolyProgrammist commented Jun 24, 2022 •

edited

alexey-milovidov commented Jul 21, 2022

tavplubix commented Jul 21, 2022

PolyProgrammist commented Jul 21, 2022

tavplubix Jul 21, 2022

tavplubix Jul 21, 2022 •

edited

PolyProgrammist Jul 21, 2022 •

edited

tavplubix Jul 21, 2022

PolyProgrammist Jul 21, 2022

PolyProgrammist Jul 21, 2022

tavplubix Jul 21, 2022

PolyProgrammist Jul 21, 2022

PolyProgrammist Jul 21, 2022 •

edited

PolyProgrammist Aug 16, 2022

tavplubix left a comment

ianton-ru commented Aug 18, 2022

tavplubix commented Aug 18, 2022

vdimir commented Aug 18, 2022 •

edited

tavplubix commented Aug 18, 2022

tavplubix commented Aug 19, 2022

ianton-ru commented Aug 20, 2022

tavplubix commented Aug 26, 2022

alexey-milovidov commented Sep 6, 2022

Fix SYSTEM UNFREEZE for ordinary database #38262

Fix SYSTEM UNFREEZE for ordinary database #38262

Conversation

PolyProgrammist commented Jun 21, 2022 • edited

Changelog category (leave one):

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

Choose a reason for hiding this comment

tavplubix commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavplubix commented Jun 24, 2022

tavplubix commented Jun 24, 2022

PolyProgrammist commented Jun 24, 2022 • edited

alexey-milovidov commented Jul 21, 2022

tavplubix commented Jul 21, 2022

PolyProgrammist commented Jul 21, 2022

Choose a reason for hiding this comment

tavplubix Jul 21, 2022 • edited

Choose a reason for hiding this comment

PolyProgrammist Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PolyProgrammist Jul 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavplubix left a comment

Choose a reason for hiding this comment

ianton-ru commented Aug 18, 2022

tavplubix commented Aug 18, 2022

vdimir commented Aug 18, 2022 • edited

tavplubix commented Aug 18, 2022

tavplubix commented Aug 19, 2022

ianton-ru commented Aug 20, 2022

tavplubix commented Aug 26, 2022

alexey-milovidov commented Sep 6, 2022

PolyProgrammist commented Jun 21, 2022 •

edited

PolyProgrammist commented Jun 24, 2022 •

edited

tavplubix Jul 21, 2022 •

edited

PolyProgrammist Jul 21, 2022 •

edited

PolyProgrammist Jul 21, 2022 •

edited

vdimir commented Aug 18, 2022 •

edited