fix(Migrator): preserve indexes, triggers, table options and views across table rebuilds#236
Open
h2zi wants to merge 2 commits into
Open
fix(Migrator): preserve indexes, triggers, table options and views across table rebuilds#236h2zi wants to merge 2 commits into
h2zi wants to merge 2 commits into
Conversation
…ross table rebuilds recreateTable implements ALTER TABLE operations by creating a modified copy, moving the data over, dropping the original table and renaming the copy back. Three things were lost along the way: - indexes and triggers: DROP TABLE removes them together with the old table. AutoMigrate happens to recreate indexes declared in the model afterwards, but calling AlterColumn/DropColumn/*Constraint directly silently loses every index and trigger on the table. Their DDL is now saved from sqlite_master before the rebuild and replayed afterwards; indexes referencing a dropped column can no longer apply and are skipped. - table options after the column list: the DDL parser only kept the head and the column list, so WITHOUT ROWID and STRICT silently disappeared and the rebuilt table became a plain rowid table. The parser now keeps the trailing options and compile() emits them again. - views referencing the table: the RENAME step re-resolves referencing views while the original table name doesn't exist yet, failing with "error in view ...: no such table" (go-gorm#225). The rename now runs under PRAGMA legacy_alter_table, which skips that re-resolution; the view points at the original name and is valid again right after the rename. Fixes go-gorm#225
There was a problem hiding this comment.
Pull request overview
This PR improves the SQLite migrator’s table-rebuild strategy used for ALTER TABLE emulation by preserving schema artifacts that were previously lost or could cause rebuild failures (indexes/triggers, table options like WITHOUT ROWID/STRICT, and views referencing the rebuilt table).
Changes:
- Persist and replay index/trigger DDL around
recreateTable()so rebuilds don’t silently drop them. - Extend the CREATE TABLE DDL parser to retain and re-emit trailing table options (
suffix). - Avoid view re-resolution failures during rename by toggling
PRAGMA legacy_alter_tablefor the rename step, plus add focused regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| migrator.go | Saves/replays auxiliary DDL and uses legacy_alter_table around the rename to keep views valid during rebuild. |
| ddlmod.go | Captures and re-emits CREATE TABLE trailing options via a new suffix field. |
| migrator_test.go | Adds regression tests covering preservation of indexes/triggers, table options, and view-referenced rebuilds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+453
to
+457
| // recreate the saved indexes and triggers; ones referencing a | ||
| // column that no longer exists cannot apply anymore and are skipped | ||
| for _, aux := range auxDDLs { | ||
| _ = tx.Exec(aux).Error | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Agreed — fixed in 8f94608. The replay loop now only skips no such column errors (an index or trigger referencing the dropped column, which is the one expected casualty of the rebuild); any other failure aborts the migration and rolls back the transaction.
Comment on lines
+49
to
+51
| var names []string | ||
| db.Raw("SELECT name FROM sqlite_master WHERE tbl_name = 'rebuild_idx_table' AND type IN ('index','trigger')").Scan(&names) | ||
| got := strings.Join(names, ",") |
Contributor
Author
There was a problem hiding this comment.
Fixed in 8f94608 — the test now fails on the query error instead of asserting against zero values.
Comment on lines
+66
to
+67
| var auditCount int | ||
| db.Raw("SELECT count(*) FROM rebuild_audit").Scan(&auditCount) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 8f94608 — same treatment as above.
Comment on lines
+92
to
+93
| var ddl string | ||
| db.Raw("SELECT sql FROM sqlite_master WHERE type='table' AND name='rebuild_opts_table'").Scan(&ddl) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 8f94608 — same treatment as above.
Address review feedback: replaying saved index/trigger DDL swallowed every error, silently dropping objects for unrelated reasons. Only "no such column" (an index/trigger referencing the dropped column) is skipped now; any other failure aborts the migration. Also fail the tests on query errors instead of asserting against zero values, and close the pool on cleanup so the shared in-memory database is torn down between -count=N runs.
h2zi
added a commit
to libtnb/sqlite
that referenced
this pull request
Jul 7, 2026
Replaying saved index/trigger DDL after a table rebuild swallowed every error, silently dropping objects for unrelated reasons. Only "no such column" (an index or trigger referencing the dropped column) is skipped now; any other failure aborts the migration. Also fail tests on query errors instead of asserting against zero values. Mirrors the review feedback on go-gorm/sqlite#236. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
What this PR does
recreateTableimplements ALTER TABLE operations by creating a modified copy of the table, moving the data over, dropping the original and renaming the copy back. Three kinds of schema objects were silently lost along the way:Indexes and triggers.
DROP TABLEremoves them together with the old table. A fullAutoMigratehappens to recreate the indexes declared in the model afterwards, but callingAlterColumn,DropColumnor the constraint methods directly loses every index and trigger on the table without any error.Table options after the column list. The DDL parser only kept the head and the column list, so
WITHOUT ROWIDandSTRICTdisappeared on rebuild and the table silently became a plain rowid table.Views referencing the table (#225). The RENAME step re-resolves referencing views while the original table name doesn't exist yet (it was just dropped), so the whole operation fails with
error in view ...: no such table.The fix
sqlof every index and trigger on the table fromsqlite_master, and replay them after the rename. Indexes referencing a dropped column can no longer apply and are skipped.suffixfield andcompile()emits it again, preservingWITHOUT ROWID/STRICT.PRAGMA legacy_alter_table, which skips the view re-resolution; the view points at the original table name and is valid again right after the rename. The PRAGMA is switched off immediately afterwards, whether the rename succeeds or fails.Tests
TestRecreateTablePreservesIndexesAndTriggers: an index on a kept column and a trigger surviveDropColumn(and the trigger still fires), while an index on the dropped column is removed.TestRecreateTablePreservesTableOptions:WITHOUT ROWIDsurvives a rebuild.TestRecreateTableWithView: rebuilding a table referenced by a view succeeds and the view still works (reproduces Failing to recreate altered table when referenced by view #225).Fixes #225
🤖 Generated with Claude Code