Allow altering PK columns even if they are referenced #935
Conversation
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
3a3d6f1 to
f8ee926
Compare
f8ee926 to
ae55c6b
Compare
ae55c6b to
ce7ef05
Compare
kvch
added a commit
that referenced
this pull request
Jul 1, 2025
…traints (#930) In the past `pgroll` could not preserve primary key constraint on altered columns. Instead the primary key constraint was down-graded to a unique, not null constraint. Now the PK constraint is not removed during completing the migration. Futhermore, if the table had rows, `pgroll` could not backfill rows. `pgroll` "updates" the primary keys of the backfilled table to trigger backfilling existing columns. Previously, finding the identity columns returned the duplicated column `__pgroll_new_id` instead of the correct `id`. In this case we need the current PK, so we can copy the correct values to the new column. I opened follow-up issue to support altering mutiple columns of the same composite key in the same migration: #930 Support for alter PK columns that are part of FK constraints is in a follow-up PR: #935 Closes #888
ce7ef05 to
8eb4435
Compare
8eb4435 to
0224bfe
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Comment on lines
726
to
848
| Name: "02_alter_column", | ||
| Operations: migrations.Operations{ | ||
| &migrations.OpAlterColumn{ | ||
| Table: "people", | ||
| Column: "id", | ||
| Type: ptr("bigint"), | ||
| Up: "CAST(id AS bigint)", | ||
| Down: "SELECT CASE WHEN id < 2147483647 THEN CAST(id AS int) ELSE 0 END", | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I think this test is incorrect; shouldn't we be testing that the primary key field on the referenced table can be modified, ie events, not people?
pkg/migrations/dbactions.go
Outdated
Comment on lines
895
to
899
| rows, err := a.conn.QueryContext(ctx, fmt.Sprintf(` | ||
| SELECT conname, r.conrelid::regclass, pg_catalog.pg_get_constraintdef(r.oid, true) as condef | ||
| FROM pg_catalog.pg_constraint r | ||
| WHERE confrelid = %s::regclass AND r.contype = 'f'`, | ||
| pq.QuoteIdentifier(a.table), |
There was a problem hiding this comment.
If we need this information, I think it should be present in the schema returned by read_schema. If we are missing information there we should add it to read_schema rather than running ad-hoc queries during the migration.
|
Depends on #946 |
77665f3 to
3177e00
Compare
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.
Previously,
pgrollcould not alter PK columns if they were included in a foreign key reference. The old columncould not be dropped because it was still referred by foreign key constraints. Now the foreign key constraints are
recreated so they refer to the new PK column.
Requires #930
Closes #375