Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid N+1 queries: add default_scope to Refinery::Page to include :translations #2929

Closed
wants to merge 1 commit into from

Conversation

LouisStAmour
Copy link
Contributor

Installed Bullet and besides a few bugs of my own, one of the first things it began complaining about was some of the Refinery::Page.by_slug lookups I was doing, since they'd generate a query followed by a second query for the translation lookup from Globalize.

While I was able to resolve this sometimes, it proved nearly impossible when looking up .children to also do the include. In fact, the extra query seemed to happen every time I looked up a page.

So I found an answer on StackOverflow I figured I'd report this as a pull request. I don't expect this to get automatically merged, as I haven't the time to consider any implications of this. But it works for my project as follows, in a decorator:

Refinery::Page.class_eval do
  default_scope { includes(:translations) }
end

@parndt
Copy link
Member

parndt commented Mar 16, 2015

CI is now failing for pages.

As a rule, I'm pretty uncomfortable with default_scope.. couldn't we just add the includes(:translations) to the APIs that are issuing too many queries?

@LouisStAmour
Copy link
Contributor Author

Given the StackOverflow report, it appears to be part of the magic that is Globalize, so it's quite possible this is an upstream bug. Those infinite redirects in the failing tests look painful, so let's hold off until I can take a second look I guess. :)

@bricesanchez
Copy link
Member

This hotfix :

Refinery::Page.class_eval do
  default_scope { includes(:translations, :children) }
end

helps me to avoid the warning of bullet but not the load time, which stay to ~24.00s for 30 pages.

I'm still facing a performance problem on dev : #2950

@parndt parndt closed this Jul 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants