From 51021c11647b49f067de737c78c1532b0a77db32 Mon Sep 17 00:00:00 2001 From: feng lv Date: Sun, 28 Feb 2021 05:24:39 +0000 Subject: [PATCH 1/2] forbid to drop a column if it's referenced by materialized view --- src/Interpreters/InterpreterAlterQuery.cpp | 2 +- src/Interpreters/InterpreterSelectQuery.h | 2 + src/Storages/IStorage.cpp | 17 +- src/Storages/IStorage.h | 8 +- src/Storages/MergeTree/MergeTreeData.cpp | 26 ++- src/Storages/MergeTree/MergeTreeData.h | 2 +- src/Storages/StorageBuffer.cpp | 17 +- src/Storages/StorageBuffer.h | 2 +- src/Storages/StorageDistributed.cpp | 17 +- src/Storages/StorageDistributed.h | 2 +- src/Storages/StorageMaterializedView.cpp | 3 +- src/Storages/StorageMaterializedView.h | 2 +- src/Storages/StorageMerge.cpp | 15 +- src/Storages/StorageMerge.h | 2 +- src/Storages/StorageNull.cpp | 15 +- src/Storages/StorageNull.h | 2 +- src/Storages/StorageProxy.h | 4 +- ...bid_drop_column_referenced_by_mv.reference | 0 ...46_forbid_drop_column_referenced_by_mv.sql | 172 ++++++++++++++++++ 19 files changed, 285 insertions(+), 25 deletions(-) create mode 100644 tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.reference create mode 100644 tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql diff --git a/src/Interpreters/InterpreterAlterQuery.cpp b/src/Interpreters/InterpreterAlterQuery.cpp index 6294b31cc8c..37eaecf9a90 100644 --- a/src/Interpreters/InterpreterAlterQuery.cpp +++ b/src/Interpreters/InterpreterAlterQuery.cpp @@ -137,7 +137,7 @@ BlockIO InterpreterAlterQuery::execute() StorageInMemoryMetadata metadata = table->getInMemoryMetadata(); alter_commands.validate(metadata, context); alter_commands.prepare(metadata); - table->checkAlterIsPossible(alter_commands, context.getSettingsRef()); + table->checkAlterIsPossible(alter_commands, context); table->alter(alter_commands, context, alter_lock); } diff --git a/src/Interpreters/InterpreterSelectQuery.h b/src/Interpreters/InterpreterSelectQuery.h index 1fff316e1d4..2c563c0f917 100644 --- a/src/Interpreters/InterpreterSelectQuery.h +++ b/src/Interpreters/InterpreterSelectQuery.h @@ -89,6 +89,8 @@ public: static void addEmptySourceToQueryPlan(QueryPlan & query_plan, const Block & source_header, const SelectQueryInfo & query_info); + Names getRequiredColumns() { return required_columns; } + private: InterpreterSelectQuery( const ASTPtr & query_ptr_, diff --git a/src/Storages/IStorage.cpp b/src/Storages/IStorage.cpp index 2400b0587ba..7b68e84e6a1 100644 --- a/src/Storages/IStorage.cpp +++ b/src/Storages/IStorage.cpp @@ -134,7 +134,7 @@ void IStorage::alter(const AlterCommands & params, const Context & context, Tabl } -void IStorage::checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const +void IStorage::checkAlterIsPossible(const AlterCommands & commands, const Context & /* context */) const { for (const auto & command : commands) { @@ -182,6 +182,21 @@ Names IStorage::getAllRegisteredNames() const return result; } +NameDependencies IStorage::getColumnNamesAndReferencedMvMap(const Context & context) const +{ + NameDependencies name_deps; + auto dependencies = DatabaseCatalog::instance().getDependencies(storage_id); + for (const auto & depend_id : dependencies) + { + auto depend_table = DatabaseCatalog::instance().getTable(depend_id, context); + const auto & select_query = depend_table->getInMemoryMetadataPtr()->select.inner_query; + auto required_columns = InterpreterSelectQuery(select_query, context, SelectQueryOptions{}.noModify()).getRequiredColumns(); + for (const auto & col_name : required_columns) + name_deps[col_name].push_back(depend_id.table_name); + } + return name_deps; +} + std::string PrewhereDAGInfo::dump() const { WriteBufferFromOwnString ss; diff --git a/src/Storages/IStorage.h b/src/Storages/IStorage.h index 1a27dbd637f..72eb50e8a85 100644 --- a/src/Storages/IStorage.h +++ b/src/Storages/IStorage.h @@ -57,6 +57,8 @@ struct StreamLocalLimits; class EnabledQuota; struct SelectQueryInfo; +using NameDependencies = std::unordered_map>; + struct ColumnSize { size_t marks = 0; @@ -173,8 +175,10 @@ public: virtual NamesAndTypesList getVirtuals() const; Names getAllRegisteredNames() const override; -protected: + NameDependencies getColumnNamesAndReferencedMvMap(const Context & context) const; + +protected: /// Returns whether the column is virtual - by default all columns are real. /// Initially reserved virtual column name may be shadowed by real column. bool isVirtualColumn(const String & column_name, const StorageMetadataPtr & metadata_snapshot) const; @@ -362,7 +366,7 @@ public: /** Checks that alter commands can be applied to storage. For example, columns can be modified, * or primary key can be changes, etc. */ - virtual void checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const; + virtual void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const; /** * Checks that mutation commands can be applied to storage. diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index 2d841b98c59..ed6873f8370 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -4,12 +4,11 @@ #include #include #include -#include -#include #include +#include +#include #include #include -#include #include #include #include @@ -17,10 +16,11 @@ #include #include #include +#include #include +#include #include #include -#include #include #include #include @@ -30,10 +30,11 @@ #include #include #include +#include #include #include -#include #include +#include #include #include #include @@ -1407,12 +1408,14 @@ void checkVersionColumnTypesConversion(const IDataType * old_type, const IDataTy } -void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const +void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { /// Check that needed transformations can be applied to the list of columns without considering type conversions. StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); StorageInMemoryMetadata old_metadata = getInMemoryMetadata(); + const auto & settings = context.getSettingsRef(); + if (!settings.allow_non_metadata_alters) { @@ -1484,6 +1487,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S old_types.emplace(column.name, column.type.get()); NamesAndTypesList columns_to_check_conversion; + auto name_deps = getColumnNamesAndReferencedMvMap(context); for (const AlterCommand & command : commands) { /// Just validate partition expression @@ -1563,6 +1567,16 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const S "Trying to ALTER DROP key " + backQuoteIfNeed(command.column_name) + " column which is a part of key expression", ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); } + + auto deps_mv = name_deps[command.column_name]; + if (!deps_mv.empty()) + { + throw Exception( + "Trying to ALTER DROP column " + backQuoteIfNeed(command.column_name) + " which is referenced by materialized view " + + toString(deps_mv), + ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); + } + dropped_columns.emplace(command.column_name); } else if (command.isRequireMutationStage(getInMemoryMetadata())) diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h index f03f3f1dd8c..70c78af7bda 100644 --- a/src/Storages/MergeTree/MergeTreeData.h +++ b/src/Storages/MergeTree/MergeTreeData.h @@ -517,7 +517,7 @@ public: /// - all type conversions can be done. /// - columns corresponding to primary key, indices, sign, sampling expression and date are not affected. /// If something is wrong, throws an exception. - void checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; /// Checks if the Mutation can be performed. /// (currently no additional checks: always ok) diff --git a/src/Storages/StorageBuffer.cpp b/src/Storages/StorageBuffer.cpp index e28d5f4d6d1..46cdf237d91 100644 --- a/src/Storages/StorageBuffer.cpp +++ b/src/Storages/StorageBuffer.cpp @@ -58,6 +58,7 @@ namespace ErrorCodes extern const int LOGICAL_ERROR; extern const int INFINITE_LOOP; extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; + extern const int ALTER_OF_COLUMN_IS_FORBIDDEN; } @@ -910,8 +911,9 @@ void StorageBuffer::reschedule() flush_handle->scheduleAfter(std::min(min, max) * 1000); } -void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const +void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { + auto name_deps = getColumnNamesAndReferencedMvMap(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -919,6 +921,17 @@ void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, const S throw Exception( "Alter of type '" + alterTypeToString(command.type) + "' is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED); + if (command.type == AlterCommand::Type::DROP_COLUMN) + { + auto deps_mv = name_deps[command.column_name]; + if (!deps_mv.empty()) + { + throw Exception( + "Trying to ALTER DROP column " + backQuoteIfNeed(command.column_name) + " which is referenced by materialized view " + + toString(deps_mv), + ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); + } + } } } @@ -955,7 +968,7 @@ std::optional StorageBuffer::totalBytes(const Settings & /*settings*/) c void StorageBuffer::alter(const AlterCommands & params, const Context & context, TableLockHolder &) { auto table_id = getStorageID(); - checkAlterIsPossible(params, context.getSettingsRef()); + checkAlterIsPossible(params, context); auto metadata_snapshot = getInMemoryMetadataPtr(); /// Flush all buffers to storages, so that no non-empty blocks of the old diff --git a/src/Storages/StorageBuffer.h b/src/Storages/StorageBuffer.h index 46907ca196b..f6904ddb0e4 100644 --- a/src/Storages/StorageBuffer.h +++ b/src/Storages/StorageBuffer.h @@ -99,7 +99,7 @@ public: bool mayBenefitFromIndexForIn(const ASTPtr & left_in_operand, const Context & query_context, const StorageMetadataPtr & metadata_snapshot) const override; - void checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; /// The structure of the subordinate table is not checked and does not change. void alter(const AlterCommands & params, const Context & context, TableLockHolder & table_lock_holder) override; diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index c08dc38fa2d..337b89af017 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -84,6 +84,7 @@ namespace ErrorCodes extern const int TOO_MANY_ROWS; extern const int UNABLE_TO_SKIP_UNUSED_SHARDS; extern const int INVALID_SHARD_ID; + extern const int ALTER_OF_COLUMN_IS_FORBIDDEN; } namespace ActionLocks @@ -577,8 +578,9 @@ BlockOutputStreamPtr StorageDistributed::write(const ASTPtr &, const StorageMeta } -void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const +void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { + auto name_deps = getColumnNamesAndReferencedMvMap(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN @@ -589,6 +591,17 @@ void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, co throw Exception("Alter of type '" + alterTypeToString(command.type) + "' is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED); + if (command.type == AlterCommand::DROP_COLUMN) + { + auto deps_mv = name_deps[command.column_name]; + if (!deps_mv.empty()) + { + throw Exception( + "Trying to ALTER DROP column " + backQuoteIfNeed(command.column_name) + " which is referenced by materialized view " + + toString(deps_mv), + ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); + } + } } } @@ -596,7 +609,7 @@ void StorageDistributed::alter(const AlterCommands & params, const Context & con { auto table_id = getStorageID(); - checkAlterIsPossible(params, context.getSettingsRef()); + checkAlterIsPossible(params, context); StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); params.apply(new_metadata, context); DatabaseCatalog::instance().getDatabase(table_id.database_name)->alterTable(context, table_id, new_metadata); diff --git a/src/Storages/StorageDistributed.h b/src/Storages/StorageDistributed.h index 4d3869f7c5c..3980ea8244d 100644 --- a/src/Storages/StorageDistributed.h +++ b/src/Storages/StorageDistributed.h @@ -85,7 +85,7 @@ public: void rename(const String & new_path_to_table_data, const StorageID & new_table_id) override; void renameOnDisk(const String & new_path_to_table_data); - void checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; /// in the sub-tables, you need to manually add and delete columns /// the structure of the sub-table is not checked diff --git a/src/Storages/StorageMaterializedView.cpp b/src/Storages/StorageMaterializedView.cpp index 2d211c8061b..4022529edae 100644 --- a/src/Storages/StorageMaterializedView.cpp +++ b/src/Storages/StorageMaterializedView.cpp @@ -296,8 +296,9 @@ void StorageMaterializedView::alter( } -void StorageMaterializedView::checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const +void StorageMaterializedView::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { + const auto & settings = context.getSettingsRef(); if (settings.allow_experimental_alter_materialized_view_structure) { for (const auto & command : commands) diff --git a/src/Storages/StorageMaterializedView.h b/src/Storages/StorageMaterializedView.h index 4b10cf7a9b5..ccb56ec9fe5 100644 --- a/src/Storages/StorageMaterializedView.h +++ b/src/Storages/StorageMaterializedView.h @@ -54,7 +54,7 @@ public: void checkMutationIsPossible(const MutationCommands & commands, const Settings & settings) const override; - void checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; Pipe alterPartition(const StorageMetadataPtr & metadata_snapshot, const PartitionCommands & commands, const Context & context) override; diff --git a/src/Storages/StorageMerge.cpp b/src/Storages/StorageMerge.cpp index 91ebfaa3a27..1048cbc6aa3 100644 --- a/src/Storages/StorageMerge.cpp +++ b/src/Storages/StorageMerge.cpp @@ -37,6 +37,7 @@ namespace ErrorCodes extern const int ILLEGAL_PREWHERE; extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; extern const int SAMPLING_NOT_SUPPORTED; + extern const int ALTER_OF_COLUMN_IS_FORBIDDEN; } namespace @@ -472,8 +473,9 @@ DatabaseTablesIteratorPtr StorageMerge::getDatabaseIterator(const Context & cont } -void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const +void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { + auto name_deps = getColumnNamesAndReferencedMvMap(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -481,6 +483,17 @@ void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, const Se throw Exception( "Alter of type '" + alterTypeToString(command.type) + "' is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED); + if (command.type == AlterCommand::Type::DROP_COLUMN) + { + auto deps_mv = name_deps[command.column_name]; + if (!deps_mv.empty()) + { + throw Exception( + "Trying to ALTER DROP column " + backQuoteIfNeed(command.column_name) + " which is referenced by materialized view " + + toString(deps_mv), + ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); + } + } } } diff --git a/src/Storages/StorageMerge.h b/src/Storages/StorageMerge.h index 3ac251fbe52..eaffd34a379 100644 --- a/src/Storages/StorageMerge.h +++ b/src/Storages/StorageMerge.h @@ -38,7 +38,7 @@ public: size_t max_block_size, unsigned num_streams) override; - void checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; /// you need to add and remove columns in the sub-tables manually /// the structure of sub-tables is not checked diff --git a/src/Storages/StorageNull.cpp b/src/Storages/StorageNull.cpp index f324d502834..8b2c96ba436 100644 --- a/src/Storages/StorageNull.cpp +++ b/src/Storages/StorageNull.cpp @@ -16,6 +16,7 @@ namespace ErrorCodes { extern const int NOT_IMPLEMENTED; extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; + extern const int ALTER_OF_COLUMN_IS_FORBIDDEN; } @@ -35,8 +36,9 @@ void registerStorageNull(StorageFactory & factory) }); } -void StorageNull::checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const +void StorageNull::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { + auto name_deps = getColumnNamesAndReferencedMvMap(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -44,6 +46,17 @@ void StorageNull::checkAlterIsPossible(const AlterCommands & commands, const Set throw Exception( "Alter of type '" + alterTypeToString(command.type) + "' is not supported by storage " + getName(), ErrorCodes::NOT_IMPLEMENTED); + if (command.type == AlterCommand::DROP_COLUMN) + { + auto deps_mv = name_deps[command.column_name]; + if (!deps_mv.empty()) + { + throw Exception( + "Trying to ALTER DROP column " + backQuoteIfNeed(command.column_name) + " which is referenced by materialized view " + + toString(deps_mv), + ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); + } + } } } diff --git a/src/Storages/StorageNull.h b/src/Storages/StorageNull.h index 7d3d15f1b0f..943c056a588 100644 --- a/src/Storages/StorageNull.h +++ b/src/Storages/StorageNull.h @@ -41,7 +41,7 @@ public: return std::make_shared(metadata_snapshot->getSampleBlock()); } - void checkAlterIsPossible(const AlterCommands & commands, const Settings & /* settings */) const override; + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override; void alter(const AlterCommands & params, const Context & context, TableLockHolder & table_lock_holder) override; diff --git a/src/Storages/StorageProxy.h b/src/Storages/StorageProxy.h index fed9dd04e76..0349319d8fa 100644 --- a/src/Storages/StorageProxy.h +++ b/src/Storages/StorageProxy.h @@ -97,9 +97,9 @@ public: IStorage::setInMemoryMetadata(getNested()->getInMemoryMetadata()); } - void checkAlterIsPossible(const AlterCommands & commands, const Settings & settings) const override + void checkAlterIsPossible(const AlterCommands & commands, const Context & context) const override { - getNested()->checkAlterIsPossible(commands, settings); + getNested()->checkAlterIsPossible(commands, context); } Pipe alterPartition( diff --git a/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.reference b/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql b/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql new file mode 100644 index 00000000000..c05a7beff44 --- /dev/null +++ b/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql @@ -0,0 +1,172 @@ +-- MergeTree +DROP TABLE IF EXISTS `01746_merge_tree`; +CREATE TABLE `01746_merge_tree` +( + `n1` Int8, + `n2` Int8, + `n3` Int8, + `n4` Int8 +) +ENGINE = MergeTree +ORDER BY n1; + +DROP TABLE IF EXISTS `01746_merge_tree_mv`; +CREATE MATERIALIZED VIEW `01746_merge_tree_mv` +ENGINE = Memory AS +SELECT + n2, + n3 +FROM `01746_merge_tree`; + +ALTER TABLE `01746_merge_tree` + DROP COLUMN n3; -- { serverError 524 } + +ALTER TABLE `01746_merge_tree` + DROP COLUMN n2; -- { serverError 524 } + +-- ok +ALTER TABLE `01746_merge_tree` + DROP COLUMN n4; + +DROP TABLE `01746_merge_tree`; +DROP TABLE `01746_merge_tree_mv`; + +-- Null +DROP TABLE IF EXISTS `01746_null`; +CREATE TABLE `01746_null` +( + `n1` Int8, + `n2` Int8, + `n3` Int8 +) +ENGINE = Null; + +DROP TABLE IF EXISTS `01746_null_mv`; +CREATE MATERIALIZED VIEW `01746_null_mv` +ENGINE = Memory AS +SELECT + n1, + n2 +FROM `01746_null`; + +ALTER TABLE `01746_null` + DROP COLUMN n1; -- { serverError 524 } + +ALTER TABLE `01746_null` + DROP COLUMN n2; -- { serverError 524 } + +-- ok +ALTER TABLE `01746_null` + DROP COLUMN n3; + +DROP TABLE `01746_null`; +DROP TABLE `01746_null_mv`; + +-- Distributed + +DROP TABLE IF EXISTS `01746_local`; +CREATE TABLE `01746_local` +( + `n1` Int8, + `n2` Int8, + `n3` Int8 +) +ENGINE = Memory; + +DROP TABLE IF EXISTS `01746_dist`; +CREATE TABLE `01746_dist` AS `01746_local` +ENGINE = Distributed('test_shard_localhost', default, `01746_local`, rand()); + +DROP TABLE IF EXISTS `01746_dist_mv`; +CREATE MATERIALIZED VIEW `01746_dist_mv` +ENGINE = Memory AS +SELECT + n1, + n2 +FROM `01746_dist`; + +ALTER TABLE `01746_dist` + DROP COLUMN n1; -- { serverError 524 } + +ALTER TABLE `01746_dist` + DROP COLUMN n2; -- { serverError 524 } + +-- ok +ALTER TABLE `01746_dist` + DROP COLUMN n3; + +DROP TABLE `01746_local`; +DROP TABLE `01746_dist`; +DROP TABLE `01746_dist_mv`; + +-- Merge +DROP TABLE IF EXISTS `01746_merge_t`; +CREATE TABLE `01746_merge_t` +( + `n1` Int8, + `n2` Int8, + `n3` Int8 +) +ENGINE = Memory; + +DROP TABLE IF EXISTS `01746_merge`; +CREATE TABLE `01746_merge` AS `01746_merge_t` +ENGINE = Merge(default, '01746_merge_t'); + +DROP TABLE IF EXISTS `01746_merge_mv`; +CREATE MATERIALIZED VIEW `01746_merge_mv` +ENGINE = Memory AS +SELECT + n1, + n2 +FROM `01746_merge`; + +ALTER TABLE `01746_merge` + DROP COLUMN n1; -- { serverError 524 } + +ALTER TABLE `01746_merge` + DROP COLUMN n2; -- { serverError 524 } + +-- ok +ALTER TABLE `01746_merge` + DROP COLUMN n3; + +DROP TABLE `01746_merge_t`; +DROP TABLE `01746_merge`; +DROP TABLE `01746_merge_mv`; + +-- Buffer +DROP TABLE IF EXISTS `01746_buffer_t`; +CREATE TABLE `01746_buffer_t` +( + `n1` Int8, + `n2` Int8, + `n3` Int8 +) +ENGINE = Memory; + +DROP TABLE IF EXISTS `01746_buffer`; +CREATE TABLE `01746_buffer` AS `01746_buffer_t` +ENGINE = Buffer(default, `01746_buffer_t`, 16, 10, 100, 10000, 1000000, 10000000, 100000000); + +DROP TABLE IF EXISTS `01746_buffer_mv`; +CREATE MATERIALIZED VIEW `01746_buffer_mv` +ENGINE = Memory AS +SELECT + n1, + n2 +FROM `01746_buffer`; + +ALTER TABLE `01746_buffer` + DROP COLUMN n1; -- { serverError 524 } + +ALTER TABLE `01746_buffer` + DROP COLUMN n2; -- { serverError 524 } + +-- ok +ALTER TABLE `01746_buffer` + DROP COLUMN n3; + +DROP TABLE `01746_buffer_t`; +DROP TABLE `01746_buffer`; +DROP TABLE `01746_buffer_mv`; From a26c9e64a99f0f4d36a4fd47a1814c5f799462c1 Mon Sep 17 00:00:00 2001 From: feng lv Date: Sun, 28 Feb 2021 07:42:08 +0000 Subject: [PATCH 2/2] fix fix --- src/Storages/IStorage.cpp | 13 ++++++++----- src/Storages/IStorage.h | 2 +- src/Storages/MergeTree/MergeTreeData.cpp | 4 ++-- src/Storages/StorageBuffer.cpp | 4 ++-- src/Storages/StorageDistributed.cpp | 4 ++-- src/Storages/StorageMerge.cpp | 4 ++-- src/Storages/StorageNull.cpp | 4 ++-- .../01746_forbid_drop_column_referenced_by_mv.sql | 6 +++--- 8 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/Storages/IStorage.cpp b/src/Storages/IStorage.cpp index 7b68e84e6a1..5129d03cdee 100644 --- a/src/Storages/IStorage.cpp +++ b/src/Storages/IStorage.cpp @@ -182,17 +182,20 @@ Names IStorage::getAllRegisteredNames() const return result; } -NameDependencies IStorage::getColumnNamesAndReferencedMvMap(const Context & context) const +NameDependencies IStorage::getDependentViewsByColumn(const Context & context) const { NameDependencies name_deps; auto dependencies = DatabaseCatalog::instance().getDependencies(storage_id); for (const auto & depend_id : dependencies) { auto depend_table = DatabaseCatalog::instance().getTable(depend_id, context); - const auto & select_query = depend_table->getInMemoryMetadataPtr()->select.inner_query; - auto required_columns = InterpreterSelectQuery(select_query, context, SelectQueryOptions{}.noModify()).getRequiredColumns(); - for (const auto & col_name : required_columns) - name_deps[col_name].push_back(depend_id.table_name); + if (depend_table->getInMemoryMetadataPtr()->select.inner_query) + { + const auto & select_query = depend_table->getInMemoryMetadataPtr()->select.inner_query; + auto required_columns = InterpreterSelectQuery(select_query, context, SelectQueryOptions{}.noModify()).getRequiredColumns(); + for (const auto & col_name : required_columns) + name_deps[col_name].push_back(depend_id.table_name); + } } return name_deps; } diff --git a/src/Storages/IStorage.h b/src/Storages/IStorage.h index 72eb50e8a85..4dfd2ca50f3 100644 --- a/src/Storages/IStorage.h +++ b/src/Storages/IStorage.h @@ -176,7 +176,7 @@ public: Names getAllRegisteredNames() const override; - NameDependencies getColumnNamesAndReferencedMvMap(const Context & context) const; + NameDependencies getDependentViewsByColumn(const Context & context) const; protected: /// Returns whether the column is virtual - by default all columns are real. diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index ed6873f8370..292cd36e696 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -1487,7 +1487,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const C old_types.emplace(column.name, column.type.get()); NamesAndTypesList columns_to_check_conversion; - auto name_deps = getColumnNamesAndReferencedMvMap(context); + auto name_deps = getDependentViewsByColumn(context); for (const AlterCommand & command : commands) { /// Just validate partition expression @@ -1568,7 +1568,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, const C ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN); } - auto deps_mv = name_deps[command.column_name]; + const auto & deps_mv = name_deps[command.column_name]; if (!deps_mv.empty()) { throw Exception( diff --git a/src/Storages/StorageBuffer.cpp b/src/Storages/StorageBuffer.cpp index 46cdf237d91..e99f26c1a30 100644 --- a/src/Storages/StorageBuffer.cpp +++ b/src/Storages/StorageBuffer.cpp @@ -913,7 +913,7 @@ void StorageBuffer::reschedule() void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { - auto name_deps = getColumnNamesAndReferencedMvMap(context); + auto name_deps = getDependentViewsByColumn(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -923,7 +923,7 @@ void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, const C ErrorCodes::NOT_IMPLEMENTED); if (command.type == AlterCommand::Type::DROP_COLUMN) { - auto deps_mv = name_deps[command.column_name]; + const auto & deps_mv = name_deps[command.column_name]; if (!deps_mv.empty()) { throw Exception( diff --git a/src/Storages/StorageDistributed.cpp b/src/Storages/StorageDistributed.cpp index 337b89af017..ad904994e91 100644 --- a/src/Storages/StorageDistributed.cpp +++ b/src/Storages/StorageDistributed.cpp @@ -580,7 +580,7 @@ BlockOutputStreamPtr StorageDistributed::write(const ASTPtr &, const StorageMeta void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { - auto name_deps = getColumnNamesAndReferencedMvMap(context); + auto name_deps = getDependentViewsByColumn(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN @@ -593,7 +593,7 @@ void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, co ErrorCodes::NOT_IMPLEMENTED); if (command.type == AlterCommand::DROP_COLUMN) { - auto deps_mv = name_deps[command.column_name]; + const auto & deps_mv = name_deps[command.column_name]; if (!deps_mv.empty()) { throw Exception( diff --git a/src/Storages/StorageMerge.cpp b/src/Storages/StorageMerge.cpp index 1048cbc6aa3..46be91ba258 100644 --- a/src/Storages/StorageMerge.cpp +++ b/src/Storages/StorageMerge.cpp @@ -475,7 +475,7 @@ DatabaseTablesIteratorPtr StorageMerge::getDatabaseIterator(const Context & cont void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { - auto name_deps = getColumnNamesAndReferencedMvMap(context); + auto name_deps = getDependentViewsByColumn(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -485,7 +485,7 @@ void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, const Co ErrorCodes::NOT_IMPLEMENTED); if (command.type == AlterCommand::Type::DROP_COLUMN) { - auto deps_mv = name_deps[command.column_name]; + const auto & deps_mv = name_deps[command.column_name]; if (!deps_mv.empty()) { throw Exception( diff --git a/src/Storages/StorageNull.cpp b/src/Storages/StorageNull.cpp index 8b2c96ba436..ed9a7fffc63 100644 --- a/src/Storages/StorageNull.cpp +++ b/src/Storages/StorageNull.cpp @@ -38,7 +38,7 @@ void registerStorageNull(StorageFactory & factory) void StorageNull::checkAlterIsPossible(const AlterCommands & commands, const Context & context) const { - auto name_deps = getColumnNamesAndReferencedMvMap(context); + auto name_deps = getDependentViewsByColumn(context); for (const auto & command : commands) { if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN @@ -48,7 +48,7 @@ void StorageNull::checkAlterIsPossible(const AlterCommands & commands, const Con ErrorCodes::NOT_IMPLEMENTED); if (command.type == AlterCommand::DROP_COLUMN) { - auto deps_mv = name_deps[command.column_name]; + const auto & deps_mv = name_deps[command.column_name]; if (!deps_mv.empty()) { throw Exception( diff --git a/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql b/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql index c05a7beff44..f084cae7780 100644 --- a/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql +++ b/tests/queries/0_stateless/01746_forbid_drop_column_referenced_by_mv.sql @@ -75,7 +75,7 @@ ENGINE = Memory; DROP TABLE IF EXISTS `01746_dist`; CREATE TABLE `01746_dist` AS `01746_local` -ENGINE = Distributed('test_shard_localhost', default, `01746_local`, rand()); +ENGINE = Distributed('test_shard_localhost', currentDatabase(), `01746_local`, rand()); DROP TABLE IF EXISTS `01746_dist_mv`; CREATE MATERIALIZED VIEW `01746_dist_mv` @@ -111,7 +111,7 @@ ENGINE = Memory; DROP TABLE IF EXISTS `01746_merge`; CREATE TABLE `01746_merge` AS `01746_merge_t` -ENGINE = Merge(default, '01746_merge_t'); +ENGINE = Merge(currentDatabase(), '01746_merge_t'); DROP TABLE IF EXISTS `01746_merge_mv`; CREATE MATERIALIZED VIEW `01746_merge_mv` @@ -147,7 +147,7 @@ ENGINE = Memory; DROP TABLE IF EXISTS `01746_buffer`; CREATE TABLE `01746_buffer` AS `01746_buffer_t` -ENGINE = Buffer(default, `01746_buffer_t`, 16, 10, 100, 10000, 1000000, 10000000, 100000000); +ENGINE = Buffer(currentDatabase(), `01746_buffer_t`, 16, 10, 100, 10000, 1000000, 10000000, 100000000); DROP TABLE IF EXISTS `01746_buffer_mv`; CREATE MATERIALIZED VIEW `01746_buffer_mv`