ReplicatedMergeTree tables as readonly until attach is finished
#40148
Conversation
robot-ch-test-poll
added
the
pr-improvement
|
Btw, we already have test for this, you only need to uncomment it: ClickHouse/tests/integration/test_replicated_database/test.py Lines 779 to 780 in 94dd80e |
antonio2368
changed the title
ReplicatedMergeTree tables as readonly until attach is finishedReplicatedMergeTree tables as readonly until attach is finished
antonio2368
force-pushed
the
replicate-tables-readonly
branch
from
5a1d039
to
b0b9f96
Compare
| /// In old tables this node may missing or be empty | ||
| String replica_metadata; | ||
| const bool replica_metadata_exists = withRetries([&] { return zookeeper->tryGet(replica_path + "/metadata", replica_metadata); }); |
There was a problem hiding this comment.
Such tables are probably too old and we can remove compatibility with them and throw an exception that says that upgrade from 20.3 and older to 22.9 and newer should be done through an intermediate version.
There was a problem hiding this comment.
maybe we can remove this in a different PR to focus on the main part of this PR?
There was a problem hiding this comment.
Yes, we can do it in separate PR. Btw, removal of this code will fix a bug: #30826 (comment)
|
@tavplubix I tried to make it as simple as possible PTAL. |
| bool maybe_has_metadata_in_zookeeper = false; | ||
| { | ||
| std::lock_guard lock{initialization_mutex}; | ||
| maybe_has_metadata_in_zookeeper = !initialization_done || !has_metadata_in_zookeeper.has_value() || *has_metadata_in_zookeeper; |
There was a problem hiding this comment.
Do we really need to check initialization_done here? Seems like !has_metadata_in_zookeeper.has_value() is enough.
There was a problem hiding this comment.
But has_metadata_in_zookeeper isn't an atomic and can change in parallel in AttachThread.
Am I missing something?
There was a problem hiding this comment.
shutdown() must be called before drop() and shutdown() stops AttachThread, so it cannot change in parallel
There was a problem hiding this comment.
okay
then the shutdown call in drop gives a different impression.
Should that be an assert then? (that shutdown was called)
There was a problem hiding this comment.
Hm, this shutdown() call is from 2014 and it's not clear why it was added (b4d85c1). AFAIK, now we always call shutdown() before dropping (or detaching) a table
Should that be an assert then? (that shutdown was called)
Yep
There was a problem hiding this comment.
It seems assert fails.
I think it's connected to the case when CH gets restarted before the table is dropped. On the next start, it will be read from metadata_dropped, recreated, and not started. Because of that shutdown won't be called.
Did I miss something?
There was a problem hiding this comment.
Sorry, I forgot about this case. In this case we don't need to call shutdown() because startup() was not called (so either AttachThread was not started at all or it was already stopped)
|
Performance Comparison Aarch64 - something weird with docker: Tests on the prev commit were fine (except the assertion), so I think we can merge |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
During startup and ATTACH call,
ReplicatedMergeTreetables will be readonly until the ZooKeeper connection is made and the setup is finished.@tavplubix this is the basic idea. Hope it's okay.
Closes #40105