Skip to content

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?

  1. app/views/groups/group_members/index.html.haml calls group_members_list_data_attributes (in a few places).
  2. Groups::GroupMembersHelper#group_members_list_data_attributes calls members_data_json.
  3. Groups::GroupMembersHelper#members_data_json calls MemberSerializer.
  4. MemberSerializer uses the MemberEntity.
  5. MemberEntity exposes can_update and can_remove, which are invoked in the Vue app through the app/assets/javascripts/members/utils.js file.
  6. When can_update? or can_remove? is called, it triggers a condition in GroupMemberPolicy which causes the n+1.

Why does this fix the n+1?

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 by owners qualification.
      • Here is one where I substitutes ids and pluck with SQL versions of it and came closer to the real thing for GitLab, but not exactly since the select used for the in didn't get optimized like the AR version - https://explain.depesz.com/s/q7Op

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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

Edited by Doug Stull

Merge request reports

Loading