Skip to content

Commit 56b84ad

Browse files
evilliqueevillique
authored
Merge pull request ClickHouse#80298 from evillique/fix-replicated-db-sleepy-alter
Less strict metadata checks for RMT in Replicated database
2 parents d6b27f2 + 05b575b commit 56b84ad

File tree

4 files changed

+100
-49
lines changed

4 files changed

+100
-49
lines changed

‎src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.cpp‎

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -250,105 +250,113 @@ ReplicatedMergeTreeTableMetadata ReplicatedMergeTreeTableMetadata::parse(const S
250250
return metadata;
251251
}
252252

253-
static void throwTableMetadataMismatch(
253+
static void handleTableMetadataMismatch(
254254
const std::string & table_name_for_error_message,
255255
std::string_view differs_in,
256256
const auto & stored_in_zk,
257257
const std::string & parsed_from_zk,
258-
const auto & local)
258+
const auto & local,
259+
bool strict_check = true,
260+
LoggerPtr logger = nullptr)
259261
{
260-
if (parsed_from_zk.empty())
261-
{
262-
throw Exception(
263-
ErrorCodes::METADATA_MISMATCH,
264-
"Metadata of table {} in ZooKeeper differs in {}. "
265-
"Stored in ZooKeeper: {}, local: {}",
266-
table_name_for_error_message, differs_in, stored_in_zk, local);
267-
}
262+
String metadata_string;
263+
if (!parsed_from_zk.empty())
264+
metadata_string = fmt::format("Stored in ZooKeeper: {}, parsed from ZooKeeper: {}, local: {}", stored_in_zk, parsed_from_zk, local);
268265
else
269-
{
266+
metadata_string = fmt::format("Stored in ZooKeeper: {}, local: {}", stored_in_zk, local);
267+
268+
if (strict_check)
270269
throw Exception(
271270
ErrorCodes::METADATA_MISMATCH,
272-
"Metadata of table {} in ZooKeeper differs in {}. "
273-
"Stored in ZooKeeper: {}, parsed from ZooKeeper: {}, local: {}",
274-
table_name_for_error_message, differs_in, stored_in_zk, parsed_from_zk, local);
275-
}
271+
"Metadata of table {} in ZooKeeper differs in {}. {}",
272+
table_name_for_error_message, differs_in, metadata_string);
273+
274+
if (logger)
275+
LOG_WARNING(logger,
276+
"Metadata of table {} in ZooKeeper differs in {}. {}",
277+
table_name_for_error_message, differs_in, metadata_string);
276278
};
277279

278280
void ReplicatedMergeTreeTableMetadata::checkImmutableFieldsEquals(
279281
const ReplicatedMergeTreeTableMetadata & from_zk,
280282
const ColumnsDescription & columns,
281283
const std::string & table_name_for_error_message,
282-
ContextPtr context) const
284+
ContextPtr context,
285+
bool check_index_granularity) const
283286
{
284287

285288
if (data_format_version < MERGE_TREE_DATA_MIN_FORMAT_VERSION_WITH_CUSTOM_PARTITIONING)
286289
{
287290
if (date_column != from_zk.date_column)
288-
throwTableMetadataMismatch(table_name_for_error_message, "date index column", from_zk.date_column, "", date_column);
291+
handleTableMetadataMismatch(table_name_for_error_message, "date index column", from_zk.date_column, "", date_column);
289292
}
290293
else if (!from_zk.date_column.empty())
291294
{
292-
throwTableMetadataMismatch(table_name_for_error_message, "date index column", from_zk.date_column, "", "custom-partitioned");
295+
handleTableMetadataMismatch(table_name_for_error_message, "date index column", from_zk.date_column, "", "custom-partitioned");
293296
}
294297

295-
if (index_granularity != from_zk.index_granularity)
296-
throwTableMetadataMismatch(table_name_for_error_message, "index granularity", DB::toString(from_zk.index_granularity), "", DB::toString(index_granularity));
298+
if (check_index_granularity && index_granularity != from_zk.index_granularity)
299+
handleTableMetadataMismatch(table_name_for_error_message, "index granularity", DB::toString(from_zk.index_granularity), "", DB::toString(index_granularity));
297300

298301
if (merging_params_mode != from_zk.merging_params_mode)
299-
throwTableMetadataMismatch(table_name_for_error_message, "mode of merge operation", DB::toString(from_zk.merging_params_mode), "", DB::toString(merging_params_mode));
302+
handleTableMetadataMismatch(table_name_for_error_message, "mode of merge operation", DB::toString(from_zk.merging_params_mode), "", DB::toString(merging_params_mode));
300303

301304
if (sign_column != from_zk.sign_column)
302-
throwTableMetadataMismatch(table_name_for_error_message, "sign column", from_zk.sign_column, "", sign_column);
305+
handleTableMetadataMismatch(table_name_for_error_message, "sign column", from_zk.sign_column, "", sign_column);
303306

304307
if (merge_params_version >= REPLICATED_MERGE_TREE_METADATA_WITH_ALL_MERGE_PARAMETERS && from_zk.merge_params_version >= REPLICATED_MERGE_TREE_METADATA_WITH_ALL_MERGE_PARAMETERS)
305308
{
306309
if (version_column != from_zk.version_column)
307-
throwTableMetadataMismatch(table_name_for_error_message, "version column", from_zk.version_column, "", version_column);
310+
handleTableMetadataMismatch(table_name_for_error_message, "version column", from_zk.version_column, "", version_column);
308311

309312
if (is_deleted_column != from_zk.is_deleted_column)
310-
throwTableMetadataMismatch(table_name_for_error_message, "is_deleted column", from_zk.is_deleted_column, "", is_deleted_column);
313+
handleTableMetadataMismatch(table_name_for_error_message, "is_deleted column", from_zk.is_deleted_column, "", is_deleted_column);
311314

312315
if (columns_to_sum != from_zk.columns_to_sum)
313-
throwTableMetadataMismatch(table_name_for_error_message, "sum columns", from_zk.columns_to_sum, "", columns_to_sum);
316+
handleTableMetadataMismatch(table_name_for_error_message, "sum columns", from_zk.columns_to_sum, "", columns_to_sum);
314317

315318
if (graphite_params_hash != from_zk.graphite_params_hash)
316-
throwTableMetadataMismatch(table_name_for_error_message, "graphite params", from_zk.graphite_params_hash, "", graphite_params_hash);
319+
handleTableMetadataMismatch(table_name_for_error_message, "graphite params", from_zk.graphite_params_hash, "", graphite_params_hash);
317320
}
318321

319322
/// NOTE: You can make a less strict check of match expressions so that tables do not break from small changes
320323
/// in formatAST code.
321324
String parsed_zk_primary_key = formattedAST(KeyDescription::parse(from_zk.primary_key, columns, context, true).getOriginalExpressionList());
322325
if (primary_key != parsed_zk_primary_key)
323-
throwTableMetadataMismatch(table_name_for_error_message, "primary key", from_zk.primary_key, parsed_zk_primary_key, primary_key);
326+
handleTableMetadataMismatch(table_name_for_error_message, "primary key", from_zk.primary_key, parsed_zk_primary_key, primary_key);
324327

325328
if (data_format_version != from_zk.data_format_version)
326-
throwTableMetadataMismatch(table_name_for_error_message, "data format version", DB::toString(from_zk.data_format_version.toUnderType()), "", DB::toString(data_format_version.toUnderType()));
329+
handleTableMetadataMismatch(table_name_for_error_message, "data format version", DB::toString(from_zk.data_format_version.toUnderType()), "", DB::toString(data_format_version.toUnderType()));
327330

328331
String parsed_zk_partition_key = formattedAST(KeyDescription::parse(from_zk.partition_key, columns, context, false).expression_list_ast);
329332
if (partition_key != parsed_zk_partition_key)
330-
throwTableMetadataMismatch(table_name_for_error_message, "partition key expression", from_zk.partition_key, parsed_zk_partition_key, partition_key);
333+
handleTableMetadataMismatch(table_name_for_error_message, "partition key expression", from_zk.partition_key, parsed_zk_partition_key, partition_key);
331334
}
332335

333-
void ReplicatedMergeTreeTableMetadata::checkEquals(
336+
bool ReplicatedMergeTreeTableMetadata::checkEquals(
334337
const ReplicatedMergeTreeTableMetadata & from_zk,
335338
const ColumnsDescription & columns,
336339
const std::string & table_name_for_error_message,
337-
ContextPtr context) const
340+
ContextPtr context,
341+
bool check_index_granularity,
342+
bool strict_check,
343+
LoggerPtr logger) const
338344
{
339-
340-
checkImmutableFieldsEquals(from_zk, columns, table_name_for_error_message, context);
345+
bool is_equal = true;
346+
checkImmutableFieldsEquals(from_zk, columns, table_name_for_error_message, context, check_index_granularity);
341347

342348
String parsed_zk_sampling_expression = formattedAST(KeyDescription::parse(from_zk.sampling_expression, columns, context, false).definition_ast);
343349
if (sampling_expression != parsed_zk_sampling_expression)
344350
{
345-
throwTableMetadataMismatch(table_name_for_error_message, "sample expression", from_zk.sampling_expression, parsed_zk_sampling_expression, sampling_expression);
351+
handleTableMetadataMismatch(table_name_for_error_message, "sampling expression", from_zk.sampling_expression, parsed_zk_sampling_expression, sampling_expression, strict_check, logger);
352+
is_equal = false;
346353
}
347354

348355
String parsed_zk_sorting_key = formattedAST(extractKeyExpressionList(KeyDescription::parse(from_zk.sorting_key, columns, context, true).definition_ast));
349356
if (sorting_key != parsed_zk_sorting_key)
350357
{
351-
throwTableMetadataMismatch(table_name_for_error_message, "sorting key expression", from_zk.sorting_key, parsed_zk_sorting_key, sorting_key);
358+
handleTableMetadataMismatch(table_name_for_error_message, "sorting key expression", from_zk.sorting_key, parsed_zk_sorting_key, sorting_key, strict_check, logger);
359+
is_equal = false;
352360
}
353361

354362
auto parsed_primary_key = KeyDescription::parse(primary_key, columns, context, true);
@@ -357,41 +365,49 @@ void ReplicatedMergeTreeTableMetadata::checkEquals(
357365
TTLTableDescription::parse(from_zk.ttl_table, columns, context, parsed_primary_key, /* is_attach = */ true).definition_ast);
358366
if (ttl_table != parsed_zk_ttl_table)
359367
{
360-
throwTableMetadataMismatch(table_name_for_error_message, "TTL", from_zk.ttl_table, parsed_zk_ttl_table, ttl_table);
368+
handleTableMetadataMismatch(table_name_for_error_message, "TTL", from_zk.ttl_table, parsed_zk_ttl_table, ttl_table, strict_check, logger);
369+
is_equal = false;
361370
}
362371

363372
String parsed_zk_skip_indices = IndicesDescription::parse(from_zk.skip_indices, columns, context).toString();
364373
if (skip_indices != parsed_zk_skip_indices)
365374
{
366-
throwTableMetadataMismatch(table_name_for_error_message, "skip indexes", from_zk.skip_indices, parsed_zk_skip_indices, skip_indices);
375+
handleTableMetadataMismatch(table_name_for_error_message, "skip indexes", from_zk.skip_indices, parsed_zk_skip_indices, skip_indices, strict_check, logger);
376+
is_equal = false;
367377
}
368378

369379
String parsed_zk_projections = ProjectionsDescription::parse(from_zk.projections, columns, context).toString();
370380
if (projections != parsed_zk_projections)
371381
{
372-
throwTableMetadataMismatch(table_name_for_error_message, "projections", from_zk.projections, parsed_zk_projections, projections);
382+
handleTableMetadataMismatch(table_name_for_error_message, "projections", from_zk.projections, parsed_zk_projections, projections, strict_check, logger);
383+
is_equal = false;
373384
}
374385

375386
String parsed_zk_constraints = ConstraintsDescription::parse(from_zk.constraints).toString();
376387
if (constraints != parsed_zk_constraints)
377388
{
378-
throwTableMetadataMismatch(table_name_for_error_message, "constraints", from_zk.constraints, parsed_zk_constraints, constraints);
389+
handleTableMetadataMismatch(table_name_for_error_message, "constraints", from_zk.constraints, parsed_zk_constraints, constraints, strict_check, logger);
390+
is_equal = false;
379391
}
380392

381-
if (from_zk.index_granularity_bytes_found_in_zk && index_granularity_bytes != from_zk.index_granularity_bytes)
393+
if (check_index_granularity && from_zk.index_granularity_bytes_found_in_zk && index_granularity_bytes != from_zk.index_granularity_bytes)
382394
{
383-
throwTableMetadataMismatch(table_name_for_error_message, "index granularity bytes", from_zk.index_granularity_bytes, "", index_granularity_bytes);
395+
handleTableMetadataMismatch(table_name_for_error_message, "index granularity bytes", from_zk.index_granularity_bytes, "", index_granularity_bytes, strict_check, logger);
396+
is_equal = false;
384397
}
398+
399+
return is_equal;
385400
}
386401

387402
ReplicatedMergeTreeTableMetadata::Diff
388403
ReplicatedMergeTreeTableMetadata::checkAndFindDiff(
389404
const ReplicatedMergeTreeTableMetadata & from_zk,
390405
const ColumnsDescription & columns,
391406
const std::string & table_name_for_error_message,
392-
ContextPtr context) const
407+
ContextPtr context,
408+
bool check_index_granularity) const
393409
{
394-
checkImmutableFieldsEquals(from_zk, columns, table_name_for_error_message, context);
410+
checkImmutableFieldsEquals(from_zk, columns, table_name_for_error_message, context, check_index_granularity);
395411

396412
Diff diff;
397413

‎src/Storages/MergeTree/ReplicatedMergeTreeTableMetadata.h‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,25 +79,30 @@ struct ReplicatedMergeTreeTableMetadata
7979
StorageInMemoryMetadata getNewMetadata(const ColumnsDescription & new_columns, ContextPtr context, const StorageInMemoryMetadata & old_metadata) const;
8080
};
8181

82-
void checkEquals(
82+
bool checkEquals(
8383
const ReplicatedMergeTreeTableMetadata & from_zk,
8484
const ColumnsDescription & columns,
8585
const std::string & table_name_for_error_message,
86-
ContextPtr context) const;
86+
ContextPtr context,
87+
bool check_index_granularity = true,
88+
bool strict_check = true,
89+
LoggerPtr logger = nullptr) const;
8790

8891
Diff checkAndFindDiff(
8992
const ReplicatedMergeTreeTableMetadata & from_zk,
9093
const ColumnsDescription & columns,
9194
const std::string & table_name_for_error_message,
92-
ContextPtr context) const;
95+
ContextPtr context,
96+
bool check_index_granularity = true) const;
9397

9498
private:
9599

96100
void checkImmutableFieldsEquals(
97101
const ReplicatedMergeTreeTableMetadata & from_zk,
98102
const ColumnsDescription & columns,
99103
const std::string & table_name_for_error_message,
100-
ContextPtr context) const;
104+
ContextPtr context,
105+
bool check_index_granularity = true) const;
101106

102107
bool index_granularity_bytes_found_in_zk = false;
103108
};

‎src/Storages/StorageReplicatedMergeTree.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1652,7 +1652,7 @@ bool StorageReplicatedMergeTree::checkTableStructureAttempt(
16521652
Coordination::Stat metadata_stat;
16531653
String metadata_str = zookeeper->get(fs::path(zookeeper_prefix) / "metadata", &metadata_stat);
16541654
auto metadata_from_zk = ReplicatedMergeTreeTableMetadata::parse(metadata_str);
1655-
old_metadata.checkEquals(metadata_from_zk, metadata_snapshot->getColumns(), getStorageID().getNameForLogs(), getContext());
1655+
bool is_metadata_equal = old_metadata.checkEquals(metadata_from_zk, metadata_snapshot->getColumns(), getStorageID().getNameForLogs(), getContext(), /*check_index_granularity*/ true, strict_check, log.load());
16561656

16571657
if (metadata_version)
16581658
*metadata_version = metadata_stat.version;
@@ -1661,7 +1661,7 @@ bool StorageReplicatedMergeTree::checkTableStructureAttempt(
16611661
auto columns_from_zk = ColumnsDescription::parse(zookeeper->get(fs::path(zookeeper_prefix) / "columns", &columns_stat));
16621662

16631663
const ColumnsDescription & old_columns = metadata_snapshot->getColumns();
1664-
if (columns_from_zk == old_columns)
1664+
if (columns_from_zk == old_columns && is_metadata_equal)
16651665
return true;
16661666

16671667
if (!strict_check && metadata_stat.version != 0)

‎tests/integration/test_replicated_database/test.py‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1593,3 +1593,33 @@ def test_alter_rename(started_cluster):
15931593
settings=settings,
15941594
)
15951595
assert "PROJECTION" in res
1596+
1597+
1598+
@pytest.mark.parametrize("engine", ["ReplicatedMergeTree"])
1599+
def test_create_alter_sleeping(started_cluster, engine):
1600+
competing_node.query("DROP DATABASE IF EXISTS create_alter_sleeping")
1601+
dummy_node.query("DROP DATABASE IF EXISTS create_alter_sleeping")
1602+
1603+
competing_node.query(
1604+
"CREATE DATABASE create_alter_sleeping ENGINE = Replicated('/clickhouse/databases/create_alter_sleeping', 'shard1', 'replica1');"
1605+
)
1606+
dummy_node.query(
1607+
"CREATE DATABASE create_alter_sleeping ENGINE = Replicated('/clickhouse/databases/create_alter_sleeping', 'shard1', 'replica2');"
1608+
)
1609+
1610+
dummy_node.stop_clickhouse()
1611+
competing_node.query(
1612+
f"""
1613+
CREATE TABLE create_alter_sleeping.t (n int) ENGINE={engine} ORDER BY n;
1614+
ALTER TABLE create_alter_sleeping.t ADD INDEX n_idx n TYPE minmax GRANULARITY 10;
1615+
""",
1616+
settings={"distributed_ddl_task_timeout": 0},
1617+
)
1618+
1619+
dummy_node.start_clickhouse()
1620+
assert "n_idx" in dummy_node.query(
1621+
"""
1622+
SYSTEM SYNC DATABASE REPLICA create_alter_sleeping;
1623+
SHOW CREATE TABLE create_alter_sleeping.t;
1624+
""", timeout=10
1625+
)

0 commit comments

Comments
 (0)