Conversation
robot-clickhouse
added
the
pr-bugfix
alesapin
left a comment
alesapin
left a comment
There was a problem hiding this comment.
I understand the current implementation, but I don't understand how it has been working before...
And maybe a backward-incompatible change would be OK.
|
|
||
| /// Return {} because selection of merges in the partition where the column is cleared | ||
| /// should not be blocked (only execution of merges should be blocked). | ||
| if (type == CLEAR_COLUMN || type == CLEAR_INDEX) |
There was a problem hiding this comment.
We don't use these types of entries in the queue, they are left just for compatibility. Maybe update comment.
| getFakePartCoveringAllPartsInPartition(partition_id, drop_range, true); | ||
| std::optional<EphemeralLockInZooKeeper> delimiting_block_lock; | ||
| bool partition_was_empty = !getFakePartCoveringAllPartsInPartition(partition_id, drop_range, delimiting_block_lock, true); | ||
| if (replace && partition_was_empty) |
There was a problem hiding this comment.
But where is the guarantee that no concurrent insert can happen here? Or in this case it's OK?
There was a problem hiding this comment.
It's ok, all blocks with numbers less than delimiting_block_lock->getNumber() will be dropped, and newer blocks with greater numbers will not. But we must hold delimiting_block_lock until creation of DROP_RANGE/REPLACE_RANGE entry in replication log to avoid merges intersecting delimiting_block_lock->getNumber().
| ops_src.emplace_back(zkutil::makeCreateRequest( | ||
| zookeeper_path + "/log/log-", entry_delete.toString(), zkutil::CreateMode::PersistentSequential)); | ||
| ops_dest.emplace_back(zkutil::makeSetRequest(zookeeper_path + "/log", "", -1)); /// Just update version | ||
| ops_src.emplace_back(zkutil::makeSetRequest(zookeeper_path + "/log", "", -1)); /// Just update version |
There was a problem hiding this comment.
Maybe add to comment: because merges assignment relies on it or something like this.
| parts_to_remove_str += " "; | ||
| } | ||
| LOG_TRACE(log, "Replacing {} parts {}with {} parts {}", parts_to_remove.size(), parts_to_remove_str, | ||
| final_parts.size(), boost::algorithm::join(final_part_names, ", ")); |
There was a problem hiding this comment.
btw fmt::join requires less characters :)
| Strings res = replace_range_entry->new_part_names; | ||
| auto drop_range_info = MergeTreePartInfo::fromPartName(replace_range_entry->drop_range_part_name, format_version); | ||
| assert(drop_range_info.getBlocksCount() != 0); | ||
| if (drop_range_info.getBlocksCount() > 1) |
There was a problem hiding this comment.
Sounds weird, but maybe even create a separate function for this check?)
| /// so we can distinguish dummy drop range from any real or virtual part. | ||
| /// But we should never construct such part name, even for virtual part, | ||
| /// because it can be confused with real part <partition>_0_0_0. | ||
| /// TODO get rid of this. |
There was a problem hiding this comment.
ohhh, maybe just add a flag and make it backward-incompatible?
There was a problem hiding this comment.
It will not be possible to backport the fix with such change
| /// - drop range for MOVE PARTITION/ATTACH PARTITION FROM always contains 1 block | ||
|
|
||
| --right; | ||
| /// NOTE UINT_MAX was previously used as max level for REPLACE/MOVE PARTITION (it was incorrect) |
There was a problem hiding this comment.
I'd prefer to forget about it :)
| { | ||
| /// Intersect left border | ||
| String error = "Unexpected merged part " + part->name + " intersecting drop range " + drop_range.getPartName(); | ||
| if (!skip_intersecting_parts) |
| std::lock_guard merge_selecting_lock(merge_selecting_mutex); | ||
| queue.disableMergesInBlockRange(drop_range_fake_part_name); | ||
| } | ||
| /// Optional step |
There was a problem hiding this comment.
Bad comment, need clarification.
|
|
Backport #23997 to 21.4: Try fix intersecting parts
Backport #23997 to 21.5: Try fix intersecting parts
Backport #23997 to 21.3: Try fix intersecting parts
3 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Some
ALTER PARTITIONqueries might causePart A intersects previous part BandUnexpected merged part C intersecting drop range Derrors in replication queue. It's fixed.Fixes #23296.
Detailed description / Documentation draft:
Fixed two independent bugs:
MERGE_PARTSandDROP_RANGE/REPLACE_RANGEon multiple leadersREPLACE_RANGEwhich breaks invariants in replication queue and merge predicate