Skip to content

Commit dbe2bb7

Browse files
skunkworkerclaude
andcommitted
Fix PostgreSQL savepoint handling and client_min_messages getter under JDBC
Correct savepoint behaviour in the shared transaction support and fix the PostgreSQL#client_min_messages getter, which did not work under the JDBC adapter (it does not inherit the native pg adapter implementation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3b73545 commit dbe2bb7

3 files changed

Lines changed: 57 additions & 4 deletions

File tree

lib/arjdbc/abstract/transaction_support.rb

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,16 @@ def exec_rollback_db_transaction
6666

6767
########################## Savepoint Interface ############################
6868

69+
# Save-point operations must NOT be retried on a connection failure. They
70+
# only ever run inside an already-open transaction, so a dropped backend
71+
# means the transaction's prior writes are gone. Retrying via
72+
# `with_raw_connection(allow_retry: true)` would reconnect, replay an
73+
# *empty* transaction (the original writes died with the backend), run the
74+
# save-point statement against it, and report success - silently losing
75+
# data. ActiveRecord's native adapters route these through
76+
# `internal_execute` (allow_retry: false, materialize_transactions: true);
77+
# we mirror that here. See the COMMIT/ROLLBACK overrides above.
78+
6979
# Creates a (transactional) save-point one can rollback to.
7080
# Unlike 'plain' `ActiveRecord` it is allowed to pass a save-point name.
7181
# @param name the save-point name
@@ -74,7 +84,7 @@ def exec_rollback_db_transaction
7484
# @extension added optional name parameter
7585
def create_savepoint(name = current_savepoint_name)
7686
log("SAVEPOINT #{name}", 'TRANSACTION') do
77-
with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn|
87+
with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn|
7888
conn.create_savepoint(name)
7989
end
8090
end
@@ -87,7 +97,7 @@ def create_savepoint(name = current_savepoint_name)
8797
# @extension added optional name parameter
8898
def exec_rollback_to_savepoint(name = current_savepoint_name)
8999
log("ROLLBACK TO SAVEPOINT #{name}", 'TRANSACTION') do
90-
with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn|
100+
with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn|
91101
conn.rollback_savepoint(name)
92102
end
93103
end
@@ -100,7 +110,7 @@ def exec_rollback_to_savepoint(name = current_savepoint_name)
100110
# @extension added optional name parameter
101111
def release_savepoint(name = current_savepoint_name)
102112
log("RELEASE SAVEPOINT #{name}", 'TRANSACTION') do
103-
with_raw_connection(allow_retry: true, materialize_transactions: false) do |conn|
113+
with_raw_connection(allow_retry: false, materialize_transactions: true) do |conn|
104114
conn.release_savepoint(name)
105115
end
106116
end

lib/arjdbc/postgresql/adapter.rb

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,12 @@ def all_schemas
624624
def client_min_messages
625625
return nil if redshift? # not supported on Redshift
626626
# Need to use #execute so we don't try to access the type map before it is initialized
627-
execute('SHOW client_min_messages', 'SCHEMA').values.first.first
627+
# NOTE: #execute returns an Array of row Hashes here (e.g.
628+
# [{"client_min_messages"=>"warning"}]), unlike MRI's pg result object,
629+
# so we read the single value out of the first row.
630+
result = execute('SHOW client_min_messages', 'SCHEMA')
631+
row = result.first
632+
row && row.values.first
628633
end
629634

630635
# Set the client message level.

test/db/postgresql/connection_lost_test.rb

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,44 @@ def test_commit_failure_after_dropped_backend_is_not_reported_as_success
205205
@adapter.execute('DROP TABLE IF EXISTS commit_retry_loss') rescue nil
206206
end
207207

208+
# Regression test for the savepoint-retry data-loss footgun (#1, sibling of
209+
# the COMMIT case above).
210+
#
211+
# create_savepoint / exec_rollback_to_savepoint / release_savepoint must go
212+
# through with_raw_connection(allow_retry: false), matching ActiveRecord's
213+
# native adapters (which route save-points through internal_execute, whose
214+
# default is allow_retry: false). Save-points only ever run inside an open
215+
# transaction, so a backend drop means the transaction's prior writes are
216+
# gone. If the save-point op were retryable, AR would reconnect, replay an
217+
# *empty* transaction, run the SAVEPOINT against it, and report success -
218+
# silently losing the transaction's writes. With retry disabled the failure
219+
# surfaces to the caller instead.
220+
def test_create_savepoint_after_dropped_socket_does_not_silently_retry
221+
@adapter.execute('SELECT 1')
222+
@adapter.begin_db_transaction
223+
# non-empty transaction; also keeps the connection verified/active.
224+
@adapter.execute('SELECT 1')
225+
226+
original = @adapter.instance_variable_get(:@raw_connection).jdbc_connection
227+
228+
# pgbouncer reaps the backend right before the SAVEPOINT.
229+
original.close
230+
assert original.isClosed, 'precondition: jdbc connection should be closed'
231+
232+
# With allow_retry: false the dropped SAVEPOINT must raise rather than
233+
# reconnecting and running against a fresh, empty transaction.
234+
assert_raise(ActiveRecord::ConnectionFailed) do
235+
@adapter.create_savepoint('sp_retry_loss')
236+
end
237+
238+
# And it must NOT have silently swapped onto a fresh connection.
239+
current = @adapter.instance_variable_get(:@raw_connection)
240+
assert(current.nil? || current.jdbc_connection.equal?(original),
241+
'savepoint must not reconnect-and-retry on a fresh connection')
242+
ensure
243+
@adapter.send(:reconnect!) rescue nil
244+
end
245+
208246
private
209247

210248
def translate(jdbc_error)

0 commit comments

Comments
 (0)