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
Add support for exclusion constraints (PostgreSQL-only) #40224
Conversation
8ab6c83
to
1ac2c8e
Compare
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. |
I know this missed the 6.1 release milestone, but I'd still love to get some eyes on this if someone has time! |
8e10e68
to
425de72
Compare
+1 |
@agrobbin you have some merge conflicts, could you resolve them and maybe put a thread up on the mailing list if you can't get anyone's attention here? |
425de72
to
8ec4fcb
Compare
@hernanat merge conflicts resolved! I will post something on the discussion forum in a bit. |
👀 This seems like a really thorough piece of work, @agrobbin nice work! I'm not sure I have enough context here but from what I've read so far it looks good. Hopefully we can get some more 👀 on it and move things forward 🙏 |
activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb
Outdated
Show resolved
Hide resolved
# [<tt>:name</tt>] | ||
# The constraint name. Defaults to <tt>excl_rails_<identifier></tt>. | ||
def add_exclusion_constraint(table_name, expression, **options) | ||
return unless supports_exclusion_constraints? |
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.
I am not sure what the usual approach in Rails is here, but thinking out loud - I feel like you'd want your migration to fail noisely, not silently, if you ran a command that your DB doesn't support.
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.
that's a great point
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.
As with some of your other comments, I don't necessarily disagree with them, but I also think there is a lot of value in consistency of this functionality with the check constraint functionality that already exists in AR, which is why I did this explicit return
.
@@ -237,6 +250,27 @@ def check_constraints_in_create(table, stream) | |||
end | |||
end | |||
|
|||
def exclusion_constraints_in_create(table, stream) | |||
if (exclusion_constraints = @connection.exclusion_constraints(table)).any? |
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.
do you need to check @connection.supports_exclusion_constraints?
here?
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.
Nope, we shouldn't need to, as the caller of this private method does that for us!
end | ||
|
||
def test_add_exclusion_constraint_should_be_noop | ||
@connection.add_exclusion_constraint :test_exclusion_constraints, "daterange(start_date, end_date) WITH &&", using: :gist, where: "start_date IS NOT NULL and end_date IS NOT NULL", name: "date_overlap" |
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.
as above. i think these should fail loudly.
left another reply on the mailing list thread trying to get this moving forward |
@ghiculescu thanks for the review here! I left some responses on some comments, but I think there are a couple of overarching questions that the Rails Core Team needs to answer ...
I followed the existing convention for both of those things, and while I think they're both great questions, they are significant deviations from the existing patterns, so I don't feel comfortable going down those roads without input from the Core Team. |
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. |
I would still love to get some resolution here! |
+1 |
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. |
e3cf854
to
427b5e9
Compare
@yahonda I just rebased off of the latest |
While check constraints are popular and available for MySQL 8.0.16+, PostgreSQL, and SQLite3, exclusion constraints are available for PostgreSQL only. Therefore |
427b5e9
to
28cfdc5
Compare
@yahonda updated! |
@yahonda I'm going to do a quick pass and move some more of this into the PG-specific adapter code. |
81abed9
to
34b5d06
Compare
@yahonda OK, just passed tests, and should be more isolated to the PG-specific adapter code! Let me know if you see anything else worth adjusting, and thanks for the review. |
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.
Left some review comments. Actually, this pull request size is relatively large (for me). I'll add some other reviews later.
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb
Outdated
Show resolved
Hide resolved
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.
Add some more review comments.
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
Outdated
Show resolved
Hide resolved
activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
Outdated
Show resolved
Hide resolved
34b5d06
to
1f6bae9
Compare
It would be appreciated if you push another commit, not force one because the review is in progress and it makes it difficult for me to review new changes. Once reviews are completed, I (or someone else) may ask you to squash commits. |
@yahonda sorry about that, I'll do that going forward. |
It looks good to me. Please address the conflict with CHANGELOG.md and squash your commits. |
```ruby add_exclusion_constraint :invoices, "daterange(start_date, end_date) WITH &&", using: :gist, name: "invoices_date_overlap" remove_exclusion_constraint :invoices, name: "invoices_date_overlap" ``` See PostgreSQL's [`CREATE TABLE ... EXCLUDE ...`](https://www.postgresql.org/docs/12/sql-createtable.html#SQL-CREATETABLE-EXCLUDE) documentation for more on exclusion constraints.
1f6bae9
to
1ceffeb
Compare
@yahonda conflict addressed and squash+rebased off of the latest |
Woo, thanks so much for your help getting this over the line @yahonda! |
Summary
Similar to #31323, this extends Active Record's migration/schema dumping to support exclusion constraints!
Hopefully the approach is reasonable, but definitely let me know if there is anything that looks out of whack.