#5958 Collection rendering with superfluous database queries - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#5958 ✓committed

Collection rendering with superfluous database queries

Reported by Ivan Ukhov | November 12th, 2010 @ 10:16 AM | in 3.x

When passing some Relation to render :collection => ..., it produces two queries. One for counting (SELECT COUNT) and another for fetching (just SELECT). For example:

render :partial => 'some_partial', :collection => SomeModel.some_scope

Suppose this is because of the following line:

https://github.com/rails/rails/blob/master/actionpack/lib/action_vi...

My workaround:

module ActionView
  module Partials
    class PartialRenderer
      private
      def collection
        c = if @object.respond_to?(:to_ary)
          @object
        elsif @options.key?(:collection)
          @options[:collection] || []
        end
        c.try :length
        c
      end
    end
  end
end

Comments and changes to this ticket

Santiago Pastorino November 12th, 2010 @ 03:08 PM
- Importance changed from “” to “Low”
Can you provide a failing test case and a patch following http://rails.lighthouseapp.com/projects/8994/sending-patches
Santiago Pastorino November 12th, 2010 @ 03:09 PM
- State changed from “new” to “open”
- Milestone set to “3.x”
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 03:25 PM
Sorry, I'm really not good at writing test cases, to my shame I haven't even written one. I just want you to know about the problem. Please don't put it aside, everyone who is interested in performance will be especially upset observing two queries instead of one... It could save a bunch of time.

The issue isn't hard to understand, so I hope someone will takes responsibility to fix it.

Thank you.
Neeraj Singh November 12th, 2010 @ 04:23 PM
- Tag set to “patched”
Attached is code patch. Not sure how to count how many sql statements have been generated in the whole view.
- 5958-fix-against_rails-edge.diff 1.4 KB
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 04:32 PM
The patch is not correct, because it still calls size, but should call length. size in case of not loaded collection fires a counting query.

So, in the patch instead of:
```
size = @collection.size
```
should be:
```
size = @collection.length
```
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 04:41 PM
[PATCH] Do not execute query to get count of records twice.

The problem is not in how many times it counts, but in the fact that it produces two different queries: one is specially for just counting, another for fetching data (rows from a table). But if we anyway are going to render the whole collection, why shouldn't we just pull the data and count the number of records? For me it is an obvious wasting of time.
Neeraj Singh November 12th, 2010 @ 04:55 PM
- Assigned user set to “Santiago Pastorino”
When the execution comes to @collection, at that time @collection is a Relation and that is not loaded. Since it is not loaded whether you call .length or .size it will make a call to sql.

Other thing is that if the collection is not already loaded then .size internally calls .length .

I tested the patch. Before the patch it was making two calls. After the patch it is making one call.

Assigning it to Santiago since he is watching this ticket. :-)
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 05:05 PM
And what about the following line?

https://github.com/rails/rails/blob/eeb9b379f9dba0c692856b2f5576cd3...
Neeraj Singh November 12th, 2010 @ 05:38 PM
That's what I was saying. May be I was not clear enough.

This blog has more info on the same topic with greater clarity http://blog.hasmanythrough.com/2008/2/27/count-length-size .
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 05:48 PM
You say:
Other thing is that if the collection is not already loaded then .size internally calls .length .

Josh says:
If the collection has already been loaded, it will return its length just like calling #length. If it hasn't been loaded yet, it's like calling #count

We should always call length in order to always count an assiciation by loading the contents of the association into memory and then returning the number of elements loaded.

So, you still insist on using size?
Neeraj Singh November 12th, 2010 @ 06:57 PM
You are right. It should be .length. I usually avoid length because it does select and not select count but in this case if the number of records is more than one then anyway we are going to load all the records.
```
# fires three queries select count * ...
def self.lab
  brakes = Car.first.brakes
  puts brakes.size
  puts brakes.size
  puts brakes.size
end

# fires only one query but it is select * 
def self.lab2
  brakes = Car.first.brakes
  puts brakes.length
  puts brakes.length
  puts brakes.length
end
```
Sorry for being thick. Appreciate your patience.

I will add a new patch.
Neeraj Singh November 12th, 2010 @ 07:03 PM
My previous patch reduced the number of sql queries from 3 to 2.

Then new attached patch is reducing the number of sql queries from 3 to 1.
- 5958-fix-against-edge.diff 1.3 KB
Santiago Pastorino November 12th, 2010 @ 07:58 PM
- Assigned user changed from “Santiago Pastorino” to “Aaron Patterson”
Santiago Pastorino November 12th, 2010 @ 09:51 PM
- Assigned user changed from “Aaron Patterson” to “Santiago Pastorino”
You flagged this item as spam.
Ivan Ukhov November 12th, 2010 @ 11:16 PM
Neeraj Singh, not a problem at all, it's a pleasure for me to contribute a bit to this great framework) Thank you for your consideration and quick responce.
Repository November 13th, 2010 @ 06:03 AM
- State changed from “open” to “committed”
(from [27f4ffd11a91b534fde9b484cb7c4e515ec0fe77]) Make collection and collection_from_object methods return an array

This transforms for instance scoped objects into arrays and avoid unneeded queries

[#5958 state:committed] https://github.com/rails/rails/commit/27f4ffd11a91b534fde9b484cb7c4...
Repository November 13th, 2010 @ 06:07 AM
(from [b8701933453c82bdb968532bdd7dee8cec775b72]) Make collection and collection_from_object methods return an array

This transforms for instance scoped objects into arrays and avoid
unneeded queries

[#5958 state:committed] https://github.com/rails/rails/commit/b8701933453c82bdb968532bdd7de...
Neeraj Singh November 13th, 2010 @ 05:23 PM
@Santiago

I tested your patch and it works great. Great solution. Thanks for teaching.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Shared Ticket Bins (Sort)

↓↑ drag 455 Open Bugs
↓↑ drag 0 Recent Open Tickets
↓↑ drag 87 Open Patches
↓↑ drag 0 Open Doc Patches
↓↑ drag 542 Stale Tickets
↓↑ drag 3 Verified Patches
↓↑ drag 87 Stale Patches
↓↑ drag 0 Yesterday's Tickets
↓↑ drag 0 Two Days Ago
↓↑ drag 0 This Week
↓↑ drag 0 Last Week
↓↑ drag 7 Bugmash
↓↑ drag 0 Bugmash review queue
↓↑ drag 5 Rails 3 High Priority Tickets

People watching this ticket

Attachments

5958-fix-against_rails-edge... 1.4 KB
5958-fix-against-edge.diff 1.3 KB

Referenced by

5958 Collection rendering with superfluous database queries [#5958 state:committed] https://github.com/rails/rails/c...
5958 Collection rendering with superfluous database queries [#5958 state:committed] https://github.com/rails/rails/c...

Rails Ruby on Rails → 3.x