This project is archived and is in readonly mode.

Collection rendering with superfluous database queries

#5958

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

Reported by Ivan Ukhov · November 12th, 2010 @ 10:16 AM

Activity

Santiago Pastorino
- Importance changed from to Low
Can you provide a failing test case and a patch following http://rails.lighthouseapp.com/projects/8994/sending-patches

November 12th, 2010 @ 03:08 PM
Santiago Pastorino
- State changed from new to open
- Milestone set to 3.x
November 12th, 2010 @ 03:09 PM
Ivan Ukhov
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.

November 12th, 2010 @ 03:25 PM
Neeraj Singh
- 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.36 KB
November 12th, 2010 @ 04:23 PM
Ivan Ukhov
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
```
November 12th, 2010 @ 04:32 PM
Ivan Ukhov
[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.

November 12th, 2010 @ 04:41 PM
Neeraj Singh
- 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. :-)

November 12th, 2010 @ 04:55 PM
Ivan Ukhov
And what about the following line?

https://github.com/rails/rails/blob/eeb9b379f9dba0c692856b2f5576cd3...

November 12th, 2010 @ 05:05 PM
Neeraj Singh
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 .

November 12th, 2010 @ 05:38 PM
Ivan Ukhov
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?

November 12th, 2010 @ 05:48 PM
Neeraj Singh
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.
November 12th, 2010 @ 06:57 PM
Neeraj Singh
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.34 KB
November 12th, 2010 @ 07:03 PM
Santiago Pastorino
- Assigned user changed from Santiago Pastorino to Aaron Patterson
November 12th, 2010 @ 07:58 PM
Santiago Pastorino
- Assigned user changed from Aaron Patterson to Santiago Pastorino
November 12th, 2010 @ 09:51 PM
Ivan Ukhov
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.

November 12th, 2010 @ 11:16 PM
Repository
- 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 Collection rendering with superfluous database queries state:committed] https://github.com/rails/rails/commit/27f4ffd11a91b534fde9b484cb7c4...

November 13th, 2010 @ 06:03 AM
Repository

(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 Collection rendering with superfluous database queries state:committed] https://github.com/rails/rails/commit/b8701933453c82bdb968532bdd7de...

November 13th, 2010 @ 06:07 AM
Neeraj Singh
@Santiago

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

November 13th, 2010 @ 05:23 PM