Skip to content

Duplicate unique and check constraints correctly #466

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

Merged
merged 15 commits into from
Nov 18, 2024

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Nov 14, 2024

Previously, unique and check constraints with multiple columns were not duplicated correctly.

We had two issues:

  1. Pgroll tried to create the same duplicated index for each renamed column. From now on pgroll creates the index once with the new column names.
  2. Pgroll tried to convert the unique index to a constraint multiple times. From now on, the index is only converted once.

Required by #464

@kvch
Copy link
Contributor Author

kvch commented Nov 14, 2024

It is highly likely we need to adjust the check constraint migrations as well. I will address those issues in a follow-up PR.

@kvch kvch requested a review from andrew-farries November 14, 2024 14:55
@kvch kvch requested a review from andrew-farries November 14, 2024 19:59
@kvch
Copy link
Contributor Author

kvch commented Nov 14, 2024

I pushed the multi-column duplicator change. I also tested it with the new check constraint PR and it fixes the problem with the example.

@kvch kvch changed the title Duplicate unique constraints correctly Duplicate unique and check constraints correctly Nov 14, 2024
@kvch kvch requested a review from andrew-farries November 15, 2024 13:38
@kvch
Copy link
Contributor Author

kvch commented Nov 15, 2024

I have split out the statement builder from the duplicator and added unit tests to make sure we generate the correct SQL statements for constraints.

Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks for adding the tests for the duplicators.

Would it be possible to combine the ColumnDuplicator and the ColumnGroupDuplicator so that a duplicator could be created with one or more columns?

@kvch kvch requested a review from andrew-farries November 15, 2024 16:24
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

Great 🎉

Approved with one more suggesion. 👍

@kvch
Copy link
Contributor Author

kvch commented Nov 18, 2024

Updated the PR with the suggestion, and merging once the tests pass.

@kvch kvch enabled auto-merge (squash) November 18, 2024 22:18
@kvch kvch merged commit 5d63a7f into xataio:main Nov 18, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants