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

Don't update counter cache unless the record is actually saved #33913

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Sep 18, 2018

This is a 4th attempt to make counter cache transactional completely.

Past attempts: #9236, #14849, #23357.

All existing counter cache issues (increment/decrement twice, lost
increment) are caused due to updating counter cache on the outside of
the record saving transaction by assigning belongs_to record, even
though assigning that doesn't cause the record saving.

We have the @_after_replace_counter_called guard condition to mitigate
double increment/decrement issues, but we can't completely prevent that
inconsistency as long as updating counter cache on the outside of the
transaction, since saving the record is not always happened after that.

We already have handling counter cache after create/update/destroy,

def _create_record(attribute_names = self.attribute_names)
id = super
each_counter_cached_associations do |association|
if send(association.reflection.name)
association.increment_counters
end
end
id
end
def destroy_row
affected_rows = super
if affected_rows > 0
each_counter_cached_associations do |association|
foreign_key = association.reflection.foreign_key.to_sym
unless destroyed_by_association && destroyed_by_association.foreign_key.to_sym == foreign_key
if send(association.reflection.name)
association.decrement_counters
end
end
end
end
affected_rows
end

def belongs_to_counter_cache_after_update(reflection)
foreign_key = reflection.foreign_key
cache_column = reflection.counter_cache_column
if @_after_replace_counter_called ||= false
@_after_replace_counter_called = false
elsif association(reflection.name).target_changed?
if reflection.polymorphic?
model = attribute_in_database(reflection.foreign_type).try(:constantize)
model_was = attribute_before_last_save(reflection.foreign_type).try(:constantize)
else
model = reflection.klass
model_was = reflection.klass
end
foreign_key_was = attribute_before_last_save foreign_key
foreign_key = attribute_in_database foreign_key
if foreign_key && model < ActiveRecord::Base
counter_cache_target(reflection, model, foreign_key).update_counters(cache_column => 1)
end
if foreign_key_was && model_was < ActiveRecord::Base
counter_cache_target(reflection, model_was, foreign_key_was).update_counters(cache_column => -1)
end
end
end

so just removing assigning logic on the belongs_to association makes
counter cache transactional completely.

Closes #14849.
Closes #23357.
Closes #31493.
Closes #31494.
Closes #32372.
Closes #33113.
Closes #33117
Closes #33129.
Closes #33458.

This is a 4th attempt to make counter cache transactional completely.

Past attempts: rails#9236, rails#14849, rails#23357.

All existing counter cache issues (increment/decrement twice, lost
increment) are caused due to updating counter cache on the outside of
the record saving transaction by assigning belongs_to record, even
though assigning that doesn't cause the record saving.

We have the `@_after_replace_counter_called` guard condition to mitigate
double increment/decrement issues, but we can't completely prevent that
inconsistency as long as updating counter cache on the outside of the
transaction, since saving the record is not always happened after that.

We already have handling counter cache after create/update/destroy,

https://github.com/rails/rails/blob/1b90f614b1b3d06b7f02a8b9ea6cd84f15d58643/activerecord/lib/active_record/counter_cache.rb#L162-L189
https://github.com/rails/rails/blob/1b90f614b1b3d06b7f02a8b9ea6cd84f15d58643/activerecord/lib/active_record/associations/builder/belongs_to.rb#L33-L59

so just removing assigning logic on the belongs_to association makes
counter cache transactional completely.

Closes rails#14849.
Closes rails#23357.
Closes rails#31493.
Closes rails#31494.
Closes rails#32372.
Closes rails#33113.
Closes rails#33117
Closes rails#33129.
Closes rails#33458.
@kamipo kamipo merged commit fe929c1 into rails:master Sep 20, 2018
kamipo added a commit that referenced this pull request Sep 20, 2018
Don't update counter cache unless the record is actually saved
@kamipo kamipo deleted the counter_cache branch September 20, 2018 05:36
kamipo added a commit that referenced this pull request Sep 25, 2018
`counter_cache_target` is called only when updated counter cache in
replacing target, but it was already removed at #33913.
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

1 participant