check_box_tag includes n+1 query

Question

The following line in a list of stories an article belongs to—on an edit article page, allowing me to assign the article to stories—works but produces an n+1 query (I think).

The bit that is causing the problem is @article.stories.include?(s), which checks to see if an article already belongs to a story:

<%= check_box_tag "article[story_ids][]", s.id, @article.stories.include?(s) %>

Log says Rails is doing 50+ queries (all stories) because of that one bit; article_id = 266 in the example:

Story Exists (0.4ms)  SELECT  1 AS one FROM "stories" INNER JOIN "articles_stories" ON "stories"."id" = "articles_stories"."story_id" WHERE "articles_stories"."article_id" = $1 AND "stories"."id" = $2 LIMIT 1  [["article_id", 266], ["id", 6]]
Story Exists (0.3ms)  SELECT  1 AS one FROM "stories" INNER JOIN "articles_stories" ON "stories"."id" = "articles_stories"."story_id" WHERE "articles_stories"."article_id" = $1 AND "stories"."id" = $2 LIMIT 1  [["article_id", 266], ["id", 51]]
Story Exists (0.3ms)  SELECT  1 AS one FROM "stories" INNER JOIN "articles_stories" ON "stories"."id" = "articles_stories"."story_id" WHERE "articles_stories"."article_id" = $1 AND "stories"."id" = $2 LIMIT 1  [["article_id", 266], ["id", 41]]
Etc., etc.

The actual number of stories article_id = 266 belongs to is…2:

 select * from articles_stories where article_id = 266;

 article_id | story_id 
 ------------+----------
    266 |       35
    266 |        5

 (2 rows)

How do I fix it?

UPDATE: DB, whole chunk & controller

Article.rb:

has_and_belongs_to_many :stories

Story.rb:

has_and_belongs_to_many :articles

And the whole chunk of code the check_box_tag is in—prints a list of stories by category you can assign to an article—is:

<% @stories.group_by(&:category).sort_by{|c, stories| c.category}.each do |c, stories| %>
  <div class="admin-stories-in-categories">
    <strong><%= c.category unless c.nil? %></strong>
      <ul>
        <% for s in stories %>
          <li>
            <%= check_box_tag "article[story_ids][]", s.id, @article.stories.include?(s) %>
            <%= s.story %>
          </li>
        <% end %>
      </ul>
  </div>
<% end %>

In articles_controller.rb:

@stories = Story.all.includes(:category).order(:story)

ANSWER:

AbM's answer below makes the N+1 queries go away and instead returns a single query:

Returns `(0.8ms)  SELECT "stories"."id" FROM "stories" INNER JOIN "articles_stories" ON "stories"."id" = "articles_stories"."story_id" WHERE "articles_stories"."article_id" = $1  [["article_id", 266]]`

There is a lot of question and tips to fix n+1 query, have you tried something? — Roman Kiselenko, Jan 06 '18 at 18:39
I have. They didn't work, hence the question. You edited out "Happy New Year"? — Matthew Bennett, Jan 06 '18 at 18:53
Why didn't work? Could you be more clearly, what is not work and what is you have tried? — Roman Kiselenko, Jan 06 '18 at 19:01

score 1 · Answer 1 · answered Jan 06 '18 at 18:59

1

We need your database schema and the query you're using from your controller (if any) — but you'll likely just have to do something like Article.includes(:stories).find(params[:id])

I'd recommend plugging in something like Bullet into your development stack.

https://github.com/flyerhzm/bullet

answered Jan 06 '18 at 18:59

Josh Brody

5,153
1
14
25

I added the relations and controller bits. – Matthew Bennett Jan 06 '18 at 19:13

AbM · Accepted Answer · 2018-01-06T19:24:24.000

1

Thanks because the include? method on a active record relation does an EXIST operation in sql if the relation is not loaded as shown here

Try storing the article stories in a variable:

<% article_stories_ids = @article.stories.pluck(:id) %>
<% @stories.group_by(&:category).sort_by{|c, stories| c.category}.each do |c, stories| %>
  <div class="admin-stories-in-categories">
    <strong><%= c.category unless c.nil? %></strong>
      <ul>
        <% stories.each do |s| %>
          <li>
            <%= check_box_tag "article[story_ids][]", s.id, article_stories_ids.include?(s.id) %>
            <%= s.story %>
          </li>
        <% end %>
      </ul>
  </div>
<% end %>

edited Jan 06 '18 at 19:24

answered Jan 06 '18 at 19:17

AbM

7,326
2
25
28

please dont use `for` loops in ruby – Roman Kiselenko Jan 06 '18 at 19:20
I just used their example for the existing loop. The modification is just for storing the ids in a variable – AbM Jan 06 '18 at 19:21
Ok, thanks @AbM, that works. Will post what it returns. – Matthew Bennett Jan 06 '18 at 19:22
Why not @Зелёный? What's the alternative? – Matthew Bennett Jan 06 '18 at 19:23
Updated the answer to use `.each` instead of `for` loop – AbM Jan 06 '18 at 19:24
Is there some performance difference between `.each do` and `for x in xs`? – Matthew Bennett Jan 06 '18 at 19:27
1

@MatthewBennett https://stackoverflow.com/questions/10396475/ruby-for-loop-a-trap – Roman Kiselenko Jan 06 '18 at 19:27

check_box_tag includes n+1 query

2 Answers2