-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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.
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
Skip materialized view checking if source table does not exist #47975
Skip materialized view checking if source table does not exist #47975
Conversation
src/Core/Settings.h
Outdated
@@ -467,6 +467,7 @@ class IColumn; | |||
M(Int64, max_partitions_to_read, -1, "Limit the max number of partitions that can be accessed in one query. <= 0 means unlimited.", 0) \ | |||
M(Bool, check_query_single_value_result, true, "Return check query result as single 1/0 value", 0) \ | |||
M(Bool, allow_drop_detached, false, "Allow ALTER TABLE ... DROP DETACHED PART[ITION] ... queries", 0) \ | |||
M(Bool, skip_materialized_view_checking_if_source_table_not_exist, false, "Allow attaching to a materialized view even if dependent tables are inaccessible.", 0) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that an additional setting is required. For me this PR a bug fix that makes behaviour of ATTACH query consistent for the both ordinary and materialized views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that setting is probably redundant, ok to remove. (Though if not removing it - in my opinion its name should end with _on_attach
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always skip the check on ATTACH query, the check makes sense for CREATE only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should always skip the check on ATTACH query, the check makes sense for CREATE only
If I understood you correctly, we should check type compatibility only in cases of CREATE
. I have reverted changes and added a simple check that the query is not attached.
src/Core/Settings.h
Outdated
@@ -467,6 +467,7 @@ class IColumn; | |||
M(Int64, max_partitions_to_read, -1, "Limit the max number of partitions that can be accessed in one query. <= 0 means unlimited.", 0) \ | |||
M(Bool, check_query_single_value_result, true, "Return check query result as single 1/0 value", 0) \ | |||
M(Bool, allow_drop_detached, false, "Allow ALTER TABLE ... DROP DETACHED PART[ITION] ... queries", 0) \ | |||
M(Bool, skip_materialized_view_checking_if_source_table_not_exist, false, "Allow attaching to a materialized view even if dependent tables are inaccessible.", 0) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that setting is probably redundant, ok to remove. (Though if not removing it - in my opinion its name should end with _on_attach
)
if (StoragePtr to_table = DatabaseCatalog::instance().tryGetTable( | ||
{create.to_table_id.database_name, create.to_table_id.table_name, create.to_table_id.uuid}, | ||
getContext() | ||
)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor suggestion: consider using "early return": https://dev.to/jpswade/return-early-12o5
|
Backport #47975 to 23.2: Skip materialized view checking if source table does not exist
Backport #47975 to 22.8: Skip materialized view checking if source table does not exist
Backport #47975 to 23.1: Skip materialized view checking if source table does not exist
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed
UNKNOWN_TABLE
exception when attaching to a materialized view that has dependent tables that are not available.This might be useful when trying to restore state from a backup.
Steps to reproduce:
Expected behavior: attaching materialized view succeeds regardless of whether it's valid and references existing table, or not. It's important for backup-restore scenarios.
With ordinary view works as expected.