From 7c4891885049eb99853005b02af62ed2484cf0e5 Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Thu, 22 Feb 2024 09:35:03 -0500 Subject: [PATCH] Retry SELECT queries on connection-related exceptions SELECT queries are idempotent and thus safe to retry automatically. This commit leverages `allow_retry` to enable automatic retries on queries that go through `ActiveRecord::ConnectionAdaptersDatabaseStatements#select`. This should help with connection-related exceptions stemming from network issues, such as `Trilogy::EOFError` or `Mysql2::Error`. Co-authored-by: Gannon McGibbon --- activerecord/CHANGELOG.md | 8 ++++++++ .../abstract/database_statements.rb | 4 ++-- .../mysql2/database_statements.rb | 2 +- .../sqlite3/database_statements.rb | 2 +- .../trilogy/database_statements.rb | 4 ++-- activerecord/test/cases/adapter_test.rb | 12 ++++++++++-- 6 files changed, 24 insertions(+), 8 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f4efdf4aae2f0..2c928bee59412 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Retry SELECT queries on connection-related exceptions + + SELECT queries are idempotent and thus safe to retry automatically. Previously, + adapters such as `TrilogyAdapter` would raise `ActiveRecord::ConnectionFailed: Trilogy::EOFError` + when encountering a connection error mid-request during a SELECT query. + + *Adrianna Chang*, *Gannon McGibbon* + * Add support for encrypting binary columns Ensure encryption and decryption pass `Type::Binary::Data` around for binary data. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 7c7a36acf645c..2993f3e1791ad 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -507,7 +507,7 @@ def high_precision_current_timestamp HIGH_PRECISION_CURRENT_TIMESTAMP end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: raise NotImplementedError end @@ -627,7 +627,7 @@ def select(sql, name = nil, binds = [], prepare: false, async: false) return future_result end - result = internal_exec_query(sql, name, binds, prepare: prepare) + result = internal_exec_query(sql, name, binds, prepare: prepare, allow_retry: true) if async FutureResult.wrap(result) else diff --git a/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb index ce79cd5a88531..122f47758204b 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb @@ -18,7 +18,7 @@ def select_all(*, **) # :nodoc: result end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: if without_prepared_statement?(binds) execute_and_free(sql, name, async: async) do |result| if result diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb index a91f08b6f15f7..feffa32690448 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb @@ -21,7 +21,7 @@ def explain(arel, binds = [], _options = []) SQLite3::ExplainPrettyPrinter.new.pp(result) end - def internal_exec_query(sql, name = nil, binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) diff --git a/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb b/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb index 8f145b155e6a9..9349dc038c25b 100644 --- a/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb @@ -12,12 +12,12 @@ def select_all(*, **) # :nodoc: result end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) mark_transaction_written_if_write(sql) - result = raw_execute(sql, name, async: async) + result = raw_execute(sql, name, async: async, allow_retry: allow_retry) ActiveRecord::Result.new(result.fields, result.to_a) end diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index 5fb8863c14e16..cf4d780b2cd56 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -632,17 +632,25 @@ def teardown assert_predicate @connection, :active? end - test "querying after a failed query restores and succeeds" do + test "querying after a failed non-retryable query restores and succeeds" do Post.first # Connection verified (and prepared statement pool populated if enabled) remote_disconnect @connection assert_raises(ActiveRecord::ConnectionFailed) do - Post.first # Connection no longer verified after failed query + @connection.execute("INSERT INTO posts(title, body) VALUES ('foo', 'bar')") end assert Post.first # Verifying the connection causes a reconnect and the query succeeds + assert_predicate @connection, :active? + end + + test "select queries are retried and result in a reconnect" do + Post.first # Connection verified (and prepared statement pool populated if enabled) + + remote_disconnect @connection + assert Post.first assert_predicate @connection, :active? end