ActiveRecord should use savepoints for nested transactions

Hey Laas, nice work and good idea. I did some work with savepoints but found they are not too useful in the general case because Foo.transaction { ... } typically means 'require execution within a transaction', not 'require a new transaction'.

How about a module method to declare the transactional requirements of its methods:

class Person < ActiveRecord::Base
  transactional :requires => [:save, :destroy],
    :unsupported => :method_affecting_nontransactional_resource
end

class OrderTest < Test::Unit::TestCase
  transactional :require_new => :run, :nested => /^test_/
end

Hello!

Nice to hear that my ideas are welcome!

I think that in general case (as in testing with transactional fixtures) the code that uses a transaction does not have to know about enclosing transactions, so when it rolls back a transaction, it expects it to roll back to the nearest Foo.transaction{} declaration, therefore it should mean 'require new transaction'.

When it is interpreted as 'require execution in transaction' the following code will have unexpected results:

foo.name = "before"
foo.save
Foo.transaction(foo){
  foo.name="after1"
  foo.save
  begin
    Foo.transaction(foo){
      foo.name="after2"
      foo.save
      raise
    }
  rescue
    puts foo.name
  end
}

In current AR implementation, this would output "after2", but expected result would have been 'after1', because the inner transaction should have been rolled back, not just ignored.

I think that when knowingly nesting transactions, people expect them to roll back to the nearest declaration and when unaware of the nesting (e.g. in testing correct handling of rollback), they would be very surprised about the result (as above).

Your idea of requirement declaration is not bad, but I think it would mean unnecessary extra work for both AR developers to implement this behavior and to Rails users to declare all uses of nested/not-nested transactions etc.

IMHO making AR aware of savepoints does not affect the majority of rails users who do not need/use nested transactions as the savepoints are never created. But it does help those, who decide to use nested transactions and would spend days on debugging the results before digging into AR code to find out that inner transactions are just ignored (as I did).

As having savepoint when not knowing about them does not do anything bad, I vote for turning savepoints on by default.

PS. For the next month I will have very infrequent e-mail access, so I might not answer questions very promptly and do not have time to do proper dev work on this matter. Back on-line again in august.

Best, Laas

+1

I've resorted to turning off transactional fixtures because of this problem.

Alternative method.

Just thought I´d add my two cents, having had to deal with this problem recently too. Sorry it´s not nicely packaged as a plugin, but you can drop it in your lib directory and require it in config/environment.rb. It is only a PostgreSQL one... but it should work for others that support saveponts, by just changing the name of the class and fixing the SQL for your database, if necessary.

How this one works:

• Replaces the rollback_db_transaction, commit_db_transaction and begin_db_transaction. As all transactions (in the core, you might need to egrep for any '\<(BEGIN|START|TRANSACTION)\>' s in your vendor plugins) call these three functions only for, it will work for transactions anywhere.
• Maintains a transaction nest count and starts a virtual nested transaction, using a series of savepoints if a new transaction is started from within an existing one.

It does require that calls to these functions are symmetric, ie. that each begin call is terminated by exactly one commit or rollback (which they are in the core rails).

Basically, it differs from the above approach in that it slightly 'lower level'.

I find it useful for avoiding all the problems with unit tests and fixtures and calling destroy and save from other transactions (as those two functions are implicitly wrapped). It will stop these committing in unit tests and producing all those warnings about transactions already started.

This appears to be a derelict ticket, but hopefully I can revive some interest/discussion. I just ran into this problem when making some tests to check some transaction code. The tests insisted things were broken but manually performing those actions caused no problems.

While I agree that needing nested transactions in the actual application probably isn't that common, it seems as though anyone who writes any transaction code in their application (nested or not) that doesn't return an exception outside the outermost block will run into problems when they try to test it.

For now I've turned off transaction fixtures, but I really would like them on. Is there any hope for this change? The savepoint idea above seemed fairly promising, but it looks like the plugin hasn't been touched in almost a year so I don't know how viable it is at this point. If not, maybe include something in the docs indicating this could be a problem?

I've been using the transactional migrations plugin for a while now with great success, all my tests use transactional migrations. Would be great if this was integrated to rails core.

transactions are deprecated

"Object transactions" are deprecated, but as far as I know normal database transactions are not deprecated so I don't think this ticket should be closed.

The patch leads to a proliferation of savepoints when flat transactions are almost always what's needed.

Best implemented as a plugin with explicit savepoints.

I really think you should reconsider this patch. The arguments against it seem to be

1. No one ever needs it.
2. It will significatly increase overhead

I don't think the first is true. Suppose someone had code like the following in their controller

begin
  myfoo = Foo.new
  mybaz = Baz.new
  Bar.transaction do
    mybar = Bar.new
    mybar.prop = "invalid value"
    mybar.save!
    myfoo.prop = "foo!"
    mybaz.prop = "baz!"
  end
  myfoo!
  mybaz!
rescue Exception => boom
  logger.error("Crikey! Invalid save attempt! Better look into that...")
ensure
  #some stuff
end

It would work when actually used in the interface, but any tests depending on the rollback of that transaction would fail because transactional fixtures causes invisible transaction nesting and the exception is caught in the controller. Actually let me get that on a line by itself.

In the above case, there is no bug but the tests fail.

That seems bad to me, and figuring out why the tests fail in this instance is incredibly difficult.

As for the overhead, I'm not an expert but it seems that if most people only need flat transactions then performance in the common case would not be greatly affected. Overall, it seems like this a low cost way to make a commonly misunderstood part of Rails behave as expected and remove some bad side effects along the way. If this particular patch is not good enough, I'd be happy to work something up.

It just doesn't come up much. Almost never. I appreciate your opinion here, but let's see it in plugin form garnering wide adoption first.

It just happened to me in one of my test cases. I believe this might come up more frequently than you assume.

You can still test that a rollback occurred. I encourage you to write the plugin and see how it fares!

The plugin is almost a year old now, still works fine though: http://rubyforge.org/projects/arnesttransacts/

Cool, that's a good starting point.

The plugin overuses savepoints: nested transaction transaction blocks are typically equivalent to EJB 'requires' declaration, but this plugin starts new pseudo-transactions for each, the equivalent of the far less common 'requires new' declaration.

If I understood you correctly, the problem is that for every "transaction { }" block a new (savepoint) transaction is created instead of the current one being reused (if one already exists) unless a new transaction is somehow explicitly required?

If so, then how do you think this requirement should be specified? Should implicit transactions inside save() and destroy() automatically require a new transaction?, should it somehow be optional? or should they always execute within their parent transaction if one exists and create one if not?

I'm also a bit concerned about the amount of savepoints being created, but I don't have a clear idea on how to best solve it.

Yes, exactly. It breaks the existing transaction { ... } semantics.

For the plugin, I'd pass an argument to transaction or use a different method to explicitly indicate that you want to start a new transaction.

For example,

transaction { transaction { whatever } }

gives SQL

BEGIN
# whatever
COMMIT

whereas

transaction do
  transaction :requires_new do
    whatever
  end
end

gives SQL

BEGIN
SAVEPOINT marker
# whatever
RELEASE SAVEPOINT marker
COMMIT

I don't know a good API to express this, though. Ideas?

Perhaps

# Requires transaction, but does not start if one's already active.
transaction do
  # Requires transaction. Always start a new one.
  transaction :force => true do
    whatever
  end
end

There is also another issue, the implicit transactions, should it be possible to make an activerecord class default to running create/update/save/destroy inside an explicit transaction or should users who need explicit transactions always wrap those method calls inside a transaction block that forces a transaction?

If the former, then I'd suggest something like this:

class MyRecord < ActiveRecord::Base
  requires_new_transaction
  # or
  self.requires_new_transaction = true
end

Or perhaps even more flexible:

class MyRecord < ActiveRecord::Base
  requires_new_transaction :only => [ :create, :destroy ]
end

I've attached a patch (against edge but should work with 1.2.3 with minor changes) which implements nested transactions similiar to the original arnesttransacts plugin, but also uses the savepoint naming convention from the "Alternative method" implementation posted in this ticket.

In addition to nested transactions there is also support for configuring "implicit" transaction type for activerecord classes. By default every activerecord class should behave as it does with the current rails transaction implementation (does not create another transaction if it already runs inside a transaction) but it is also possible to make either save (save, save! and update_attributes) or destroy methods to create a nested transaction even if they already run inside a transaction (Or not create a transaction at all. I took the liberty of adding this as another option, not sure if it's a good idea though.)

I also changed transactional tests support a bit so a transaction that is created inside a transactional test does not "see" the "outer" transaction, so an action (that uses transactions) that is run inside a transactional test should behave exactly the same as the action run inside a non-transactional test.

The patch only implements nested transactions for postgresql as it is the only database I'm familiar enough with, if anyone is interested in mysql (or some other db) support it would be great if you could try to add it in the same way it is already done for postgresql. (btw. I'm afraid a couple of tests now give different results depending on if the database has nested transactions support, this is because now the code inside the test does not see the outside transaction and thus the tested SQL query count changes (because of the savepoint creation), for these tests transactions should probably be switched off)

I'm considering also releasing this patch as a plug-in but at the moment I don't have a good place for hosting it yet.

Could anyone from the core team comment on the chances of something like this getting added to rails?

Oh, another thing. Currently my patch only allows configuring transaction type per-activerecord-class, perhaps it would be a good idea to allow global default configuration so one could say that all implicit transactions have to be nested by-default or that there should be no implicit transactions at all?

Reopening this so someone would notice it again.

Added Mysql support (in addition to Postgresql)

We are also experiencing the bug that no one would ever experience unless they used transactions and tested them.

Thanks for the patch tarmo. I applied it against 8142 and got the following test failures:

[[[

1) Failure:

test_transaction_type_destroy_flat(NestedTransactionsTest)

[./test/abstract_unit.rb:41:in `assert_queries'

./test/transactions_test.rb:370:in `test_transaction_type_destroy_flat' ./test/transactions_test.rb:390:in `with_topic_transaction_options' ./test/transactions_test.rb:369:in `test_transaction_type_destroy_flat' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `send' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:

12 instead of 10 queries were executed. <10> expected but was <12>.

2) Failure:

test_transaction_type_destroy_nested(NestedTransactionsTest)

[./test/abstract_unit.rb:41:in `assert_queries'

./test/transactions_test.rb:376:in `test_transaction_type_destroy_nested' ./test/transactions_test.rb:390:in `with_topic_transaction_options' ./test/transactions_test.rb:375:in `test_transaction_type_destroy_nested' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `send' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:

14 instead of 12 queries were executed. <12> expected but was <14>.

3) Failure:

test_transaction_type_destroy_none(NestedTransactionsTest)

[./test/abstract_unit.rb:41:in `assert_queries'

./test/transactions_test.rb:364:in `test_transaction_type_destroy_none' ./test/transactions_test.rb:390:in `with_topic_transaction_options' ./test/transactions_test.rb:363:in `test_transaction_type_destroy_none' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `send' /opt/local/lib/ruby/gems/1.8/gems/mocha-0.5.5/lib/mocha/test_case_adapter.rb:19:in `run']:

10 instead of 8 queries were executed. <8> expected but was <10>. ]]]

Have you tested your patch against a version of rails that recent? Unfortunately mysql transactions are outside my current skill level & project scope except as black boxes, but I'm loathe to turn off transactional fixtures because I want to keep everyone in my project using autotest and they have slow pcs :(

Can you let me know a version of edge that the tests in your patch pass against?

I'm guessing the behaviour of destroy has changed since the patch was created.

Hi, I've run into this problem with tests failing because transactions aren't nested. bitsweat, would you recommend not using transactions in unit tests?

Replying to brian.wong:

Hi, I've run into this problem with tests failing because transactions aren't nested. bitsweat, would you recommend not using transactions in unit tests?

I haven't tested this but I think you can disable transactions for a specific test with "uses_transaction :method_name".

From activerecord/test/associations_test.rb:

  uses_transaction :test_dependence_with_transaction_support_on_failure
  def test_dependence_with_transaction_support_on_failure
    ..
  end

Replying to lastobelus: I've not tested the patch for a long time as I've been quite busy and the and it's not easy to get a patch like this accepted, although I'm hoping it will eventually get accepted in some form.

The tests seem to only fail if you run all the tests at once, but not if you only run "ruby -I connections/native_postgresql transactions_test.rb". So I'm not really sure what the exact issue is. I'll see if I can find out.

tarmo, we're having some trouble understanding how the patch is intended to work. Should it make tests with transactions with caught exceptions pass, or is it intended that we have to change our code to XXXX.transaction(:force => true) for tests with transactional fixtures & transactions to pass?

Currently our unit tests with transactions still fail with the patch, but pass if we change to using XXXX.transaction(:force => true) they pass, and we aren't sure if that is intended or if it is a result of the patch not working correctly with current edge rails.

Replying to lastobelus: I think the last patch is broken, not sure why. My first patch uses the Thread.current[:transactional_test] variable to decide if another transaction should be opened but the second patch for some reason is missing that part which is why your transactional tests don't get a nested transaction in the code that they test. You are right though that the intention is for the code inside the transactional test to get another nested transaction (which from the perspective of the code itself would be the first transaction as the tested code should not be aware of the surrounding transactional test's transaction).

Restored the support for nested transactions in tests and fixed some assert_queries() which with the nested transactions support are getting two more queries for the transaction creation. The broken delete tests indeed appear two now have two more queries, unrelated to the nested transactions.

Adding support for Oracle would be dead simple: it uses the same syntax as MySQL and Postgres for create_savepoint and rollback_to_savepoint. It doesn't support releasing savepoints, however.

I've made a new plugin that works with the latest git version. Supports :force => true as an option to transaction to specifiy a savepoint, and fixes the issues with tests and transactional fixtures.

http://svn.viney.net.nz/things/rails/plugins/savepoints/

Copied over to the new rails lighthouse: http://rails.lighthouseapp.com/projects/8994/tickets/383-activerecord-should-use-savepoints-for-nested-transactions