From 11ffe4c86c6a0b0ec995003632cc09f549c6a000 Mon Sep 17 00:00:00 2001 From: Conrad Ludgate Date: Tue, 24 Jun 2025 16:46:37 +0100 Subject: [PATCH] remove unused error retries --- proxy/src/proxy/retry.rs | 120 +------------------------------- proxy/src/serverless/backend.rs | 43 ------------ 2 files changed, 1 insertion(+), 162 deletions(-) diff --git a/proxy/src/proxy/retry.rs b/proxy/src/proxy/retry.rs index e9eca95724..32868e444c 100644 --- a/proxy/src/proxy/retry.rs +++ b/proxy/src/proxy/retry.rs @@ -1,4 +1,3 @@ -use std::error::Error; use std::io; use tokio::time; @@ -31,71 +30,6 @@ impl CouldRetry for io::Error { } } -impl CouldRetry for postgres_client::error::DbError { - fn could_retry(&self) -> bool { - use postgres_client::error::SqlState; - matches!( - self.code(), - &SqlState::CONNECTION_FAILURE - | &SqlState::CONNECTION_EXCEPTION - | &SqlState::CONNECTION_DOES_NOT_EXIST - | &SqlState::SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION, - ) - } -} -impl ShouldRetryWakeCompute for postgres_client::error::DbError { - fn should_retry_wake_compute(&self) -> bool { - use postgres_client::error::SqlState; - // Here are errors that happens after the user successfully authenticated to the database. - // TODO: there are pgbouncer errors that should be retried, but they are not listed here. - let non_retriable_pg_errors = matches!( - self.code(), - &SqlState::TOO_MANY_CONNECTIONS - | &SqlState::OUT_OF_MEMORY - | &SqlState::SYNTAX_ERROR - | &SqlState::T_R_SERIALIZATION_FAILURE - | &SqlState::INVALID_CATALOG_NAME - | &SqlState::INVALID_SCHEMA_NAME - | &SqlState::INVALID_PARAMETER_VALUE, - ); - if non_retriable_pg_errors { - return false; - } - // PGBouncer errors that should not trigger a wake_compute retry. - if self.code() == &SqlState::PROTOCOL_VIOLATION { - // Source for the error message: - // https://github.com/pgbouncer/pgbouncer/blob/f15997fe3effe3a94ba8bcc1ea562e6117d1a131/src/client.c#L1070 - return !self - .message() - .contains("no more connections allowed (max_client_conn)"); - } - true - } -} - -impl CouldRetry for postgres_client::Error { - fn could_retry(&self) -> bool { - if let Some(io_err) = self.source().and_then(|x| x.downcast_ref()) { - io::Error::could_retry(io_err) - } else if let Some(db_err) = self.source().and_then(|x| x.downcast_ref()) { - postgres_client::error::DbError::could_retry(db_err) - } else { - false - } - } -} -impl ShouldRetryWakeCompute for postgres_client::Error { - fn should_retry_wake_compute(&self) -> bool { - if let Some(db_err) = self.source().and_then(|x| x.downcast_ref()) { - postgres_client::error::DbError::should_retry_wake_compute(db_err) - } else { - // likely an IO error. Possible the compute has shutdown and the - // cache is stale. - true - } - } -} - impl CouldRetry for compute::ConnectionError { fn could_retry(&self) -> bool { match self { @@ -105,6 +39,7 @@ impl CouldRetry for compute::ConnectionError { } } } + impl ShouldRetryWakeCompute for compute::ConnectionError { fn should_retry_wake_compute(&self) -> bool { match self { @@ -120,56 +55,3 @@ pub(crate) fn retry_after(num_retries: u32, config: RetryConfig) -> time::Durati .base_delay .mul_f64(config.backoff_factor.powi((num_retries as i32) - 1)) } - -#[cfg(test)] -mod tests { - use postgres_client::error::{DbError, SqlState}; - - use super::ShouldRetryWakeCompute; - - #[test] - fn should_retry_wake_compute_for_db_error() { - // These SQLStates should NOT trigger a wake_compute retry. - let non_retry_states = [ - SqlState::TOO_MANY_CONNECTIONS, - SqlState::OUT_OF_MEMORY, - SqlState::SYNTAX_ERROR, - SqlState::T_R_SERIALIZATION_FAILURE, - SqlState::INVALID_CATALOG_NAME, - SqlState::INVALID_SCHEMA_NAME, - SqlState::INVALID_PARAMETER_VALUE, - ]; - for state in non_retry_states { - let err = DbError::new_test_error(state.clone(), "oops".to_string()); - assert!( - !err.should_retry_wake_compute(), - "State {state:?} unexpectedly retried" - ); - } - - // Errors coming from pgbouncer should not trigger a wake_compute retry - let non_retry_pgbouncer_errors = ["no more connections allowed (max_client_conn)"]; - for error in non_retry_pgbouncer_errors { - let err = DbError::new_test_error(SqlState::PROTOCOL_VIOLATION, error.to_string()); - assert!( - !err.should_retry_wake_compute(), - "PGBouncer error {error:?} unexpectedly retried" - ); - } - - // These SQLStates should trigger a wake_compute retry. - let retry_states = [ - SqlState::CONNECTION_FAILURE, - SqlState::CONNECTION_EXCEPTION, - SqlState::CONNECTION_DOES_NOT_EXIST, - SqlState::SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION, - ]; - for state in retry_states { - let err = DbError::new_test_error(state.clone(), "oops".to_string()); - assert!( - err.should_retry_wake_compute(), - "State {state:?} unexpectedly skipped retry" - ); - } - } -} diff --git a/proxy/src/serverless/backend.rs b/proxy/src/serverless/backend.rs index 57457c337e..ff2c20c9ea 100644 --- a/proxy/src/serverless/backend.rs +++ b/proxy/src/serverless/backend.rs @@ -29,7 +29,6 @@ use crate::control_plane::errors::{GetAuthInfoError, WakeComputeError}; use crate::error::{ErrorKind, ReportableError, UserFacingError}; use crate::intern::EndpointIdInt; use crate::proxy::connect_compute::TcpMechanism; -use crate::proxy::retry::{CouldRetry, ShouldRetryWakeCompute}; use crate::rate_limiter::EndpointRateLimiter; use crate::types::{EndpointId, LOCAL_PROXY_SUFFIX}; @@ -429,33 +428,6 @@ impl UserFacingError for HttpConnError { } } -impl CouldRetry for HttpConnError { - fn could_retry(&self) -> bool { - match self { - HttpConnError::ConnectError(e) => e.could_retry(), - HttpConnError::PostgresConnectionError(e) => e.could_retry(), - HttpConnError::LocalProxyConnectionError(e) => e.could_retry(), - HttpConnError::ComputeCtl(_) => false, - HttpConnError::ConnectionClosedAbruptly(_) => false, - HttpConnError::JwtPayloadError(_) => false, - HttpConnError::GetAuthInfo(_) => false, - HttpConnError::AuthError(_) => false, - HttpConnError::WakeCompute(_) => false, - HttpConnError::TooManyConnectionAttempts(_) => false, - } - } -} -impl ShouldRetryWakeCompute for HttpConnError { - fn should_retry_wake_compute(&self) -> bool { - match self { - HttpConnError::PostgresConnectionError(e) => e.should_retry_wake_compute(), - // we never checked cache validity - HttpConnError::TooManyConnectionAttempts(_) => false, - _ => true, - } - } -} - impl ReportableError for LocalProxyConnError { fn get_error_kind(&self) -> ErrorKind { match self { @@ -470,21 +442,6 @@ impl UserFacingError for LocalProxyConnError { } } -impl CouldRetry for LocalProxyConnError { - fn could_retry(&self) -> bool { - match self { - LocalProxyConnError::H2(_) => false, - } - } -} -impl ShouldRetryWakeCompute for LocalProxyConnError { - fn should_retry_wake_compute(&self) -> bool { - match self { - LocalProxyConnError::H2(_) => false, - } - } -} - async fn authenticate( ctx: &RequestContext, pool: &Arc>>,