Switch to non-closure based transactions #87

fishcakez · 2017-07-04T23:20:07Z

Requiring a fun for a transaction and non-local return for a rollback
is a very limiting API. With this breaking change the API becomes more
flexible, in particular it allows transaction to continue over multiple
OTP behaviour callbacks and use of (potentially nested) savepoints.

Removes:

DBConnection.transaction/3
DBConnection.rollback/2

Adds:

DBConnection.checkout/2
DBConnection.checkin/2
DBConnection.begin/2
DBConnection.commit/2
DBConnection.rollback/2
DBConnection.checkout_begin/2
DBConnection.commit_checkin/2
DBConnection.rollback_checkin/2

Requiring a fun for a transaction and non-local return for a rollback is a very limiting API. With this breaking change the API becomes more flexible, in particular it allows transaction to continue over multiple OTP behaviour callbacks and use of (potentially nested) savepoints. Removes: * DBConnection.transaction/3 * DBConnection.rollback/2 Adds: * DBConnection.checkout/2 * DBConnection.checkin/2 * DBConnection.begin/2 * DBConnection.commit/2 * DBConnection.rollback/2 * DBConnection.checkout_begin/2 * DBConnection.commit_checkin/2 * DBConnection.rollback_checkin/2

josevalim · 2017-07-05T07:47:43Z

Do we need all 8 new functions? I assume the last three are for convenience?

josevalim · 2017-07-05T07:48:55Z

lib/db_connection.ex

+    after
+      checkin(conn, opts)
+    end
+  end


New line missing?

josevalim · 2017-07-05T07:49:06Z

lib/db_connection.ex

+  @doc """
+  Acquire a lock on a connection.
+
+  Returns `{:ok, conn}` on success or `{;error, exception}` if there is an


josevalim · 2017-07-05T07:49:39Z

lib/db_connection.ex

+  Returns `{:ok, conn}` on success or `{;error, exception}` if there is an
+  error.
+
+  To use the locked connection call requests with the connection


I had a hard time understanding this first sentence.

josevalim · 2017-07-05T07:49:54Z

lib/db_connection.ex

+
+  To use the locked connection call requests with the connection
+  reference returned. If the connection disconnects all future calls using that
+  connection reference will fail.


Can we be clearer on the meaning of fail? Is it raise or an error tuple?

What happens if I call it multiple times from the same process? Error? I get multiple connections? Other?

Made clearer (and elsewhere)

josevalim · 2017-07-05T07:51:25Z

lib/db_connection.ex

+  @doc """
+  Begin a transaction.
+
+  Return `{:ok, result}` on sucess or `{:error, exception}` if there was an


What about :aborted_transaction and :already_in_transaction? What happens if it is invoked multiple times?

This approach doesn't work nicely with logging, because these requests will be logged when no query is occurring, and if either of those are returned it will crash loggers.

There are also issues with some DBs. PostgreSQL ignores (but warns on) multiple start transactions, whereas MySQL will do an implicit commit and start a fresh transaction. Also PostgreSQL will implicitly turn committing transaction into rolling back transaction in an aborted state. This means providing the safe/ecto style is also very opinionated, and doesn't fit any database behaviour.

Ok, so we need a mechanism to get the transaction status. And if we don't need those operations, maybe there is little to no purpose in providing begin,rollback,commit in isolation? Shouldn't we just use the checkout_begin and variants instead? Shouldn't we aim to have the smallest possible API?

What about using db_connection with databases that don't have transactions? checkout makes sense in that case, but checkout_begin does not.

I think we can definitely ditch commit_checkin and rollback_checkin because the semantics are not 100% clear. There is checkout and checkin for non-transaction dbs.

I think that begin on its own can be useful for mode: :savepoint - i believe we will need this feature in the sandbox.

josevalim · 2017-07-05T07:53:58Z

lib/db_connection.ex

-  reference passed as the single argument to the `fun`. If the
-  connection disconnects all future calls using that connection
-  reference will fail.
+  Returns `:ok` on success, otherwise `{:error, exception}` on error.


What happens if the connection was already checked in? Or if it was disconnected between the last command and checking in?

michalmuskala · 2017-07-06T11:27:17Z

lib/db_connection.ex

+  disconnects all future calls using that connection reference will return (or
+  raise if bang(!) variant) a `%DBConnection.ConnectionError{}` error.
+
+  Use `checkin/2`, `commit_checkin/2` or `rollback_checkin/2` to release the


What will happen in case of plain checkin/2?

Nothing, but note we are going to have an option to force rollback on checkin (inside the pool).

michalmuskala · 2017-07-06T11:28:40Z

lib/db_connection.ex

+  @doc """
+  Acquire a lock on a connection and begin a transaction.
+
+  Return `{:ok, conn, result}` on success or raise an exception if there was


The return is {conn, result}

michalmuskala · 2017-07-06T11:31:28Z

lib/db_connection.ex

+    return =
+      with {:ok, result, meter}
+            <- run(conn, &run_commit/4, meter(opts), opts) do
+        checkin(conn, opts)


Can checkin/2 fail here?

Define fail? checkin/2 will not raise, but it may return an {:error, %DBConnection.Error{}} if the socket is disconnected. For the socket to be disconnect at this moment the commit must have disconnected, and for that happen the with pattern doesn't match and checkin/2 isn't called. We are removing this function though because it is not easy to understand.

fishcakez · 2017-07-06T12:23:08Z

lib/db_connection.ex

+  def begin(conn, opts \\ []) do
+    conn
+    |> run(&run_begin/4, meter(opts), opts)
+    |> log(:begin, :begin, nil)


As this function would never be used with a pool (unless crazy) we could merge it with checkout_begin, such that if the pool is used it performs the current checkout_begin. The pool version implemented here could be emulated by immediately checking in. This would mean begin(conn, opts) :: {:ok, t, result}.

What about a case where you check out a connection and do multiple transactions using the same connection? I could imagine this could be useful if you know you need to do couple transactions one after another.

That would be begin(t, opts):: {:ok, t, result} which is a subset of begin(conn, opts) :: {:ok, t, result}. The merging means that if conn is a GenServer.server (aka a pool) it performs a checkout and a begin and if conn is DBConnection.t (aka connection reference) it performs a begin. This supports both nested and multiple serial transactions (should a database support it).

What in the case of non-transactional dbs. Do they also use begin?

We have checkout(pool, opts) :: {:ok, t} and checkin(t) :: :ok.

Ah, so it's only checkout_begin we're getting rid of. Then 👍. I understood it in a way that the only think we're going to leave is begin.

fishcakez · 2017-07-11T16:39:28Z

Closed in a favour of #88 #89 #90

fishcakez requested review from josevalim and michalmuskala July 4, 2017 23:20

fishcakez mentioned this pull request Jul 4, 2017

Introduce DBConnection.Stage #82

Closed

josevalim reviewed Jul 5, 2017

View reviewed changes

Improve pool/transaction function docs

8930aac

michalmuskala reviewed Jul 6, 2017

View reviewed changes

fishcakez commented Jul 6, 2017

View reviewed changes

florinpatrascu mentioned this pull request Jul 9, 2017

Migrate to the DBConnection framework florinpatrascu/bolt_sips#30

Merged

2 tasks

fishcakez closed this Jul 11, 2017

josevalim deleted the jf-no-fun-transaction branch September 24, 2018 08:20

florinpatrascu mentioned this pull request Oct 20, 2018

Update docs #166

Merged

Switch to non-closure based transactions #87

Switch to non-closure based transactions #87

Uh oh!

Conversation

fishcakez commented Jul 4, 2017

Uh oh!

josevalim commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

michalmuskala Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fishcakez Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

fishcakez commented Jul 11, 2017

Uh oh!

Uh oh!

josevalim commented Jul 5, 2017 •

edited

Loading

michalmuskala Jul 5, 2017 •

edited

Loading

fishcakez Jul 6, 2017 •

edited

Loading