GroupsController#show suffers from an N+1 query problem related to epics and plans
When viewing a group with sub-groups (e.g. https://gitlab.com/gitlab-org), we will execute a query like this for every group displayed:
SELECT "plans".* FROM "plans" WHERE "plans"."id" IN (WITH RECURSIVE "base_and_ancestors" AS (SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."type" IN ('Group') AND "namespaces"."id" = 1920469
UNION
SELECT "namespaces".* FROM "namespaces", "base_and_ancestors" WHERE "namespaces"."type" IN ('Group') AND "namespaces"."id" = "base_and_ancestors"."parent_id") SELECT "plan_id" FROM "base_and_ancestors" AS "namespaces" WHERE ("namespaces"."plan_id" IS NOT NULL))
This query will get the plans for the group in question, and all of its ancestors. This query is then used to determine if the :epics
feature is enabled for the group. Doing this produces the following stacktrace:
ee/app/models/ee/namespace.rb:87:in `block (2 levels) in feature_available_in_plan?'
ee/app/models/ee/namespace.rb:91:in `feature_available_in_plan?'
ee/app/models/ee/namespace.rb:178:in `load_feature_available'
ee/app/models/ee/namespace.rb:75:in `block (2 levels) in feature_available?'
ee/app/models/ee/namespace.rb:79:in `feature_available?'
ee/app/policies/ee/group_policy.rb:8:in `block (2 levels) in <module:GroupPolicy>'
lib/declarative_policy/condition.rb:21:in `instance_eval'
lib/declarative_policy/condition.rb:21:in `compute'
lib/declarative_policy/condition.rb:42:in `block in pass?'
lib/declarative_policy/base.rb:280:in `cache'
lib/declarative_policy/condition.rb:42:in `pass?'
lib/declarative_policy/rule.rb:79:in `pass?'
lib/declarative_policy/step.rb:79:in `pass?'
lib/declarative_policy/runner.rb:98:in `block in run'
lib/declarative_policy/runner.rb:177:in `block in steps_by_score'
lib/declarative_policy/runner.rb:146:in `loop'
lib/declarative_policy/runner.rb:146:in `steps_by_score'
lib/declarative_policy/runner.rb:79:in `run'
lib/declarative_policy/runner.rb:57:in `pass?'
lib/declarative_policy/base.rb:215:in `block in allowed?'
lib/declarative_policy/base.rb:215:in `each'
lib/declarative_policy/base.rb:215:in `all?'
lib/declarative_policy/base.rb:215:in `allowed?'
lib/declarative_policy/base.rb:207:in `can?'
app/models/ability.rb:70:in `allowed?'
ee/app/finders/epics_finder.rb:56:in `block (2 levels) in groups_user_can_read_epics'
ee/app/finders/epics_finder.rb:56:in `block in groups_user_can_read_epics'
lib/declarative_policy/preferred_scope.rb:7:in `with_preferred_scope'
lib/declarative_policy/preferred_scope.rb:17:in `user_scope'
ee/app/finders/epics_finder.rb:55:in `groups_user_can_read_epics'
ee/app/finders/epics_finder.rb:47:in `init_collection'
ee/app/finders/epics_finder.rb:9:in `execute'
The entry point is as follows:
def groups_user_can_read_epics(groups)
DeclarativePolicy.user_scope do
groups.select { |g| Ability.allowed?(current_user, :read_epic, g) }
end
end
To reproduce this you must enable the check_namespace_plan
setting:
ApplicationSetting.first.update(check_namespace_plan: true)
Unfortunately, solving this problem will be quite hard. The "plans" query is not a regular query produced by an ActiveRecord association, so we can't eager load it. We could get all the groups, then get all their ancestors, but this would require us to then manually associate these ancestors with the proper groups. Basically in this setup you'd do the following:
- Get all the group IDs we display
- Run a query producing one row with two columns for every group: a column containing the group ID, and a column containing all parent IDs
- Grab all the unique parent IDs, merge these with the group IDs, then get all the plans belonging to this set
- Using the result returned by step 2 we can then figure out which groups have the
:epics
feature enabled
This however is hard to do, as the above query is triggered deep down in our policy code. This code in turn can only deal with one group at a time.
Because of all this we currently spend about 1/4th of the SQL time running the above queries. With the improvements made in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18973 we'll probably end up spending half the time in just the above queries, maybe even more.