Skip to content

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

@github-actions
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

@chenjian2664
Copy link
Contributor

@raunaqmorarka @mayankvadariya @ebyhr Can we revisit this now - add it back, since the #26230 probably fixes the thread safety that in the original 4813578

see #26310

@raunaqmorarka
Copy link
Member

@chenjian2664 There were two reasons for reverting this

  1. It was causing REFRESH MV to be slow. We would need to verify that this would no longer be the case with new implementation.
  2. It was potentially deleting snapshots that some concurrent table scan on the MV could have been reading. We should always keep at least the previous snapshot to the new one around for running queries. For MVs which update frequently and have long running queries on them, we could expose a snapshots_retention_threshold MV property have it default to 4h or something similar.

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.

4 participants