Resolve group_member policy n+1
What does this MR do?
Improves the overall query count and timing when loading Groups::GroupMembersController#index
as part of effort to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/21033
- Resolves n+1 query issues on checking GroupMember policy when using the MemberSerializer.
- There are more n+1's that can and will be resolved through #21033. We'll focus on just this one for now.
- Comparing !58668 (0aee052e) to the current state shows the vastly different approach I first attempted.
What did this do before?
-
app/views/groups/group_members/index.html.haml
calls group_members_list_data_attributes (in a few places). -
Groups::GroupMembersHelper#group_members_list_data_attributes
calls members_data_json. -
Groups::GroupMembersHelper#members_data_json
calls MemberSerializer. -
MemberSerializer
uses the MemberEntity. -
MemberEntity
exposes can_update and can_remove, which are invoked in the Vue app through theapp/assets/javascripts/members/utils.js
file. - When
can_update?
orcan_remove?
is called, it triggers a condition in GroupMemberPolicy which causes then+1
.
n+1
?
Why does this fix the As part of the modification to Group#member_last_owner?
and Group#member_last_blocked_owner?
we allow the methods to return early if the virtual attributes that we set on the member have been set. This avoids the n+1
query while still allowing our original functionality to remain.
Performance analysis locally on GDK
Caveat This was done on a group where there were a handful of members(9), some of them being invited(45). The numbers and query times will vary based upon size.
Before(after gdk restart and cache warm-up)
- time:
~280ms
- queries(cached/uncached):
217
After(after gdk restart and cache warm-up)
- time:
~203ms
- queries(cached/uncached):
138
Queries
-
members_with_parents.owners.where(user_id: member.pluck(:user_id)).ids
- https://explain.depesz.com/s/qvmM- used GitLab group id in database-lab for numbers below.
- trivial
in
clause used here, members will be larger on big groups, but will be trimmed byowners
qualification.- Here is one where I substitutes
ids
andpluck
with SQL versions of it and came closer to the real thing for GitLab, but not exactly since theselect
used for thein
didn't get optimized like the AR version - https://explain.depesz.com/s/q7Op
- Here is one where I substitutes
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because developer-facing change.
-
-
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
Related to #21033