Skip to content

Commit 974015b

Browse files
committed
Honor intent.prepare in _exec_insert to restore pre-Rails-8 caching behavior
Background ---------- Before the Rails 8 adoption commit (baf9fd4 "Rails 8 support", 2025-02-23), `_exec_insert` decided whether to reuse a cached prepared statement using `without_prepared_statement?(binds)`, which factored in the `prepared_statements` config: if without_prepared_statement?(binds) cursor = _connection.prepare(sql) # fresh cursor else @Statements[sql] ||= _connection.prepare(sql) # cached cursor cursor = @Statements[sql] cursor.bind_params(...) ... end `baf9fd48` replaced that helper with the following predicate, in line with the broader Rails 8 changes that retired `without_prepared_statement?`: if binds.nil? || binds.empty? cursor = _connection.prepare(sql) # fresh cursor else @Statements[sql] ||= _connection.prepare(sql) # cached cursor ... end After that change, the `prepared_statements` config is no longer part of the cache decision in `_exec_insert`. Calls that arrive with non-empty binds — including raw `connection.insert("INSERT ... VALUES ('literal')", nil, "id")`, where `sql_for_insert` appends a single `:returning_id` placeholder — go through the cached-cursor branch on every invocation. How it surfaced --------------- Three things lined up: 1. A set of pre-2018 specs that issue the same raw-SQL INSERT with `RETURNING ... INTO :returning_id` more than once on the same adapter were removed in 2017 (commit ef321a1). 2. While those specs were absent, baf9fd4 (Rails 8 support, Feb 2025) widened `_exec_insert`'s cached-cursor branch as described above. 3. The same spec patterns were restored in 2026 as part of porting the trigger-based primary-key option (#2615), and ran against the post-baf9fd48 cache decision for the first time. Under that combination, plus `cursor_sharing = FORCE` (the adapter default), executing the same cached cursor a second time produces a SQL*Net half-duplex deadlock between ruby-oci8 and the Oracle 23ai server: the server waits in `SQL*Net more data from client` while ruby-oci8 stays in `OCIStmtExecute -> nttfprd -> read()`. See #2619 for the full diagnosis and a 5-line standalone repro. Fix --- Reintroduce the prepared-statements check using the Rails 8.2 QueryIntent successor to `without_prepared_statement?`: `intent.prepare`. PostgreSQL gates on the same flag (rails-3d7458fecf49 postgresql/database_statements.rb:167). For raw SQL paths, `intent.prepare` is left as its default `false` (set by `connection.insert` in abstract/database_statements.rb), so the fresh-cursor path runs and the cached-cursor combination above is not reached. Behavior after this change: - `klass.create!(attrs)` (Arel insert, prepared_statements=true) keeps the cached path. - `connection.insert("INSERT ...", nil, "id")` (raw SQL) takes the fresh-cursor path and never reuses a cursor with a stale OCI bind state. - Connections with `prepared_statements: false` always use fresh cursors, matching the pre-`baf9fd48` behavior. Tradeoff -------- The change moves raw-SQL `connection.insert` paths to a fresh cursor on every call, giving up the cached-cursor reuse benefit for those calls. That tradeoff is acceptable because: - Issuing the exact same raw SQL string through `connection.insert` twice in a row is uncommon in Rails application code (model paths go through Arel and parameterise their bind values). - A SQL*Net deadlock that wedges the rest of the suite is a larger problem than losing per-call cursor reuse on a rarely exercised path. The cached path is preserved unchanged for ORM (Arel-based) inserts, which are the high-volume callers. Regression spec --------------- `spec/.../exec_insert_caching_spec.rb` issues the same `INSERT ... NEXTVAL ... RETURNING "ID" INTO :returning_id` SQL twice through `connection.insert`. Without the fix the second exec hits the half-duplex deadlock and trips the spec's 5-second `Timeout`; with the fix it completes well under 100ms. A short comment notes the dependency on the default `cursor_sharing = force` so a future change in defaults does not silently turn the regression test into a no-op. Closes #2619.
1 parent 903067d commit 974015b

2 files changed

Lines changed: 43 additions & 1 deletion

File tree

lib/active_record/connection_adapters/oracle_enhanced/database_statements.rb

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,18 @@ def _exec_insert(intent, pk = nil, sequence_name = nil, returning: nil) # :nodoc
6161
cursor = nil
6262
returning_id_col = returning_id_index = nil
6363
with_retry do
64-
if binds.nil? || binds.empty?
64+
if !intent.prepare || binds.nil? || binds.empty?
6565
cursor = _connection.prepare(sql)
66+
67+
unless binds.nil? || binds.empty?
68+
cursor.bind_params(type_casted_binds)
69+
70+
if sql.include?(":returning_id")
71+
# it currently expects that returning_id comes last part of binds
72+
returning_id_index = binds.size
73+
cursor.bind_returning_param(returning_id_index, Integer)
74+
end
75+
end
6676
else
6777
unless @statements.key?(sql)
6878
@statements[sql] = _connection.prepare(sql)
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
# frozen_string_literal: true
2+
3+
require "timeout"
4+
5+
describe "_exec_insert prepared-statement cache gating" do
6+
before(:all) do
7+
ActiveRecord::Base.establish_connection(CONNECTION_PARAMS)
8+
@conn = ActiveRecord::Base.lease_connection
9+
@conn.create_table :test_exec_insert_caching, force: true, id: false do |t|
10+
t.integer :id, null: false
11+
t.string :name
12+
end
13+
@conn.execute "CREATE SEQUENCE test_exec_insert_caching_seq"
14+
end
15+
16+
after(:all) do
17+
@conn.execute "BEGIN EXECUTE IMMEDIATE 'DROP SEQUENCE test_exec_insert_caching_seq'; EXCEPTION WHEN OTHERS THEN NULL; END;"
18+
@conn.drop_table :test_exec_insert_caching, if_exists: true
19+
end
20+
21+
# Depends on the default cursor_sharing=force; under :exact the deadlock
22+
# does not manifest, so this spec would silently pass without exercising
23+
# the regression. See #2619.
24+
it "does not deadlock when the same raw SQL connection.insert with RETURNING is issued twice" do
25+
sql = "INSERT INTO test_exec_insert_caching (id, name) VALUES (test_exec_insert_caching_seq.NEXTVAL, 'alpha')"
26+
Timeout.timeout(5) do
27+
@conn.insert(sql, nil, "id")
28+
@conn.insert(sql, nil, "id")
29+
end
30+
expect(@conn.select_value("SELECT COUNT(*) FROM test_exec_insert_caching")).to eq(2)
31+
end
32+
end

0 commit comments

Comments
 (0)