Skip to content

Commit 73edae7

Browse files
Recover from failed connections in PostgreSQL
Fixes rails#49781 Prior to this commit, the PostgreSQL adapter could end up holding onto a broken connection longer than expected, potentially **into the next request after the executor checkin/checkout**. The problem is that the PostgreSQL adapter was using a [two separate calls] to `with_raw_connection` for preparing vs. executing a prepared statement. That's fine when the statement is first prepared, but once it is in the statement pool the first call to `with_raw_connection` [does not actually use the connection]. But that doesn't stop it from marking the connection as [`@verified`][verified], preventing the second `with_raw_connection` from verifying and getting a chance to reconnect. [two separate calls]: https://github.com/rails/rails/blob/e33dbe1b3d423143685154b97ae0992f4ab821b8/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L910-L917 [does not actually use the connection]: https://github.com/rails/rails/blob/490b9fc22faefca30565a970a8727c915de589ff/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L964-L966 [verified]: https://github.com/rails/rails/blob/e33dbe1b3d423143685154b97ae0992f4ab821b8/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L1032 ```rb Post.first => prepare_statement verifies the connection (if needed) and populates the statement pool => exec_prepared succeeds => Oops!, we lost the connection Post.first => prepare_statement returns the statement from the statement pool, connection still marked as verified => exec_prepared fails, marks the connection as unverified Post.first => prepare_statement returns the statement from the statement pool, marks the connection as verified!!! => exec_prepared fails, marks the connection as unverified => request finishes, connection gets marked as unverified for the next request Post.first => prepare_statement returns the statement from the statement pool, marks the connection as verified!!! => exec_prepared fails, marks the connection as unverified ``` The only way to recover is to execute a statement that hasn't been prepared yet, one that isn't prepared because it has no binds, or one that has `allow_retry: true` (which I think is limited to a few internal queries at the moment—we don't pass the value through to `with_raw_connection` for prepared statements anyway). This commit fixes the problem by preparing and executing the statement within a single `with_raw_connection` block. This is what the other adapters with prepared statements already do. It makes more sense this way—even if we managed to reconnect before executing the statement in the old code, we'd be referencing a prepared statement from a different connection, so it'd fail anyway. Having the prepare and execute in the same `with_raw_connection` means if we do need to reconnect, we'll start from the beginning and prepare the statement again on the fresh connection. Note that this change does get rid of connection retries on preparing statements, but I think that's OK. It matches the other adapters, matches the Rails 7.0 behavior, and it's still better than ending up with a broken connection.
1 parent 56b7357 commit 73edae7

File tree

2 files changed

+29
-17
lines changed

2 files changed

+29
-17
lines changed

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -907,10 +907,10 @@ def exec_cache(sql, name, binds, async:, allow_retry:, materialize_transactions:
907907

908908
update_typemap_for_default_timezone
909909

910-
stmt_key = prepare_statement(sql, binds)
911-
type_casted_binds = type_casted_binds(binds)
912-
913910
with_raw_connection do |conn|
911+
stmt_key = prepare_statement(sql, binds, conn)
912+
type_casted_binds = type_casted_binds(binds)
913+
914914
log(sql, name, binds, type_casted_binds, stmt_key, async: async) do
915915
conn.exec_prepared(stmt_key, type_casted_binds)
916916
end
@@ -960,22 +960,20 @@ def sql_key(sql)
960960

961961
# Prepare the statement if it hasn't been prepared, return
962962
# the statement key.
963-
def prepare_statement(sql, binds)
964-
with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn|
965-
sql_key = sql_key(sql)
966-
unless @statements.key? sql_key
967-
nextkey = @statements.next_key
968-
begin
969-
conn.prepare nextkey, sql
970-
rescue => e
971-
raise translate_exception_class(e, sql, binds)
972-
end
973-
# Clear the queue
974-
conn.get_last_result
975-
@statements[sql_key] = nextkey
963+
def prepare_statement(sql, binds, conn)
964+
sql_key = sql_key(sql)
965+
unless @statements.key? sql_key
966+
nextkey = @statements.next_key
967+
begin
968+
conn.prepare nextkey, sql
969+
rescue => e
970+
raise translate_exception_class(e, sql, binds)
976971
end
977-
@statements[sql_key]
972+
# Clear the queue
973+
conn.get_last_result
974+
@statements[sql_key] = nextkey
978975
end
976+
@statements[sql_key]
979977
end
980978

981979
# Connects to a PostgreSQL server and sets up the adapter depending on the

activerecord/test/cases/adapter_test.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,20 @@ def teardown
612612
assert_predicate @connection, :active?
613613
end
614614

615+
test "querying after a failed query restores and succeeds" do
616+
Post.first # Connection verified (and prepared statement pool populated if enabled)
617+
618+
remote_disconnect @connection
619+
620+
assert_raises(ActiveRecord::ConnectionFailed) do
621+
Post.first # Connection no longer verified after failed query
622+
end
623+
624+
assert Post.first # Verifying the connection causes a reconnect and the query succeeds
625+
626+
assert_predicate @connection, :active?
627+
end
628+
615629
test "transaction restores after remote disconnection" do
616630
remote_disconnect @connection
617631
Post.transaction do

0 commit comments

Comments
 (0)