Skip to content

Fix a bug in dependencies causing MVs to miss INSERTs#82222

Merged
evillique merged 4 commits intoClickHouse:masterfrom
evillique:fix-mv-missing-insert
Jun 25, 2025
Merged

Fix a bug in dependencies causing MVs to miss INSERTs#82222
evillique merged 4 commits intoClickHouse:masterfrom
evillique:fix-mv-missing-insert

Conversation

@evillique
Copy link
Copy Markdown
Member

@evillique evillique commented Jun 19, 2025

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix a bug in table dependencies causing Materialized Views to miss INSERT queries.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@evillique evillique mentioned this pull request Jun 19, 2025
Merged
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 19, 2025 •
edited
Loading

Workflow [PR], commit [32737e3]

Summary: ✅

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Jun 19, 2025
@antaljanosbenjamin antaljanosbenjamin self-assigned this Jun 19, 2025
@evillique
Copy link
Copy Markdown
Member Author

evillique commented Jun 20, 2025 •
edited
Loading

The bug description:

In the beginning, when we execute the CREATE query, we add a db.mv node into our view_dependencies.

ClickHouse/src/Interpreters/InterpreterCreateQuery.cpp

Line 1494 in 090cb5a

DatabaseCatalog::instance().addDependencies(qualified_name, ref_dependencies.dependencies, loading_dependencies, ref_dependencies.mv_from_dependency ? TableNamesSet{ref_dependencies.mv_from_dependency->getQualifiedName()} : TableNamesSet{});

During the ALTER query we update this db.mv node to a node with UUID, so db.mv (xxxx-...).

ClickHouse/src/Databases/DatabaseOrdinary.cpp

Line 660 in 090cb5a

DatabaseCatalog::instance().updateDependencies(table_id, ref_dependencies.dependencies, loading_dependencies, ref_dependencies.mv_from_dependency ? TableNamesSet{ref_dependencies.mv_from_dependency->getQualifiedName()} : TableNamesSet{});

After that if we rename db.mv (xxxx-...) to db.mv_2 (xxxx-...) without remove_isolated_tables flag it will leave db.mv (xxxx-...) behind, and even after dropping this table, it will be left in view_dependencies.

ClickHouse/src/Interpreters/InterpreterRenameQuery.cpp

Line 152 in 090cb5a

std::tie(from_ref_dependencies, from_loading_dependencies, from_mv_dependencies) = database_catalog.removeDependencies(from_table_id, check_ref_deps, check_loading_deps, false, /*mv*/ true);

ClickHouse/src/Interpreters/DatabaseCatalog.cpp

Line 1601 in 090cb5a

view_dependencies.removeDependency(the_table_from, table_id);

So now, if we to create another mv with the same name db.mv (yyyy-...), then during create query we would try to add a node db.mv, and it would simply match the old, forgot-to-be-removed node db.mv (xxxx-...) and do nothing.

So finally we have db.mv (yyyy-...) as existing table and db.mv (xxxx-...) in view_dependencies. We plan the INSERT using these dependencies, and now we are trying to find db.mv (xxxx-...) table, don't find it and so now all the INSERTs will be missed by this MV.

From fiddle it seems like the first version where it reproduces is 25.5, so we probably only started to use these dependencies for inserts since #79963.

antaljanosbenjamin
antaljanosbenjamin approved these changes Jun 23, 2025
View reviewed changes
src/Interpreters/DatabaseCatalog.cpp
assert(tables_from.size() == 1);
const auto & the_table_from = *tables_from.begin();

view_dependencies.removeDependency(the_table_from, StorageID{old_name, table_name});
Copy link
Copy Markdown
Member

@antaljanosbenjamin antaljanosbenjamin Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I see, we always call removeDependency with remove_isolated_tables = true. What do you think about either removing remove_isolated_tables or changing its default value to make TablesDependencyGraph less bugphrone?

Copy link
Copy Markdown
Member Author

@evillique evillique Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@evillique evillique added this pull request to the merge queue Jun 25, 2025
Merged via the queue into ClickHouse:master with commit a198d48 Jun 25, 2025
121 checks passed
@evillique evillique deleted the fix-mv-missing-insert branch June 25, 2025 12:02
@robot-ch-test-poll4 robot-ch-test-poll4 mentioned this pull request Jun 25, 2025
Merged
robot-ch-test-poll4 added a commit that referenced this pull request Jun 25, 2025
@robot-ch-test-poll4
Merge pull request #82587 from ClickHouse/cherrypick/25.5/82222
e604ea1 
Cherry pick #82222 to 25.5: Fix a bug in dependencies causing MVs to miss INSERTs
robot-clickhouse added a commit that referenced this pull request Jun 25, 2025
@robot-clickhouse
Backport #82222 to 25.5: Fix a bug in dependencies causing MVs to mis…
576bfa5 
…s INSERTs
This was referenced Jun 25, 2025
Merged
Merged
robot-ch-test-poll4 added a commit that referenced this pull request Jun 25, 2025
@robot-ch-test-poll4
Merge pull request #82589 from ClickHouse/cherrypick/25.6/82222
6838946 
Cherry pick #82222 to 25.6: Fix a bug in dependencies causing MVs to miss INSERTs
robot-clickhouse added a commit that referenced this pull request Jun 25, 2025
@robot-clickhouse
Backport #82222 to 25.6: Fix a bug in dependencies causing MVs to mis…
e4d2016 
…s INSERTs
@robot-ch-test-poll4 robot-ch-test-poll4 mentioned this pull request Jun 25, 2025
Merged
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 25, 2025
@robot-ch-test-poll robot-ch-test-poll added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jun 25, 2025
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-backports-created-cloud deprecated label, NOOP label Jun 25, 2025
evillique added a commit that referenced this pull request Jun 25, 2025
@evillique
Merge pull request #82590 from ClickHouse/backport/25.6/82222
64f544c 
Backport #82222 to 25.6: Fix a bug in dependencies causing MVs to miss INSERTs
evillique added a commit that referenced this pull request Jun 25, 2025
@evillique
Merge pull request #82588 from ClickHouse/backport/25.5/82222
d86a096 
Backport #82222 to 25.5: Fix a bug in dependencies causing MVs to miss INSERTs
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antaljanosbenjamin antaljanosbenjamin antaljanosbenjamin approved these changes

Assignees

@antaljanosbenjamin antaljanosbenjamin

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-bugfix Pull request with bugfix, not backported by default pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v25.6-must-backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@evillique @antaljanosbenjamin @robot-clickhouse @robot-ch-test-poll @robot-ch-test-poll1 @robot-clickhouse-ci-2