Skip to content

Make SQLServer deadlock query retries reusable #14947

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
hashhar merged 2 commits into from
Nov 15, 2022
Merged

Make SQLServer deadlock query retries reusable #14947

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
714c013
Extract constant for SQLServer deadlock error code
hashhar Nov 8, 2022
5de6556
Extract a retry policy for deadlocked queries
hashhar Nov 8, 2022
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
21 changes: 14 additions & 7 deletions plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import io.trino.spi.type.VarcharType;
import net.jodah.failsafe.Failsafe;
import net.jodah.failsafe.RetryPolicy;
import net.jodah.failsafe.function.CheckedSupplier;
import org.jdbi.v3.core.Handle;
import org.jdbi.v3.core.Jdbi;

Expand Down Expand Up @@ -190,6 +191,8 @@ public class SqlServerClient
// SqlServer supports 2100 parameters in prepared statement, let's create a space for about 4 big IN predicates
public static final int SQL_SERVER_MAX_LIST_EXPRESSIONS = 500;

public static final int SQL_SERVER_DEADLOCK_ERROR_CODE = 1205;

public static final JdbcTypeHandle BIGINT_TYPE = new JdbcTypeHandle(Types.BIGINT, Optional.of("bigint"), Optional.empty(), Optional.empty(), Optional.empty(), Optional.empty());

private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("uuuu-MM-dd");
Expand Down Expand Up @@ -1068,22 +1071,26 @@ private static Optional<DataCompression> getTableDataCompression(Handle handle,

private static Optional<DataCompression> getTableDataCompressionWithRetries(Handle handle, JdbcTableHandle table)
{
// DDL operations can take out locks against system tables causing the `getTableDataCompression` query to deadlock
final int maxAttemptCount = 3;
RetryPolicy<Optional<DataCompression>> retryPolicy = new RetryPolicy<Optional<DataCompression>>()
return retryOnDeadlock(() -> getTableDataCompression(handle, table), "error when getting table compression info for '%s'".formatted(table));
}

public static <T> T retryOnDeadlock(CheckedSupplier<T> supplier, String attemptLogMessage)
Copy link
Copy Markdown
Member

@kokosing kokosing Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to be so specific to stress the OnDeadlock part? I think there are certain operation that are retry-able where deadlock could be one of them. We may want retry in other places too so maybe having some utility for retries for sql-server could be beneficial.

Just a nit.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 15, 2022

Choose a reason for hiding this comment

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

Yes, would be useful. I don't any such cases today. Once we have them we can refactor - refactor without existing usage makes it difficult to decide optimal API.

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 15, 2022

Choose a reason for hiding this comment

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

Note that when we introduce such retry I expect it to actually live in BaseJdbcClient (for metadata) and JdbcRecordSetProvider (for data).

{
// DDL operations can take out locks against system tables causing queries against them to deadlock
int maxAttemptCount = 3;
RetryPolicy<T> retryPolicy = new RetryPolicy<T>()
Copy link
Copy Markdown
Member

@kokosing kokosing Nov 15, 2022

Choose a reason for hiding this comment

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

nit: retry policies can be extracted as constants

Copy link
Copy Markdown
Member Author

@hashhar hashhar Nov 15, 2022

Choose a reason for hiding this comment

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

that was the initial version but it looks cleaner this way - especially since the policy has no use outside of this very specific case (at-least today).

.withMaxAttempts(maxAttemptCount)
.handleIf(throwable ->
{
final int deadlockErrorCode = 1205;
Throwable rootCause = Throwables.getRootCause(throwable);
return rootCause instanceof SQLServerException &&
((SQLServerException) (rootCause)).getSQLServerError().getErrorNumber() == deadlockErrorCode;
((SQLServerException) (rootCause)).getSQLServerError().getErrorNumber() == SQL_SERVER_DEADLOCK_ERROR_CODE;
})
.onFailedAttempt(event -> log.warn(event.getLastFailure(), "Attempt %d of %d: error when getting table compression info for '%s'", event.getAttemptCount(), maxAttemptCount, table));
.onFailedAttempt(event -> log.warn(event.getLastFailure(), "Attempt %d of %d: %s", event.getAttemptCount(), maxAttemptCount, attemptLogMessage));

return Failsafe
.with(retryPolicy)
.get(() -> getTableDataCompression(handle, table));
.get(supplier);
}

private static class StatisticsDao
Expand Down