Honor prepared_statements: false in perform_query and _exec_insert#2651
Merged
Conversation
Both `OracleEnhanced::DatabaseStatements#perform_query` and `#_exec_insert` previously took the same branch whenever binds were present: cache the cursor in `@statements[sql]`, bind values, execute. That branch ignored the connection's `prepared_statements` flag, so a connection configured with `prepared_statements: false` (or one running inside an `unprepared_statement do ... end` block) still had its bind-ful queries prepared and cached client-side. This was inconsistent with the user's intent for `prepared_statements: false` — namely, do not keep prepared cursor handles around — and it also masked the real cost of bound dictionary queries (rsim#2629) for those connections, since the server-side shared pool was being primed by the cached client-side handle just like in the prepared case. Add a `prepared_statements?` branch on the bind-present path: if prepared_statements? @Statements[sql] ||= raw_connection.prepare(sql) # cached, reused cursor = @Statements[sql] cached = true else cursor = raw_connection.prepare(sql) # one-shot end cursor.bind_params(type_casted_binds) When `prepared_statements?` is false the cursor is prepared, bound, and released by `cursor.close unless cached` at the tail of the surrounding `log` block. The Oracle DB shared pool may still reuse the parsed plan based on SQL-text matching, but no client-side handle is retained — analogous to PostgreSQL's `exec_params` path. Three secondary fixes go with the branch: - Add a `rescue` inside the `with_retry` block in both methods that closes the one-shot cursor when `exec` raises, so a retry that re-prepares does not leave the previous handle dangling. The close is swallowed (`cursor.close rescue nil`) so it never masks the original error. Cached cursors stay in `@statements` and are released by the pool's `dealloc` when `@statements.clear` runs. - Make `with_retry`'s `@statements.clear` conditional on `prepared_statements?`. With the new branch the pool is empty in unprepared mode, so the clear is wasted work. - Both ruby-oci8 and the JDBC backend use the same `prepare(sql) → bind → exec → close` lifecycle through the shared `Cursor` abstraction, so the change applies uniformly to both. The two existing statement-pool specs in `oracle_enhanced_adapter_spec` (`should clear older cursors`, `should cache UPDATE statements with bind variables`) asserted behavior that is now specific to `prepared_statements: true`; gate them on `@conn.prepared_statements?`. A new symmetric spec covers the `prepared_statements: false` path: bind-ful UPDATE no longer grows the pool.
041ec70 to
437b0e3
Compare
3 tasks
yahonda
added a commit
to yahonda/oracle-enhanced
that referenced
this pull request
Apr 29, 2026
Extends the bind-variable pattern introduced in rsim#2629 to the remaining all_* / user_* dictionary queries that were still routing through the `select_values_forcing_binds` / `select_value_forcing_binds` helpers. After this commit every dictionary query in `OracleEnhancedAdapter` calls plain `select_values(sql, name, binds)` / `select_value(sql, name, binds)` with a `:placeholder` raw SQL and an explicit `binds` array — one consistent shape across the file. As a consequence, the two helpers no longer have any callers and are removed. Methods touched (`oracle_enhanced_adapter.rb`): * identity_primary_key? * trigger_backed_primary_key? * pk_and_sequence_for (×2 callsites — sequence lookup and PK lookup) * primary_keys * temporary_table? The `_forcing_binds` helpers were added in rsim#2125 (2021) to make dictionary queries use bind variables even inside an `unprepared_statement do ... end` block, by temporarily removing the connection from `prepared_statements_disabled_cache` so AR's visitor wouldn't inline the binds. Two recent changes made that mechanism unnecessary: 1. rsim#2629 finished converting the remaining literal-interpolated dictionary queries to raw SQL with `:placeholder` markers and explicit `binds` arrays. AR's visitor isn't involved on the raw-SQL-with-binds path, so there is nothing to inline. 2. rsim#2651 made `OracleEnhanced::DatabaseStatements#perform_query` honor `prepared_statements?` directly. Inside an `unprepared_statement` block, dictionary queries now prepare-bind-exec-close one-shot instead of caching the cursor in `@statements`, but bind values still flow to the server. SQL-text matching keeps the shared cursor reusable in the database's shared pool — the original server-side cursor-sharing goal of rsim#2125 is preserved without the helper. The helpers were defined under `private` and were not part of the documented adapter API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OracleEnhanced::DatabaseStatements#perform_queryand#_exec_insertpreviously took the same branch whenever binds were present — cache the cursor in@statements[sql], bind values, execute — regardless of the connection'sprepared_statementsflag. As a result, even connections configured withprepared_statements: false(or running inside anunprepared_statement do ... endblock) had their bind-ful queries prepared and cached client-side.This PR adds a
prepared_statements?branch on the bind-present path so thatprepared_statements: falseconnections prepare-and-execute one-shot (no cache), matching PostgreSQL'sexec_paramspath and the spirit of the AR setting.cursor.close unless cachedat the tail already releases the one-shot cursor. The Oracle DB shared pool may still recognize the SQL text and reuse its parsed plan, but no client-side handle is retained.Why
prepared_statements: falsewas, until now, still keeping prepared cursor handles for every distinct bind-ful SQL text. That's inconsistent with the AR contract.prepared_statements: falsecallers. Without this branch, the cached client-side handle was masking the per-call parsing the user explicitly opted into.intent.preparefalse throughexec_params(parameterized but not prepared). Oracle now has the analogous path through OCI8 / JDBC's prepare-bind-exec-close lifecycle.Backend coverage
OCI8 and JDBC both expose the same
Cursorabstraction withprepare(sql) → bind_params → exec → close. The branch lives in the shareddatabase_statements.rb, so the change applies uniformly to both backends with no per-backend duplication.Spec adjustments
The two existing statement-pool specs in
oracle_enhanced_adapter_spec.rbasserted that bind-ful queries grow@statements. That's now specific toprepared_statements: true:should clear older cursors when statement limit is reached— gated on@conn.prepared_statements?should cache UPDATE statements with bind variables— gated on@conn.prepared_statements?Plus a new symmetric spec for the new path:
should not cache UPDATE statements with bind variables when prepared_statements is false— asserts the pool size doesn't grow.Test plan
prepared_statements: trueworkflow)master test_prepared_statements_falseworkflow green after mergeprepared_statements: true→ 609 examples, 0 failures, 7 pendingprepared_statements: false→ 609 examples, 0 failures, 8 pending (one extrapendingis the now-skippedprepared_statements: true-only assertion)