Fix N+1 in `MergeRequest#merge_request_diff_for` (!17908) · Merge requests · GitLab.org / GitLab FOSS

Sean McGivern requested to merge reduce-query-count-for-mergerequestscontroller-show into master Mar 21, 2018

What does this MR do?

Addresses the first part of https://gitlab.com/gitlab-org/gitlab-ce/issues/43114#note_63661914.

Are there points in the code the reviewer needs to double check?

I don't think so, but I'll go into a bit more detail about the change: previously, we did it this way because loading a record from merge_request_diffs could be expensive (as these had very large serialised columns for diffs and commits). Now they don't, so loading all MR diffs at once is actually pretty reasonable IMHO.

Why was this MR needed?

There are too many queries run for large MRs. I ran this for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15695. Before:

I, [2018-03-21T16:03:28.554680 #112001]  INFO -- : MergeRequestDiff total (17): 70.8ms
I, [2018-03-21T16:03:28.554821 #112001]  INFO -- : Upload total (11): 38.3ms
I, [2018-03-21T16:03:28.554860 #112001]  INFO -- : Build total (10): 36.0ms
I, [2018-03-21T16:03:28.554893 #112001]  INFO -- : User total (14): 31.3ms
I, [2018-03-21T16:03:28.554943 #112001]  INFO -- : Project total (8): 22.9ms
I, [2018-03-21T16:03:28.555058 #112001]  INFO -- : Group total (7): 21.6ms
I, [2018-03-21T16:03:28.555242 #112001]  INFO -- : Note total (1): 21.2ms
I, [2018-03-21T16:03:28.561281 #112001]  INFO -- : Namespace total (4): 18.8ms
I, [2018-03-21T16:03:28.561340 #112001]  INFO -- : GroupMember total (4): 17.8ms
I, [2018-03-21T16:03:28.561436 #112001]  INFO -- : SystemNoteMetadata total (1): 12.8ms
I, [2018-03-21T16:03:28.561523 #112001]  INFO -- : JobArtifact total (4): 12.5ms
I, [2018-03-21T16:03:28.561597 #112001]  INFO -- : MergeRequest total (4): 11.4ms
I, [2018-03-21T16:03:28.561674 #112001]  INFO -- : ProjectFeature total (4): 11.1ms
I, [2018-03-21T16:03:28.561746 #112001]  INFO -- : AwardEmoji total (2): 10.8ms
I, [2018-03-21T16:03:28.561825 #112001]  INFO -- : Issue total (2): 9.5ms
I, [2018-03-21T16:03:28.561932 #112001]  INFO -- : ApproverGroup total (2): 8.8ms
I, [2018-03-21T16:03:28.561977 #112001]  INFO -- : Approver total (2): 8.8ms
I, [2018-03-21T16:03:28.562064 #112001]  INFO -- : GitlabIssueTrackerService total (1): 8.7ms
I, [2018-03-21T16:03:28.562168 #112001]  INFO -- : Pipeline total (4): 8.2ms
I, [2018-03-21T16:03:28.562242 #112001]  INFO -- : Route total (4): 6.9ms
I, [2018-03-21T16:03:28.562353 #112001]  INFO -- : CommitStatus total (1): 5.2ms
I, [2018-03-21T16:03:28.562464 #112001]  INFO -- : ProtectedBranch total (3): 5.2ms
I, [2018-03-21T16:03:28.562537 #112001]  INFO -- : ProjectMember total (2): 3.9ms
I, [2018-03-21T16:03:28.562607 #112001]  INFO -- : Label total (1): 2.7ms
I, [2018-03-21T16:03:28.562678 #112001]  INFO -- : MergeRequestDiffCommit total (1): 2.6ms
I, [2018-03-21T16:03:28.562749 #112001]  INFO -- : IssueAssignee total (1): 2.3ms
I, [2018-03-21T16:03:28.562833 #112001]  INFO -- : ForkNetwork total (1): 2.2ms
I, [2018-03-21T16:03:28.562909 #112001]  INFO -- : MergeAccessLevel total (1): 2.1ms
I, [2018-03-21T16:03:28.562978 #112001]  INFO -- : Milestone total (1): 1.9ms
I, [2018-03-21T16:03:28.563048 #112001]  INFO -- : ForkedProjectLink total (1): 1.9ms
I, [2018-03-21T16:03:28.563142 #112001]  INFO -- : License total (1): 1.9ms
I, [2018-03-21T16:03:28.563217 #112001]  INFO -- : Plan total (1): 1.6ms

After:

I, [2018-03-21T16:08:11.193812 #112001]  INFO -- : User total (14): 49.2ms
I, [2018-03-21T16:08:11.194044 #112001]  INFO -- : Build total (10): 42.8ms
I, [2018-03-21T16:08:11.194102 #112001]  INFO -- : Upload total (11): 35.2ms
I, [2018-03-21T16:08:11.194137 #112001]  INFO -- : Project total (8): 22.3ms
I, [2018-03-21T16:08:11.194184 #112001]  INFO -- : Pipeline total (4): 19.9ms
I, [2018-03-21T16:08:11.194210 #112001]  INFO -- : Note total (1): 18.6ms
I, [2018-03-21T16:08:11.194230 #112001]  INFO -- : Group total (7): 17.9ms
I, [2018-03-21T16:08:11.194260 #112001]  INFO -- : ProjectFeature total (4): 16.2ms
I, [2018-03-21T16:08:11.194291 #112001]  INFO -- : GroupMember total (4): 13.1ms
I, [2018-03-21T16:08:11.194321 #112001]  INFO -- : Route total (4): 11.2ms
I, [2018-03-21T16:08:11.194353 #112001]  INFO -- : Namespace total (4): 10.4ms
I, [2018-03-21T16:08:11.194383 #112001]  INFO -- : Issue total (2): 9.1ms
I, [2018-03-21T16:08:11.194414 #112001]  INFO -- : ProtectedBranch total (3): 8.7ms
I, [2018-03-21T16:08:11.194462 #112001]  INFO -- : MergeRequest total (4): 8.1ms
I, [2018-03-21T16:08:11.194527 #112001]  INFO -- : ProjectMember total (2): 8.1ms
I, [2018-03-21T16:08:11.194558 #112001]  INFO -- : Approver total (2): 7.8ms
I, [2018-03-21T16:08:11.194581 #112001]  INFO -- : CommitStatus total (1): 7.1ms
I, [2018-03-21T16:08:11.194600 #112001]  INFO -- : MergeAccessLevel total (1): 6.6ms
I, [2018-03-21T16:08:11.194620 #112001]  INFO -- : Milestone total (1): 6.3ms
I, [2018-03-21T16:08:11.194639 #112001]  INFO -- : IssueAssignee total (1): 5.9ms
I, [2018-03-21T16:08:11.194658 #112001]  INFO -- : JobArtifact total (4): 5.8ms
I, [2018-03-21T16:08:11.194679 #112001]  INFO -- : AwardEmoji total (2): 5.7ms
I, [2018-03-21T16:08:11.194888 #112001]  INFO -- : SystemNoteMetadata total (1): 4.7ms
I, [2018-03-21T16:08:11.194919 #112001]  INFO -- : MergeRequestDiff total (2): 4.1ms
I, [2018-03-21T16:08:11.194941 #112001]  INFO -- : MergeRequestDiffCommit total (1): 3.4ms
I, [2018-03-21T16:08:11.194971 #112001]  INFO -- : ApproverGroup total (2): 2.7ms
I, [2018-03-21T16:08:11.195002 #112001]  INFO -- : Label total (1): 1.9ms
I, [2018-03-21T16:08:11.195034 #112001]  INFO -- : ForkNetwork total (1): 1.8ms
I, [2018-03-21T16:08:11.195067 #112001]  INFO -- : GitlabIssueTrackerService total (1): 1.5ms
I, [2018-03-21T16:08:11.195095 #112001]  INFO -- : ForkedProjectLink total (1): 1.4ms
I, [2018-03-21T16:08:11.195114 #112001]  INFO -- : Plan total (1): 1.3ms
I, [2018-03-21T16:08:11.195156 #112001]  INFO -- : License total (1): 1.1ms

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

Changelog entry added, if necessary
Tests added for this feature/bug
Review
- Has been reviewed by Backend
Conform by the merge request performance guides
Conform by the style guides
Squashed related commits together

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/43114 (doesn't close it, though).

Fix N+1 in `MergeRequest#merge_request_diff_for`