Skip to content

Oracle turns off auto commit to drop temporary table #17756

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
losipiuk merged 1 commit into from
Jun 6, 2023
Merged

Oracle turns off auto commit to drop temporary table #17756

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
5 changes: 5 additions & 0 deletions plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ protected void dropTable(ConnectorSession session, RemoteTableName remoteTableNa
String dropTableSql = "DROP TABLE " + quotedTable;
try (Connection connection = connectionFactory.openConnection(session)) {
if (temporaryTable) {
// Turn off auto-commit so the lock is held until after the DROP
connection.setAutoCommit(false);
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 5, 2023

Choose a reason for hiding this comment

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

what is the motivation behind the change? Should we commit explicitly then? Or was switching auto-commit back to true just before connection is closed for that?

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 5, 2023

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 5, 2023

Choose a reason for hiding this comment

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

I see:

image

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 5, 2023

Choose a reason for hiding this comment

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

Please add a comment below - or just call commit() - will be more readable I think

Copy link
Copy Markdown
Contributor Author

@mwd410 mwd410 Jun 5, 2023

Choose a reason for hiding this comment

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

The motivation is that, because autoCommit is true when we get the Connection, the lock is immediately released because that LOCK TABLE statement is auto-committed, so the Connection has to re-acquire the lock for the DROP TABLE, so we have the same situation as we did before. Turning off auto commit holds onto that lock until after the drop table. Oracle's docs say DDL always auto-commits, but I reset autoCommit to true after the drop just in case.

Copy link
Copy Markdown
Contributor Author

@mwd410 mwd410 Jun 5, 2023

Choose a reason for hiding this comment

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

Oh, weird - when i typed out my reply, i only had your first comment in this thread visible to me.

I think i want to keep it as setAutoCommit(true) as it properly handles both cases:
image

// By default, when dropping a table, oracle does not wait for the table lock.
// If another transaction is using the table at the same time, DROP TABLE will throw.
// The solution is to first lock the table, waiting for other active transactions to complete.
Expand All @@ -315,6 +317,9 @@ protected void dropTable(ConnectorSession session, RemoteTableName remoteTableNa
dropTableSql += " PURGE";
}
execute(session, connection, dropTableSql);
// Commit the transaction (for temporaryTables), or a no-op for regular tables.
// This is better than connection.commit() because you're not supposed to commit() if autoCommit is true.
connection.setAutoCommit(true);
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jun 5, 2023

Choose a reason for hiding this comment

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

this is the last operation on connection - why switch back?

Copy link
Copy Markdown
Contributor Author

@mwd410 mwd410 Jun 5, 2023

Choose a reason for hiding this comment

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

I think this is answered by my most recent response in the other thread

}
catch (SQLException e) {
throw new TrinoException(JDBC_ERROR, e);
Expand Down