refactoring an expensive query in rails

Question

I been all day trying to refactor my queries. It was taking several minutes for a table with 20 records to load. At first, I blamed the query in this stackoverflow post:

rails - using Rails.cache gives error

However, after doing some testing, I realized that query was loading very fast, and was not the culprit.

Then I blamed it on kaminari, as I thought that kaminari was populating thousands of records into ruby objects, as I explained in this post:

rails and kaminari

But that was wrong too. Kaminari was only loading 20 records at a time (I used the logger to check how many reports where being loaded and it showed 20).

So finally I think I found the real culprit. It is another query that occurs during the pagination of that page. I realized that this query is taking more than 20 seconds to load each time it is run:

def virtual_fence_duration
    inside_fence_time = nil
    outside_fence_time = nil

    outside_fence_time = self.time

    previous_reports = Report.where{(time    <  my{self.time}) &
                                   (unit_id == my{self.unit_id})}.order('time desc')

    previous_reports.each do |report|
      alerts = report.alerts.collect { |a| a.code_name }
      if alerts.include? "Inside virtual fence"
        inside_fence_time = report.time
        break
      end
    end

    if inside_fence_time && outside_fence_time
      "Virtual Fence Elapsed Time: #{(((outside_fence_time - inside_fence_time).to_i).to_f/60.0).ceil} minutes" 
    else
      ""
    end    
  end

Basically, this method gets called when there is an alert that is "outside virtual fence". I store the time. And then I query all the reports previous to it (reports has_many alerts). Then I use the each iterator of ruby to go through every single report and then the alerts associated with those reports to find the alert that is "inside virtual fence". Then I store the time of that report and take the difference between the two times. This logic seems to be taking forever if there are alot of reports between the two durations. Is there a more efficient way to do this in sql (mysql) or ruby?

That `where` syntax isn't standard ActiveRecord, are you using squeel? Also, is `alerts` a `has_many` association of Report? — PinnyM, Mar 20 '13 at 21:15
@PinnyM Yes, it appears squeel is being used. If it is causing performance issues, then I dont mind to remove it from the query. And yes a Report has_many alerts — JohnMerlino, Mar 20 '13 at 21:18

score 0 · Answer 1 · answered Mar 20 '13 at 21:20

0

First of all i'm not familiar with a query that uses "==" in mysql. It should be just "=". Secondly, what is the "my()" function that is used in your query?

Unless i'm either not understanding your question, or the advanced things that you're doing, i would think that a query similar to this would get you what you're looking for.

Report.where('time < ? and unit_id = ?', self.time, self.unit_id).order('time desc')

If it's not, post the query that is actually being run from the log file.

answered Mar 20 '13 at 21:20

Catfish

18,876
54
209
353

After reading other comments, i see you're using squeel. I'm not at all familiar with that so my answer is based on active record. – Catfish Mar 20 '13 at 21:20
The OP is using the [squeel gem](https://github.com/ernie/squeel) which is basically a DSL interpreter for ruby to SQL – PinnyM Mar 20 '13 at 21:20

PinnyM · Accepted Answer · 2013-03-20T21:33:04.607

0

This appears to be an N+1 performance issue. Try eager loading using includes:

previous_reports = Report.where{(time    <  my{self.time}) &
                               (unit_id == my{self.unit_id})}.
                          includes(:alerts).order('time desc')

This should certainly have at least some impact, although it may still be slow depending on the number of rows in your tables. If you aren't satisfied at this point, you'll need to add indices to the time and unit_id columns of your reports table, and check that the alerts table has an index on the report_id column.

You can also perform all this logic in a single query, which should have a significant improvment over the current approach. Of course, having the correct indices wouldn't hurt either:

prev_report = Report.joins(:alerts)
                where{time < my{self.time} &&
                      unit_id == my{self.unit_id} &&
                      alerts.code_name.like '%Inside virtual fence%'}.
                order("reports.time DESC").first

inside_fence_time = prev_report.time if prev_report

edited Mar 20 '13 at 21:33

answered Mar 20 '13 at 21:20

PinnyM

35,165
3
73
81

Not using that squeel also improved performance. My final query: Report. joins({:alerts => :alert_code}). where(:unit_id => unit_id). where("reports.time < ?", time). where("alert_codes.name = ?", "Inside virtual fence"). order("reports.time DESC").first – JohnMerlino Mar 21 '13 at 14:46
Just a quick question related to this. As you can see, that query hasa joins call, two where calls, and an order call. Should I be using scopes to make this more elegant, rather than having one long piece of code like this? – JohnMerlino Mar 21 '13 at 14:48
@JohnMerlino: if you use these query refinements in several places, then by all means make a scope. – PinnyM Mar 21 '13 at 14:51
why in the first query example you use includes and in the second you use joins. Shouldnt you use includes for both of them? – JohnMerlino Mar 25 '13 at 15:07
@JohnMerlino: `includes` is useful for eager loading or when a LEFT JOIN is required. `joins` will use an INNER JOIN which is faster for the database, but does not allow for eager loading - in this case this is sufficient since we won't need to load the child objects. – PinnyM Mar 25 '13 at 15:17
Oh I see because in this case, we do a direct query with no ruby each, so eager loading not necessary – JohnMerlino Mar 25 '13 at 15:19

refactoring an expensive query in rails

2 Answers2