feat: production-readiness improvements (PK tracking, transactions, CONCURRENTLY, rename detection)#13
Conversation
…s, CONCURRENTLY, and rename detection - fix(loader): resolve pg_sequences panic by casting regtype to text and handling nullable fields - feat(model): add PrimaryKey variant to ConstraintKind, track PKs across all loaders (PG, MySQL, SQLite, SQL files) - feat(cli): add --no-transaction, --concurrently, --detect-renames flags - feat(migration): wrap migrations in BEGIN/COMMIT by default, support CREATE INDEX CONCURRENTLY (PG only) - feat(diff): column/table rename detection heuristic (opt-in, experimental) - High confidence: type+nullability+default match - Medium confidence: type+nullability match (SQL commented out for safety) - refactor: extract command handlers from main.rs into src/commands/ module - refactor: split migration.rs into migration/mod.rs + migration/sql.rs - fix(ci): DROP COLUMN now correctly reported as blocking in CI report - fix(ci): renamed tables/columns now included in CI report changes - fix(migration): rollback renames now execute before constraint re-addition - fix(migration): MySQL PK uses ADD PRIMARY KEY without named constraint - fix(output): print_diff now displays renamed tables and columns - fix(error): replace unwrap_or_default with proper error propagation for JSON/YAML serialization - fix(loader): SQLite error propagation instead of silent filter_map(ok()) - fix(loader): emit warning when sequence metadata is NULL (permissions) - chore: remove unused Trigger/Function model types - chore: add RenameConfidence Copy/Display derives - test: add 22 new tests for PK, renames, CONCURRENTLY, transactions
- fix(migration): rollback medium-confidence renames now commented out (matching forward migration) - fix(output): DiffSummary now includes tables_renamed and columns_renamed counts - fix(commands): emit warning when --concurrently used with non-PostgreSQL dialect
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d2f3c9aed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| statements.push(MigrationStatement { | ||
| sql: create_index_sql(idx, dialect, concurrently), | ||
| warnings, | ||
| is_blocking: !concurrently, // CONCURRENTLY is non-blocking | ||
| }); |
There was a problem hiding this comment.
This marks every added index as non-blocking whenever concurrently is true, but create_index_sql only emits CONCURRENTLY for PostgreSQL. On MySQL/SQLite, --concurrently still produces plain CREATE INDEX while is_blocking becomes false, so CI/reporting can incorrectly treat a blocking index build as safe (notably with --fail-on-blocking).
Useful? React with 👍 / 👎.
| // High confidence: identical column definitions | ||
| if rem.columns == add.columns { | ||
| return Some((ai, RenameConfidence::High)); | ||
| } | ||
| // Medium confidence: same column count, ≥80% column name overlap |
There was a problem hiding this comment.
High-confidence table rename matching is based only on columns, so a removed/added pair can be converted into a rename even when indexes or constraints differ. Because matched tables are then removed from added/removed sets in this function, those index/constraint deltas never reach modified_tables, and migration generation can emit only RENAME TABLE while silently skipping required schema changes.
Useful? React with 👍 / 👎.
Table rename detection removed matched tables from added/removed sets without diffing non-column properties, causing index and constraint changes to be silently dropped from migration output.
There was a problem hiding this comment.
💡 Codex Review
Lines 50 to 51 in 06be941
Reverse all object types in rollback CI classification
from_diff_reversed delegates to build(..., true), but build only flips ADD/DROP semantics for tables; column/index/constraint branches still use forward matching (e.g. added_columns looks for ADD COLUMN). In --direction down, rollback SQL contains DROP COLUMN/DROP INDEX, so lookups miss and default is_blocking values are wrong, which can underreport blocking rollback steps and let --ci --fail-on-blocking return EXIT_DRIFT instead of EXIT_UNSAFE.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if candidates.len() == 1 { | ||
| let (ai, confidence) = &candidates[0]; | ||
| matched_removed.push(ri); | ||
| matched_added.push(*ai); | ||
| renames.push(TableRename { |
There was a problem hiding this comment.
The rename detector accepts a rename whenever a removed table has exactly one unmatched added candidate, but it does not verify the reverse uniqueness (that this added table is uniquely claimed by only one removed table). With --detect-renames, if two removed tables share the same column shape and one added table matches both, iteration order picks one arbitrary rename and drops the other table, which can preserve/drop the wrong dataset while still producing “high confidence” rename SQL.
Useful? React with 👍 / 👎.
…re bijective table rename matching - CI report now correctly flips SQL patterns and blocking defaults for columns, indexes, and constraints in reverse (rollback) mode, not just tables. Previously, rollback lookups missed statements and used wrong default is_blocking values, underreporting blocking rollback steps. - Table rename detector now requires bijective (1-to-1) matching: a rename is only accepted when the removed table has exactly one added candidate AND that added table is claimed by only one removed table. Previously, iteration order could pick an arbitrary rename when two removed tables shared the same column shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c760ee86d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if candidates.len() == 1 { | ||
| let (ai, confidence) = &candidates[0]; | ||
| matched_removed.push(ri); | ||
| matched_added.push(*ai); |
There was a problem hiding this comment.
The column rename matcher accepts a rename whenever a removed column currently has one unmatched candidate, but it never enforces reverse uniqueness. With --detect-renames, if two removed columns have the same type/nullability signature and only one added column fits, iteration order will rename one arbitrarily and leave the other as a drop, which can preserve/drop the wrong data column. Please require that the chosen added column is claimed by exactly one removed column (bijective matching), as you already do for table renames.
Useful? React with 👍 / 👎.
| for (old_table, new_table) in &rename_pairs { | ||
| let table_diff = | ||
| diff_tables(&new_table.name, old_table, new_table, detect_renames); |
There was a problem hiding this comment.
For detected table renames, diff_tables(&new_table.name, old_table, new_table, ...) builds removed_constraints/removed_indexes from old_table objects, so they keep the old table_name. Migration generation renames the table first, then emits drop statements from those removed objects; that makes DROP CONSTRAINT target a non-existent table after rename (and DROP INDEX ... ON old_table on MySQL). This breaks forward migrations whenever renamed tables also have constraint/index deltas, so removed objects should be rebased to the new table name before being added to modified_tables.
Useful? React with 👍 / 👎.
…e detection When a table rename is detected with concurrent index/constraint changes, removed_indexes and removed_constraints retained the old table name. This caused DROP statements to target the pre-rename name, breaking forward migrations on MySQL (DROP INDEX ... ON old_table) and PostgreSQL (ALTER TABLE old_table DROP CONSTRAINT ...).
1 participant
Summary
pg_sequencespanic by castingregtypeto text and handling nullable fields with permission warningsPrimaryKeyvariant toConstraintKind, track PKs across all loaders (PostgreSQL, MySQL, SQLite, SQL files)--no-transaction,--concurrently,--detect-renamesflagsBEGIN/COMMITby default; supportCREATE INDEX CONCURRENTLY(PostgreSQL only, auto-disables transactions)--detect-renames, experimental)main.rsintosrc/commands/module; splitmigration.rsintomigration/{mod,sql}.rsDROP COLUMNnow correctly reported as blocking; renamed tables/columns included in CI reportADD PRIMARY KEY(no named constraint); medium-confidence rollback renames commented outprint_diffandDiffSummarynow display renamed tables/columnsunwrap_or_defaultwith proper error propagation for JSON/YAML serialization; SQLite error propagation instead of silentfilter_map(ok())Trigger/Functionmodel types; addRenameConfidenceCopy/DisplayderivesTest plan
cargo test— 166 tests pass (was 144)cargo clippy— no warningsaccountsvsfd_accounting)--detect-renameswith a real column rename scenario--concurrentlygenerates correct SQL and--no-transactionomits BEGIN/COMMIT--ci --format jsonincludes renames in report