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

Projects
None yet
1 participant
@kamipo
Member

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.

Don't update counter cache unless the record is actually saved
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,

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 #14849.
Closes #23357.
Closes #31493.
Closes #31494.
Closes #32372.
Closes #33113.
Closes #33117
Closes #33129.
Closes #33458.

@kamipo kamipo merged commit fe929c1 into rails:master Sep 20, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

kamipo added a commit that referenced this pull request Sep 20, 2018

Merge pull request #33913 from kamipo/counter_cache
Don't update counter cache unless the record is actually saved

@kamipo kamipo deleted the kamipo:counter_cache branch Sep 20, 2018

kamipo added a commit that referenced this pull request Sep 25, 2018

Remove `counter_cache_target` which is no longer called
`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