diff options
author | Andreas Brandl <abrandl@gitlab.com> | 2019-08-27 19:15:28 +0200 |
---|---|---|
committer | Andreas Brandl <abrandl@gitlab.com> | 2019-09-03 12:16:03 +0200 |
commit | 53801b1206ff9e165b30117e3c3bcf543db7ad2c (patch) | |
tree | 724b80019bb368f6f7e270209a612e32d578c756 | |
parent | f15e3efba0e0523575ce61bd80396abda94b8cec (diff) | |
download | gitlab-ce-ab-routable-nplus1.tar.gz |
Preload routes informationab-routable-nplus1
This fixes a high frequency N+1 issue:
`RoutableActions#find_routable!` is used across many controllers to
retrieve e.g. the Project or Namespace by path. The `#find_routable!`
method calls `#ensure_canonical_path` which in turn retrieves
`#full_path` from the given Routable.
This in turn triggers a lookup on `routes`, leading to a high frequency
of these queries:
```sql
SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = $1 AND
"routes"."source_type" = $2 LIMIT $3
```
This is unnecessary as we already join `routes` in
`Routable#find_by_full_path` anyways.
-rw-r--r-- | app/controllers/admin/groups_controller.rb | 6 | ||||
-rw-r--r-- | app/models/concerns/routable.rb | 4 | ||||
-rw-r--r-- | changelogs/unreleased/ab-routable-nplus1.yml | 5 | ||||
-rw-r--r-- | spec/models/concerns/routable_spec.rb | 7 |
4 files changed, 19 insertions, 3 deletions
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 6317fa7c8d1..32a36da56fe 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -14,7 +14,11 @@ class Admin::GroupsController < Admin::ApplicationController # rubocop: disable CodeReuse/ActiveRecord def show - @group = Group.with_statistics.joins(:route).group('routes.path').find_by_full_path(params[:id]) + # Group.with_statistics doesn't behave nicely when including other relations. + # Group.find_by_full_path includes the routes relation to avoid a common N+1 + # (at the expense of this action: there are two queries here to find and retrieve + # the Group with statistics). + @group = Group.with_statistics.find(group&.id) @members = present_members( @group.members.order("access_level DESC").page(params[:members_page])) @requesters = present_members( diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 3a486632800..07d22641a0a 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -38,7 +38,7 @@ module Routable if Feature.enabled?(:routable_two_step_lookup) # Case sensitive match first (it's cheaper and the usual case) # If we didn't have an exact match, we perform a case insensitive search - found = joins(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take + found = includes(:route).find_by(routes: { path: path }) || where_full_path_in([path]).take else order_sql = Arel.sql("(CASE WHEN routes.path = #{connection.quote(path)} THEN 0 ELSE 1 END)") found = where_full_path_in([path]).reorder(order_sql).take @@ -67,7 +67,7 @@ module Routable "(LOWER(routes.path) = LOWER(#{connection.quote(path)}))" end - joins(:route).where(wheres.join(' OR ')) + includes(:route).where(wheres.join(' OR ')).references(:routes) end # Temporary instrumentation of method calls diff --git a/changelogs/unreleased/ab-routable-nplus1.yml b/changelogs/unreleased/ab-routable-nplus1.yml new file mode 100644 index 00000000000..7b59bc78dae --- /dev/null +++ b/changelogs/unreleased/ab-routable-nplus1.yml @@ -0,0 +1,5 @@ +--- +title: Preload routes information to fix N+1 issue +merge_request: 32352 +author: +type: performance diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index cff86afe768..cad705ee594 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -66,6 +66,13 @@ describe Group, 'Routable' do it { expect(described_class.find_by_full_path(group.to_param.upcase)).to eq(group) } it { expect(described_class.find_by_full_path(nested_group.to_param)).to eq(nested_group) } it { expect(described_class.find_by_full_path('unknown')).to eq(nil) } + + it 'includes route information when loading a record' do + path = group.to_param + control_count = ActiveRecord::QueryRecorder.new { described_class.find_by_full_path(path) }.count + + expect { described_class.find_by_full_path(path).route }.not_to exceed_all_query_limit(control_count) + end end context 'with redirect routes' do |