Rails N+1問題で大量のSQLが発行されます。

Question

前提・実現したいこと

独学でRailsを学び始めて２ヶ月目の初学者です。
半日試行錯誤したものの、どうしても分からず初投稿します。
現在、Ruby on Railsでお酒と料理の組み合わせ（ペアリング）を投稿・評価するアプリを作っています。

環境：Ruby 2.4.1/ Rails 5.2.1

発生している問題

VIEW（トップページ）に「お酒の名前」「料理名」「ユーザーの評価の平均値（星マーク）」をeach処理で表示しているのですが、ページの読み込みにかなりの時間がかかっています。

ログを確認すると大量のSQLクエリが発行されており、いろいろと調べた結果、N+１問題が発生していると判明しました。大量発生しているSQL文は、Pairモデルに記述しているevaluetion_average内で、全ユーザーの評価の平均を算出する為に使っているsumとcountでした。

いろいろと検索して調べた結果、preloadやeager_loadを使えばいいというところまでは分かりましたが（合っているか自信がありませんが）、解説をしてくれているサイトの使用例と違い、自分の場合はActiveRecordが入れ子になっているわけでもなく、どこに挟めばいいのかがわからずにいます。

そもそもの問題は、そもそもモデルの関連付けをうまく利用できていないからにも思えますが・・・
どこを改善すればN+１問題が解決するか、ご教授いただければと思います。

該当のソースコード

▼model
【sake.rb】 カラム: :name, :kind

class Sake < ApplicationRecord
  has_many :pairs
  has_many :foods, through: :pairs
end

【food.rb】 カラム: :name

class Food < ApplicationRecord
  has_many :pairs
  has_many :sakes, through: :pairs
end

【pair.rb】 カラム: :sake_id, :food_id, :user_id

class Pair < ApplicationRecord
  belongs_to :sake
  belongs_to :food
  belongs_to :user, optional: true
  has_many :evaluations

  # ここのメソッド内のActiveRecordが大量のSQLを生み出していました。
  def evaluation_average
    begin
      average = Evaluation.where(pair_id: self.id).sum(:point) / Evaluation.where(pair_id: self.id).count.to_f
      return average
    rescue
      average = 0
    end
  end
  
end

【evaluation.rb】 カラム: :sake_id, :food_id, :user_id

class Evaluation < ApplicationRecord
  belongs_to :user
  belongs_to :pair
end

▼Controller

class PairsController < ApplicationController
  
    def index
      @pairs = Pair.all.includes(:sake, :food)
    end
    〜省略〜
end

▼View

<% @pairs.each do |pair| %>
  <p><%=link_to "#{pair.sake.name} (#{pair.sake.kind}) × #{pair.food.name}", pair_path(pair) %></p>
  <span style="--rate: <%= pair.evaluation_average*20.0 %>%;" class="evaluation-star"></span>
  <span class="evaluation-number"><%= pair.evaluation_average.to_f.round(1) %></span>
<% end %>

お手数ですが、
ご回答のほど、よろしくお願い致します。

Answer 1

BigeastTtexanさんの回答を基本的に踏襲しつつ、
特定のカラムを新たに追加せずN+1クエリー問題を解決する方法について以下はどうでしょうか。

---

average = self.evaluations.sum(:point) / self.evaluations.count.to_f
の箇所を

total_point = self.evaluations.inject(0){|sum, add| sum + add.point}
number_of_people = self.evaluations.inject(0){|sum| sum + 1}.to_f
average = total_point / number_of_people

---

injectメソッドを使えば新たにSQLを発行することなく、レシーバの数や合計値を計算できるので、
N+1クエリー問題は回避できると思います。

Comments

[TakashiOda]

trochさん

毎度のご提案、ありがとうございます！独学なので、このようにお相手してくださり非常に心強く思います。

さて、ご提案していただいた方法を試してみたのですが、
SQLの発行回数はインスタンスの数分だけ発行されてしまいます。
私の伝え方が悪いのかもしれません（汗）

ちなみに発行されるSQLはこちらです。

(1) SELECT "pairs".* FROM "pairs"

(2) SELECT "sakes".* FROM "sakes" WHERE "sakes"."id" IN (?, ?, ?) [["id", 1], ["id", 2], ["id", 3]]

(3)　SELECT "foods".* FROM "foods" WHERE "foods"."id" IN (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) [["id", 1], ["id", 2], ["id", 3], ["id", 4], ["id", 5], ["id", 6], ["id", 7], ["id", 8], ["id", 9], ["id", 10], ["id", 11], ["id", 12], ["id", 13], ["id", 14], ["id", 15], ["id", 16], ["id", 17], ["id", 18], ["id", 19], ["id", 20], ["id", 21], ["id", 22], ["id", 23], ["id", 24], ["id", 25], ["id", 26], ["id", 27], ["id", 28], ["id", 29], ["id", 30], ["id", 31], ["id", 32], ["id", 33], ["id", 34], ["id", 35], ["id", 36], ["id", 37]]

(4)　SELECT "evaluations".* FROM "evaluations" WHERE "evaluations"."pair_id" = ? [["pair_id", 1]]
　　　〜（途中省略）〜
　　 SELECT "evaluations".* FROM "evaluations" WHERE "evaluations"."pair_id" = ? [["pair_id", 101]]

SQL(1) ~ (3)はそれぞれ１回づつですが、SQL (4)の部分が1~101の102回（インスタンスの数だけ）呼ばれています。

初心者ながらこの問題の根本を考えてみたのですが、
eachなどの繰り返し処理の中で、SQL文が発行されてしまうメソッドを呼び出すこと自体がN+1問題を生み出しているような気がしてならないのですが、この考えは間違ってますか？それとも問題はあくまで他の部分にあるのでしょうか？

[troch]

> eachなどの繰り返し処理の中で、SQL文が発行されてしまうメソッドを呼び出すこと自体がN+1問題を生み出しているような気がしてならないのですが、この考えは間違ってますか？

私もTakashiOdaさんと同じ考え方です。
なのでSELECT "evaluations".* …というSQLが大量に発行されることに困惑してます；
includesメソッドで事前にevaluationsテーブルを読み込んでおけばそれは解決される認識でしたので…

PairsControllerのindexアクションを以下のように書き換えてみてもN+1クエリー問題は発生するでしょうか？？
------------------------------------------------------
def index
@pairs = Pair.all.includes(:sake, :food, :evaluations)
end
------------------------------------------------------
宜しくお願い致します。

[TakashiOda]

ご指摘ありがとうございます！
上記の内容で書き換えたところ、うまくいきました！
根本的なところを飛ばしてしまっていました。。。
本当にありがとうございました！

[TakashiOda]

trochさん

可能であれば一つお伺いしたいのですが、
こういう問題にぶち当たった時、私が思いついた応急処置「評価の平均値カラムをPairsテーブルに追加する」という解決方法はやはりよろしく無いのでしょうか？
実際、新たなカラムを追加したことで、コントローラ の処理も増えました。

しかし結果として速度だけを見ると、カラムを追加した方が読み込み速度が早かったのですが、やはり出来るだけテーブル、コントローラの処理はシンプルに保つ方が大事、ということなのでしょうか？

[troch]

TakashiOdaさん

お疲れ様です。
ひとまずN+1クエリー問題が解決してよかったです。

> こういう問題にぶち当たった時、私が思いついた応急処置「評価の平均値カラムをPairsテーブルに追加する」という解決方法はやはりよろしく無いのでしょうか？

私もTakashiOdaさんと同じくRailsの初学者です。
まだまだ未熟だからこそ、自分が躓いた所やわからなかった箇所を同じ初心者に対して教えられればと思って、teratailで回答したりしています。

モデル設計については私も勉強中です。
https://qiita.com/kikunantoka/items/61ab74c7f50dedace211
↑の記事によると、Railsのモデル設計で多いのは、一つのテーブルに6つのカラムをもたせる、というものだったので、
余りカラム数を増やさない方が良いのかと思い、
今回、インスタンスメソッド側で対処する方法を考えてみましたが…
TakashiOdaさんのおっしゃる通り、
Viewで出力する際の計算量が増えるのでデータの読み込み速度は下がるとは思います；

どのようなモデル設計が良いのかにつきましては、
別途質問を立てて有識者からの回答を募るか、
MENTAのようなサービスでプロのエンジニアのご指導を受ける、
等をした方が正しい知識が得られると思います。

余りお力になれず申し訳ないですが、
頑張ってください！

[TakashiOda]

trochさん
ご丁寧にありがとうございます！
教えていただいた記事を参考にしてみたいと思います！
モデル設計、正解が無いので、悩みますよね。。

またよろしくお願い致します！

Answer 2

【自己解決】
VIEW側のeach処理の中で、毎回平均値を計算させるようなモデルからメソッドを呼び出していること自体をやめないと、N+1問題は解決しないと考え、クエリの発行回数にのみ焦点を絞り、解決策を練りました。

結論から先に言うと　pairクラスの中でメソッドを定義して呼び出すことをやめました。そしてpairsテーブルの中に評価の平均値カラムを作り、Evaluationインスタンスがcreateされる旅に、評価の平均値が格納（再代入）されるようにしました。（スマートなやり方ではないのだろうとは自覚しておりますが・・・）

⑴pairsテーブルに average_evaluation というカラム（データ型:float）を追加。

⑵Pairの子クラスであるEvaluationクラスのインスタンスがcreate/updateされるたびに（ペアリングの評価が投稿・修正される度に）、平均値を算出し、Pairsテーブルに格納する。

class EvaluationsController < ApplicationController
（途中省略）
def create
    @pair = Pair.find(params[:pair_id])
    @evaluation = Evaluation.new(evaluation_params)
    @evaluation.user_id = current_user.id
    @evaluation.pair_id = @pair.id
　　# 以下の１行を追加
    @pair.average_evaluation = Evaluation.where(pair_id: @pair.id).average(:point) 
    @pair.save!
    @evaluation.save
    redirect_to pair_evaluation_path(@pair, @evaluation)
  end
（途中省略）
end

⑶そしてそもそもクエリを大量発行していた集合関数のsumとcountを使って平均を出すのではなく、一発で特定のカラムの平均値が算出できるaverageと言う集合関数があることを知り使うことにしました。

<% @pairs.each do |pair| %>
   <%=link_to "#{pair.sake.name} (#{pair.sake.kind}) × #{pair.food.name}", pair_path(pair) %>
   <span style="--rate: <%= pair.average_evaluation.round(1) * 20 %>%;" class="evaluation-star"></span>
   <span class="evaluation-number"><%= pair.average_evaluation.round(1) %></span>
<% end %>

結果だけを見ると、111回あったクエリの数は４回に減りました。
この解決策が正しいかどうかはわかりませんが・・・
そもそもActiveRecordに頼りすぎて、SQLやモデル設計についてもっと知っておかないといけないのだなと気づかされました。

他にもより良い解決策などございましたら、随時教えていただけたらと思います。

Answer 3

コントローラーでPairにevaluationsもincludesし、

def evaluation_average
  begin
    average = Evaluation.where(pair_id: self.id).sum(:point) / Evaluation.where(pair_id: self.id).count.to_f
    return average
  rescue
    average = 0
  end
end

で、evaluation_averageをself.evaluation_averageとし、

average = Evaluation.where(pair_id: self.id).sum(:point) / Evaluation.where(pair_id: self.id).count.to_f

の部分を

average = self.evaluations.sum(:point) / self.evaluations.count.to_f

としてみてはいかがでしょうか？

Comments

[troch]

メソッド名をself.evaluation_averageとするとクラスメソッドになりませんでしょうか？
インスタンスから呼び出す（インスタンスメソッドとして使う）のであれば、
メソッド名はevaluation_averageのままで良いと思います。

[troch]

それ以外は私もBigeastTtexanさんと同意見です。

[退会済みユーザー]

あ、そうですね。そのままでいいですね。

[TakashiOda]

迅速なご回答、ありがとうございます。
ご提案していただいた方法を試したところクエリの回数は半減しましたが、それでもまだ発行されるクエリは100近くあり、まだやはりN+1問題は未解決のままです。

ですが、お二人のご指摘を受けて再考したところ、そもそも私のモデル設計に原因があると考えられるようになりました。
なのでベストアンサーにさせていただこうと思います。
BigeastTtexanさん、trouchさん、本当にありがとうございました。