rails/rails — pull request #14735 by byroot (2014-04-13 16:54:54+00:00)
Title: Idempotent counter caches, fix concurrency issues with counter caches

Fixes #7965 among other that I remember but can't find anymore.

# Problem

In SQL a `DELETE` statement is inherently idempotent. You can repeat it indefinitely, it will always succeed.

On the other hand counter cache decrementation is not. So if you have 2 process following this scenario:

- Process 1 load the record
- Process 2 load the record
- Process 1 destroy the record
- Process 2 destroy the record

Then the counter cache is decremented twice.

# Solution

This patch make the decrementation on `after_destroy` so it can check the `affected_rows` value returned by the database to only perform the decrementation if a record was actually deleted.

We (Shopify) are running a [monkey patch version](https://gist.github.com/byroot/9809265) of this PR since 3 months now, and so far the problem disappeared.

# Implementation

So far I'm not very happy with my implementation, but I think the only way to make it better is to make the counter cache logic happen in `destroy_row`, `insert_row` etc, instead of doing it in callbacks.

But it mean moving a lot of code, so I prefer to validates the idea before attempting it.

# Non addressed issues

The counter update that happen when you change the foreign key, is subject to the same issue, but way harder to fix using this pattern. It would mean updating foreign key in distinct `UPDATE` queries before the main update, and checking the `affected_rows` for each before incrementing / decrementing the counters.

So it's feasible but again I prefer to validate the idea before attempting it.

/cc @arthurn @jonleighton @tenderlove : What do you think?