- Notifications
You must be signed in to change notification settings - Fork22.1k
Allow disabling transactional callbacks on a model#56376
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
| self.transactional_callbacks=false | ||
| end | ||
| deftest_save_without_callbacks_does_not_materialize_transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's worth adding at least one test case that asserts the transactionis opened on the connection whentransactional_callbacks istrue.
There's no test case in this file that explicitly checks for that. The closest, I think, is"test_cancellation_from_before_filters_rollbacks_in_#{filter}" (line 715) that asserts the transaction gets rolled back when the validation fails, which technically does cover this behaviour but in a very oblique way.
eileencodes left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a changelog? I also left a comment about the API.
| :before_commit, | ||
| scope:[:kind,:name] | ||
| class_attribute:transactional_callbacks,instance_writer:false,default:true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this make sense as an application wide setting as well? Something likeconfig.active_record.disable_transactional_callbacks = true?
I also think the controller level API should bedisable_transactional_callbacks and default tofalse. So it's clearer what's happening in a controller
All validations and model callbacks are wrapped into
BEGIN/COMMITfor transactional consistency.However, in a large-scale apps with high concurrency and high DB connections usage, that brings the downside of that
BEGINholding the connection from being reused by another workers.On top of that, in a large scale app, you'd run in isolation level like
READ COMMITTEDwhere keeping callbacks part of the same txn doesn't give you much.I've tried to write a version of this PR that would automatically kick in if the model has no callbacks, but then I've realized that validations are also part of this. And there's no good way to check whether validations are pure Ruby (
validates :author_id, presence: true) or actual database (validates :author, presence: true).So I've opted to make this a manual thing per model.
This would already help us at Shopify to save some connections usage across some very hot paths.