-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix deadlock failure when listing objects in SQL Server connector #27259
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
base: master
Are you sure you want to change the base?
Conversation
f2ed50d to
207e1f9
Compare
| throws SQLException | ||
| { | ||
| Connection connection = connectionFactory.openConnection(session); | ||
| connection.setTransactionIsolation(TRANSACTION_READ_UNCOMMITTED); |
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.
What is the downside of using TRANSACTION_READ_UNCOMMITTED for table listing?
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.
The downside is potential risk of dirty reads. For instance, the connector might see a table that will be rolled back (doesn't really exist).
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.
I don't think "read uncommitted" is desired.
the sql server's snapshot isolation was supposed to avoid deadlocks.
Besides, listing operations should be retried on retryable exceptions (as should be all other forms of network communication with external services), and those retries would likely solve the problem at hand
plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerConnectorTest.java
Show resolved
Hide resolved
It would be worthwhile to explain what is the rationale for going with the TRANSACTION_READ_UNCOMMITTED isolation level. |
| { | ||
| super.testShowTablesLike(); | ||
| try { | ||
| super.testUpdateRowConcurrently(); |
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.
This test creates test_update_row... tables which will likely left unaffected the SHOW TABLES LIKE 'or%' query from testShowTablesLike.
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.
That is fine because the condition isn't pushed-down to SQL Server. I confirmed the current test can reproduce a deadlock issue.
This resolve "Transaction was deadlocked on lock resources with another process" when listing objects in SQL Server connector.
207e1f9 to
17700c0
Compare
|
CI hit #26567 |
Description
Fixes #10846
Another approach would be adding retry logic.
Release notes