Introduce begin, rollback and commit #88

fishcakez · 2017-07-09T23:52:00Z

Add begin(!)/1,2 to checkout and begin
Add rollback(!)/1,2 to rollback and checkin
Add commit(!)/1,2 to commit (or rollback) and checkin

rollback/2 is overloaded and has old behaviour inside transaction/3 for
backwards compatibility. However transaction/3 is deprecated because is
makes assumptions about database status that may not hold.

New return values from handle_begin/2, handle_rollback/2 and
handle_commit/2 allow to signal mistaken database status.

* Add begin(!)/1,2 to checkout and begin * Add rollback(!)/1,2 to rollback and checkin * Add commit(!)/1,2 to commit (or rollback) and checkin rollback/2 is overloaded and has old behaviour inside transaction/3 for backwards compatibility. However transaction/3 is deprecated because is makes assumptions about database status that may not hold. New return values from handle_begin/2, handle_rollback/2 and handle_commit/2 allow to signal mistaken database status.

fishcakez · 2017-07-09T23:53:19Z

lib/db_connection.ex

-      {:error, err, _meter} ->
+  @spec rollback(conn, (opts :: Keyword.t) | reason :: term) ::
+    {:ok, result} | {:error, Exception.t}
+  def rollback(conn, opts \\ [])


Given the transaction/3 behaviour is this incorrect?

fishcakez · 2017-07-09T23:57:53Z

lib/db_connection.ex

+    meter = event(meter, query)
+    msg = "can not #{query} inside legacy transaction"
+    err = DBConnection.ConnectionError.exception(msg)
+    delete_disconnect(conn, conn_state, err, opts)


disconnect or just error? mark as failed transaction?

I don't think we need to disconnect, do we?

We don't need but I worry what might happen if anyone hits this path. If we fail the transaction I guess that stop any issue - i.e. force a rollback at end of transaction/3.

michalmuskala · 2017-07-10T07:39:57Z

lib/db_connection.ex

-  and disconnect.
+  to continue, `{:begin, state}` to open transaction before continuing,
+  `{:rollback, state}` to rollback aborted transaction,
+  `{:error, exception, state}` to abort the transaction and continue or


It might be that I'm not that familiar with db_connection, but what does "abort the transaction & continue" mean here?

In this case it probably needs rewording. The error will cause {:error, exception} to be returned by rollback/2. For example this might occur if the driver can't do a side effect free transaction status check.

michalmuskala · 2017-07-10T07:40:34Z

lib/db_connection.ex

+  `{:disconnect, exception, state}` to abort the transaction and disconnect.
+
+  A callback implementation should only return `:begin` or `:rollback` if it
+  can determine the database's transaction status without side effect.


Maybe "without a roundtrip" would be clearer here?

With a roundtrip is fine. We will need to do this for the sandbox.

michalmuskala · 2017-07-10T07:43:57Z

lib/db_connection.ex

+        DBConnection.execute!(conn, "SELECT * FROM table", [])
+        DBConnection.commit(conn)
+      after
+        DBConnection.rollback(conn)


How does always calling rollback work in this case? Should it only be called in case execute! raises?

I guess this is explained in rollback with:

It is possible to issue multiple rollbacks requests without a begin (begin/2
or checkout_begin/2). The semantics are left to the callback
implementation.

But it still seems a bit wired to me

Should it only be called in case execute! raises?

Yes, this is simplest/safest way to handle a transaction.

Oops i missed removing checkout_begin after copy and paste of docs.

But it still seems a bit wired to me

We are depending on the database's state, not our own, so the callback implementation will use information from the last request when available.

Sorry to clarify this is safe to call because the connection is checked in by commit so rollback won't do anything if it doesn't raise. This is a standard approach and imo should be the encouraged way as it ensures safety with minimal fuss.

Ok, sounds great. If it's an optimal approach maybe we should talk about it in the docs?

Okay, i will add it the examples and mention in the docs for begin.

michalmuskala · 2017-07-10T07:47:38Z

lib/db_connection.ex

+
+  ### Example
+
+      {:ok, result} = DBConnection.begin(conn)


begin returns a 3 tuple, doesn't it?

yes, nice catch

michalmuskala · 2017-07-10T07:51:18Z

lib/db_connection.ex

+    end
+  end
+  defp checkin(pool, fun, meter, opts) do
+    run(pool, fun, meter, opts)


I'm not sure what will happen in this case. Are we going to check out a connection, call checkin from the fun and then check the connection back in?

I'm not sure I understand the use case of calling checkin with the pool & without an actual connection.

This is for API completeness. Also someone using the ownership pool may wish to commit/rollback their transaction before returning connection to the inner pool. I am happy to remove this option.

michalmuskala · 2017-07-10T07:53:22Z

lib/db_connection.ex

+    meter = event(meter, query)
+    msg = "can not #{query} inside legacy transaction"
+    err = DBConnection.ConnectionError.exception(msg)
+    delete_disconnect(conn, conn_state, err, opts)


I don't think we need to disconnect, do we?

josevalim

Looks good to me. We just need to be careful with versioning here. If we plan this change to emit warnings, it will emit warnings for everyone using postgrex and mariaex today. So we probably want to soft-deprecate (no warnings) OR simply release 2.0 and remove the old chunks.

fishcakez · 2017-07-10T12:53:21Z

This change is backwards compatible, so the plan is to soft deprecated transaction/3.

fishcakez requested a review from josevalim July 9, 2017 23:52

fishcakez commented Jul 9, 2017

View reviewed changes

fishcakez requested a review from michalmuskala July 10, 2017 00:01

michalmuskala reviewed Jul 10, 2017

View reviewed changes

josevalim approved these changes Jul 10, 2017

View reviewed changes

fishcakez added 2 commits July 10, 2017 21:21

Improve docs

5cbc6ac

Fail transaction/3 on explicit begin/rollback/commit

b8d1281

fishcakez merged commit ab041a1 into master Jul 10, 2017

fishcakez deleted the jf-trans-calls branch July 10, 2017 20:54

fishcakez mentioned this pull request Jul 11, 2017

Switch to non-closure based transactions #87

Closed

dnesteryuk mentioned this pull request Oct 16, 2017

Migrate to the DBConnection framework florinpatrascu/bolt_sips#30

Merged

2 tasks

Introduce begin, rollback and commit #88

Introduce begin, rollback and commit #88

Uh oh!

Conversation

fishcakez commented Jul 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim left a comment

Choose a reason for hiding this comment

Uh oh!

fishcakez commented Jul 10, 2017

Uh oh!

Uh oh!