Skip to content

fix(postgres): Error on new PK column#6749

Closed
erezrokah wants to merge 1 commit into
cloudquery:mainfrom
erezrokah:fix/postgres_error_on_new_pk
Closed

fix(postgres): Error on new PK column#6749
erezrokah wants to merge 1 commit into
cloudquery:mainfrom
erezrokah:fix/postgres_error_on_new_pk

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Jan 12, 2023

Summary

Related to #6600.

This changes the Postgres migration logic to fail when a new column is added and it's a PK.
This is the same as the SQLite plugin:

cloudquery/plugins/destination/sqlite/client/migrate.go

Line 93 in bfd1494

return fmt.Errorf("failed to auto-migrate table %s: primary key changes are not supported. please drop and re-run", table.Name)

I think we can improve this as only error if the table exists and is not empty as for empty tables this shouldn't be an issue

@erezrokah erezrokah changed the title fix(postgres): Error new PK column fix(postgres): Error on new PK column Jan 12, 2023
@erezrokah erezrokah mentioned this pull request Jan 12, 2023
Closed
yevgenypats
yevgenypats suggested changes Jan 12, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good but before we do that I think we might want to introduce the --force on the SDK/destination plugin level as we do change PKs quite a lot and most of the time I think it worked so I would worry of adding a blocker without introducing the easy workaround that most people basically use now by default?

@erezrokah
Copy link
Copy Markdown
Member Author

erezrokah commented Jan 12, 2023

The change looks good but before we do that I think we might want to introduce the --force on the SDK/destination plugin level as we do change PKs quite a lot and most of the time I think it worked so I would worry of adding a blocker without introducing the easy workaround that most people basically use now by default?

Sure. There's some refactoring needed to do before that as we'll need to show users all the changes that need "forcing" (currently we error out only on the first one).
I'm working on it atm

@erezrokah
Copy link
Copy Markdown
Member Author

erezrokah commented Jan 19, 2023

I'll merge this after we add force migration support like we did in SQLite #6763

@erezrokah
Copy link
Copy Markdown
Member Author

erezrokah commented Jan 25, 2023

Going to close this and do it once I add migrate_mode: forced

@erezrokah erezrokah closed this Jan 25, 2023
@erezrokah erezrokah deleted the fix/postgres_error_on_new_pk branch January 25, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erezrokah @yevgenypats @cq-bot