From 0a072ccc571bbb48146119f826963bd428b3897d Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Thu, 2 May 2019 18:13:57 +0300 Subject: [PATCH 1/2] Added a test for race condition --- ...41_system_columns_race_condition.reference | 0 .../00941_system_columns_race_condition.sh | 32 +++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/00941_system_columns_race_condition.reference create mode 100755 dbms/tests/queries/0_stateless/00941_system_columns_race_condition.sh diff --git a/dbms/tests/queries/0_stateless/00941_system_columns_race_condition.reference b/dbms/tests/queries/0_stateless/00941_system_columns_race_condition.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/0_stateless/00941_system_columns_race_condition.sh b/dbms/tests/queries/0_stateless/00941_system_columns_race_condition.sh new file mode 100755 index 00000000000..3d668e61067 --- /dev/null +++ b/dbms/tests/queries/0_stateless/00941_system_columns_race_condition.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash + +# Test fix for issue #5066 + +CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +. $CURDIR/../shell_config.sh + +set -e + +$CLICKHOUSE_CLIENT -q "DROP TABLE IF EXISTS alter_table" +$CLICKHOUSE_CLIENT -q "CREATE TABLE alter_table (a UInt8, b Int16, c Float32, d String, e Array(UInt8), f Nullable(UUID), g Tuple(UInt8, UInt16)) ENGINE = MergeTree ORDER BY a" + +function thread1() +{ + while true; do $CLICKHOUSE_CLIENT --query "SELECT * FROM system.columns FORMAT Null"; done +} + +function thread2() +{ + while true; do $CLICKHOUSE_CLIENT -n --query "ALTER TABLE alter_table ADD COLUMN h String; ALTER TABLE alter_table MODIFY COLUMN h UInt64; ALTER TABLE alter_table DROP COLUMN h;"; done +} + +# https://stackoverflow.com/questions/9954794/execute-a-shell-function-with-timeout +export -f thread1; +export -f thread2; + +timeout 5 bash -c thread1 & +timeout 5 bash -c thread2 & + +wait + +$CLICKHOUSE_CLIENT -q "DROP TABLE alter_table" From e4ba3a679263533b0826adf66020613fdc3b6d1c Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Fri, 3 May 2019 02:56:42 +0300 Subject: [PATCH 2/2] Fixed bad code --- dbms/src/Storages/AlterCommands.cpp | 11 ++++++----- dbms/src/Storages/AlterCommands.h | 9 +++++---- dbms/src/Storages/IStorage.cpp | 2 +- dbms/src/Storages/MergeTree/MergeTreeData.cpp | 2 +- dbms/src/Storages/StorageMergeTree.cpp | 2 +- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/dbms/src/Storages/AlterCommands.cpp b/dbms/src/Storages/AlterCommands.cpp index 1ff32b3d3b5..88f3e909f49 100644 --- a/dbms/src/Storages/AlterCommands.cpp +++ b/dbms/src/Storages/AlterCommands.cpp @@ -214,7 +214,7 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri column.codec = codec; } - if (!is_mutable()) + if (!isMutable()) { column.comment = comment; return; @@ -306,7 +306,7 @@ void AlterCommand::apply(ColumnsDescription & columns_description, IndicesDescri throw Exception("Wrong parameter type in ALTER query", ErrorCodes::LOGICAL_ERROR); } -bool AlterCommand::is_mutable() const +bool AlterCommand::isMutable() const { if (type == COMMENT_COLUMN) return false; @@ -328,6 +328,7 @@ void AlterCommands::apply(ColumnsDescription & columns_description, IndicesDescr for (const AlterCommand & command : *this) if (!command.ignore) command.apply(new_columns_description, new_indices_description, new_order_by_ast, new_primary_key_ast, new_ttl_table_ast); + columns_description = std::move(new_columns_description); indices_description = std::move(new_indices_description); order_by_ast = std::move(new_order_by_ast); @@ -495,7 +496,7 @@ void AlterCommands::validate(const IStorage & table, const Context & context) /// column has no associated alter command, let's create it /// add a new alter command to modify existing column this->emplace_back(AlterCommand{AlterCommand::MODIFY_COLUMN, - column.name, explicit_type, column.default_desc.kind, column.default_desc.expression}); + column.name, explicit_type, column.default_desc.kind, column.default_desc.expression, {}, {}, {}, {}}); command = &back(); } @@ -534,11 +535,11 @@ void AlterCommands::apply(ColumnsDescription & columns_description) const columns_description = std::move(out_columns_description); } -bool AlterCommands::is_mutable() const +bool AlterCommands::isMutable() const { for (const auto & param : *this) { - if (param.is_mutable()) + if (param.isMutable()) return true; } diff --git a/dbms/src/Storages/AlterCommands.h b/dbms/src/Storages/AlterCommands.h index e1600a9b9a6..8871f768020 100644 --- a/dbms/src/Storages/AlterCommands.h +++ b/dbms/src/Storages/AlterCommands.h @@ -73,8 +73,8 @@ struct AlterCommand AlterCommand() = default; AlterCommand(const Type type, const String & column_name, const DataTypePtr & data_type, const ColumnDefaultKind default_kind, const ASTPtr & default_expression, - const String & after_column = String{}, const String & comment = "", - const bool if_exists = false, const bool if_not_exists = false) // TODO: разобраться здесь с параметром по умолчанию + const String & after_column, const String & comment, + const bool if_exists, const bool if_not_exists) : type{type}, column_name{column_name}, data_type{data_type}, default_kind{default_kind}, default_expression{default_expression}, comment(comment), after_column{after_column}, if_exists(if_exists), if_not_exists(if_not_exists) @@ -84,8 +84,9 @@ struct AlterCommand void apply(ColumnsDescription & columns_description, IndicesDescription & indices_description, ASTPtr & order_by_ast, ASTPtr & primary_key_ast, ASTPtr & ttl_table_ast) const; + /// Checks that not only metadata touched by that command - bool is_mutable() const; + bool isMutable() const; }; class IStorage; @@ -101,7 +102,7 @@ public: void apply(ColumnsDescription & columns_description) const; void validate(const IStorage & table, const Context & context); - bool is_mutable() const; + bool isMutable() const; }; } diff --git a/dbms/src/Storages/IStorage.cpp b/dbms/src/Storages/IStorage.cpp index 697e919987c..147bc36e414 100644 --- a/dbms/src/Storages/IStorage.cpp +++ b/dbms/src/Storages/IStorage.cpp @@ -9,7 +9,7 @@ void IStorage::alter(const AlterCommands & params, const String & database_name, { for (const auto & param : params) { - if (param.is_mutable()) + if (param.isMutable()) throw Exception("Method alter supports only change comment of column for storage " + getName(), ErrorCodes::NOT_IMPLEMENTED); } diff --git a/dbms/src/Storages/MergeTree/MergeTreeData.cpp b/dbms/src/Storages/MergeTree/MergeTreeData.cpp index 453f680c6ba..4ce1c9f068a 100644 --- a/dbms/src/Storages/MergeTree/MergeTreeData.cpp +++ b/dbms/src/Storages/MergeTree/MergeTreeData.cpp @@ -1233,7 +1233,7 @@ void MergeTreeData::checkAlter(const AlterCommands & commands, const Context & c for (const AlterCommand & command : commands) { - if (!command.is_mutable()) + if (!command.isMutable()) { continue; } diff --git a/dbms/src/Storages/StorageMergeTree.cpp b/dbms/src/Storages/StorageMergeTree.cpp index 6ff9959a2f0..85bbcdb4bf7 100644 --- a/dbms/src/Storages/StorageMergeTree.cpp +++ b/dbms/src/Storages/StorageMergeTree.cpp @@ -200,7 +200,7 @@ void StorageMergeTree::alter( const Context & context, TableStructureWriteLockHolder & table_lock_holder) { - if (!params.is_mutable()) + if (!params.isMutable()) { lockStructureExclusively(table_lock_holder, context.getCurrentQueryId()); auto new_columns = getColumns();