Backport #94262 to 25.8: Fix data race in DataPartStorageOnDiskBase::remove vs system.parts

robot-clickhouse · robot-clickhouse · commit e78e97df7e03 · 2026-01-16T21:14:42.000Z
diff --git a/src/Storages/MergeTree/DataPartStorageOnDiskBase.cpp b/src/Storages/MergeTree/DataPartStorageOnDiskBase.cpp
@@ -748,7 +748,9 @@ void DataPartStorageOnDiskBase::remove(
         try
         {
             disk->moveDirectory(from, to);
-            part_dir = part_dir_without_slash;
+            /// NOTE: we intentionally don't update part_dir here because it would cause a data race
+            /// with concurrent readers (e.g. system.parts table queries calling getFullPath()).
+            /// The part is being removed anyway, so the path doesn't need to be updated.
         }
         catch (const Exception & e)
         {
diff --git a/tests/queries/0_stateless/03791_system_parts_race_condition_drop_part.reference b/tests/queries/0_stateless/03791_system_parts_race_condition_drop_part.reference
@@ -0,0 +1 @@
+OK
diff --git a/tests/queries/0_stateless/03791_system_parts_race_condition_drop_part.sh b/tests/queries/0_stateless/03791_system_parts_race_condition_drop_part.sh
@@ -0,0 +1,66 @@
+#!/usr/bin/env bash
+# Tags: race, no-parallel
+# Test for a race condition between reading system.parts and removing parts.
+# The race was in DataPartStorageOnDiskBase::remove() modifying part_dir
+# while getFullPath() was reading it concurrently.
+
+CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+# shellcheck source=../shell_config.sh
+. "$CURDIR"/../shell_config.sh
+
+set -e
+
+${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "DROP TABLE IF EXISTS part_race"
+${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "CREATE TABLE part_race (x UInt64) ENGINE = MergeTree ORDER BY x PARTITION BY x % 10
+    SETTINGS old_parts_lifetime = 0, cleanup_delay_period = 0, cleanup_delay_period_random_add = 0,
+    cleanup_thread_preferred_points_per_iteration = 0, max_cleanup_delay_period = 0"
+
+TIMEOUT=30
+
+function thread_insert()
+{
+    local TIMELIMIT=$((SECONDS+TIMEOUT))
+    local i=0
+    while [ $SECONDS -lt "$TIMELIMIT" ]
+    do
+        ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "INSERT INTO part_race SELECT $i" 2>/dev/null
+        ((i++)) || true
+    done
+}
+
+function thread_drop_partition()
+{
+    local TIMELIMIT=$((SECONDS+TIMEOUT))
+    while [ $SECONDS -lt "$TIMELIMIT" ]
+    do
+        ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "ALTER TABLE part_race DROP PARTITION ID '$((RANDOM % 10))'" 2>/dev/null
+        sleep 0.0$RANDOM
+    done
+}
+
+function thread_select_parts()
+{
+    local TIMELIMIT=$((SECONDS+TIMEOUT))
+    while [ $SECONDS -lt "$TIMELIMIT" ]
+    do
+        ${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "SELECT name, path FROM system.parts WHERE database = '${CLICKHOUSE_DATABASE}' AND table = 'part_race' FORMAT Null" 2>/dev/null
+    done
+}
+
+# Start multiple instances of each thread
+thread_insert &
+thread_insert &
+
+thread_drop_partition &
+thread_drop_partition &
+
+thread_select_parts &
+thread_select_parts &
+thread_select_parts &
+thread_select_parts &
+
+wait
+
+${CLICKHOUSE_CURL} -sS "${CLICKHOUSE_URL}" -d "DROP TABLE part_race"
+
+echo "OK"

Original file line number	Diff line number	Diff line change
`@@ -748,7 +748,9 @@ void DataPartStorageOnDiskBase::remove(`
`748`	`748`	`try`
`749`	`749`	`{`
`750`	`750`	`disk->moveDirectory(from, to);`
`751`		`- part_dir = part_dir_without_slash;`
	`751`	`+ /// NOTE: we intentionally don't update part_dir here because it would cause a data race`
	`752`	`+ /// with concurrent readers (e.g. system.parts table queries calling getFullPath()).`
	`753`	`+ /// The part is being removed anyway, so the path doesn't need to be updated.`
`752`	`754`	`}`
`753`	`755`	`catch (const Exception & e)`
`754`	`756`	`{`