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

Adds support for deferrable foreign key constraints in PostgreSQL #41487

Merged
merged 1 commit into from Sep 22, 2021
Merged

Adds support for deferrable foreign key constraints in PostgreSQL #41487

merged 1 commit into from Sep 22, 2021

Conversation

benedikt
Copy link
Contributor

@benedikt benedikt commented Feb 18, 2021

Summary

By default, foreign key constraints in PostgreSQL are checked after each statement. This works for most use cases, but becomes a major limitation when creating related records before the parent record is inserted into the database.

One example of this is looking up / creating a person via one or more unique alias.

Person.transaction do
  alias = Alias
    .create_with(user_id: SecureRandom.uuid)
    .create_or_find_by(name: "DHH")

  person = Person
    .create_with(name: "David Heinemeier Hansson")
    .create_or_find_by(id: alias.user_id)
end

Using the default behavior, the transaction would fail when executing the first INSERT statement.

This pull request adds support for deferrable foreign key constraints by adding a new option to the add_foreign_key statement in migrations:

add_foreign_key :aliases, :person, deferrable: true

The deferrable: true leaves the default behavior, but allows manually deferring the checks using SET CONSTRAINTS ALL DEFERRED within a transaction. This will cause the foreign keys to be checked after the transaction.

It's also possible to adjust the default behavior from an immediate check (after the statement), to a deferred check (after the transaction).

add_foreign_key :aliases, :person, deferrable: :deferred

Other Information

There is a pull request #17094 from 2014 that implements generic SQL options on add_foreign_key. Pull request #40192 recently implemented the validate aspect of that.

@rails-bot
Copy link

rails-bot bot commented May 30, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label May 30, 2021
@zzak
Copy link
Member

zzak commented May 31, 2021

@benedikt I will have to do some more research on this but is this something you're still interested in picking up?

@rails-bot rails-bot bot removed the stale label May 31, 2021
@benedikt
Copy link
Contributor Author

@zzak Yes, absolutely! Let me know how I can help with research.

@zzak
Copy link
Member

zzak commented May 31, 2021

@benedikt First thing I notice is that CI failed, but I'm not entirely sure it is related yet.

Failure:
--
  | ActiveRecord::ConnectionAdapters::ConnectionPoolTest#test_checkout_fairness [/rails/activerecord/test/cases/connection_pool_test.rb:395]:
  | --- expected
  | +++ actual
  | @@ -1 +1 @@
  | -[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
  | +[1, 2, 3, 4, 5, 6, 10, 7, 8, 9]

Will have to take a closer look at this, thank you for your patience 🙇

@benedikt
Copy link
Contributor Author

@zzak Just merged the current main into this branch to resolve conflicts which caused the CI to fail. I don't think this failure is related, but I can look into it more.

@zzak
Copy link
Member

zzak commented May 31, 2021

@benedikt Thanks! I will check back on this issue shortly but feel free to ping me if you have any new ideas 🙇

@benedikt
Copy link
Contributor Author

benedikt commented May 31, 2021

@zzak Tried to reproduce this locally but didn't see the failure there.

After rebasing, the CI also didn't complain didn't complain about it again.

As the failure was in the Sqlite3 tests and this pull request only touches PostgreSQL, I think it's save to assume it was an unrelated failure.

There now seems to be a failure related to memcache timeout, but I guess that's also not related to the changes of this pull request.

@benedikt
Copy link
Contributor Author

benedikt commented Jun 8, 2021

@zzak It's a miracle, but all tests are passing. Even the unrelated ones. 😄

@benedikt
Copy link
Contributor Author

benedikt commented Jun 8, 2021

Thanks for your review, @ghiculescu. As mentioned in the individual comments, I adjusted the pull request accordingly.

Unfortunately, the build once again isn't green because of an HTTP timeout in an unrelated part.

@benedikt benedikt requested a review from ghiculescu June 8, 2021 15:45
Copy link
Member

@ghiculescu ghiculescu 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 great to me, I will wait for someone from the core team to review. Thanks for your work on this!

@zzak
Copy link
Member

zzak commented Jun 9, 2021

@benedikt You will have to squash the commits but maybe better to wait until another committer reviews this 🙏

@benedikt
Copy link
Contributor Author

benedikt commented Jun 9, 2021

@zzak Yup, it's already just one commit.

@rails-bot
Copy link

rails-bot bot commented Sep 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added stale and removed stale labels Sep 16, 2021
@benedikt
Copy link
Contributor Author

@zzak @ghiculescu Anything I can do to move this forward? 😅

@ghiculescu
Copy link
Member

There’s a merge conflict, other than that it’s waiting on the core team to review.

@benedikt
Copy link
Contributor Author

@ghiculescu Yeah, I keep fixing that particular merge conflict for the last couple of months. How do we get the core team to review this? 😄

By default, foreign key constraints in PostgreSQL are checked after each statement. This works for most use cases, but becomes a major limitation when creating related records before the parent record is inserted into the database.

One example of this is looking up / creating a person via one or more unique alias.

```ruby
Person.transaction do
  alias = Alias
    .create_with(user_id: SecureRandom.uuid)
    .create_or_find_by(name: "DHH")

  person = Person
    .create_with(name: "David Heinemeier Hansson")
    .create_or_find_by(id: alias.user_id)
end
```

Using the default behavior, the transaction would fail when executing the first `INSERT` statement.

This pull request adds support for deferrable foreign key constraints by adding a new option to the `add_foreign_key` statement in migrations:

```ruby
add_foreign_key :aliases, :person, deferrable: true
```

The `deferrable: true` leaves the default behavior, but allows manually deferring the checks using `SET CONSTRAINTS ALL DEFERRED` within a transaction. This will cause the foreign keys to be checked after the transaction.

It's also possible to adjust the default behavior from an immediate check (after the statement), to a deferred check (after the transaction).

```ruby
add_foreign_key :aliases, :person, deferrable: :deferred
```
@ghiculescu ghiculescu added the ready PRs ready to merge label Sep 16, 2021
@ghiculescu
Copy link
Member

Patience is the name of the game :) There's lots of open PRs and they're all pretty busy with Rails + real jobs. They do eventually get to most PRs but you just have to wait it out. The best advice I've heard is "make the PR easy to merge", which I think you have done a good job of here.

@benedikt
Copy link
Contributor Author

@rafaelfranca Thank you for merging this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activerecord ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants