Skip to content

Commit 531227d

Browse files
zvonandzvonand
authored
Merge branch 'releases/25.8.13' into backports/25.8.13/88341
2 parents e089108 + a4ce138 commit 531227d

5 files changed

Lines changed: 120 additions & 2 deletions

File tree

‎src/Storages/StorageDistributed.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1690,9 +1690,11 @@ Cluster::Addresses StorageDistributed::parseAddresses(const std::string & name)
16901690
continue;
16911691
}
16921692

1693-
if (address.replica_index > replicas)
1693+
if (address.replica_index == 0 || address.replica_index > replicas)
16941694
{
1695-
LOG_ERROR(log, "No shard with replica_index={} ({})", address.replica_index, name);
1695+
LOG_ERROR(log, "Invalid replica_index={} for directory '{}' (cluster has {} replicas for shard {}). "
1696+
"Expected directory format: 'shardN_replicaM' or 'shardN_all_replicas'",
1697+
address.replica_index, dirname, replicas, address.shard_index);
16961698
continue;
16971699
}
16981700

‎src/TableFunctions/TableFunctionRemote.cpp‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <Core/Defines.h>
2222
#include <Core/Settings.h>
2323
#include <TableFunctions/registerTableFunctions.h>
24+
#include <Access/Common/AccessFlags.h>
2425

2526

2627
namespace DB
@@ -315,6 +316,22 @@ StoragePtr TableFunctionRemote::executeImpl(const ASTPtr & /*ast_function*/, Con
315316
cached_columns = getActualTableStructure(context, is_insert_query);
316317

317318
assert(cluster);
319+
320+
bool has_local_shard = false;
321+
for (const auto & shard_info : cluster->getShardsInfo())
322+
{
323+
if (shard_info.isLocal())
324+
{
325+
has_local_shard = true;
326+
break;
327+
}
328+
}
329+
330+
if (has_local_shard && !is_insert_query)
331+
context->checkAccess(AccessType::SELECT, remote_table_id);
332+
else if (has_local_shard)
333+
context->checkAccess(AccessType::INSERT, remote_table_id);
334+
318335
StoragePtr res = std::make_shared<StorageDistributed>(
319336
StorageID(getDatabaseName(), table_name),
320337
cached_columns,

‎tests/integration/test_distributed_format/test.py‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,3 +213,72 @@ def test_remove_replica(started_cluster):
213213
"/etc/clickhouse-server/config.d/another_remote_servers.xml",
214214
]
215215
)
216+
217+
def test_invalid_shard_directory_format(started_cluster):
218+
"""
219+
Test that ClickHouse doesn't crash when it encounters
220+
a malformed directory name like 'shard1_all_replicas_bkp'
221+
during distributed table initialization.
222+
"""
223+
node.query("drop table if exists test.dist_invalid sync")
224+
node.query("drop table if exists test.local_invalid sync")
225+
node.query(
226+
"create table test.local_invalid (x UInt64, s String) engine = MergeTree order by x"
227+
)
228+
node.query(
229+
"create table test.dist_invalid (x UInt64, s String) "
230+
"engine = Distributed('test_cluster_internal_replication', test, local_invalid)"
231+
)
232+
233+
node.query(
234+
"insert into test.dist_invalid values (1, 'a'), (2, 'bb')",
235+
settings={"use_compact_format_in_distributed_parts_names": "1"},
236+
)
237+
238+
data_path = node.query(
239+
"SELECT arrayElement(data_paths, 1) FROM system.tables "
240+
"WHERE database='test' AND name='dist_invalid'"
241+
).strip()
242+
243+
# Create a malformed directory that would cause the bug
244+
malformed_dir = f"{data_path}/shard1_all_replicas_bkp"
245+
node.exec_in_container(["mkdir", "-p", malformed_dir])
246+
247+
# Create a dummy file so the directory isn't considered empty
248+
node.exec_in_container(["touch", f"{malformed_dir}/dummy.txt"])
249+
250+
invalid_formats = [
251+
"shard1_all_replicas_backup",
252+
"shard1_all_replicas_old",
253+
"shard2_all_replicas_tmp",
254+
]
255+
for invalid_dir in invalid_formats:
256+
invalid_path = f"{data_path}/{invalid_dir}"
257+
node.exec_in_container(["mkdir", "-p", invalid_path])
258+
# just dummy file to have something in the directory
259+
node.exec_in_container(["touch", f"{invalid_path}/dummy.txt"])
260+
261+
# Reproduce server restart with detach and attach
262+
node.query("detach table test.dist_invalid")
263+
node.query("attach table test.dist_invalid")
264+
265+
node.query("SYSTEM FLUSH LOGS system.text_log")
266+
267+
error_logs = node.query(
268+
"""
269+
SELECT count()
270+
FROM system.text_log
271+
WHERE level = 'Error'
272+
AND message LIKE '%Invalid replica_index%'
273+
AND message LIKE '%shard1_all_replicas%'
274+
"""
275+
).strip()
276+
277+
# We should have at least one error log for each malformed directory
278+
# But we don't strictly require this in case logging is disabled
279+
# The important thing is that the server didn't crash
280+
print(f"Found {error_logs} error log entries for invalid directories")
281+
282+
# Clean up
283+
node.query("drop table test.dist_invalid sync")
284+
node.query("drop table test.local_invalid sync")

‎tests/queries/0_stateless/03727_alter_with_localhost_remote.reference‎

Whitespace-only changes.

‎tests/queries/0_stateless/03727_alter_with_localhost_remote.sql‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
-- Tags: no-replicated-database, no-parallel
2+
3+
DROP USER IF EXISTS test_03727;
4+
CREATE USER test_03727;
5+
6+
CREATE TABLE normal
7+
(
8+
n Int32,
9+
s String
10+
)
11+
ENGINE = MergeTree()
12+
ORDER BY n;
13+
14+
CREATE TABLE secret
15+
(
16+
s String
17+
)
18+
ENGINE = MergeTree()
19+
ORDER BY s;
20+
21+
INSERT INTO normal VALUES (1, '');
22+
INSERT INTO secret VALUES ('secret');
23+
24+
GRANT ALTER UPDATE ON normal TO test_03727;
25+
GRANT READ ON REMOTE to test_03727;
26+
GRANT CREATE TEMPORARY TABLE ON *.* TO test_03727;
27+
28+
EXECUTE AS test_03727 ALTER TABLE normal UPDATE s = (SELECT * FROM remote('localhost', currentDatabase(), 'secret') LIMIT 1) WHERE n=1; -- { serverError ACCESS_DENIED }
29+
30+
DROP USER IF EXISTS test_03727;

0 commit comments

Comments
 (0)