Feature/nested transactions #27

bcardiff · 2016-12-13T20:11:34Z

Add support for transactions using BEGIN/ROLLBACK/END statements by default.
Add support to mimic nested transactions using SAVEPOINTS.
Transactions statements are executed using non prepared statements (overridable if needed per driver).

Some checks ensure there are no overlapping transactions per connection or per parent transaction.

db.transaction do |tx|
  tx.connection.query ....
  # raise exception to rollback exception will bubble   
  # or tx.rollback or tx.commit
  # or raise DB::Rollback exception will not bubble

  tx.transaction do |inner|
    inner.connection.query ...
    # same as tx for rollback
  end

  other_inner = tx.begin_transaction
  # use other_inner transaction without block
  other_inner.commit
end

* dsl, state checks * define transaction sql commands in connection

bcardiff · 2016-12-13T20:20:13Z

@will after merging will/crystal-pg#66 this will work also in crystal-pg and I will PR the specs of https://github.com/bcardiff/crystal-pg/tree/feature/nested-transactions like in mysql and sqlite.

Your input is welcome for sure.

spalladino · 2016-12-13T20:38:12Z

spec/transaction_spec.cr

+      end
+      db.pool.is_available?(cnn).should be_true
+    end
+  end


I'd add another spec here: ensure that a connection is not returned to the pool if a nested transaction is commited/rolledback, but there is still an outer active one.

spalladino · 2016-12-13T20:40:57Z

src/db/begin_transaction.cr

+        # a design flaw. Transaction might been commited already.
+        # Maybe we should wrap e in another exception that clarifies
+        # this scenario.
+        tx.rollback


+1 to adding unless tx.closed?

If we're at this point, there has already been an exception thrown from the user's code, which will bubble and be catched by the client code. If the tx is already closed, it means that he manually commited or rolled it back, so it makes no sense to me to try and do that again, triggering an additional exception. The design flaw is exposed by the original exception, there's no need for another one.

@asterite maybe you want to break the tie? :-P

done. unless tx.closed? added

spalladino · 2016-12-13T20:42:47Z

src/db/error.cr

@@ -17,4 +17,7 @@ module DB
    def initialize(@connection)
    end
  end
+
+  class Rollback < Exception
+  end


I'm a bit confused about Error vs Exception here. Could you explain?

DB::Error are used when the origin of the exception was an interaction with the database.
Rollback is initiated by the user.

spalladino · 2016-12-13T20:44:00Z

src/db/connection.cr

+    protected def create_transaction : Transaction
+      TopLevelTransaction.new(self)
+    end
+
    protected def do_close
      @statements_cache.each_value &.close
      @statements_cache.clear
      @database.pool.delete self
    end


Is it safe to close a connection without rolling back its transactions? I assume it is, but just checking.

Connection can always go down, so yes, they are not committed by deafault.

spalladino · 2016-12-13T20:45:02Z

src/db/connection.cr


    # :nodoc:
    getter database
    @statements_cache = StringKeyCache(Statement).new
+    @transaction = false


Why not store the TopLevelTransaction object itself here, so we can keep a reference to it from the Connection?

It was simpler to just store the bool.
We can have a TopLevelTransaction or WeakRef(TopLevelTransaction) but if doing that we should deal with explicit management of what will happen when the connection or parent transaction is closed IMO.

we should deal with explicit management of what will happen when the connection or parent transaction is closed

Well, we could, though don't need to do that now. What kind of things were you considering that would need to be managed, and are not being managed now?

If the public api is used I can't think of anything that could go wrong. Right now we don't have errors regarding trying to close a connection explicitly when there is a ongoing transaction.

On nested transaction it is more evident maybe. Commit/Rollback a parent with nested, the user will still have a reference to the inner transaction. Should that transaction be marked as closed in crystal side since it should be closed in the db?

Well, tracking the object force either a refactor of how the BEGIN/CREATE SAVEPOINT is done OR a :nodoc: setter is needed. Because we need to flag the connection as in-transaction before the sql command is sent.

I think I would prefer to keep bools for now, the required change is not that nice. Either way it should not impact to drivers implementors since they don't need to inherit Transaction class, or if done, they should call base constructor.

spalladino · 2016-12-13T20:45:54Z

src/db/transaction.cr

+
+    private def close!
+      raise DB::Error.new("Transaction already closed") if closed?
+      close


Maybe there is an abstract def close missing here? Or is that covered by Disposable?

it is covered in Disposable which is defined also in DB ;-)

spalladino · 2016-12-13T20:47:49Z

src/db/transaction.cr

+
+    def begin_transaction : Transaction
+      raise DB::Error.new("There is an existing nested transaction in this transaction") if @nested_transaction
+      @nested_transaction = true


Same as with @transaction at Connection

spalladino · 2016-12-13T20:51:03Z

src/db/transaction.cr

+                         previous_savepoint.succ
+                       else
+                         # random prefix to avoid determinism
+                         "crystal_#{Random.rand(10_000)}_00001"


I'd increase 10_000 a bit more. Just in case.

1_000_000 ?

the usage of the prefix is also to when dealing with a pool, so not all savepoints of all connections are named equally.

When the same connection is returned to the pool it should have no longer outstanding transactions. dsl and the design enforce that when using the public api. So the risk is about dirty state between checkouts in the same connection. For long living connections the numbers will be reused, but again, between different connections the names can be the same without problems to the db.

We can prefix with the connection.object_id also...

The object_id approach sounds good to me

spalladino · 2016-12-13T20:52:39Z

src/db/transaction.cr

+  class SavePointTransaction < Transaction
+    getter connection : Connection
+
+    def initialize(@parent : Transaction,


-1 to argument wrap. But I'm just being picky.

We could go expose top_level_transaction in Transaction to avoid the wrap. That will allow us to remove SavePointTransaction#create_save_point_transaction but the SavePoint needs:

parent to release/unlock

top_level to increase new savepoint name

connection to exec

I was just talking about coding style here, that is, keep both args to init in the same line :-)

spalladino · 2016-12-13T20:54:39Z

src/db/transaction.cr

+    end
+
+    private def close!
+      raise DB::Error.new("Transaction already closed") if closed?


I'm wondering if we maybe should let just the DB fail in this case, rather than try to play smarter and throw the exception ourselves. However, I can't think of any scenario where we think the tx is closed? but it actually isn't.

I think is safe to track this state and offer better errors in this scenario.
Also it is not clear that the driver can query the status of the transaction, so I will keep this design.

I wasn't thinking about querying the status, but rather simply issuing the close statement to the DB, and catch the "already closed" exception. However, either option is ok for me.

Well, I prefer caution here. If a TopTransaction ref somehow leak at hold a reference to an open connection it could commit/rollback a not intended transaction. It should not happen, unless bugs, but let's keep this.

s/crystal/cr/ to shorten a bit the name.

spalladino · 2016-12-14T14:40:30Z

LGTM!

Brian J. Cardiff added 3 commits December 13, 2016 16:47

Transactions

751f8b2

* dsl, state checks * define transaction sql commands in connection

add todo for corner case of transaction(&block)

9bde768

add nested transaction with savepoints

c491bd8

bcardiff requested a review from spalladino December 13, 2016 20:11

This was referenced Dec 13, 2016

add spec for transactions and nested transactions crystal-lang/crystal-mysql#20

Merged

add spec for transactions and nested transactions crystal-lang/crystal-sqlite3#12

Merged

spalladino reviewed Dec 13, 2016

View reviewed changes

Brian J. Cardiff added 4 commits December 14, 2016 10:57

specs to ensure connection is not returned to pool in nested transaction

882253b

add connection.object_id to savepoint names.

c6b6517

s/crystal/cr/ to shorten a bit the name.

unwrap args

dc26498

allow raise after explicit rollback within #transaction(&block)

ed3ed71

minimal docs for transactions

d7cceca

bcardiff merged commit 8a1824a into master Dec 14, 2016

bcardiff mentioned this pull request Dec 14, 2016

Transactions #6

Closed

drujensen mentioned this pull request Jul 7, 2017

Dj/keep db open amberframework/granite#39

Merged

This was referenced Oct 23, 2017

Run in a transaction luckyframework/lucky_migrator#11

Closed

Save inside of a transaction luckyframework/lucky_record#50

Closed

bcardiff deleted the feature/nested-transactions branch November 8, 2017 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/nested transactions #27

Feature/nested transactions #27

bcardiff commented Dec 13, 2016

bcardiff commented Dec 13, 2016

spalladino Dec 13, 2016

bcardiff Dec 14, 2016

spalladino Dec 13, 2016

bcardiff Dec 14, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 14, 2016 •

edited

Loading

bcardiff Dec 14, 2016

bcardiff Dec 14, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 13, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 14, 2016

bcardiff Dec 14, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 14, 2016 •

edited

Loading

bcardiff Dec 14, 2016

spalladino Dec 13, 2016

bcardiff Dec 13, 2016

spalladino Dec 14, 2016

bcardiff Dec 14, 2016

spalladino commented Dec 14, 2016

Feature/nested transactions #27

Feature/nested transactions #27

Conversation

bcardiff commented Dec 13, 2016

bcardiff commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalladino commented Dec 14, 2016

spalladino Dec 14, 2016 •

edited

Loading

spalladino Dec 14, 2016 •

edited

Loading