Skip to content

Commit

Permalink
Avoid deadlock for syncing pinned part UUIDs on startup
Browse files Browse the repository at this point in the history
  • Loading branch information
azat committed Jun 12, 2023
1 parent 14ecff6 commit d89561c
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/Storages/MergeTree/ReplicatedMergeTreeAttachThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ void ReplicatedMergeTreeAttachThread::runImpl()
storage.clearOldWriteAheadLogs();

storage.createNewZooKeeperNodes();
storage.syncPinnedPartUUIDs();
storage.syncPinnedPartUUIDs(/* startup= */ true);

std::lock_guard lock(storage.table_shared_id_mutex);
storage.createTableSharedID();
Expand Down
13 changes: 9 additions & 4 deletions src/Storages/StorageReplicatedMergeTree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ StorageReplicatedMergeTree::StorageReplicatedMergeTree(
}

createNewZooKeeperNodes();
syncPinnedPartUUIDs();
syncPinnedPartUUIDs(/* startup= */ true);

if (!has_metadata_in_zookeeper.has_value() || *has_metadata_in_zookeeper)
createTableSharedID();
Expand Down Expand Up @@ -1374,7 +1374,7 @@ void StorageReplicatedMergeTree::checkParts(bool skip_sanity_checks)
}


void StorageReplicatedMergeTree::syncPinnedPartUUIDs()
void StorageReplicatedMergeTree::syncPinnedPartUUIDs(bool startup)
{
auto zookeeper = getZooKeeper();

Expand All @@ -1395,7 +1395,12 @@ void StorageReplicatedMergeTree::syncPinnedPartUUIDs()
/// pinned_part_uuids, then it will receive some UUIds from remote
/// shard, and only then pinned_part_uuids will be set, and this will
/// lead to duplicated data.
auto exclusive_lock = lockExclusively(RWLockImpl::NO_QUERY, getContext()->getSettingsRef().lock_acquire_timeout);
TableExclusiveLockHolder exclusive_lock;
/// On startup we cannot take exclusive lock since some already held on
/// this storage (and it is not required, since this table is not
/// visible yet, though there is one exception - switching from readonly)
if (!startup)
exclusive_lock = lockExclusively(RWLockImpl::NO_QUERY, getContext()->getSettingsRef().lock_acquire_timeout);
pinned_part_uuids = new_pinned_part_uuids;
}
}
Expand Down Expand Up @@ -1704,7 +1709,7 @@ bool StorageReplicatedMergeTree::executeLogEntry(LogEntry & entry)
case LogEntry::ALTER_METADATA:
return executeMetadataAlter(entry);
case LogEntry::SYNC_PINNED_PART_UUIDS:
syncPinnedPartUUIDs();
syncPinnedPartUUIDs(/* startup= */ false);
return true;
case LogEntry::CLONE_PART_FROM_SHARD:
executeClonePartFromShard(entry);
Expand Down
2 changes: 1 addition & 1 deletion src/Storages/StorageReplicatedMergeTree.h
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ class StorageReplicatedMergeTree final : public MergeTreeData

/// Synchronize the list of part uuids which are currently pinned. These should be sent to root query executor
/// to be used for deduplication.
void syncPinnedPartUUIDs();
void syncPinnedPartUUIDs(bool startup);

/** Check that the part's checksum is the same as the checksum of the same part on some other replica.
* If no one has such a part, nothing checks.
Expand Down

0 comments on commit d89561c

Please sign in to comment.