Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix repeated column lookups in 3.0.5+ (eager loaded has_one association with conditions) #246

Closed
wants to merge 1 commit into from

2 participants

@baconpat

The following issue affects the 3-0-stable branch:

https://rails.lighthouseapp.com/projects/8994/tickets/6599-305-introduced-repeated-column-lookups-for-eager-loaded-has_one-with-conditions

The AssociationProxy code appears to have been completely rewritten on master so I am not sure if this is a problem there or not (I am also not able to run my app against master right now to test it). But in the meantime I am asking that this patch get pulled into 3.0 stable to improve the situation for the 3.0.x line.

Even with this patch applied I am still seeing slower performance with (patched) 3.0.5+ vs 3.0.4, but it is much improved (3 - 4 times faster for a page loading many rows).

@baconpat baconpat Added back the use of the Reflection module's cached sanitized_condit…
…ions in an AssociationProxy. This was recently removed and when a has_one association with conditions is eager loaded the conditions would be sanitized once for every result row, causing a database hit to fetch the columns.
03852b6
@tenderlove
Owner

Merged. This seems like a bad regression, so I'm putting it in the 3.0.6.rc2. Thank you.

@tenderlove tenderlove closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 30, 2011
  1. @baconpat

    Added back the use of the Reflection module's cached sanitized_condit…

    baconpat authored
    …ions in an AssociationProxy. This was recently removed and when a has_one association with conditions is eager loaded the conditions would be sanitized once for every result row, causing a database hit to fetch the columns.
This page is out of date. Refresh to see the latest.
View
6 activerecord/lib/active_record/associations/association_proxy.rb
@@ -102,7 +102,7 @@ def aliased_table_name
# Returns the SQL string that corresponds to the <tt>:conditions</tt>
# option of the macro, if given, or +nil+ otherwise.
def conditions
- @conditions ||= @reflection.options[:conditions] && interpolate_and_sanitize_sql(@reflection.options[:conditions])
+ @conditions ||= interpolate_sanitized_sql(@reflection.sanitized_conditions) if @reflection.sanitized_conditions
end
alias :sql_conditions :conditions
@@ -161,6 +161,10 @@ def dependent?
@reflection.options[:dependent]
end
+ def interpolate_sanitized_sql(sql, record = nil, sanitize_klass = @reflection.klass)
+ @owner.send(:interpolate_sanitized_sql, sql, record, sanitize_klass)
+ end
+
def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = @reflection.klass)
@owner.send(:interpolate_and_sanitize_sql, sql, record, sanitize_klass)
end
View
7 activerecord/lib/active_record/base.rb
@@ -1733,7 +1733,10 @@ def quote_value(value, column = nil)
def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = self.class)
sanitized = sanitize_klass.send(:sanitize_sql, sql)
+ interpolate_sanitized_sql(sanitized, record, sanitize_klass)
+ end
+ def interpolate_sanitized_sql(sanitized, record = nil, sanitize_klass = self.class)
if sanitized =~ /\#\{.*\}/
ActiveSupport::Deprecation.warn(
'String-based interpolation of association conditions is deprecated. Please use a ' \
@@ -1741,8 +1744,8 @@ def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = self.class)
'should be changed to has_many :older_friends, :conditions => proc { "age > #{age}" }.'
)
instance_eval("%@#{sanitized.gsub('@', '\@')}@", __FILE__, __LINE__)
- elsif sql.respond_to?(:to_proc)
- sanitize_klass.send(:sanitize_sql, instance_exec(record, &sql))
+ elsif sanitized.respond_to?(:to_proc)
+ sanitize_klass.send(:sanitize_sql, instance_exec(record, &sanitized))
else
sanitized
end
View
17 activerecord/test/cases/associations/eager_test.rb
@@ -670,6 +670,23 @@ def test_preload_with_deprecated_interpolation
assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
end
+ def test_preload_has_one_with_conditions
+ # pre-heat our cache
+ Post.arel_table.columns
+ Comment.columns
+
+ Post.connection.column_calls_by_table['comments'] = 0
+ Post.includes(:very_special_comment).all.to_a
+ assert_equal 0, Post.connection.column_calls_by_table['comments']
+
+ Post.connection.column_calls_by_table['comments'] = 0
+ Post.includes(:first_post_comment).all.to_a
+
+ # Don't care exactly how many column lookup are done,
+ # as long as the number is small
+ assert(Post.connection.column_calls_by_table['comments'] < 3)
+ end
+
def test_polymorphic_type_condition
post = Post.find(posts(:thinking).id, :include => :taggings)
assert post.taggings.include?(taggings(:thinking_general))
View
5 activerecord/test/cases/helper.rb
@@ -39,11 +39,14 @@ def execute_with_query_record(sql, name = nil, &block)
end
ActiveRecord::Base.connection.class.class_eval {
- attr_accessor :column_calls
+ attr_accessor :column_calls, :column_calls_by_table
def columns_with_calls(*args)
@column_calls ||= 0
+ @column_calls_by_table ||= Hash.new {|h,table| h[table] = 0}
+
@column_calls += 1
+ @column_calls_by_table[args.first.to_s] += 1
columns_without_calls(*args)
end
View
1  activerecord/test/models/post.rb
@@ -44,6 +44,7 @@ def find_most_recent
:conditions => proc { ["#{"#{aliased_table_name}." rescue ""}body = ?", 'Thank you for the welcome'] }
has_many :comments_with_deprecated_interpolated_conditions, :class_name => 'Comment',
:conditions => ['#{"#{aliased_table_name}." rescue ""}body = ?', 'Thank you for the welcome']
+ has_one :first_post_comment, :class_name => 'Comment', :conditions => {:body => 'First Post!'}
has_one :very_special_comment
has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post
Something went wrong with that request. Please try again.