Skip to content

Revert "Cleanup previous snapshot files during materialized view refresh" #26051

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

Merged

Conversation

mayankvadariya
Copy link
Contributor

@mayankvadariya mayankvadariya commented Jun 23, 2025

Description

This reverts commit 4813578.

The current change may disrupt an existing running query that is accessing a materialized view snapshot, which could be cleaned up by a 'REFRESH MATERIALIZED VIEW' command. This change needs to be reapplied, with the retention period for past snapshots controlled through a configuration setting.

The current change was also observed to cause REFRESH MV commands to stay stuck in FINISHING state for a long time due to slow cleanup of old snapshots

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## Iceberg
* Fix latency regression and potential query failures for 'REFRESH MATERIALIZED VIEW' command from 475 release. ({issue}`26051`)

@cla-bot cla-bot bot added the cla-signed label Jun 23, 2025
@github-actions github-actions bot added the iceberg Iceberg connector label Jun 23, 2025
@raunaqmorarka raunaqmorarka added the bug Something isn't working label Jun 23, 2025
@mayankvadariya mayankvadariya marked this pull request as draft June 23, 2025 15:55
@mayankvadariya mayankvadariya marked this pull request as ready for review June 23, 2025 15:58
@mayankvadariya
Copy link
Contributor Author

@raunaqmorarka thanks, please run CI with secrets.

@raunaqmorarka
Copy link
Member

@raunaqmorarka thanks, please run CI with secrets.

Why aren't the changes in TestIcebergFileOperations part of the revert commit ?

@raunaqmorarka
Copy link
Member

/test-with-secrets sha=4ef626791288aa8e0e5171f314caa3222342434e

Copy link

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/15830221921

@mayankvadariya
Copy link
Contributor Author

@raunaqmorarka thanks, please run CI with secrets.

Why aren't the changes in TestIcebergFileOperations part of the revert commit ?

I also wondered the same when I changed this PR to draft but then I realized the test had been removed through 5210b0e#diff-33f2d439b6cac05c1a8528131271f2ff815478c6344b174870c8688370056de1

@raunaqmorarka raunaqmorarka merged commit e2157e8 into trinodb:master Jun 23, 2025
42 checks passed
@github-actions github-actions bot added this to the 477 milestone Jun 23, 2025
@okayhooni
Copy link
Contributor

Thanks! @mayankvadariya

After upgrading Trino to version 476, we received a user report that the refresh query for an Iceberg materialized view no longer works and gets stuck at 100%.

Following the content of this PR, we will revert the regression-causing commit to address the issue!

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed iceberg Iceberg connector
Development

Successfully merging this pull request may close these issues.

3 participants