Moebius should use supervised connections about moebius HOT 8 CLOSED

Adding a supervisor/use once pool, with the same pattern as :ssl, to db_connection is quite simple as @josevalim says. I will try to add it this week. Hopefully we could get postgrex using db_connection as soon as possible too, though I would like to add a per connection prepared query cache to db_connection first. It is hard to make good use of the prepare/execute pattern in postgrex master at present and it may change the API.

from moebius.

I have talked to @fishcakez and we have decided to add an :owner feature to db_connection. This way a simple one for one supervisor will be enough as long as we mark the current process as the owner.

from moebius.

@josevalim @fishcakez I've submitted PR #38 that implements the Runner as a GenServer and adding it to the Supervisor. I assume that this is similar to what this issue is requesting. We've actually been working through & debating a possible broker for the connection pooling for some time which included adding the Runner to the supervision tree. Once @robconery reviews/merges the PR, I believe we can close this issue

from moebius.

@nurugger07 sorry, that is not what we are proposing. Your pull request makes all database requests go through a single GenServer which will very quickly become a bottleneck in your application. It also introduces serious concurrency issues: if a request issues a transaction, that transaction will apply to all other concurrent requests in the system, since you are effectively sharing the connection.

Even worse, if a process starts a transaction and exits due to a link, the connection in the GenServer will be forever inside the transaction and all other operations from all other processes will happen inside the transaction that won't be committed. The transaction may be rolled back just later when there is eventually a crash.

I have mentioned this to @robconery but please don't reinvent pooling or brokering. I know I wouldn't. I only have confidence on the Ecto pooling implementation because of @fishcakez. It is extremely hard to get it right once there are transactions because the database state becomes shared with multiple processes. That's also why @fishcakez is working on extracting it, so others can leverage it.

If you believe you should do your own brokering, please write an extensive test suite considering different scenarios with and without transactions. Preferably, build property-based testing models to validate your implementation. Both poolboy and sbroker implementations have been extensibly tested with such tools.

Sorry for being emphatic about this but this is extremely important. We talk about how Elixir is great for writing robust and maintainable applications. If we drop OTP principles or introduce race conditions that locks a connection in a transaction, it goes directly against what we have been saying.

Here is what I would do. I would wait until Postgrex is ported to db_connection. From there on, you will have two options:

1. Have no pool. Instead you would start a new connection every time there is a need by using a simple one for one supervisor, quite similar to how you use it today, the only difference is the connection process is supervised. To implement this without the issues I mentioned above, we need to add an :owner feature to db_connection + postgrex.
2. Use poolboy or sbroker (pools) with db_connection.

From previous discussions, it looks like you want to go with 1 which is the simplest approach. I will gladly review the code too.

from moebius.

It is extremely hard to get it right once there are transactions because the database state becomes shared with multiple processes.

With db_connection we actually move all transaction state to one process. For postgrex we will be able to check that the local transaction state is in sync with the postgres server process (using a protocol value we currently have to ignore) so that we should crash if there is a bug in the logic.

from moebius.

Well, there are still two processes, the connection and the one that started the transaction. They are still sharing some state. :) Anyway, two is better than three, so 👍.

PS: I was wrong. @fishcakez was awesome enough to handle it all on a single process.

from moebius.

@josevalim @fishcakez Yes! We would prefer the connection management to be offloaded :) It's something we've said from the beginning. It feels like the natural solution to ask for a connection from Postgrex, more so than managing connections in Moebius. Thanks @fishcakez for the work on db_connection!

from moebius.

This is done now.

from moebius.