Optimize issuable updates
What does this MR do?
- Reduce N+1 queries when bulk-updating
- Avoid looking up blank milestones in the hierarchy
- Use
.size
to avoid a redundant.count
query
Related to #21068
Query count differences in spec/services/issuable/bulk_update_service_spec.rb
--- before.txt 2021-04-08 16:29:44.209679331 +0200
+++ after.txt 2021-04-08 16:31:39.262232001 +0200
@@ -5,113 +5,113 @@
Issuable::BulkUpdateService
with issuables at a project level
with unpermitted attributes
-DB queries: 33
+DB queries: 25
does not update the issues
close issues
-DB queries: 102
+DB queries: 81
succeeds and returns the correct number of issues updated
-DB queries: 102
+DB queries: 81
closes all the issues passed
reopen issues
-DB queries: 86
+DB queries: 65
succeeds and returns the correct number of issues updated
-DB queries: 86
+DB queries: 65
reopens all the issues passed
updating merge request assignee
when the new assignee ID is a valid user
-DB queries: 108
+DB queries: 110
succeeds
-DB queries: 107
+DB queries: 109
updates the assignee to the user ID passed
when the new assignee ID is 0
-DB queries: 73
+DB queries: 75
unassigns the issues
when the new assignee ID is not present
-DB queries: 24
+DB queries: 26
does not unassign
updating issue assignee
when the new assignee ID is a valid user
-DB queries: 78
+DB queries: 72
succeeds
-DB queries: 78
+DB queries: 72
updates the assignee to the user ID passed
when the new assignee ID is 0
-DB queries: 53
+DB queries: 47
unassigns the issues
when the new assignee ID is not present
-DB queries: 17
+DB queries: 19
does not unassign
updating milestones
behaves like updates milestones
-DB queries: 42
+DB queries: 37
succeeds
-DB queries: 42
+DB queries: 37
updates the issuables milestone
updating labels
behaves like updating labels
when add_label_ids are passed
-DB queries: 114
+DB queries: 85
adds those label IDs to all issues passed
-DB queries: 114
+DB queries: 85
does not update issues not passed in
when remove_label_ids are passed
-DB queries: 106
+DB queries: 78
removes those label IDs from all issues passed
-DB queries: 106
+DB queries: 78
does not update issues not passed in
when add_label_ids and remove_label_ids are passed
-DB queries: 134
+DB queries: 103
adds the label IDs to all issues passed
-DB queries: 134
+DB queries: 103
removes the label IDs from all issues passed
-DB queries: 134
+DB queries: 103
does not update issues not passed in
subscribe to issues
-DB queries: 47
+DB queries: 39
subscribes the given user
unsubscribe from issues
-DB queries: 47
+DB queries: 39
unsubscribes the given user
updating issues from external project
-DB queries: 77
+DB queries: 70
updates only issues that belong to the parent project
with issuables at a group level
updating milestones
when issues
behaves like updates milestones
-DB queries: 99
+DB queries: 78
succeeds
-DB queries: 99
+DB queries: 78
updates the issuables milestone
when merge requests
behaves like updates milestones
-DB queries: 135
+DB queries: 118
succeeds
-DB queries: 135
+DB queries: 118
updates the issuables milestone
updating labels
behaves like updating labels
when add_label_ids are passed
-DB queries: 130
+DB queries: 101
adds those label IDs to all issues passed
-DB queries: 130
+DB queries: 101
does not update issues not passed in
when remove_label_ids are passed
-DB queries: 122
+DB queries: 98
removes those label IDs from all issues passed
-DB queries: 122
+DB queries: 98
does not update issues not passed in
when add_label_ids and remove_label_ids are passed
-DB queries: 152
+DB queries: 121
adds the label IDs to all issues passed
-DB queries: 152
+DB queries: 121
removes the label IDs from all issues passed
-DB queries: 152
+DB queries: 121
does not update issues not passed in
with issues from external group
-DB queries: 103
+DB queries: 87
updates issues that belong to the parent group or descendants
-Finished in 46.89 seconds (files took 9.15 seconds to load)
+Finished in 42.11 seconds (files took 15.99 seconds to load)
37 examples, 0 failures
Query count differences in ee/app/services/ee/issuable/bulk_update_service.rb
--- before_ee.txt 2021-04-12 18:28:25.602398912 +0200
+++ after_ee.txt 2021-04-12 18:31:09.266918261 +0200
@@ -1,57 +1,57 @@
Run options: include {:focus=>true}
All examples were filtered out; ignoring {:focus=>true}
Issuable::BulkUpdateService
with issues
updating health status and epic
when features are enabled
DB queries: 160
succeeds and returns the correct number of issuables updated
when params value is '0'
DB queries: 80
succeeds and remove values
when epic param is incorrect
returns error
when feature issuable_health_status is disabled
behaves like does not update issuables attribute
does not update attribute
when user can not update issues
behaves like does not update issuables attribute
does not update attribute
behaves like does not update issuables attribute
does not update attribute
when user can not admin epic
behaves like does not update issuables attribute
does not update attribute
updating iterations
at group level
when issues
behaves like updates iterations
DB queries: 58
succeeds
DB queries: 59
updates the issuables iteration
at project level
behaves like updates iterations
DB queries: 35
succeeds
DB queries: 36
updates the issuables iteration
with epics
updating labels
when epics are enabled
behaves like updates issuables attribute
-DB queries: 59
+DB queries: 58
succeeds and returns the correct number of issuables updated
when epics are disabled
behaves like does not update issuables attribute
-DB queries: 7
+DB queries: 9
does not update attribute
when issuable_ids contain external epics
-DB queries: 59
+DB queries: 58
updates epics that belong to the parent group or descendants
-Finished in 18.77 seconds (files took 2.73 seconds to load)
+Finished in 18.16 seconds (files took 7.29 seconds to load)
14 examples, 0 failures
Debugging code to generate the above
diff --git i/app/services/issuable/bulk_update_service.rb w/app/services/issuable/bulk_update_service.rb
index 13e289716ef..9e226235bf8 100644
--- i/app/services/issuable/bulk_update_service.rb
+++ w/app/services/issuable/bulk_update_service.rb
@@ -13,7 +13,13 @@ def initialize(parent, user = nil, params = {})
def execute(type)
ids = params.delete(:issuable_ids).split(",")
set_update_params(type)
- items = update_issuables(type, ids)
+
+ items = nil
+ queries = ActiveRecord::QueryRecorder.new do
+ items = update_issuables(type, ids)
+ end
+
+ $stdout.puts "DB queries: #{queries.count}"
response_success(payload: { count: items.size })
rescue ArgumentError => e
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry.
-
- [-] Documentation (if required)
-
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides - [-] Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Markus Koller