Sounds good. Glad to have a decision on this so we can get away from random names.
I'm happy to do the work and get a PR in. Should hopefully have some time in the next week. Cheers, Chris On 30 January 2015 at 10:48, Yves Senn <yves.s...@gmail.com> wrote: > I'm not convinced that we need human readable foreign key names right now. > Since 4.2. is out with this issue, for the time being I'd like to focus on > a backportable solution. > > A pull request with deterministic names but still non-human readable is > very welcome. > This should be a fairly controlled change that we can bring back to > `4.2.x` to solve the issue. > > We're open to consider human readable names if they become more relevant > in the future (what Simon Riggs suggests). > If you happen to know more about these plans, please keep us posted so we > can adjust accordingly. > > @Chris Sinjakli: Thank you for your patience. Please let me know if you > are up to work on a PR. > Otherwise it would be good to file an issue on GitHub so that someone else > can take over. > > Cheers, > -- Yves Senn > > On Monday, January 26, 2015 at 5:36:15 PM UTC+1, Chris Sinjakli wrote: >> >> I think changing the semantics of `add_foreign_key` is best saved for >> another discussion. The naming change seems to be contentious enough by >> itself. ;) >> >> A quick summary of the tradeoffs for human readable names: >> >> + Consistency with index naming >> + When they show up in query plans, they'll be understandable >> - They can fall out of date >> - They can be too long and require manual naming >> >> To my mind, the code for renaming them shouldn't be a huge leap from the >> same code that's there for indexes. The most obvious issue is support for >> constraint renaming in different databases. Postgres only has this in 9.2+, >> so we'd have to do version checks. By contrast index renaming support goes >> back to at least 8.0. I have no idea about MySQL. >> >> That said, the foreigner gem (which this built-in functionality replaces) >> has been generating human readable FK names without renaming them for ages. >> I'd be really interested in knowing whether this was a major issue for >> people using it (other than me). From what I can see, this got brought up >> once (https://github.com/matthuhiggins/foreigner/issues/141) and wasn't >> discussed. >> >> I'm getting the feeling this is an issue which just needs someone who's >> in charge of activerecord to make a call. I'd like to have the readable >> names back myself, but it's not my call and opinions are pretty divided on >> here. >> >> On 17 January 2015 at 12:08, Will Bryant <will....@gmail.com> wrote: >> >>> True - good point. Using random values is a bit weird though, since two >>> people running the same migration will get different names, and then their >>> schemas will never quite match. This means dev machines’ schemas will >>> differ from staging/test environments and them from production. >>> >>> Wouldn’t a simple CRC hash of the input data equally solve the issue of >>> names potentially getting out of date, without introducing non-determinancy? >>> >>> More way-too-late bikeshedding: IMHO the method in 4.2 should be renamed >>> to add_foreign_key_with_constraint. A foreign key is just the key for >>> an association. A foreign key constraint is a different concept to a >>> foreign key. >>> >>> IMHO it’s really important that users understand the difference, and >>> only add constraints when they want them. >>> >>> The thing is, if you live your life on recent versions of PostgreSQL, >>> they work well enough to be usable. But on MySQL particularly - and in >>> PostgreSQL before they implemented the special weakened lock types - they >>> cause a lot of unintended side effects because they result in a standard >>> shared lock being taken on the parent row. >>> >>> On one of my big apps we put FKCs in in ~2008, and we’re now actually >>> going through dropping FKCs when we get the chance, because they just cause >>> too many problems at runtime. >>> >>> Basically, if table A is a parent to table B, and you have an FKC >>> linking the two, then an update to a B row will take some kind of shared >>> lock on A. But it’s also common to have a form for A records which nests B >>> records, and then in practice the #update action is going to save A, and >>> then save the Bs… and then you get deadlocks. >>> >>> I’ve been finding this sort of problem (there’s others) comes up so >>> often that I’ve gone right off the idea of foreign key constraints, and now >>> tell ppl to only add them when they specifically need them. (And I find >>> usually it’s relatively rare that you do in practice because a lot of >>> parent tables don’t get deletes, and those that do you tend to want to >>> seralize things using update locks anyway.) >>> >>> TL;DR I would like people to learn from our mistakes and not to >>> accidentally get foreign key constraints when they think they are just >>> adding a foreign key :). >>> >>> >>> On 17/01/2015, at 23:32 , Yves Senn <yves...@gmail.com> wrote: >>> >>> Yes, they are shown in constraint error messages, but so are the >>> metadata (table name and referenced column name). >>> Following are example messages: >>> >>> MySQL: >>> ----------- >>> ERROR 1452 (23000): Cannot add or update a child row: a foreign key >>> constraint fails (`activerecord_unittest`.`fk_test_has_fk`, >>> CONSTRAINT `fk_rails_bde121d5a6` FOREIGN KEY (`fk_id`) REFERENCES >>> `fk_test_has_pk` (`id`)) >>> >>> PostgreSQL: >>> ----------------- >>> ERROR: insert or update on table "fk_test_has_fk" violates foreign key >>> constraint "fk_rails_bde121d5a6" >>> DETAIL: Key (fk_id)=(98493849) is not present in table "fk_test_has_pk". >>> >>> Both messages have all the detail needed to understand what's going on. >>> From that perspective >>> I don't see the need to duplicate the metadata in the constraint name. >>> >>> Simon: I didn't know about that. Is there a discussion or documentation >>> that goes into >>> further detail? I'd like to read up on these planed changes. This could >>> affect what >>> implementation we are going to pursue. >>> >>> Cheers, >>> -- Yves >>> >>> On Saturday, January 17, 2015 at 2:01:25 AM UTC+1, will.bryant wrote: >>>> >>>> They may not be shown in query plans, but they are shown in constraint >>>> error messages, and the possibility of hitting those errors is the whole >>>> reason one adds foreign key constraints. It’s helpful to have an >>>> indication which kind of constraint was invalidated without having to look >>>> through the schema. >>>> >>>> We do already tolerate the index names being irrelevant if they become >>>> inconsistent, but in my personal experience that doesn’t happen very often >>>> at all, since I’ve found indexed columns tend to be important and keep >>>> their names. >>>> >>>> So I would vote for meaningful names too. Just my 2c. >>>> >>>> >>>> On 16/01/2015, at 01:42 , Yves Senn <yves...@gmail.com> wrote: >>>> >>>> Hey Chris >>>> >>>> Sorry for my late response. >>>> >>>> tl;dr: I'd rather not go for consistency with index naming but for a >>>> consistent hash approach. >>>> >>>> We have descriptive names for indexes because they are shown to the >>>> user in query plans. >>>> This is not the case with foreign keys. When inspecting a database you >>>> can look at the >>>> constraint metadata. I don't see much value in encoding that metadata >>>> into the name. >>>> >>>> In my opinion the drawbacks in the implementation are more sever. We >>>> faced a lot of issues >>>> when automatically renaming indexes. There are still cases where they >>>> fall out of sync >>>> with the actual metadata. Having descriptive names, which no longer >>>> represent the metadata >>>> is way worse in my opinion. >>>> Also constant renaming when renaming tables and columns is not very >>>> practical. The added >>>> issue of the limited size makes this even worse. >>>> >>>> My gut feeling is in favor of constant non-descriptive names. I do >>>> remember that I implemented >>>> that exact behavior but we pivoted to purely random names. I'd like to >>>> go through our archived >>>> discussions regarding that change to make sure we have all the needed >>>> information. >>>> I haven't had the time to do so. >>>> >>>> Cheers, >>>> -- Yves >>>> >>>> On Thursday, January 15, 2015 at 1:25:18 PM UTC+1, ch...@sinjakli.co.uk >>>> wrote: >>>>> >>>>> My own preference is towards the longer, descriptive names. Partly for >>>>> consistency with index naming, and partly because it's nice to be able to >>>>> glance at them and know what they are (though I understand the argument >>>>> for >>>>> them falling out of date). >>>>> >>>>> I'll go ahead and make a PR for that option. If there's serious >>>>> objections to it, hashing is easy enough to add. Thanks for sharing your >>>>> thoughts on this! >>>>> >>>>> Cheers, >>>>> Chris >>>>> >>>>> On Tuesday, January 13, 2015 at 9:19:37 PM UTC, James Coleman wrote: >>>>>> >>>>>> As far as the Postgres limit, that also applies to index names. So I >>>>>> don't see why we need to handle it any different for foreign keys (i.e., >>>>>> with indexes it just means that it fails on creation if the >>>>>> auto-generated >>>>>> one is too long and the dev has to provide a manual name.) >>>>>> >>>>>> On Tue, Jan 13, 2015 at 4:15 PM, Amiel Martin <am...@carnesmedia.com> >>>>>> wrote: >>>>>> >>>>>>> My vote would be to be consistent with the way index names work; the >>>>>>> long human readable version. >>>>>>> >>>>>>> -Amiel >>>>>>> >>>>>>> On Tue, Jan 13, 2015 at 12:31 PM, Ken Collins <k...@actionmoniker. >>>>>>> com> wrote: >>>>>>> >>>>>>>> Though I have not felt the pain, I have been a long time user of >>>>>>>> structure files in legacy environments and think consistent names are a >>>>>>>> good idea. On the flip side, the same could be said for constraint name >>>>>>>> “churn” too. I have seen this in big Oracle teams and unavoidable to my >>>>>>>> knowledge and/or only fixed with a custom script after the rake >>>>>>>> migration >>>>>>>> task. >>>>>>>> >>>>>>>> All that said, I think this is something Rails could solve and the >>>>>>>> PG limit does seem like a big constraint too. >>>>>>>> >>>>>>>> - Cheers, >>>>>>>> Ken >>>>>>>> >>>>>>>> On January 13, 2015 at 11:12:47 AM, ch...@sinjakli.co.uk (ch.. >>>>>>>> .@sinjakli.co.uk) wrote: >>>>>>>> >>>>>>>> I'm really glad to see foreign key support has made its way into >>>>>>>> Rails migrations, but very quickly ran into an issue with it when >>>>>>>> working >>>>>>>> in teams using structure.sql. >>>>>>>> >>>>>>>> Previously, we'd been using the foreigner gem, which generated FK >>>>>>>> names based on the table and column. This meant the name was consistent >>>>>>>> every time you ran the migration. With the version that made it into >>>>>>>> 4.2, part >>>>>>>> of the FK name is random >>>>>>>> <https://github.com/senny/rails/commit/8768305f20d12c40241396092a63e0d56269fefe#diff-a0775e1ec64264dc76a8ee71a5211ae3R697>. >>>>>>>> This means every dev who runs the migration locally generates a >>>>>>>> different >>>>>>>> version of structure.sql, leading to conflicts in version control. >>>>>>>> >>>>>>>> Right now we're working round this by explicitly setting the FK >>>>>>>> name in our migrations. I'd like to find a way of fixing this in Rails >>>>>>>> itself though. >>>>>>>> >>>>>>>> My understanding was the reason for switching away from the old >>>>>>>> format was that the name is arbitrary, and can fall out of date on >>>>>>>> column >>>>>>>> renames. By itself I'm not totally convinced that's enough reason to >>>>>>>> get >>>>>>>> rid of the more descriptive names (though I agree it's debatable). The >>>>>>>> restriction on the length of the name (63 characters in Postgres, for >>>>>>>> example) is a more compelling reason for me. >>>>>>>> >>>>>>>> Either way, I'd like to contribute a patch to make migrations >>>>>>>> generate consistent FK names. I'm happy to implement either of the >>>>>>>> following (or something else if anyone's got a better ideas): >>>>>>>> >>>>>>>> * Switch back to generating human-readable names, accept that >>>>>>>> people with long table/column names will occasionally have to set them >>>>>>>> manually >>>>>>>> * Generate that human-readable name, hash it, and append a >>>>>>>> substring of the hash to "fk_rails_". This would give you a name which >>>>>>>> is >>>>>>>> both short and consistent. >>>>>>>> >>>>>>>> Would really appreciate feedback on those approaches. >>>>>>>> >>>>>>>> Cheers, >>>>>>>> Chris >>>>>>>> -- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "Ruby on Rails: Core" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to rubyonrails-co...@googlegroups.com. >>>>>>>> To post to this group, send email to rubyonra...@googlegroups.com. >>>>>>>> Visit this group at http://groups.google.com/group/rubyonrails-core >>>>>>>> . >>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> You received this message because you are subscribed to the Google >>>>>>>> Groups "Ruby on Rails: Core" group. >>>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>>> send an email to rubyonrails-co...@googlegroups.com. >>>>>>>> To post to this group, send email to rubyonra...@googlegroups.com. >>>>>>>> Visit this group at http://groups.google.com/group/rubyonrails-core >>>>>>>> . >>>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> You received this message because you are subscribed to the Google >>>>>>> Groups "Ruby on Rails: Core" group. >>>>>>> To unsubscribe from this group and stop receiving emails from it, >>>>>>> send an email to rubyonrails-co...@googlegroups.com. >>>>>>> To post to this group, send email to rubyonra...@googlegroups.com. >>>>>>> Visit this group at http://groups.google.com/group/rubyonrails-core. >>>>>>> For more options, visit https://groups.google.com/d/optout. >>>>>>> >>>>>> >>>>>> >>>> -- >>>> You received this message because you are subscribed to the Google >>>> Groups "Ruby on Rails: Core" group. >>>> To unsubscribe from this group and stop receiving emails from it, send >>>> an email to rubyonrails-co...@googlegroups.com. >>>> To post to this group, send email to rubyonra...@googlegroups.com. >>>> Visit this group at http://groups.google.com/group/rubyonrails-core. >>>> For more options, visit https://groups.google.com/d/optout. >>>> >>>> >>>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Ruby on Rails: Core" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to rubyonrails-core+unsubscr...@googlegroups.com. >>> To post to this group, send email to rubyonra...@googlegroups.com. >>> Visit this group at http://groups.google.com/group/rubyonrails-core. >>> For more options, visit https://groups.google.com/d/optout. >>> >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "Ruby on Rails: Core" group. >>> To unsubscribe from this group and stop receiving emails from it, send >>> an email to rubyonrails-co...@googlegroups.com. >>> To post to this group, send email to rubyonra...@googlegroups.com. >>> Visit this group at http://groups.google.com/group/rubyonrails-core. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> -- > You received this message because you are subscribed to the Google Groups > "Ruby on Rails: Core" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to rubyonrails-core+unsubscr...@googlegroups.com. > To post to this group, send email to rubyonrails-core@googlegroups.com. > Visit this group at http://groups.google.com/group/rubyonrails-core. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-core+unsubscr...@googlegroups.com. To post to this group, send email to rubyonrails-core@googlegroups.com. Visit this group at http://groups.google.com/group/rubyonrails-core. For more options, visit https://groups.google.com/d/optout.
