Restore trigger-based primary key generation as opt-in primary_key_trigger: option by yahonda · Pull Request #2615 · rsim/oracle-enhanced

yahonda · 2026-04-27T15:02:53Z

Motivation / Background

Closes #2597.

Trigger-based primary-key generation was removed in 61f8a305 (2018, adapter 6.0) without a deprecation cycle. Several user reports since then (#2426, #2431, #2468) re-implemented the missing functionality in user space; #2597 captured the demand and proposed restoring it as a symmetric opt-in.

This PR is Phase 3 of the primary-key-generation work. The three phases:

Phase 1 (Add explicit identity: option to create_table for Oracle 12.1+ #2579, merged) — create_table :foo, identity: true for Oracle 12.1+ (GENERATED BY DEFAULT AS IDENTITY).
Phase 2 (Phase 2: Auto-enable identity primary keys in Migration[8.2]+ (depends on Rails per-adapter-migration-compatibility) [DRAFT] #2596, draft) — auto-enable identity: in Migration[8.2]+.
Phase 3 (this PR, Restore trigger-based primary key generation as opt-in primary_key_trigger: option #2597) — restore trigger-based PK generation as create_table :foo, primary_key_trigger: true.

create_table :foo                              # sequence + prefetch (default, unchanged)
create_table :foo, identity: true              # GENERATED BY DEFAULT AS IDENTITY (Phase 1)
create_table :foo, primary_key_trigger: true   # sequence + BEFORE INSERT trigger + RETURNING (this PR)

Detail

Two commits:

Restore trigger-based primary key generation as opt-in primary_key_trigger: — the feature commit. Wires the option allowlist, validators, emitter, prefetch detection, schema-dump round-trip, and regression specs in a single self-contained change.
Skip :returning_id logging spec pending #2619 — temporarily marks the restored Symbol-bind logging regression test as skip because it trips an unrelated SQL*Net half-duplex deadlock between ruby-oci8 and Oracle when cursor_sharing=force is combined with RETURNING ... INTO :returning_id. The deadlock is diagnosed and tracked in OCI8 + cursor_sharing=force x INSERT ... RETURNING INTO :returning_id hangs in SQL*Net half-duplex deadlock #2619; the adapter-side fix is in Honor intent.prepare in _exec_insert to avoid OCI8 half-duplex deadlock #2620. Once Honor intent.prepare in _exec_insert to avoid OCI8 half-duplex deadlock #2620 lands, a follow-up commit on this branch will remove the skip.

The feature commit covers:

:primary_key_trigger and :trigger_name allowlist — added to valid_primary_key_options and valid_column_definition_options so Rails' validate_create_table_options! accepts the new keys via both create_table and add_column.
validate_primary_key_trigger_options! validator — mirrors validate_identity_options!. Rejects id != :primary_key, composite primary key, and the identity: true combination. Stricter than pre-2018 (which silently accepted these and produced broken schemas); see "Difference from pre-2018" below. Wired into create_table. add_column independently rejects primary_key_trigger: true on non-:primary_key column types so add_column :foo, :bar, :integer, primary_key_trigger: true fails fast instead of silently no-oping.
create_pk_trigger(table_name, pk_column, options) emitter — emits the CREATE OR REPLACE TRIGGER ... BEFORE INSERT ON ... FOR EACH ROW PL/SQL block as a single execute call. Default trigger name comes from default_trigger_name(table_name), exposed as a public :nodoc: helper alongside default_sequence_name. Truncation is byte-bounded with a valid_encoding? back-off — fixes a latent multibyte bug from the pre-2018 char-indexed truncation. Called from both create_table (alongside create_pk_sequence) and add_column (PK column path) so add_column :foo, :id, :primary_key, primary_key_trigger: true works the same way as create_table primary_key_trigger: true.
prefetch_primary_key? short-circuit + trigger_backed_primary_key?(owner, desc_table_name) detection — the cache value becomes has_pk && !has_identity_pk && !has_trigger_backed_pk. The detection query restricts the all_triggers scan with an EXISTS over all_source matching NEXTVAL INTO :NEW, so unrelated BEFORE INSERT row triggers (audit, last_modified, ...) do not flip prefetch off on tables that still rely on Rails-side sequence prefetch. With prefetch off, sql_for_insert (added in Add explicit identity: option to create_table for Oracle 12.1+ #2579) automatically appends RETURNING <pk> INTO :returning_id — no INSERT-side changes needed.
SchemaDumper round-trip via trigger_backed_table_names — same body filter as the prefetch query. Bulk fetch {table_name => trigger_name} once at dump start (1 query, not N), look up per-table during dump. Emit , primary_key_trigger: true for trigger-backed tables; emit , trigger_name: "..." only when the actual trigger name differs from the default <table>_pkt.
Regression specs — new primary_key_trigger_spec.rb, mirror of identity_primary_key_spec.rb. Coverage groups:
- Default (no option) and primary_key_trigger: false — sequence-only, prefetch on.
- primary_key_trigger: true — sequence + trigger created, prefetch off, INSERT without/with explicit id, :trigger_name override.
- add_column paths — success on :primary_key type, rejection on other types via the add_column validator.
- prefetch_primary_key? cache invalidation across drop-and-recreate, both directions.
- Invalid combinations — id: false / id: :integer / id: :uuid / composite primary key / identity: true — all raise ArgumentError.
- schema_dumper round-trip — emits primary_key_trigger: true for trigger-backed, not for plain or identity, with trigger_name: only when non-default.
- False-positive guard — tables with unrelated BEFORE INSERT row triggers retain prefetch_primary_key? = true and are not emitted as primary_key_trigger: true in schema dumps, ensuring the body filter actually scopes detection.
- Restored from the 2018 deletion: low-level INSERT via @conn.execute and @conn.insert(SQL, nil, "id"); non-default :primary_key; non-default :sequence_name; :returning_id Symbol regression guard from Trigger based primary key sequence not returned when prefetch_primary_key? is false #2426 (currently skipped pending OCI8 + cursor_sharing=force x INSERT ... RETURNING INTO :returning_id hangs in SQL*Net half-duplex deadlock #2619 / Honor intent.prepare in _exec_insert to avoid OCI8 half-duplex deadlock #2620); manual PL/SQL trigger DDL acceptance.
- Long identifiers (Oracle 12.2+) — verifies default_trigger_name truncates to fit max_identifier_length, byte-bounded, multibyte-safe.

Why preserve the trailing `RETURNING` flow rather than reinvent it

OracleEnhanced::DatabaseStatements#sql_for_insert already handles the trigger-backed case identically to the identity case: when no PK bind is supplied, it appends RETURNING <pk> INTO :returning_id to the INSERT and reads the value back through the returning_id out-bind. Once prefetch_primary_key? returns false for a trigger-backed table, Rails ships the INSERT without the PK column bound, and the existing flow fires. No INSERT-side code change in this PR.

Difference from pre-2018

The pre-2018 implementation accepted any combination silently and produced broken schemas for invalid ones (e.g. primary_key_trigger: true, id: :uuid — the trigger does SELECT <seq>.NEXTVAL INTO :new.<pk> FROM dual and assumes a numeric column). Phase 1 (#2579) made the same call for identity: and required id: :primary_key; Phase 3 mirrors it for the same reason — fail fast at create-table time rather than at first-insert time.

The pre-2018 dumper emitted a separate add_primary_key_trigger "test_posts" line after create_table. The new dumper emits , primary_key_trigger: true inline on the create_table line, matching the identity: true shape and avoiding a public add_primary_key_trigger method that would duplicate the create_table form.

The default trigger name (<table>_pkt) is unchanged from pre-2018. The truncation logic is rewritten to be byte-bounded with valid_encoding? back-off (matching default_sequence_name) — a multibyte table name no longer risks producing an invalid UTF-8 trigger name. Identifier limit comes from max_identifier_length (30 on Oracle 11g/12.1, 128 on 12.2+) rather than the pre-2018 IDENTIFIER_MAX_LENGTH = 30 constant; long-table-name specs verify this on extended-identifier databases.

rename_table does not rename or recreate the trigger — same as pre-2018. Users renaming a table whose primary key is trigger-backed need to drop and recreate the trigger explicitly (the sequence rename that rename_table still performs leaves the trigger body referencing the old sequence name and turns it ORA-04098 invalid on the next insert). Out of scope for this PR.

Behavior contract after this PR

Form	sequence	trigger	prefetch_primary_key?	INSERT path
`create_table :foo` (default)	✓	✗	true	Rails computes id from sequence, sends as bind
`create_table :foo, identity: true` (Phase 1)	✗	✗	false	Rails sends INSERT without id; identity column generates it; RETURNING reads back
`create_table :foo, primary_key_trigger: true` (this PR)	✓	✓	false	Rails sends INSERT without id; trigger pulls from sequence into `:new.id`; RETURNING reads back
`create_table :foo, identity: true, primary_key_trigger: true`	—	—	—	`ArgumentError` at create_table time (mutual exclusion)

add_column :foo, :id, :primary_key, primary_key_trigger: true produces the same artifacts as the third row, applied to an existing table. add_column :foo, :bar, :integer, primary_key_trigger: true raises ArgumentError (the column type must be :primary_key).

Notes

No public add_primary_key_trigger / has_primary_key_trigger? method is restored. The Phase 3 issue scopes the API to create_table primary_key_trigger: only; if a user needs to add a trigger separately to an existing table, they call add_column :foo, :id, :primary_key, primary_key_trigger: true (which goes through the same create_pk_trigger path).
No version gate. Trigger-based PKs work on every Oracle version this adapter targets.
The validate_primary_key_trigger_options! mutual-exclusion check lives only on the trigger side (does not double-check from validate_identity_options!). Whichever validator is consulted first raises; this avoids a circular cross-check that would always re-detect the same condition twice.
trigger_backed_primary_key? is intentionally narrow: it only matches triggers whose body contains NEXTVAL INTO :NEW, the exact pattern emitted by create_pk_trigger. Tables with unrelated BEFORE INSERT row triggers (audit, last_modified, ...) keep prefetch_primary_key? = true and are not emitted as trigger-backed in schema dumps. False negatives — user-handwritten PK-fill triggers with different formatting — fall back to Rails-side prefetch, which is harmless because the trigger's own IS NULL guard skips when Rails supplies the id.

Checklist

This Pull Request is related to one change.
Commit message has a detailed description of what changed and why.
Tests are added (full mirror of identity_primary_key_spec.rb plus restored pre-2018 low-level tests, new long-identifier and false-positive guard specs).

… OCI8 deadlock) The "does not raise NoMethodError for :returning_id Symbol when logging" example reliably hangs against Oracle Database 23.26.1.0.0 + ruby-oci8 because the adapter's default `ALTER SESSION SET CURSOR_SHARING = FORCE` rewrites the IN literal to `:"SYS_B_0"` while the example issues `RETURNING "ID" INTO :returning_id`. The two-bind shape sends the OCI piecewise (DATA_AT_EXEC) protocol out of sync: the server enters `SQL*Net more data from client` while ruby-oci8 stays in `OCIStmtExecute -> nttfprd -> read()`, producing a half-duplex deadlock that wedges the rest of the suite. Diagnosis was confirmed via: - v$session: ORACLE_ENHANCED session waiting on `SQL*Net more data from client`, blocking_session NULL, no row locks held. - macOS `sample` of the rspec PID: main thread stuck in oci8_stmt_execute -> OCIStmtExecute -> kpuexec -> ... -> nttfprd -> read. - v$parameter: cursor_sharing=EXACT system-wide, but session-level FORCE applied by oracle_enhanced_adapter.rb:843. The hang is unrelated to PR rsim#2615 (primary_key_trigger). It exposes a pre-existing OCI8 / cursor_sharing=force x RETURNING INTO issue that needs a separate fix; tracked in rsim#2619.

@Statements

…ehavior 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 (rsim#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 rsim#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 rsim#2619.

… OCI8 deadlock) The "does not raise NoMethodError for :returning_id Symbol when logging" example reliably hangs against Oracle Database 23.26.1.0.0 + ruby-oci8 because the adapter's default `ALTER SESSION SET CURSOR_SHARING = FORCE` rewrites the IN literal to `:"SYS_B_0"` while the example issues `RETURNING "ID" INTO :returning_id`. The two-bind shape sends the OCI piecewise (DATA_AT_EXEC) protocol out of sync: the server enters `SQL*Net more data from client` while ruby-oci8 stays in `OCIStmtExecute -> nttfprd -> read()`, producing a half-duplex deadlock that wedges the rest of the suite. Diagnosis was confirmed via: - v$session: ORACLE_ENHANCED session waiting on `SQL*Net more data from client`, blocking_session NULL, no row locks held. - macOS `sample` of the rspec PID: main thread stuck in oci8_stmt_execute -> OCIStmtExecute -> kpuexec -> ... -> nttfprd -> read. - v$parameter: cursor_sharing=EXACT system-wide, but session-level FORCE applied by oracle_enhanced_adapter.rb:843. The hang is unrelated to PR rsim#2615 (primary_key_trigger). It exposes a pre-existing OCI8 / cursor_sharing=force x RETURNING INTO issue that needs a separate fix; tracked in rsim#2619.

…igger: Closes rsim#2597. Trigger-based primary-key generation was removed in `61f8a305` (2018, adapter 6.0). Several user reports (rsim#2426, rsim#2431, rsim#2468) re-implemented the missing functionality in user space; rsim#2597 captured the demand and proposed restoring it as a symmetric opt-in. This is Phase 3 of the primary-key-generation work, sibling to the Phase 1 `identity:` opt-in (rsim#2579). Default behavior is unchanged: tables without `primary_key_trigger:` keep using sequence-only primary keys. The change adds: * `:primary_key_trigger` and `:trigger_name` to `valid_primary_key_options` and `valid_column_definition_options` so the new keys flow through `create_table` / `add_column` validation. * `validate_primary_key_trigger_options!` enforcing the `id: :primary_key` requirement, the composite-PK rejection, and the mutual exclusion with `identity: true`. Wired into `create_table`. `add_column` independently rejects `primary_key_trigger: true` on non-`:primary_key` column types. * `create_pk_trigger(table_name, pk_column, options)` emitter that creates a sequence + BEFORE INSERT trigger filling `:new.<pk>` from the sequence's `NEXTVAL` when the column is NULL. Uses multibyte-safe default trigger-name truncation for Oracle's identifier length budget; `default_trigger_name` is exposed as a public `:nodoc:` helper alongside `default_sequence_name`. * `prefetch_primary_key?` short-circuit + `trigger_backed_primary_key?` helper. The detection query restricts the `all_triggers` scan with an `EXISTS` over `all_source` matching `NEXTVAL INTO :NEW`, so unrelated `BEFORE INSERT` row triggers (audit, last_modified, ...) do not flip prefetch off on tables that still rely on Rails-side sequence prefetch. * `SchemaDumper` round-trip via `trigger_backed_table_names` (same body filter): emits `primary_key_trigger: true` and the optional `trigger_name:` when present, skips for plain sequence-backed and identity tables. * Regression specs covering the full contract: validation, sequence + trigger creation, NEXTVAL fill-on-NULL, explicit-id pass-through, `:trigger_name` override, `add_column` paths (success and rejection), `prefetch_primary_key?` cache invalidation across drop/recreate cycles, schema_dumper round-trip, long-identifier truncation, and a false-positive guard ensuring tables with unrelated `BEFORE INSERT` row triggers retain `prefetch_primary_key? = true` and are not emitted as `primary_key_trigger: true` in schema dumps. `rename_table` does not rename or recreate the trigger; the trigger remains under its original name regardless of whether it was created with the default or a custom `:trigger_name`. This matches the pre-2018 implementation. Users renaming a table that has a trigger-backed primary key will need to drop and recreate the trigger explicitly in a follow-up migration step, since the sequence rename that `rename_table` still performs leaves the trigger body referencing the old sequence name and turns it ORA-04098 invalid on the next insert.

The "does not raise NoMethodError for :returning_id Symbol when logging" example exposes a SQL*Net half-duplex deadlock when ruby-oci8 + cursor_sharing=force are combined with RETURNING ... INTO :returning_id; see rsim#2619 for the full diagnosis and minimal repro. The fix is tracked on the adapter side in rsim#2620. Skip the example until rsim#2619 / rsim#2620 lands; restore in a follow-up commit on this branch once those merge.

@Statements

…ehavior 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 (rsim#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 rsim#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 rsim#2619.

Closes rsim#2513. Adds a second `structure_dump` implementation built on Oracle's `DBMS_METADATA.GET_DDL` / `GET_DEPENDENT_DDL`, the Oracle-native equivalent of `pg_dump --schema-only` / `mysqldump --no-data`. The original `ALL_*` data-dictionary-based implementation in `StructureDump` is left untouched. A new module `StructureDump::DbmsMetadata` prepends to `StructureDump` and dispatches each entry point — `structure_dump`, `structure_dump_db_stored_code`, `structure_dump_synonyms` — based on `OracleEnhancedAdapter.structure_dump_method`: * `:dbms_metadata` (default) — call the new DBMS_METADATA path. * `:data_dictionary` — fall through via `super` to the existing `ALL_*`-based implementation. The toggle is intentionally a Rails-app-global `cattr_accessor` rather than a per-connection `database.yml` key. The choice of structure-dump backend is an implementation strategy, not something that varies across the databases an app connects to. The `:data_dictionary` path is retained as a fallback while the new backend stabilises and may be deprecated and eventually removed in a future release. Defaulting to `:dbms_metadata` ensures real-world usage (and bug reports against the new path) before the old one is dropped — the original `ALL_*` implementation has needed regular touch-ups for schema features (most recently CHECK constraints in rsim#2500) that `DBMS_METADATA` would surface for free. Output transforms are configured to match the spirit of mysqldump / pg_dump --schema-only: * STORAGE / TABLESPACE / SEGMENT_ATTRIBUTES → FALSE (no installation-specific noise) * EMIT_SCHEMA → FALSE (portable across schemas) * SQLTERMINATOR → FALSE (Rails uses STATEMENT_TOKEN to split DDL on load) * CONSTRAINTS → TRUE, REF_CONSTRAINTS → FALSE (inline column constraints with the table; emit referential constraints separately as ALTER TABLE statements after all tables are created so DDL load order is correct) * PRETTY → TRUE Triggers emitted by the `primary_key_trigger:` opt-in (rsim#2615) are picked up via `GET_DEPENDENT_DDL('TRIGGER', table_name)` so a structure dump + load round-trip recreates the table + sequence + trigger faithfully. Tests: * `structure_dump_spec.rb` — existing examples assume the data-dictionary backend's exact DDL text; pinned to `structure_dump_method = :data_dictionary` in `before(:all)` so they keep verifying that path. * `dbms_metadata_structure_dump_spec.rb` — new file covering the DBMS_METADATA path. Asserts on the dump's structural shape (presence of CREATE TABLE / CREATE INDEX / ALTER TABLE … REFERENCES / COMMENT ON statements; absence of STORAGE / TABLESPACE clauses) rather than exact DDL text. Also covers the toggle: default value, delegation to `:data_dictionary`, and `ArgumentError` on unknown values. Test plan: * `bundle exec rspec` on Oracle 23ai — 711 examples, 0 failures, 11 pending. * `bundle exec rubocop` — clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

rename_table renames the table and the table's sequence, but never touched the BEFORE INSERT trigger that primary_key_trigger: true emits. Because the trigger body references the sequence by name (`<old>_seq.NEXTVAL`) and the sequence has just been renamed, the trigger compiles invalid (ORA-04098) and the next INSERT against the renamed table fails. This is a long-standing latent bug. The first version of rename_table in this repository (commit 1b8281d, "Modify directory structure", predating the 2018 trigger-removal in 61f8a30) already renamed the sequence; pre-2018 — when sequence + trigger PKs were the default — the trigger-rename would have hit the same ORA-04098 had anyone exercised the rename_table + default-PK path. The 2018 deletion hid the issue; rsim#2615 restored opt-in trigger-based PKs and called out the rename_table gap as out of scope. The fix is to re-emit the trigger body after the sequence rename so it points at the renamed sequence: * `rename_pk_trigger(old, new)` looks up the existing trigger via `trigger_backed_table_names[NEW.upcase]` (Oracle auto-updates `all_triggers.table_name` to the renamed table, so the lookup uses the new name). Returns early if the table has no trigger-backed PK. * When the trigger uses the default `<old>_pkt` name, recreate it as `<new>_pkt` (via `create_pk_trigger` with no explicit `:trigger_name`) and drop the old trigger so the schema matches a freshly-created table with `primary_key_trigger: true`. The schema dumper will then emit the renamed table with `primary_key_trigger: true` alone, no `:trigger_name` override needed. * When the trigger has a user-customized name, keep the name and just refresh the body in place via `CREATE OR REPLACE TRIGGER`. Renaming a user-chosen identifier behind their back would be surprising. `trigger_backed_table_names` only matches triggers whose body contains `NEXTVAL INTO :NEW`, so unrelated `BEFORE INSERT` row triggers (audit, last_modified, etc.) are not touched — same false-positive guard as the rest of the trigger-backed-PK detection. Specs: five cases under `rename_table on a trigger-backed table` — INSERT-after-rename succeeds, default-named trigger is renamed, custom-named trigger is preserved with refreshed body, plain sequence-backed tables are not touched, and the schema dump round-trips correctly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Closes rsim#2513. Adds a second `structure_dump` implementation built on Oracle's `DBMS_METADATA.GET_DDL` / `GET_DEPENDENT_DDL`, the Oracle-native equivalent of `pg_dump --schema-only` / `mysqldump --no-data`. The original `ALL_*` data-dictionary-based implementation in `StructureDump` is left untouched. A new module `StructureDump::DbmsMetadata` prepends to `StructureDump` and dispatches each entry point — `structure_dump`, `structure_dump_db_stored_code`, `structure_dump_synonyms` — based on `OracleEnhancedAdapter.structure_dump_method`: * `:dbms_metadata` (default) — call the new DBMS_METADATA path. * `:data_dictionary` — fall through via `super` to the existing `ALL_*`-based implementation. The toggle is intentionally a Rails-app-global `cattr_accessor` rather than a per-connection `database.yml` key. The choice of structure-dump backend is an implementation strategy, not something that varies across the databases an app connects to. The `:data_dictionary` path is retained as a fallback while the new backend stabilises and may be deprecated and eventually removed in a future release. Defaulting to `:dbms_metadata` ensures real-world usage (and bug reports against the new path) before the old one is dropped — the original `ALL_*` implementation has needed regular touch-ups for schema features (most recently CHECK constraints in rsim#2500) that `DBMS_METADATA` would surface for free. Output transforms are configured to match the spirit of mysqldump / pg_dump --schema-only: * STORAGE / TABLESPACE / SEGMENT_ATTRIBUTES → FALSE (no installation-specific noise) * EMIT_SCHEMA → FALSE (portable across schemas) * SQLTERMINATOR → FALSE (Rails uses STATEMENT_TOKEN to split DDL on load) * CONSTRAINTS → TRUE, REF_CONSTRAINTS → FALSE (inline column constraints with the table; emit referential constraints separately as ALTER TABLE statements after all tables are created so DDL load order is correct) * PRETTY → TRUE Triggers emitted by the `primary_key_trigger:` opt-in (rsim#2615) are picked up via `GET_DEPENDENT_DDL('TRIGGER', table_name)` so a structure dump + load round-trip recreates the table + sequence + trigger faithfully. Tests: * `structure_dump_spec.rb` — existing examples assume the data-dictionary backend's exact DDL text; pinned to `structure_dump_method = :data_dictionary` in `before(:all)` so they keep verifying that path. * `dbms_metadata_structure_dump_spec.rb` — new file covering the DBMS_METADATA path. Asserts on the dump's structural shape (presence of CREATE TABLE / CREATE INDEX / ALTER TABLE … REFERENCES / COMMENT ON statements; absence of STORAGE / TABLESPACE clauses) rather than exact DDL text. Also covers the toggle: default value, delegation to `:data_dictionary`, and `ArgumentError` on unknown values. Test plan: * `bundle exec rspec` on Oracle 23ai — 711 examples, 0 failures, 11 pending. * `bundle exec rubocop` — clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Closes rsim#2513. Adds a second `structure_dump` implementation built on Oracle's `DBMS_METADATA.GET_DDL` / `GET_DEPENDENT_DDL` alongside the existing data-dictionary (`ALL_*`) one. The new backend lives in a separate file and installs via `prepend DbmsMetadata`, so the legacy path in `structure_dump.rb` is untouched. Selectable globally via `OracleEnhancedAdapter.structure_dump_method`: - `:auto` (default) — `:dbms_metadata` on Oracle 12.1+, `:data_dictionary` otherwise. The 12.1 floor is a project-policy version gate (the IDENTITY / EDITIONABLE / modern SET_TRANSFORM_PARAM era), exposed as `use_dbms_metadata_dump?`, not a strict database capability check. - `:dbms_metadata` — force the new path; raises `ArgumentError` on pre-12.1 (mirrors PR rsim#2576's `identifier_max_length: :long` fail-fast policy). - `:data_dictionary` — force the original implementation. Remains explicitly selectable on 12.1+ as well. Output is informed by `mysqldump --no-data` / `pg_dump --schema-only`: - `STORAGE` / `TABLESPACE` / `SEGMENT_ATTRIBUTES` / `EMIT_SCHEMA` suppressed via `DBMS_METADATA.SET_TRANSFORM_PARAM` so the dump is portable across installations and schemas. - `REF_CONSTRAINTS` emitted as separate `ALTER TABLE … ADD CONSTRAINT` statements after all tables. - Inline constraints via Oracle's `CONSTRAINTS=TRUE` default; UNIQUE backing indexes that DBMS_METADATA already inlines into the table DDL are filtered out so they aren't emitted twice. - `STATEMENT_TOKEN` separation preserved (Oracle's `SQLTERMINATOR=FALSE` default) so `execute_structure_dump` splits as before. - `primary_key_trigger:` (rsim#2615) row triggers picked up via `GET_DEPENDENT_DDL("TRIGGER", table_name)`. - COMMENT ON TABLE / COMMENT ON COLUMN queried directly because `GET_DEPENDENT_DDL("COMMENT")` is unreliable. - `structure_dump_db_stored_code` selects `'PROCEDURE', 'PACKAGE', 'FUNCTION', 'TRIGGER', 'TYPE'` only — `GET_DDL("PACKAGE", ...)` already returns spec + body, so selecting `'PACKAGE BODY'` would emit the body twice. Specs follow the `pg_dump` / `mysqldump` testing convention: assert output shape (`CREATE TABLE` / `CREATE INDEX` / `ALTER TABLE … ADD CONSTRAINT` / `COMMENT` / no `STORAGE` / `TABLESPACE` / `PCTFREE`) rather than exact byte-level DDL. The existing `structure_dump_spec` pins `:data_dictionary` in `before(:all)` so its exact-DDL assertions still apply to the legacy backend. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Closes rsim#2513. Adds a second `structure_dump` implementation built on Oracle's `DBMS_METADATA.GET_DDL` / `GET_DEPENDENT_DDL` alongside the existing data-dictionary (`ALL_*`) one. The new backend lives in a separate file and installs via `prepend DbmsMetadata`, so the legacy path in `structure_dump.rb` is untouched. Selectable globally via `OracleEnhancedAdapter.structure_dump_method`: - `:auto` (default) — `:dbms_metadata` on Oracle 12.1+, `:data_dictionary` otherwise. The 12.1 floor is a project-policy version gate (the IDENTITY / EDITIONABLE / modern SET_TRANSFORM_PARAM era), exposed as `use_dbms_metadata_dump?`. - `:dbms_metadata` — force the new path; raises `ArgumentError` on pre-12.1 (mirrors PR rsim#2576's `identifier_max_length: :long` fail-fast policy). - `:data_dictionary` — force the original implementation. Remains explicitly selectable on 12.1+ as well. Implementation walks `ALL_OBJECTS` per object_type rather than hand-rolling per-kind queries; this naturally picks up MATERIALIZED VIEW / TYPE / TRIGGER / SYNONYM / etc. without separate code paths. FK constraints (REF_CONSTRAINT) are emitted in a single trailing block; cross-table dependency order is left to the user (or to a fresh-schema reload) rather than tracked here. Output is informed by `mysqldump --no-data` / `pg_dump --schema-only`: - `STORAGE` / `TABLESPACE` / `SEGMENT_ATTRIBUTES` / `EMIT_SCHEMA` suppressed via `DBMS_METADATA.SET_TRANSFORM_PARAM` so the dump is portable across installations and schemas. Oracle honors these for TABLE / INDEX / SEQUENCE. - `MATERIALIZED VIEW` DDL is an exception: Oracle does not honour the same suppression on MV (object_type-specific override does not work either), so MV DDL retains physical attributes (PCTFREE, TABLESPACE, SEGMENT CREATION, etc.) as Oracle emits them. Since MVs aren't creatable through Rails' standard migration helpers anyway, no post-processing pass is added — Oracle's output is used as-is. - `REF_CONSTRAINTS` emitted as separate `ALTER TABLE … ADD CONSTRAINT` statements after all tables. - Inline constraints via Oracle's `CONSTRAINTS=TRUE` default; UNIQUE backing indexes that DBMS_METADATA already inlines into the table DDL are filtered out so they aren't emitted twice. - `STATEMENT_TOKEN` separation preserved (Oracle's `SQLTERMINATOR=FALSE` default) so `execute_structure_dump` splits as before. - `primary_key_trigger:` (rsim#2615) row triggers picked up via the TRIGGER object_type pass. - COMMENT ON TABLE / COMMENT ON COLUMN queried directly because `GET_DEPENDENT_DDL("COMMENT")` is unreliable. - `structure_dump_db_stored_code` selects 'PROCEDURE', 'PACKAGE', 'FUNCTION', 'TYPE' — `GET_DDL("PACKAGE", ...)` already returns spec + body, so selecting 'PACKAGE BODY' would emit the body twice. Specs follow the `pg_dump` / `mysqldump` testing convention: assert output shape rather than exact byte-level DDL. A schema-option spec verifies that `:schema` connection-time switching is honored (structure_dump walks `SYS_CONTEXT('userenv', 'current_schema')`, not the connecting user). The existing `structure_dump_spec` pins `:data_dictionary` in `before(:all)` so its exact-DDL assertions still apply to the legacy backend. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

yahonda force-pushed the primary-key-trigger-2597 branch 3 times, most recently from 6bac30e to a9e9b26 Compare April 27, 2026 15:45

yahonda mentioned this pull request Apr 27, 2026

RSpec suite runtime trending past 5 minutes per CI job — consolidation candidates #2617

Closed

yahonda marked this pull request as ready for review April 27, 2026 16:11

yahonda mentioned this pull request Apr 27, 2026

OCI8 + cursor_sharing=force x INSERT ... RETURNING INTO :returning_id hangs in SQL*Net half-duplex deadlock #2619

Open

yahonda mentioned this pull request Apr 27, 2026

Honor intent.prepare in _exec_insert to avoid OCI8 half-duplex deadlock #2620

Closed

3 tasks

yahonda force-pushed the primary-key-trigger-2597 branch from 596457f to 19f146d Compare April 27, 2026 22:29

yahonda mentioned this pull request Apr 27, 2026

Use DBMS_METADATA for structure dump #2513

Merged

4 tasks

yahonda force-pushed the primary-key-trigger-2597 branch from 19f146d to ca9348e Compare April 27, 2026 23:11

yahonda force-pushed the primary-key-trigger-2597 branch from ca9348e to baa3008 Compare April 27, 2026 23:13

yahonda added 2 commits April 28, 2026 08:18

yahonda force-pushed the primary-key-trigger-2597 branch from baa3008 to 45f8b2b Compare April 27, 2026 23:18

yahonda merged commit 48d2fc1 into rsim:master Apr 28, 2026
12 checks passed

yahonda deleted the primary-key-trigger-2597 branch April 28, 2026 02:17

yahonda mentioned this pull request Apr 29, 2026

Restore the :returning_id-logging spec disabled pending #2619 #2653

Merged

yahonda mentioned this pull request May 2, 2026

Recreate primary_key_trigger on rename_table to avoid ORA-04098 #2682

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore trigger-based primary key generation as opt-in primary_key_trigger: option#2615

Restore trigger-based primary key generation as opt-in primary_key_trigger: option#2615
yahonda merged 2 commits into
rsim:masterfrom
yahonda:primary-key-trigger-2597

yahonda commented Apr 27, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

yahonda commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation / Background

Detail

Why preserve the trailing RETURNING flow rather than reinvent it

Difference from pre-2018

Behavior contract after this PR

Notes

Checklist

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

yahonda commented Apr 27, 2026 •

edited

Loading

Why preserve the trailing `RETURNING` flow rather than reinvent it