Skip to content

Comments

Add support for exclusion constraints (PostgreSQL-only)#40224

Merged
yahonda merged 1 commit intorails:mainfrom
agrobbin:postgresql-exclusion-constraints
Jun 14, 2022
Merged

Add support for exclusion constraints (PostgreSQL-only)#40224
yahonda merged 1 commit intorails:mainfrom
agrobbin:postgresql-exclusion-constraints

Conversation

@agrobbin
Copy link
Contributor

@agrobbin agrobbin commented Sep 13, 2020

Summary

Similar to #31323, this extends Active Record's migration/schema dumping to support exclusion constraints!

add_exclusion_constraint :invoices, "daterange(start_date, end_date) WITH &&", using: :gist, name: "invoices_date_overlap"
remove_exclusion_constraint :invoices, name: "invoices_date_overlap"

Hopefully the approach is reasonable, but definitely let me know if there is anything that looks out of whack.

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from 8ab6c83 to 1ac2c8e Compare September 13, 2020 02:49
@rails-bot
Copy link

rails-bot bot commented Dec 12, 2020

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 Dec 12, 2020
@agrobbin
Copy link
Contributor Author

agrobbin commented Dec 13, 2020

I know this missed the 6.1 release milestone, but I'd still love to get some eyes on this if someone has time!

@rails-bot rails-bot bot removed the stale label Dec 13, 2020
Base automatically changed from master to main January 14, 2021 17:02
@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch 2 times, most recently from 8e10e68 to 425de72 Compare March 29, 2021 13:46
@hernanat
Copy link

hernanat commented Apr 23, 2021

+1

@hernanat
Copy link

hernanat commented May 6, 2021

@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?

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from 425de72 to 8ec4fcb Compare June 2, 2021 15:55
@agrobbin
Copy link
Contributor Author

agrobbin commented Jun 2, 2021

@hernanat merge conflicts resolved! I will post something on the discussion forum in a bit.

@zzak
Copy link
Member

zzak commented Jun 3, 2021

👀 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 🙏

ghiculescu
ghiculescu reviewed Jun 3, 2021
View reviewed changes
activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb Outdated
Copy link
Member

@ghiculescu ghiculescu Jun 3, 2021

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.

Copy link

@hernanat hernanat Jun 3, 2021

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

Copy link
Contributor Author

@agrobbin agrobbin Sep 6, 2021

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.

ghiculescu
ghiculescu reviewed Jun 3, 2021
View reviewed changes
activerecord/lib/active_record/schema_dumper.rb Outdated
Copy link
Member

@ghiculescu ghiculescu Jun 3, 2021

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?

Copy link
Contributor Author

@agrobbin agrobbin Sep 6, 2021

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!

ghiculescu
ghiculescu reviewed Jun 3, 2021
View reviewed changes
ghiculescu
ghiculescu reviewed Jun 3, 2021
View reviewed changes
activerecord/test/cases/migration/exclusion_constraint_test.rb Outdated
Copy link
Member

@ghiculescu ghiculescu Jun 3, 2021

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.

ghiculescu
ghiculescu reviewed Jun 3, 2021
View reviewed changes
@hernanat
Copy link

hernanat commented Aug 31, 2021

left another reply on the mailing list thread trying to get this moving forward

@agrobbin
Copy link
Contributor Author

agrobbin commented Sep 6, 2021

@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 ...

  • Should some of this code move to the PG-specific AR schema classes?
  • Should exceptions be raised in migrations by AR when someone tries to use a feature that is not supported by their database?

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.

@rails-bot
Copy link

rails-bot bot commented Dec 6, 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 Dec 6, 2021
@agrobbin
Copy link
Contributor Author

agrobbin commented Dec 6, 2021

I would still love to get some resolution here!

@rails-bot rails-bot bot removed the stale label Dec 6, 2021
@hernanat
Copy link

hernanat commented Dec 6, 2021

I would still love to get some resolution here!

+1

@quorak quorak mentioned this pull request Feb 28, 2022
Open
@rails-bot
Copy link

rails-bot bot commented Mar 6, 2022

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.

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from e3cf854 to 427b5e9 Compare May 26, 2022 01:26
@agrobbin
Copy link
Contributor Author

agrobbin commented May 26, 2022 •
edited
Loading

@yahonda I just rebased off of the latest main, and resolved the outstanding comment I saw from @ghiculescu. Let me know if there's anything else I missed!

@yahonda
Copy link
Member

yahonda commented May 31, 2022

While check constraints are popular and available for MySQL 8.0.16+, PostgreSQL, and SQLite3, exclusion constraints are available for PostgreSQL only.

Therefore ActiveRecord::ConnectionAdapters::PostgreSQL::SchemaCreation#visit_AlterTable should be modified to support exclusion constraints not ActiveRecord::ConnectionAdapters::SchemaCreation#visit_AlterTable.

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from 427b5e9 to 28cfdc5 Compare May 31, 2022 12:22
@agrobbin
Copy link
Contributor Author

agrobbin commented May 31, 2022

@yahonda updated!

@agrobbin
Copy link
Contributor Author

agrobbin commented May 31, 2022

@yahonda I'm going to do a quick pass and move some more of this into the PG-specific adapter code.

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch 2 times, most recently from 81abed9 to 34b5d06 Compare May 31, 2022 13:24
@agrobbin
Copy link
Contributor Author

agrobbin commented May 31, 2022

@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.

yahonda
yahonda reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@yahonda yahonda left a 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.

yahonda
yahonda reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@yahonda yahonda left a 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.

@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from 34b5d06 to 1f6bae9 Compare June 7, 2022 12:22
@yahonda
Copy link
Member

yahonda commented Jun 7, 2022

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.

@agrobbin
Copy link
Contributor Author

agrobbin commented Jun 7, 2022

@yahonda sorry about that, I'll do that going forward.

@yahonda
Copy link
Member

yahonda commented Jun 14, 2022

It looks good to me. Please address the conflict with CHANGELOG.md and squash your commits.

@agrobbin
adds support for exclusion constraints (PostgreSQL-only)
1ceffeb 
```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.
@agrobbin agrobbin force-pushed the postgresql-exclusion-constraints branch from 1f6bae9 to 1ceffeb Compare June 14, 2022 12:09
@agrobbin
Copy link
Contributor Author

agrobbin commented Jun 14, 2022

@yahonda conflict addressed and squash+rebased off of the latest main!

@yahonda yahonda merged commit 6d70379 into rails:main Jun 14, 2022
@agrobbin
Copy link
Contributor Author

agrobbin commented Jun 14, 2022

Woo, thanks so much for your help getting this over the line @yahonda!

@cbortz cbortz mentioned this pull request Jul 28, 2022
Closed
@alpaca-tc alpaca-tc mentioned this pull request Oct 4, 2022
Merged
9 tasks
@roleme roleme mentioned this pull request Nov 10, 2022
Merged
@alpaca-tc alpaca-tc mentioned this pull request Mar 13, 2023
Merged
4 tasks
@ghiculescu ghiculescu mentioned this pull request Apr 21, 2023
Closed
@agrobbin agrobbin mentioned this pull request Apr 22, 2023
Merged
@alpaca-tc alpaca-tc mentioned this pull request Oct 14, 2023
Merged
This was referenced Dec 22, 2023
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yahonda yahonda yahonda left review comments

+2 more reviewers

@hernanat hernanat hernanat left review comments

@ghiculescu ghiculescu ghiculescu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

activerecord

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@agrobbin @hernanat @zzak @jensljungblad @yahonda @ghiculescu