-
Notifications
You must be signed in to change notification settings - Fork 537
Normalize feedback primary key to (id) for upgraded installs #1801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| -- No-op: 000004 is a corrective convergence step. Both fresh installs (which | ||
| -- already had PRIMARY KEY (id) from 000001) and upgraded installs (which | ||
| -- carried the legacy composite PRIMARY KEY (id, user_id) from GORM | ||
| -- AutoMigrate) end at PRIMARY KEY (id). Once up has run, there is no way to | ||
| -- tell which path the database came from, so a symmetric down would put fresh | ||
| -- installs into a composite-PK state they never originally had. Leaving the | ||
| -- single-column PK in place on rollback is safer and the application is | ||
| -- PK-shape independent (queries do not reference the PK directly). | ||
| SELECT 1; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| -- Normalize the feedback primary key to (id) only. | ||
| -- | ||
| -- Fresh installs from 000001 already have PRIMARY KEY (id) (BIGSERIAL). | ||
| -- Installs that upgraded from the GORM-managed schema retained the legacy | ||
| -- composite PRIMARY KEY (id, user_id) created by AutoMigrate. | ||
| -- | ||
| -- Both variants use the default constraint name `feedback_pkey`, so this | ||
| -- atomic drop+add converges both paths to the single-column PK without | ||
| -- changing column types or affecting sqlc-generated code (queries do not | ||
| -- reference the PK; ListFeedback filters on user_id via idx_feedback_user_id). | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @EItanya I don't think this is necessary based on the limited usefulness of the feedback feature and the unlikelihood that this will have duplicate ids. The only reason I'm considering it is to have a clearer message for manual fixes in the unlikely event that there is an issue with this upgrade. Another option is we we could just raise it in the release notes if we want, but as before, I think this is unlikely to be an issue. |
||
| ALTER TABLE feedback | ||
| DROP CONSTRAINT feedback_pkey, | ||
| ADD CONSTRAINT feedback_pkey PRIMARY KEY (id); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would result in a fresh install env applying this migration on top having the wrong primary key. I think it's fine to leave as a result.