Stop incrementing counter caches on association assignation by byroot · Pull Request #14849 · rails/rails

byroot · 2014-04-23T17:01:06Z

Followup of #14765 and #14735
Similar in some ways to #9236
Fixes #13304 among others

Rationale

It's a very bad time to perform the increment because the foreign_key
change is not persisted yet and it happen outside the save transaction,
so if the update fail the increment is not rollbacked.

Limitation

I can't make BelongsToAssociationsTest#test_custom_counter_cache work, because Reply belongs to Topic twice, so ActiveRecord think it's 2 different counters.

I'm not sure what to do with that. I think it just don't make any sense. Maybe we could prevent targeting the same counter from two associations.

Further work

The same issue lies in has_many association the counters are decremented outside any transaction beside no record have been actually deleted. I'll see what I can do about that in a further PR if you don't mind.

@tenderlove @arthurnn @rafaelfranca thoughts?

/cc @jasl

byroot · 2014-04-23T17:02:25Z

Oh and yes I added #update_counters, #decrement_counter and #increment_counter on Relation. It should be documented I guess.

jasl · 2014-04-23T18:25:07Z

I will pull your branch to learn more and see what I can help

byroot · 2014-05-03T16:58:04Z

@tenderlove @rafaelfranca: I suppose you were busy because of Rails conf, any chance you could review this PR next week?

jeremy · 2014-05-05T16:56:58Z

activerecord/lib/active_record/associations/belongs_to_association.rb

Where is this used?

Nowhere. It's dead code, I'll get rid of it, sorry for that.

byroot · 2014-05-05T17:12:12Z

@jeremy thanks a lot for this very good code review, and sorry for the dumb mistakes I'll fix that ASAP.

In the meantime any idea on how I should take care of the failing test case? (i.e. the Limitations section of the PR)

jeremy · 2014-05-05T17:13:40Z

Agree that test case doesn't even make sense.

Also, major 👍❤️👍 for fixing has_many as well 😁

byroot · 2014-05-05T18:07:10Z

Agree that test case doesn't even make sense.

Great, but what should I do? Just deleting it?

byroot · 2014-05-05T21:50:54Z

activerecord/lib/active_record/counter_cache.rb

@jeremy So now I pre-update all foreign keys in separate UPDATE statements so I can bail out if it has already been updated.

Note that it only prevent race conditions where the same UPDATE is performed multiple times concurrently.

If the updates are different then the the counter will be desynchronized, the only way to prevent that is to lock and reload the record.

So it's definitely more work for the DB as discussed here: #14735 (comment)

Are you ok with that solution?

byroot · 2014-05-12T21:09:54Z

@jeremy ping?

jeremy · 2014-07-23T05:51:44Z

Looking good @byroot. Could use a second pair of eyes to review the concept and implementation more closely as well. cc @matthewd @eileencodes @tenderlove?

eileencodes · 2014-07-23T12:43:04Z

activerecord/lib/active_record/associations/belongs_to_association.rb

Personally I don't like when assignment happens in the conditional like this — I think it can be confusing to read and I always have to think twice about what the code is doing. Not sure what Rails prefers though 😄

This isn't a deal breaker for me but a personal preference.

It's a fairly common pattern in Ruby community (and C).
As for it's usage throughout Rails code base:

$ ag --nogroup 'if \w+ =' | wc -l 516

Actually, my regexp was wrong, it's less than that:

$ ag --nogroup 'if \w+ =\s' | wc -l 162

I know it's pretty common usage but like I said it's a personal preference but not a dealbreaker. 😸

also sorry that I posted the first message 3 times - github got weird 😳

Isn't this essentially checking the existence of the "klass" variable twice? previous_klass checks it with a try, and now we check it again here? Can we do the check once?

I don't see how.

It's a very bad time to perform the increment because the foreign_key change is not persisted yet and it happen outside the save transaction, so if the update fail the increment is not rollbacked.

byroot · 2014-08-03T16:21:36Z

I rebased master and I'm now puzzled by a broken test concerning optimistic locking and polymorphic associations with counter cache:

  def test_polymorphic_destroy_with_dependencies_and_lock_version
    car = Car.create!

    assert_difference 'car.wheels.count'  do
      car.wheels << Wheel.create!
    end
    assert_difference 'car.wheels.count', -1  do
      car.destroy
    end
    assert car.destroyed?
  end

On master car.wheels_count is not updated:

 (0.1ms)  SAVEPOINT active_record_1
SQL (0.4ms)  INSERT INTO "cars" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", "2014-08-03 16:07:21.105885"], ["updated_at", "2014-08-03 16:07:21.105885"]]
 (0.0ms)  RELEASE SAVEPOINT active_record_1
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.0ms)  SAVEPOINT active_record_1
SQL (0.8ms)  INSERT INTO "wheels" DEFAULT VALUES
 (0.1ms)  RELEASE SAVEPOINT active_record_1
 (0.0ms)  SAVEPOINT active_record_1
SQL (0.2ms)  UPDATE "wheels" SET "wheelable_id" = ?, "wheelable_type" = ? WHERE "wheels"."id" = 1  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.1ms)  RELEASE SAVEPOINT active_record_1
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.1ms)  SAVEPOINT active_record_1
FunkyBulb Load (0.2ms)  SELECT "bulbs".* FROM "bulbs" WHERE "bulbs"."name" = ? AND "bulbs"."car_id" = ?  [["name", "defaulty"], ["car_id", 1]]
FailedBulb Load (0.1ms)  SELECT "bulbs".* FROM "bulbs" WHERE "bulbs"."name" = ? AND "bulbs"."car_id" = ?  [["name", "defaulty"], ["car_id", 1]]
Engine Load (0.1ms)  SELECT "engines".* FROM "engines" WHERE "engines"."car_id" = ?  [["car_id", 1]]
Wheel Load (0.1ms)  SELECT "wheels".* FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
SQL (0.1ms)  DELETE FROM "wheels" WHERE "wheels"."id" = ?  [["id", 1]]
SQL (0.1ms)  DELETE FROM "cars" WHERE "cars"."id" = ? AND "cars"."lock_version" = ?  [["id", 1], ["lock_version", 0]]
 (0.0ms)  RELEASE SAVEPOINT active_record_1
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]

But in my branch it is, which bump the lock version and make the test fail:

 (0.0ms)  SAVEPOINT active_record_1
SQL (0.3ms)  INSERT INTO "cars" ("created_at", "updated_at") VALUES (?, ?)  [["created_at", "2014-08-03 16:10:52.696002"], ["updated_at", "2014-08-03 16:10:52.696002"]]
 (0.0ms)  RELEASE SAVEPOINT active_record_1
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.0ms)  SAVEPOINT active_record_1
SQL (1.1ms)  INSERT INTO "wheels" DEFAULT VALUES
 (0.1ms)  RELEASE SAVEPOINT active_record_1
 (0.0ms)  SAVEPOINT active_record_1
SQL (0.3ms)  UPDATE "wheels" SET "wheelable_id" = 1 WHERE "wheels"."id" = ? AND "wheels"."wheelable_id" IS NULL  [["id", 1]]

SQL (0.2ms)  UPDATE "cars" SET "wheels_count" = COALESCE("wheels_count", 0) + 1, "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "cars"."id" = ?  [["id", 1]]
SQL (0.1ms)  UPDATE "wheels" SET "wheelable_id" = ?, "wheelable_type" = ? WHERE "wheels"."id" = 1  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.0ms)  RELEASE SAVEPOINT active_record_1
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.1ms)  SELECT COUNT(*) FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
 (0.0ms)  SAVEPOINT active_record_1
FunkyBulb Load (0.1ms)  SELECT "bulbs".* FROM "bulbs" WHERE "bulbs"."name" = ? AND "bulbs"."car_id" = ?  [["name", "defaulty"], ["car_id", 1]]
FailedBulb Load (0.1ms)  SELECT "bulbs".* FROM "bulbs" WHERE "bulbs"."name" = ? AND "bulbs"."car_id" = ?  [["name", "defaulty"], ["car_id", 1]]
Engine Load (0.1ms)  SELECT "engines".* FROM "engines" WHERE "engines"."car_id" = ?  [["car_id", 1]]
Wheel Load (0.1ms)  SELECT "wheels".* FROM "wheels" WHERE "wheels"."wheelable_id" = ? AND "wheels"."wheelable_type" = ?  [["wheelable_id", 1], ["wheelable_type", "Car"]]
SQL (0.1ms)  DELETE FROM "wheels" WHERE "wheels"."id" = ?  [["id", 1]]
SQL (0.1ms)  DELETE FROM "cars" WHERE "cars"."id" = ? AND "cars"."lock_version" = ?  [["id", 1], ["lock_version", 0]]

So I tend to think the bug is on master, since the Wheel.create! should increment the counter.

@tenderlove what do you think?

ashanbrown · 2014-12-06T20:09:37Z

Just wondering if this bug is still out there. I believe it is and I've been using the counter culture gem instead in my rails4 app. I am wondering if perhaps none of this logic (aside from the code to show the cached count) should even be part of the rails core since it does seem so hard to get right. A concern I would have about completely stripping it out and just letting third party gems extend the association to redefine size is that, as I saw with the octopus gem, extending an association seems to trigger lots of constant cache invalidation in mri . Just my two cents. Would love to know if there's been progress on this. thx.

byroot · 2014-12-06T20:18:10Z

The bug is still there. The race condition on destroy have been fixed, but it can still happen on update.

It's indeed very hard to get right, or at least without big drawbacks like generating more queries, and I definitely recommend against updating a foreign key that had a counter cache.

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.

robin850 added the activerecord label Apr 24, 2014

This was referenced May 5, 2014

Bug: counter_cache update twice #13304

Closed

update behavior of counter_cache #9236

Closed

jeremy reviewed May 5, 2014
View reviewed changes

byroot reviewed May 5, 2014
View reviewed changes

eileencodes reviewed Jul 23, 2014
View reviewed changes

byroot added 2 commits August 3, 2014 11:20

Stop incrementing counter caches on association assignation

867ae87

It's a very bad time to perform the increment because the foreign_key change is not persisted yet and it happen outside the save transaction, so if the update fail the increment is not rollbacked.

Reduce the risk of race conditions on counter cached foreign key update

699b4f7

RobinClowers mentioned this pull request Aug 20, 2014

design intent of counter_cache? #8997

Closed

jonatack mentioned this pull request Mar 29, 2015

Record cache increments by only one with counter_cache and << with a collection #19550

Closed

JustinLove mentioned this pull request May 12, 2015

Editing a child page resets it to child of root page comfy/comfortable-mexican-sofa#644

Open

rails-bot assigned rafaelfranca Oct 20, 2015

kamipo mentioned this pull request Sep 18, 2018

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

Merged

kamipo closed this in #33913 Sep 20, 2018

Conversation

byroot commented Apr 23, 2014

Rationale

Limitation

Further work

Uh oh!

byroot commented Apr 23, 2014

Uh oh!

jasl commented Apr 23, 2014

Uh oh!

byroot commented May 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byroot commented May 5, 2014

Uh oh!

jeremy commented May 5, 2014

Uh oh!

byroot commented May 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byroot commented May 12, 2014

Uh oh!

jeremy commented Jul 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byroot commented Aug 3, 2014

Uh oh!

ashanbrown commented Dec 6, 2014

Uh oh!

byroot commented Dec 6, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants