Skip to content
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

[Idea] Warn on the addition of a validated foreign key #59

Closed
iamvery opened this issue Jan 23, 2019 · 5 comments
Closed

[Idea] Warn on the addition of a validated foreign key #59

iamvery opened this issue Jan 23, 2019 · 5 comments

Comments

@iamvery
Copy link
Contributor

iamvery commented Jan 23, 2019

I recently ran into a problem adding a new foreign key constraint at scale. It seems that by default, when adding a foreign key, the entire table is validated for the new constraint which performs some table locking.

From the PostgreSQL docs

If the constraint is marked NOT VALID, the potentially-lengthy initial check to verify that all rows in the table satisfy the constraint is skipped.

In my particular case, I was adding the foreign key to a new column which would be NULL for all existing records anyway thus not needing the validation at all. Here's the full example that caused me problems:

add_column :foos, :bar_id, :integer
add_foreign_key :foos, :bars

And here is the preferred way:

add_column :foos, :bar_id, :integer
add_foreign_key :foos, :bars, validate: false

Note: validate: false which results in NOT VALID option being added to the produced ALTER TABLE statement.

What do you think?

@ankane
Copy link
Owner

ankane commented Jan 29, 2019

Hey @iamvery, thanks for the suggestion. I think this is best as a custom check. I imagine most of the time foreign keys need to be valid.

@iamvery
Copy link
Contributor Author

iamvery commented Jan 30, 2019

Thanks for the response @ankane! I'm a little indifferent on this, but I understand your point. I'm indifferent, because I feel like if the purpose of this library is to protect users from themselves it might go so far to recommend against typical cases like this in the name of safety.

Strong Migrations detects potentially dangerous operations in migrations, prevents them from running by default, and provides instructions on safer ways to do what you want.

Either way, the custom checks are a great feature that we can take advantage of. Thank you!

@ankane
Copy link
Owner

ankane commented Feb 1, 2019

Reading about the issue more, agree that it'd be good for the gem to handle it, so happy to accept a PR. Since the validate option was introduced in Rails 5.2, the instructions will probably need to use execute in previous versions. There will need to be instructions for validating as well.

References:

@iamvery
Copy link
Contributor Author

iamvery commented Feb 1, 2019

Thanks for the follow up here! It's going to be a few days before I can dig in, but this sounds great 👍

@ankane
Copy link
Owner

ankane commented Feb 15, 2019

Going to close this out, but feel free to create a PR

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

No branches or pull requests

2 participants