Skip to content

Ignore non-existing table when dropping Iceberg schema with cascade #27361

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.

Sign up for GitHub

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
ebyhr merged 1 commit into from
Nov 20, 2025
Merged

Ignore non-existing table when dropping Iceberg schema with cascade #27361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,7 +1171,11 @@ public void dropSchema(ConnectorSession session, String schemaName, boolean casc
dropView(session, viewName);
}
for (SchemaTableName tableName : listTables(session, Optional.of(schemaName))) {
dropTable(session, getTableHandle(session, tableName, Optional.empty(), Optional.empty()));
ConnectorTableHandle tableHandle = getTableHandle(session, tableName, Optional.empty(), Optional.empty());
if (tableHandle != null) {
Copy link
Member

@findepi findepi Nov 19, 2025

Choose a reason for hiding this comment

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

Worth code comment

Copy link
Member

@raunaqmorarka raunaqmorarka Nov 19, 2025

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@chenjian2664 chenjian2664 Nov 19, 2025

Choose a reason for hiding this comment

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

Why view not have such issue?

Copy link
Member

@Math-ias Math-ias Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Reposting to main PR thread. I think views don't have that issue because it's a quick update to remove them from the metastore. Files connected to an iceberg table will take longer to delete and that increases risk of the table disappearing underneath.

// getTableHandle method returns null if the table is dropped concurrently
dropTable(session, tableHandle);
Copy link
Member

@Math-ias Math-ias Nov 20, 2025

Choose a reason for hiding this comment

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

So to understand the motivation of the change correctly ... Looks like dropTable is potentially blocking (for a while) the thread?

following method calls in github

Therefore tables we've listed from metadata can get dropped from beneath this thread as a DROP SCHEMA schema_name CASCADE is completing ...

I'm guessing the risk of dropping a table taking so long is mostly when there's externally managed files. The delta lake connector should be comparable. In your implementation of CASCADE there in the delta lake connector you use an additional try on dropTable even after we've checked for tableHandle existence. Is that worth bringing back to this PR as well for extra safety?

Copy link
Member Author

@ebyhr ebyhr Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(I didn't read this message carefully before merging the PR). Each Iceberg catalog maintains its own table metadata cache, so the first TrinoCatalog.loadTable call from IcebergMetadata.getTableHandle populates the cache, and the second TrinoCatalog.loadTable in TrinoCatalog.dropTable will retrieve the object from the cache. We can still add the catch block, though.

}
}
}
catalog.dropNamespace(session, schemaName);
Expand Down
Loading