Preload routes information
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.
This commit is contained in:
parent
f15e3efba0
commit
53801b1206
4 changed files with 19 additions and 3 deletions
|
@ -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(
|
||||
|
|
|
@ -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
|
||||
|
|
5
changelogs/unreleased/ab-routable-nplus1.yml
Normal file
5
changelogs/unreleased/ab-routable-nplus1.yml
Normal file
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Preload routes information to fix N+1 issue
|
||||
merge_request: 32352
|
||||
author:
|
||||
type: performance
|
|
@ -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
|
||||
|
|
Loading…
Reference in a new issue