Question is whether this is the right way to solve it. This requires controller/caller to have knowledge of all associations in the model to preload to speed up the query. Shouldn't model be the better place for this kind of knowledge?
Question is whether this is the right way to solve it. This requires controller/caller to have knowledge of all associations in the model to preload to speed up the query. Shouldn't model be the better place for this kind of knowledge?
class Blog < ActiveRecord::Base
has_many :posts, auto_include: true
end
Define the eager loading in the model as opposed to the controller. Question is do we want to take an approach like this or should we not fight rails and just embed it in the controller. Any suggestions?
I'd suggest embedding it in the controller as you said. The N+1 query is
generated only because (in your example) , you're displaying the provider
and sync_plan in the view and they need to be retrieved per product,
effectively causing several extra queries to be called in order to display
a product.
The point here is, if you remove that from the view, it won't make these
queries and Bullet -the gem that throws these warnings- won't 'complain'.
If we start doing this from the Model rather than from the Controller, we
will have more heavy queries in a ton of places where we might not need
this data (in this case, do you really use product.provider and
product.sync_plan everywhere you deal with a Product?). In fact I think
Bullet will probably complain about the opposite, tons of eager loading of
associations that are not used.
Hope this answers your question, best,
···
On Wed, Dec 3, 2014 at 5:02 PM, Partha Aji wrote:
Sorry hit send prematurely…
While playing with a couple of pages on Katello
(Product/Repository/Contentview index list pages in particular), I got some
annoying warnings.
“”"
user: paji
/katello/api/products?enabled=true&organization_id=1&page=1&paged=true&search=&sort_by=name&sort_order=ASC
N+1 Query detected
Katello:Product => [Provider]
Add to your finder: :include => [Provider]
N+1 Query method call stack
Question is whether this is the right way to solve it. This requires
controller/caller to have knowledge of all associations in the model to
preload to speed up the query. Shouldn’t model be the better place for this
kind of knowledge?
class Blog < ActiveRecord::Base
has_many :posts, auto_include: true
end
Define the eager loading in the model as opposed to the controller.
Question is do we want to take an approach like this or should we not fight
rails and just embed it in the controller. Any suggestions?
> I'd suggest embedding it in the controller as you said. The N+1 query
> is generated only because (in your example) , you're displaying the
> provider and sync_plan in the view and they need to be retrieved per
> product, effectively causing several extra queries to be called in
> order to display a product.
>
> The point here is, if you remove that from the view, it won't make
> these queries and Bullet -the gem that throws these warnings- won't
> 'complain'.
>
> If we start doing this from the Model rather than from the Controller,
> we will have more heavy queries in a ton of places where we might not
> need this data (in this case, do you really use product.provider and
> product.sync_plan everywhere you deal with a Product?). In fact I
> think Bullet will probably complain about the opposite, tons of eager
> loading of associations that are not used.
+1 The views are what are dictating what needs to be eagerly loaded
and the controller is much closer to the view than the model.
Sorry hit send prematurely...
While playing with a couple of pages on Katello
(Product/Repository/Contentview index list pages in particular), I
got some annoying warnings.
"""
user: paji
/katello/api/products?enabled=true&organization_id=1&page=1&paged=true&search=&sort_by=name&sort_order=ASC
N+1 Query detected
Katello:Product => [Provider]
Add to your finder: :include => [Provider]
N+1 Query method call stack
/home/paji/centos/projects/foreman/app/models/concerns/foreman/thread_session.rb:33:in
`clear_thread'
/home/paji/centos/projects/foreman/lib/middleware/catch_json_parse_errors.rb:9:in
`call'
user: paji
/katello/api/products?enabled=true&organization_id=1&page=1&paged=true&search=&sort_by=name&sort_order=ASC
N+1 Query detected
Katello:roduct => [:sync_plan]
Add to your finder: :include => [:sync_plan]
N+1 Query method call stack
/home/paji/centos/projects/foreman/app/models/concerns/foreman/thread_session.rb:33:in
`clear_thread'
/home/paji/centos/projects/foreman/lib/middleware/catch_json_parse_errors.rb:9:in
`call'
....
"""
Its a valid error and fixing it has the potential to cut a huge
number of DB queries while loading objects.
Read up on
http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations
or http://blog.arkency.com/2013/12/rails4-preloading/ for
explanation on the error.
I had it fixed with a change like
"""
+ Product.includes(:provider, :sync_plan, :repositories =>
[:environment, :gpg_key, roduct])
"""
Full diff in https://github.com/Katello/katello/pull/4840/files
Question is whether this is the right way to solve it. This
requires controller/caller to have knowledge of all associations
in the model to preload to speed up the query. Shouldn't model be
the better place for this kind of knowledge?
Searching online I found something like
https://github.com/salsify/goldiloader we can take an approach like..
class Blog < ActiveRecord::Base
has_many :posts, auto_include: true
end
Define the eager loading in the model as opposed to the
controller. Question is do we want to take an approach like this
or should we not fight rails and just embed it in the controller.
Any suggestions?
Partha
--
You received this message because you are subscribed to the Google
Groups "foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to foreman-dev+unsubscribe@googlegroups.com
<mailto:foreman-dev%2Bunsubscribe@googlegroups.com>.
For more options, visit https://groups.google.com/d/optout.
BTW there is other approach which is I think is included in Sequel ORM
which automatically loads the association for whole set of data when you
first hit it in the each iteration. Maybe there is an extension like
that for AR.
Users.all.each do |user|
p user.comments.size # this triggers loading of comments for all
# users, in the end this generates 2 SQL queries.
end
Petr
<details class='elided'>
<summary title='Show trimmed content'>···</summary>
On 04.12.14 17:02, Justin Sherrill wrote:
> On 12/04/2014 02:20 AM, Daniel Lobato wrote:
>> I'd suggest embedding it in the controller as you said. The N+1 query
>> is generated *only* because (in your example) , you're displaying the
>> provider and sync_plan in the view and they need to be retrieved per
>> product, effectively causing several extra queries to be called in
>> order to display a product.
>>
>> The point here is, if you remove that from the view, it won't make
>> these queries and Bullet -the gem that throws these warnings- won't
>> 'complain'.
>>
>> If we start doing this from the Model rather than from the Controller,
>> we will have more heavy queries in a ton of places where we might not
>> need this data (in this case, do you really use product.provider and
>> product.sync_plan *everywhere* you deal with a Product?). In fact I
>> think Bullet will probably complain about the opposite, tons of eager
>> loading of associations that are not used.
>
> +1 The views are what are dictating what needs to be eagerly loaded
> and the controller is much closer to the view than the model.
>
> -Justin
>
>>
>> Hope this answers your question, best,
>>
>> On Wed, Dec 3, 2014 at 5:02 PM, Partha Aji <paji@redhat.com >> <mailto:paji@redhat.com>> wrote:
>>
>> Sorry hit send prematurely...
>>
>> While playing with a couple of pages on Katello
>> (Product/Repository/Contentview index list pages in particular), I
>> got some annoying warnings.
>>
>> """
>> user: paji
>> /katello/api/products?enabled=true&organization_id=1&page=1&paged=true&search=&sort_by=name&sort_order=ASC
>> N+1 Query detected
>> Katello:Product => [Provider]
>> Add to your finder: :include => [Provider]
>> N+1 Query method call stack
>> /home/paji/centos/projects/foreman/app/models/concerns/foreman/thread_session.rb:33:in
>> `clear_thread'
>> /home/paji/centos/projects/foreman/lib/middleware/catch_json_parse_errors.rb:9:in
>> `call'
>>
>> user: paji
>> /katello/api/products?enabled=true&organization_id=1&page=1&paged=true&search=&sort_by=name&sort_order=ASC
>> N+1 Query detected
>> Katello:roduct => [:sync_plan]
>> Add to your finder: :include => [:sync_plan]
>> N+1 Query method call stack
>> /home/paji/centos/projects/foreman/app/models/concerns/foreman/thread_session.rb:33:in
>> `clear_thread'
>> /home/paji/centos/projects/foreman/lib/middleware/catch_json_parse_errors.rb:9:in
>> `call'
>>
>> ....
>> """
>>
>> Its a valid error and fixing it has the potential to cut a huge
>> number of DB queries while loading objects.
>> Read up on
>> http://guides.rubyonrails.org/active_record_querying.html#eager-loading-associations
>> or http://blog.arkency.com/2013/12/rails4-preloading/ for
>> explanation on the error.
>>
>> I had it fixed with a change like
>>
>> """
>> + Product.includes(:provider, :sync_plan, :repositories =>
>> [:environment, :gpg_key, roduct])
>> """
>>
>> Full diff in https://github.com/Katello/katello/pull/4840/files
>>
>> Question is whether this is the right way to solve it. This
>> requires controller/caller to have knowledge of all associations
>> in the model to preload to speed up the query. Shouldn't model be
>> the better place for this kind of knowledge?
>>
>> Searching online I found something like
>> https://github.com/salsify/goldiloader we can take an approach like..
>>
>> class Blog < ActiveRecord::Base
>> has_many :posts, auto_include: true
>> end
>>
>> Define the eager loading in the model as opposed to the
>> controller. Question is do we want to take an approach like this
>> or should we not fight rails and just embed it in the controller.
>> Any suggestions?
>>
>>
>> Partha
>>
>>
>> --
>> You received this message because you are subscribed to the Google
>> Groups "foreman-dev" group.
>> To unsubscribe from this group and stop receiving emails from it,
>> send an email to foreman-dev+unsubscribe@googlegroups.com
>> <mailto:foreman-dev%2Bunsubscribe@googlegroups.com>.
>> For more options, visit https://groups.google.com/d/optout.
>>
>>
>>
>>
>> --
>> Daniel Lobato
>>
>> @elobatoss
>> blog.daniellobato.me <http://blog.daniellobato.me>
>> daniellobato.me <http://daniellobato.me/>
>>
>> GPG: http://keys.gnupg.net/pks/lookup?op=get&search=0x7A92D6DD38D6DE30
>> --
>> You received this message because you are subscribed to the Google
>> Groups "foreman-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to foreman-dev+unsubscribe@googlegroups.com
>> <mailto:foreman-dev+unsubscribe@googlegroups.com>.
>> For more options, visit https://groups.google.com/d/optout.
>
> --
> You received this message because you are subscribed to the Google
> Groups "foreman-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to foreman-dev+unsubscribe@googlegroups.com
> <mailto:foreman-dev+unsubscribe@googlegroups.com>.
> For more options, visit https://groups.google.com/d/optout.
</details>
