do not load all child records for inverse case #10566
Conversation
| man = Man.create! | ||
| interest = Interest.create!(man: man) | ||
|
|
||
| man.interests.find(interest.id) |
There was a problem hiding this comment.
this line should verify the specific result returned, to ensure that even though other interests aren't loaded, your specific query still works correctly
There was a problem hiding this comment.
|
Happy to merge once the above is dealt with. |
|
@jonleighton your feedback has been taken into account and the unnecessary line has been removed. There is already another test confirming that inverse is being set on the records. |
|
Could you rebase? Thanks |
currently `post.comments.find(Comment.first.id)` would load all comments for the given post to set the inverse association. This has a huge performance penalty. Because if post has 100k records and all these 100k records would be loaded in memory even though the comment id was supplied. Fix is to use in-memory records only if loaded? is true. Otherwise load the records using full sql. Fixes #10509
|
@jonleighton rebased. |
do not load all child records for inverse case
|
Broke the build for mysql and postgresql. https://travis-ci.org/rails/rails/jobs/8224880#L119 thanks |
|
When I run full stack test The failing test is I think it should be ok to reduce the invalid_id number from such a larger number to a bit smaller number since the intent is to test |
|
@arunagw thanks for the heads up @neerajdotname I've reverted for now until we can figure out the problems. |
PR rails#10566 had to be reverted because after applying the fix test "test_raise_record_not_found_error_when_invalid_ids_are_passed" started failing. In this test invalid_id is being assigned a really large number which was causing following failure when PR rails#10566 was applied. ``` RangeError: bignum too big to convert into `long long' SELECT `interests`.* FROM `interests` WHERE `interests`.`man_id` = ? AND `interests`.`id` = ? LIMIT 1 [["man_id", 970345987], ["id", 2394823094892348920348523452345]] ``` This test is not failing in master because when test code `man.interests.find(invalid_id)` is executed then interests are fully loaded in memory and no database query is executed. After PR rails#10566 was merged then test code `man.interests.find(invalid_id)` started executing sql query and hence the error. In case someone is wondering why the second part of query is not failing, then that's because the actual query does not require any variable substituation where the number is large. In that case the sql generate is following. ``` SELECT `interests`.* FROM `interests` WHERE `interests`.`man_id` = ? AND `interests`.`id` IN (8432342, 2390102913, 2453245234523452) [["man_id", 970345987]] ```
currently
post.comments.find(Comment.first.id)would load allcomments for the given post to set the inverse association.
This has a huge performance penalty. Because if post has 100k
records and all these 100k records would be loaded in memory
even though the comment id was supplied.
Fix is to use in-memory records only if loaded? is true. Otherwise
load the records using full sql.
Fixes #10509
/cc @wangjohn @jeremy @jonleighton