-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix Iceberg materialized view storage table cleanup #20892
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
Conversation
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.
sorry if this is a silly question but why dropping storage table when update failed? I understand doing it when create failed, but for update im not so sure.
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.
So the case here is when you do a CREATE OR REPLACE MV and there is a MV with that name already. In that case we create a new storage table. If the update fails, the new storage table is cleaned up here because it is not referenced by the MV.
There is something missing here though, when you do a replace the old storage table is not cleaned up on a successful operation. Should do that here.
findepi
left a comment
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.
LGTM but mind @homar 's comment
77a242e to
305f995
Compare
|
@findepi can you kick off a test with secrets? |
ec4eeb3 to
cb07273
Compare
|
/test-with-secrets sha=cb0727336de48b9c28841c7aaf2f34fda99d8c38 |
|
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8147198341 |
cb07273 to
9f93352
Compare
|
Rebased but I think this should be good to go |
Description
Fixes: #20837
As well as some other edge cases which can happen if a
CREATE MATERIALIZED VIEWoperation fails.Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: