Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only check MV on ALTER when necessary #48062

Merged
merged 3 commits into from
Apr 3, 2023
Merged

Only check MV on ALTER when necessary #48062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1a6c19
Only check MV on ALTER when necessary
Algunenano Mar 27, 2023
bffde5e
Merge remote-tracking branch 'blessed/master' into unnecessary_alter_…
Algunenano Mar 28, 2023
e01743a
Try to fix unrelated flakiness
Algunenano Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Storages/MergeTree/MergeTreeData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2930,7 +2930,8 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
old_types.emplace(column.name, column.type.get());

NamesAndTypesList columns_to_check_conversion;
auto name_deps = getDependentViewsByColumn(local_context);

std::optional<NameDependencies> name_deps{};
for (const AlterCommand & command : commands)
{
/// Just validate partition expression
Expand Down Expand Up @@ -3012,7 +3013,9 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context

if (!command.clear)
{
const auto & deps_mv = name_deps[command.column_name];
if (!name_deps)
name_deps = getDependentViewsByColumn(local_context);
const auto & deps_mv = name_deps.value()[command.column_name];
if (!deps_mv.empty())
{
throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
Expand Down
6 changes: 4 additions & 2 deletions src/Storages/StorageBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ void StorageBuffer::reschedule()

void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const
{
auto name_deps = getDependentViewsByColumn(local_context);
std::optional<NameDependencies> name_deps{};
for (const auto & command : commands)
{
if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN
Expand All @@ -1027,7 +1027,9 @@ void StorageBuffer::checkAlterIsPossible(const AlterCommands & commands, Context

if (command.type == AlterCommand::Type::DROP_COLUMN && !command.clear)
{
const auto & deps_mv = name_deps[command.column_name];
if (!name_deps)
name_deps = getDependentViewsByColumn(local_context);
const auto & deps_mv = name_deps.value()[command.column_name];
if (!deps_mv.empty())
{
throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
Expand Down
6 changes: 4 additions & 2 deletions src/Storages/StorageDistributed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ std::optional<QueryPipeline> StorageDistributed::distributedWrite(const ASTInser

void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const
{
auto name_deps = getDependentViewsByColumn(local_context);
std::optional<NameDependencies> name_deps{};
for (const auto & command : commands)
{
if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN
Expand All @@ -1406,7 +1406,9 @@ void StorageDistributed::checkAlterIsPossible(const AlterCommands & commands, Co

if (command.type == AlterCommand::DROP_COLUMN && !command.clear)
{
const auto & deps_mv = name_deps[command.column_name];
if (!name_deps)
name_deps = getDependentViewsByColumn(local_context);
const auto & deps_mv = name_deps.value()[command.column_name];
if (!deps_mv.empty())
{
throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
Expand Down
6 changes: 4 additions & 2 deletions src/Storages/StorageMerge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ StorageMerge::DatabaseTablesIterators StorageMerge::getDatabaseIterators(Context

void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, ContextPtr local_context) const
{
auto name_deps = getDependentViewsByColumn(local_context);
std::optional<NameDependencies> name_deps{};
for (const auto & command : commands)
{
if (command.type != AlterCommand::Type::ADD_COLUMN && command.type != AlterCommand::Type::MODIFY_COLUMN
Expand All @@ -930,7 +930,9 @@ void StorageMerge::checkAlterIsPossible(const AlterCommands & commands, ContextP

if (command.type == AlterCommand::Type::DROP_COLUMN && !command.clear)
{
const auto & deps_mv = name_deps[command.column_name];
if (!name_deps)
name_deps = getDependentViewsByColumn(local_context);
const auto & deps_mv = name_deps.value()[command.column_name];
if (!deps_mv.empty())
{
throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
Expand Down
6 changes: 4 additions & 2 deletions src/Storages/StorageNull.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void registerStorageNull(StorageFactory & factory)

void StorageNull::checkAlterIsPossible(const AlterCommands & commands, ContextPtr context) const
{
auto name_deps = getDependentViewsByColumn(context);
std::optional<NameDependencies> name_deps{};
for (const auto & command : commands)
{
if (command.type != AlterCommand::Type::ADD_COLUMN
Expand All @@ -50,7 +50,9 @@ void StorageNull::checkAlterIsPossible(const AlterCommands & commands, ContextPt

if (command.type == AlterCommand::DROP_COLUMN && !command.clear)
{
const auto & deps_mv = name_deps[command.column_name];
if (!name_deps)
name_deps = getDependentViewsByColumn(context);
const auto & deps_mv = name_deps.value()[command.column_name];
if (!deps_mv.empty())
{
throw Exception(ErrorCodes::ALTER_OF_COLUMN_IS_FORBIDDEN,
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/test_version_update_after_mutation/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ def test_mutate_and_upgrade(start_cluster):

node2.query("OPTIMIZE TABLE mt FINAL")

assert node1.query("SELECT id FROM mt") == "1\n4\n"
assert node2.query("SELECT id FROM mt") == "1\n4\n"
assert node1.query("SELECT id FROM mt ORDER BY id") == "1\n4\n"
assert node2.query("SELECT id FROM mt ORDER BY id") == "1\n4\n"

for node in [node1, node2]:
node.query("DROP TABLE mt")
Expand Down
1 change: 1 addition & 0 deletions tests/queries/0_stateless/02697_alter_dependencies.reference
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0 0
16 changes: 16 additions & 0 deletions tests/queries/0_stateless/02697_alter_dependencies.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
CREATE TABLE mv_source (a Int64, insert_time DateTime) ENGINE = MergeTree() ORDER BY insert_time;
CREATE TABLE mv_target (a Int64, insert_time DateTime) ENGINE = MergeTree() ORDER BY insert_time;
CREATE MATERIALIZED VIEW source_to_target to mv_target as Select * from mv_source where a not in (Select sleepEachRow(0.1) from numbers(50));

ALTER TABLE mv_source MODIFY TTL insert_time + toIntervalDay(1);
SYSTEM FLUSH LOGS;
-- This is a fancy way to check that the MV hasn't been called (no functions executed by ALTER)
SELECT
ProfileEvents['FunctionExecute'],
ProfileEvents['TableFunctionExecute']
FROM system.query_log
WHERE
type = 'QueryFinish' AND
query like '%ALTER TABLE mv_source%' AND
current_database = currentDatabase() AND
event_time > now() - INTERVAL 10 minute;