Don't update counter cache unless the record is actually saved by kamipo · Pull Request #33913 · rails/rails

kamipo · 2018-09-18T20:39:22Z

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

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,

rails/activerecord/lib/active_record/counter_cache.rb

Lines 162 to 189 in 1b90f61

    
           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

rails/activerecord/lib/active_record/associations/builder/belongs_to.rb

Lines 33 to 59 in 1b90f61

    
           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.

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

`counter_cache_target` is called only when updated counter cache in replacing target, but it was already removed at #33913.

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

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

Merge pull request #33913 from kamipo/counter_cache

f1dcfc6

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

kamipo deleted the counter_cache branch September 20, 2018 05:36

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

Remove counter_cache_target which is no longer called

376ffe0

`counter_cache_target` is called only when updated counter cache in replacing target, but it was already removed at #33913.

aitorlb mentioned this pull request Dec 27, 2018

Fix parent assemblies children_count counter decidim/decidim#4718

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Don't update counter cache unless the record is actually saved#33913
kamipo merged 1 commit intorails:masterfrom
kamipo:counter_cache

kamipo commented Sep 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	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

Conversation

kamipo commented Sep 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant