Move counter cache update back into transaction

[magnusvk]

Currently, the counter cache update happens outside of the transaction in the after_commit hook. This is to avoid a historical deadlock issue in Postgres. However, this should be fixed in Postgres 9.3, according to this and the release notes.

Given that, it should be optional in counter_culture whether the update should happen in or outside of the transaction. It should default to inside the transaction.

[magnusvk]

cc @DavidMikeSimon

[c0m3tx]

Hi,
Any news about this issue?

P.S. don't know why but I'm unable to use test_after_commit gem. It just doesn't work. Maybe a conflict with the latest version of Rails?

[magnusvk]

No news, no. Feel free to work on it though! 😄 I've never worked with the test_after_commit gem, so no ideas there. How are you trying to use it?

[key88sf]

@magnusvk Would the path to doing this involve something like:

1. Creating an after_save_action gem similar to your after_commit_action, but instead it runs things after saves instead of commits?
2. Modifying https://github.com/magnusvk/counter_culture/blob/master/lib/counter_culture/counter.rb#L44 to use this new execute_after_save action ?

[magnusvk]

I think it's even simpler than that. It'd just need a method that conditionally yields directly or calls execute_after_commit.

[kulte]

@magnusvk Check out #145. Let me know if you'd like me to add any specs, but I'm not really sure how to reliably test what happened inside and outside of an ActiveRecord transaction.

[magnusvk]

This is no longer the case in the just-released version 1.0.0. Thank you @kulte for the pull request.

[jbritten]

I'm experiencing deadlocks using MySQL & counter_cache at scale (database under heavy load with many concurrent requests competing to update the counters).

Any chance the "execute_after_commit" option can be added back? I don't understand why it was removed from 2.x in the first place. Seems that whether to update the counters inside or outside of the transaction should be left to the user to decide, right?

I cannot revert to the 1.x branch because I need other improvements in the 2.x branch (i.e. bug fixes for discard integration).

[magnusvk]

Sorry you're having issues with this. Interestingly, this issue was first added because of deadlock issues with Postgres. It's somewhat ironic that it's now MySQL causing a similar issue.

In any case, the history of removing this comes from a bugfix to allow multiple saves in one transaction, see 81dfbf5. Plus, in the absence of issues like this with the database layer, I don't see why you'd ever want to push the counter cache update outside of the transaction. But then again, you are seeing database issues.

To address your immediate issue, you could probably run a fork of current master with 7639b08 reverted relatively easily. Now that's not a great long-term solution, obviously, but just throwing that out there.

If you can figure out how to write a test for this behavior, I think I'd probably be down to re-add this functionality, given that the original code for this was quite straightforward.

This is the first I'm hearing of this issue, so not sure how widespread it is, but of course still would be good to figure out how to address this for you. @jbritten

[jbritten]

Thank you for summarizing the history of this change and providing work-around suggestions. Ironically, I received another deadlock exception while reading your reply. I have opened a new issue to track adding back the execute_after_commit feature: #263

[magnusvk]

Yeah good call to move this to a new issue!

…

On Jul 1, 2019, at 10:28 AM, Justin Britten ***@***.***> wrote: Thank you for summarizing the history of this change and work-around suggestions. Ironically, I received another deadlock exception while reading your reply. Would you be okay with me opening a new issue to track adding back the execute_after_commit feature? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.