This project is archived and is in readonly mode.

#1185 ✓wontfix
David Stevenson

Old-single-query-includes should be globally disableable

Reported by David Stevenson | October 7th, 2008 @ 04:57 PM | in 2.x

Rails often guesses conservatively that pre-rails-2.1 style includes are required for queries that they are not. Rather than trying to fix this so it guesses perfectly, projects that are serious about include performance should be able to disable this style of includes entirely.

Comments and changes to this ticket

  • David Stevenson
  • David Stevenson

    David Stevenson October 9th, 2008 @ 05:44 PM

    • Tag changed from activerecord, config, include, query, single to activerecord, config, include, join, left, outer, patch, query, single

    Previous patch had a bad line in it, which didn't change the functionality but was misleading. Removed line from the diff and resubmitting.

  • DHH

    DHH October 16th, 2008 @ 09:15 AM

    • State changed from “new” to “wontfix”

    I don't understand the argument that "projects that are serious about include performance" should only want this. If we can come up with a fail-safe heuristic that is always faster, why wouldn't everyone want that? I take it that there are specific reasons why the previous style includes are still being selected at times.

    In any case, I don't see it being a general option. If anything, it should be a per-query option. But not until we've tried to make the underlying selector better.

  • Frederick Cheung

    Frederick Cheung October 16th, 2008 @ 11:17 AM

    We do have heuristics, but they don't always get it right. In particular they don't work when you specify a table via the :joins option, largely because in the most general case parsing a join statement is a) hard and b) database dependant (we might be able to do something for the simpler :joins => :some_association case)

  • David Stevenson

    David Stevenson October 16th, 2008 @ 06:50 PM

    I think it might be possible to find all the tables from :joins, but it would probably be easier to approach this problem the other way around:

    Why don't we make a blacklist of tables that trigger a fallback, rather than a whitelist of allowed tables? It's easier to find all the "bad" tables by traversing the include chain. This wouldn't allow for the case when you want to :join and :include from the same table, however, but would be overall an improvement.

    Since the performance difference between the old-style and new-style is pretty big, I'd imagine that high performance projects are going to want to switch ALL their queries over. I think the best way to accomplish that is to just disable the old style includes. With good test coverage, it's easy to find the queries that reference included tables and fix them up... this only took us about 15-30 minutes.

  • David Stevenson

    David Stevenson October 16th, 2008 @ 06:52 PM

    I had originally opened this ticket:

    http://rails.lighthouseapp.com/p...

    which I guess I could try to come up with a patch for. But it seems to be orthogonal to this configuration ticket. Even if it guesses better, it would still be useful to disable these across the board (especially for finding "bad queries" that reference included tables).

  • Frederick Cheung

    Frederick Cheung October 16th, 2008 @ 07:10 PM

    Even that can get hairy when table aliases are involved but at that point it's quite tricky to write the conditions anyway. Definitely worth a try

    On 16 Oct 2008, at 18:50, Lighthouse support@lighthouseapp.com wrote:

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>

Referenced by

Pages