Make ActiveRecord::Rollback bubble to outer transactions

[nikolai-b]

Summary

Re-raise the error till it reaches a savepoint or the top transaction.
The reason to stop at savepoint / requires_new transactions is that
they are actual nested transactions so are the smallest unit the
database can rollback.

Other Information

This is a breaking change but the current behaviour is documented as
being "surprising".

[matthewd]

Thanks for looking at this.. the fact Rollback can be silently swallowed by a block that isn't actually going to rollback has long been a concern to me.

I've actually been digging into transaction behaviour recently, with one side-goal being to come at this from the other direction, and hopefully move to requires_new: true as the default.

However, surprising (and IMO unreasonable) as it is, given how long this has been around, it seems safe to imagine that someone has a vital workflow that's [perhaps unknowingly] depending on this historical behaviour.

What do you think of putting in a deprecation warning for a version first?

For now we could use the same conditional, but then instead of re-raising, just warn that we will re-raise in future. In line with my planned change, we could introduce a new better name for requires_new: false, and then anyone wanting to skip the deprecation and go straight to the new behaviour could use that new name.

[nikolai-b]

A deprecation seems sensible given it has been here forever and I'm happy to hear you also want to remove the behaviour 😄 . Given the test failures it would have to handle existing rails code somehow where the rollback is expected to be swallowed.

If I change this to a deprecation warning and fix the rails code so they somehow don't warn would that fit your suggestion?

[matthewd]

I think so!

For the tests, we'll probably need to individually consider whether the test is covering the behaviour as documented [that we're deprecating] -- in which case the test would remain, but now also assert an expected deprecation; or whether it's best fixed by changing it to use a real transaction, or does specifically want the current behaviour [unlike the deprecation case, this would be "will still want to work like this even afterwards"].

[nikolai-b]

Thanks, looking at the failures most of them in the rails codebase should still work, for example saving a model with an associations, very roughly rails is often doing something like:

class OtherThing
  def save
    raise ActiveRecord::Rollback if invalid?
  end
end

class Thing
  has_many :other_things

  def save
    transaction do
      other_things.each(&:save)
      raise RecordNotSaved unless other_things.all?(&:persisted?)
    end
  end
end

I think the easiest thing would be to add an option to the transaction to say that you want it to swallow Rollback errors which would ignore the deprecation. It is complicating the system with a another option though so I'm not that happy with it. WDYT?

[matthewd]

My first instinct is that those should be upgraded to requires_new. Rollback into a not-actually-rolledback transaction is fine to communicate that the save aborted [and never attempted the UPDATE], but it's still possible that a before_save callback wrote to the database at the same time... it's again a behaviour change for us to start actually rolling those back, but it surely seems like what should be happening.

(#44526 is partly building toward a requires_new-by-default world, by making extra nested transactions zero-cost if they don't have other interleaved queries.)

[nikolai-b]

That would cause many more rails transactions to use nested transactions when I don't think that is wanted. This issue is motivated for us because our postgresqls database was running very slow due to SAVEPOINTs usage. Our reason for blaming SAVEPOINTS was because simple SELECT * WHERE ID = ? queries were waiting for many seconds on SubtransSLRU locks (shown in pg_stats). There are other people who have had issues with postgresql's SAVEPOINTs gitlab, postgres.ai, cybertec-postgresql. I believe in MySQL high usage of nested transactions can also be a bottleneck but I'm much less sure.

I like the idea of only using a real nested transaction if it is needed but I also don't want to introduce more nested transactions in general. I was thinking of

def transaction(swallow_rollback_errors: false)
  ...
rescue ActiveRecord::Rollback
  if !swallow_rollback_errors && !requires_new && transaction_open?
    ActiveSupport::Deprecation.warn(<<-MSG.squish) 
      An ActiveRecord::Rollback error is being raised inside a nested transaction without `requires_new: true`
      which will not rollback the outer transaction.  Please use `swallow_rollback_errors: true` 
      if you want this behaviour.  In future rails versions ActiveRecord::Rollback will be re-rasied
      so the outer transaction will be rolled back also.
    MSG
  end
end

[matthewd]

Thanks, that's interesting context on wanting to avoid too many savepoints. It sounds like there might be future improvements in Postgres to make it less of a danger-zone, but even assuming they merge, they're still a ways off.

I suggest we add a new join_existing: true, which will mean requires_new: false, but will re-raise Rollback.

Then we add the deprecation warning much as you describe for existing callers. They can avoid the deprecation warning by choosing either requires_new: true or join_existing: true.

I'd prefer not to add a swallow_rollback_errors-type option to the public API, so I wonder if we can change the internal callers who need that to use join_existing: true, and then wrap a silent rescue Rollback around it themselves?

[nikolai-b]

Hi @matthewd, is this what you were suggesting?

[matthewd]

Hey @nikolai-b, sorry to keep you waiting, and thanks for the ping.

I'd felt that there was a surprising quantity of join_existing: true needed in the test suite, but hadn't had a chance to dig in. I think it's mostly down to checking joinable where we need current_transaction.joinable?, but I found the conditionals a bit opaque, so I've just had a go at an alternative arrangement:

def transaction(requires_new: nil, isolation: nil, joinable: true, join_existing: nil, &block)
  if !requires_new && current_transaction.joinable?
    if isolation
      raise ActiveRecord::TransactionIsolationError, "cannot set isolation when joining a transaction"
    end
    did_join = true
    yield
  else
    transaction_manager.within_new_transaction(isolation: isolation, joinable: joinable, &block)
  end
rescue ActiveRecord::Rollback
  if did_join
    if join_existing
      raise
    else
      # Deprecation Plan: Default requires_new to !join_existing. Always raise if did_join.
      ActiveSupport::Deprecation.warn(<<-MSG.squish)
        ActiveRecord::Rollback was raised inside a `transaction` block that has not created a separate
        transaction. This interrupts the inner block, but does not rollback its changes.

        In Rails 7.2, `transaction` will always create a nested database transaction by default, and
        ActiveRecord::Rollback will have full effect.

        To opt in to the future behavior, use `transaction(requires_new: true)`.

        Alternatively, use `transaction(join_existing: true)` to re-use the parent transaction, but
        re-raise ActiveRecord::Rollback up to the parent.
      MSG
    end
  else
    # rollback is silently swallowed; within_new_transaction has already done the work
  end
end

(This is where I admit I've only written the above in my browser, and not confirmed it actually works as expected 🙈, but...) I believe that should behave the same in all of our interesting cases, but should also mean most of the other tests stop needing join_existing to avoid the deprecation.

---

I did also make a substantive change to the deprecation message: despite the considerations you raised around why one might wish to avoid savepoints, for the average user I think it's most important our API does what its name implies -- here, "transaction do should wrap its content in a transaction".

The new re-raise behaviour of join_existing is a big improvement for cases where the caller does want to re-use a parent / avoid a savepoint, but it's still imperfect: a nominally-rolled-back transaction block can be brought back to life by a rescue that exists entirely outside of it. That's fine in our internals, where we know that won't happen, and a fine trade-off to allow a savepoint-wary caller to accept, but still just feels wrong to have as the default.

[nikolai-b]

Eeek, I understand you want to make the transaction API make more sense but I'd urge you / Rails to trial this with some stacks that are using Rails and Postgresql at scale. I fear making all Rails nested transactions use a real subtransaction by default might grind some of their databases to a halt (ours for one would not cope). Is there any evidence I could provide that would make you reconsider relying more on sub transactions? Sorry for the direct ping but @stanhu as you wrote that Gitlab article maybe you'd have some more insight into the ramifications of rails defaulting to use more subtransactions.

[matthewd]

I fear making all Rails nested transactions use a real subtransaction by default might grind some of their databases to a halt (ours for one would not cope).

Does your application have that many nested explicit transaction calls? Given their limitations (especially before this PR, but also after), can you help me understand why?

[nikolai-b]

I did a quick patch of Rails' transaction method so I could give you some lots of examples from our codebase but it is hard because rails uses nested transactions so much, e.g.

rails/activerecord/lib/active_record/associations/collection_association.rb

Lines 234 to 243 in 75e6c0a

	def replace(other_array)
	other_array.each { |val| raise_on_type_mismatch!(val) }
	original_target = load_target.dup
	if owner.new_record?
	replace_records(other_array, original_target)
	else
	replace_common_records_in_memory(other_array, original_target)
	if other_array != original_target
	transaction { replace_records(other_array, original_target) }

calls replace_records which calls concat which opens another transaction

rails/activerecord/lib/active_record/associations/collection_association.rb

Lines 119 to 125 in 75e6c0a

	def concat(*records)
	records = records.flatten
	if owner.new_record?
	load_target
	concat_records(records)
	else
	transaction { concat_records(records) }

or

rails/activerecord/lib/active_record/persistence.rb

Lines 797 to 804 in 75e6c0a

	def update!(attributes)
	# The following transaction covers any possible database side-effects of the
	# attributes assignment. For example, setting the IDs of a child collection.
	with_transaction_returning_status do
	assign_attributes(attributes)
	save!
	end
	end

where you can see update! calling save! and both create transactions:

rails/activerecord/lib/active_record/transactions.rb

Lines 301 to 303 in 75e6c0a

	def save!(**) # :nodoc:
	with_transaction_returning_status { super }
	end

The same is true for us. We often do a few small things in a transaction and sometimes also group these small operations together. So far this hasn't been an issue because transaction inside transaction is a no-op. For example we allow an object to be ⭐ ed, which does the starring in a transaction, or you can do multiple starrings which creates a top level transaction and then each staring creates its own. This example is clearly trivial so if you would like more from our actual code I'd be happy to spend the time.

[matthewd]

Hmm, fair enough. As long as you never rescue, they are equivalent (and, in line with the sort of changes we already have in this PR, I expect we can rely on that assumption to elide most, if not all, Rails-internal transaction nesting)... I just don't think it's reasonable for us to default to assuming that user code never rescues. And I especially don't think it's reasonable for us to do that in order to maintain an accidental, partial, work-around to a newly-identified performance edge case in another piece of software.

Where the caller is prepared to commit to not badly rescuing an inner exception, join_existing allows them to make that promise. Without that, "fast but potentially data-destructively-wrong" seems like an unnecessarily bold choice.

[nikolai-b]

As long as you never rescue, they are equivalent

To be clear are your saying that my proposal in the PR is equivalent to using savepoints (so long as the user doesn't rescue ActiveRecord::Rollback)? I believe you are also saying that Rails should do the right thing and use savepoints for nested transactions and not be stopped by the fact that it might cause issues with postgresql, basically it would be up to postgresql to fix that bottleneck.

I think that is valid 😄 but it would still make our database unusable and a believe it would do the same for others. I don't have a good solution as if savepoint worked well I'd be in favour of using it. Can Rails at least wait till there is a version of postgresql out that handles savepoints better before using savepoints more heavily? It looks like there are some patches that might improve things.

To be sure you are saying the difference between my proposal (using join_existing) and yours (using savepoints) is this:

def do_something_else
  ActiveRecord::Base.transaction do
    thing.create!
    raise ActiveRecord::Rollback
  end
end

ActiveRecord::Base.transaction do
  do_something_else
rescue ActiveRecord::Rollback
end

with savepoints thing would not be persisted, with join_existing it would be persisted.

[matthewd]

To be sure you are saying the difference between my proposal (using join_existing) and yours (using savepoints) is [..] with savepoints thing would not be persisted, with join_existing it would be persisted.

Yes. I think it's a bit clearer if we side-step the complication of Rollback, which (per this PR) has specific current questionable behaviour, and consider any other exception. do_something_else expects that it can start making database changes, safely inside its transaction, and then discover some problem, and raise an exception... when it does so, it expects its changes to be rolled back: that's why it was setting up a transaction. The caller recognises the specific error that occurred, and has some other strategy to handle it, so they rescue and continue... but now do_something_else's "aborted" changes persist, surprising both caller and callee.

it would still make our database unusable and a believe it would do the same for others. Can Rails at least wait till there is a version of postgresql out that handles savepoints better before using savepoints more heavily?

My hope is to pretty much only affect explicit calls to transaction (and only when the nested transaction block follows other in-transaction query activity)... and still provide an option for callers to opt in to the older tag-along-and-hope-for-the-best behaviour: it's just a change of a default, the old behaviour isn't going away.

And at this stage it's only a statement of future intent: the message says "in Rails 7.2", for which we have no scheduled release date, but extrapolating from past releases is a good two years away. That doesn't leave a huge amount of time for Postgres to get ahead of us, because any possible change needs to go through their own release process, but it's very possible it'll be solved by then. And if it's still in the pipeline, you can add join_existing to your transaction calls -- or even monkey-patch to override the default, if you're comfortable with the surprise-outer-rescue risk.

I have to infer your database (and Gitlab's, etc) is in some way an outlier... savepoints are a long established and widely used technology in Postgres, so it just seems implausible that they're "never use this if you want your database to work"-level broken for everyone everywhere, and yet no-one noticed until mid last year. I'm hopeful for an upstream solution to ensure it's always performant, but I don't want to leave the surprising behaviour to trip new users for longer than the already-several-year lead time we're facing today.

[tsrivishnu]

Hi. I have a question related to the using joinable:false, requires_new: true in the current versions (at least 6.0) of Rails and I would like to understand how the future version will behave with the changes from this Pull request.

If a nested transaction is created with joinable:false, requires_new: true, the after_commit callbacks are triggered for records created inside this nested transaction even before the parent transaction is committed. If the parent transaction rollback, we will be left with the after effects of what ever happened in that after_commit.

We could use a after_rollback but might not work all situations. For example, we write change logs to Kafka using after_commit and once a message is pushed, clearing it is not really that easy depending on the setup.

For demonstration:

class User < ApplicationRecord
  after_commit :emit_change_message

  def emit_change_mess
    puts "Changing the user's name: #{first_name} #{last_name}"
    # Log changes to kafka
    ...
  end
end

User.create(first_name: "John", last_name: "Doe")

ActiveRecord::Base.transaction(joinable: false, requires_new: true) do
  User.last.update(first_name: "Transaction 1") # triggers +after_commit+
  ActiveRecord::Base.transaction(joinable: false, requires_new: true) do
    User.last.update(last_name: "Transaction 2")  # triggers +after_commit+
    raise ActiveRecord::Rollback
  end
end
# => Changing user's name: first_name: Transaction 1, last_name: Doe
# => Changing user's name: first_name: Transaction 1, last_name: Transaction 2

User.last.first_name
# => "John"
User.last.last_name
# => "Doe"

In the above example, the changes are not saved in the database as expected. However, the after_commit callbacks are triggered. This is not desired and is not intuitive.

With the proposed changes in this PR with join_existing: true, if we replace joinable: false with join_existing: true, it will function as expected -- all the changes are rollbacked and no after_commit callbacks are triggered. Is that correct?

[foreverLoveWisdom]

• Typo: re-rasied -> re-raised 😄