diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 4090cc02eeba2..7024bedbdb072 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* + * Fix `has_one` association autosave setting the foreign key attribute when it is unchanged. This behaviour is also inconsistent with autosaving `belongs_to` and can have unintended side effects like raising 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 35f453461fe05..799e5ede96338 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/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index b2241fb5d260e..ccb45dfce0d5a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -229,12 +229,12 @@ def disable_referential_integrity # :nodoc: # Mysql2Adapter doesn't have to free a result after using it, but we use this method # to write stuff in an abstract way without concerning ourselves about whether it # needs to be explicitly freed or not. - def execute_and_free(sql, name = nil, async: false) # :nodoc: + def execute_and_free(sql, name = nil, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) mark_transaction_written_if_write(sql) - yield raw_execute(sql, name, async: async) + yield raw_execute(sql, name, async: async, allow_retry: allow_retry) end def begin_db_transaction # :nodoc: 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..7d4e4105da2ef 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb @@ -18,9 +18,9 @@ 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| + execute_and_free(sql, name, async: async, allow_retry: allow_retry) do |result| if result build_result(columns: result.fields, rows: result.to_a) else diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 8dcfc85767f90..f1962ff494095 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -881,7 +881,7 @@ def exec_no_cache(sql, name, binds, async:, allow_retry:, materialize_transactio type_casted_binds = type_casted_binds(binds) log(sql, name, binds, type_casted_binds, async: async) do |notification_payload| - with_raw_connection(allow_retry: false, materialize_transactions: materialize_transactions) do |conn| + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| result = conn.exec_params(sql, type_casted_binds) verified! notification_payload[:row_count] = result.count @@ -895,7 +895,7 @@ def exec_cache(sql, name, binds, async:, allow_retry:, materialize_transactions: update_typemap_for_default_timezone - with_raw_connection(allow_retry: false, materialize_transactions: materialize_transactions) do |conn| + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| stmt_key = prepare_statement(sql, binds, conn) type_casted_binds = type_casted_binds(binds) 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 9aa44743566eb..69c732bc7bc99 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -626,17 +626,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