cfis
commented
1 year ago Thanks for the PR. Looks like we also need to implement the other changes though including using returning parameter?
Open marcoroth opened 1 year ago
cfis
commented
1 year ago Thanks for the PR. Looks like we also need to implement the other changes though including using returning parameter?
marcoroth
commented
1 year ago If I understand the versioning right we can "break" things in a new major version since every version is just designed to work with one version of ActiveRecord, right?
So a potential 15.x release doesn't have to be compatible with ActiveRecord 7.0
cfis
commented
1 year ago Yes, that is correct. Are there big differences between 7.0 and 7.1?
Note the tests failed with this change.
machty
commented
1 year ago Hello,
I'm seeing the following in Rails 7.1 beta notes:
Support for composite primary keys in Active Record
Shopify improved the performance of common queries against their largest tables by 5-6x and reduced the number of slow queries by 80% by switching to composite primary keys. The trade-off is that inserts can become significantly slower, but for very large tables that see many more reads than writes, it can be a dramatic improvement. This work has been extracted into full support for composite primary keys in Active Record.
What does this mean for this gem? Does Rails 7.1 obsolete this gem?
cfis
commented
1 year ago Good question - don't know. Hopefully.
igor-alexandrov
commented
1 year ago I am not sure about performance of composite_primary_keys VS composite primary keys in AR 7.1 or any other aspects, but from it seems like with Rails 7.1 this gems becomes obsolete. The only difference is that in Rails 7.1 you need to use self.primary_key = [] instead of self.primary_keys = [] with this gem.
igor-alexandrov
commented
1 year ago More about composite keys support here: https://guides.rubyonrails.org/active_record_composite_primary_keys.html
cfis
commented
1 year ago It seems from #608 there isn't any reason to update CPK for Rails 7.1. Thoughts?
marcoroth
commented
1 year ago I think it might make sense to support 7.1 so you have an easier upgrade path. So that you can upgrade to Rails 7.1 without also having to migrate to the new built-in composite primary keys at the same time.
cfis
commented
1 year ago Maybe, but it sounded from the other issue the upgrade was pretty easy. But I haven't tried it myself.
codeodor
commented
1 year ago When I get around to it, I'll see if I can upgrade my app by changing CPK to just add the code:
def self.primary_keys = (array)
self.primary_key = array
endWhich should theoretically work?
cfis
commented
1 year ago And the :foreign_key => [a,b] to :query_constraints it looked. On 2023-10-11 13:41, Sammy Larbi @.***> wrote:
When I get around to it, I'll see if I can upgrade my app by changing CPK to just add the code:
def self.primary_keys = (array) self.primary_key = array end
Which should theoretically work?
—
Reply to this email directly, view it on GitHub (https://github.com/composite-primary-keys/composite_primary_keys/pull/601#issuecomment-1758505894), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAAERRRQDJEZPQZVYPCGG53X63765ANCNFSM6AAAAAA2OWQUDM).
You are receiving this because you commented.Message ID: @.***>
[ { @.": "http://schema.org", @.": "EmailMessage", "potentialAction": { @.": "ViewAction", "target": "https://github.com/composite-primary-keys/composite_primary_keys/pull/601#issuecomment-1758505894", "url": "https://github.com/composite-primary-keys/composite_primary_keys/pull/601#issuecomment-1758505894", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { @.": "Organization", "name": "GitHub", "url": "https://github.com" } } ]
jarl-dk
commented
1 year ago FYI: I have opened a bug report on rails regarding composite primary keys: https://github.com/rails/rails/issues/49597
marcocarvalho
commented
4 months ago Hello @marcoroth any plans to continue this implementation?
marcoroth
commented
3 months ago @marcocarvalho I migrated to the built-in Rails solution in the meantime. I also realized that my approach is kinda flawed and somehow only just worked to make my tests pass.
I think this would need another more serious take to make it work with Rails 7.1.
Hey there 👋🏼
First of all, thanks for the long-running support and continued maintenance of the
composite_primary_keysgem! 🙏🏼While following Rails Edge in one of my apps I noticed that the pull request https://github.com/rails/rails/pull/48241 changed the method signature for
ConnectionAdapters#sql_for_insertfrom three to four arguments andPersitance#_insert_recordfrom one to two arguments.This caused applications using
composite_primary_keysfail to boot and crash with:Full Backtrace
And
Full Backtrace
This pull requests updates the methods signature for
sql_for_insertincomposite_primary_keysso it could take either 3 or 4 arguments and the call to_insert_recordfrom one to two inside_create_record.I'm also aware that Rails 7.1 is going to get some kind of support for composite primary keys, but I guess it would still make sense for
composite_primary_keysto be Rails 7.1+ compatible to provide a smoother upgrade/migration path.I'm not sure if this is the proper and correct way to fix this. At least it's making my apps test pass and lets the app boot again. So I think this pull request is at least a starting point for making
composite_primary_keysRails 7.1 compatible.