Use DBConnection

[fishcakez]

This is WIP.

Issues:

• There are failing tests because Postgrex.Connection.stop/1 has not been replaced. I could not come up with a good method to stop a supervisor without requiring Elixir 1.2.
• The parameter map is always sent to the client process and back but rarely used. Previously they were not sent. What are the use cases for this function?
• Timeouts are not handled correctly as a socket receive timeout of 5000ms is always used, which means a longer timeout will not be honoured. This can be solved by moving the timeout from the protocol's state to its request "status" term.

There are two optimisations in this change because the DBConnection API is not exactly equivalent to the old Postgrex.Connection:

• Notifications are sent as soon as they are received and are not accumulated.
• Type information for a query can be looked up without blocking the connection. I am unsure how useful it is as the client will likely be holding on to the connection about to execute.

Will need to do benchmark/profile to check performance does not regress before merging.

I have not added any new features in this PR and would prefer these to come one by one in separate PRs. The features (pooling, transactions, network failure handling, idle timeout) require implementing simple callbacks, documenting options and/or wrapping DBConnection calls so are almost free.

[josevalim]

❤️ 💚 💙 💛 💜

[josevalim]

There are failing tests because Postgrex.Connection.stop/1 has not been replaced. I could not come up with a good method to stop a supervisor without requiring Elixir 1.2.

Chants: one two! one two! one two!

We gotta give what the crowds ask for.

[ericmj]

The parameter map is always sent to the client process and back but rarely used. Previously they were not sent. What are the use cases for this function?

Not sure I understand, what was changed and why? The parameters are used to get connection parameters which can be useful to get information like the server timezone.

[fishcakez]

The full parameter map is passed to the client process on every checkout.
So when doing queries it is still sent and if using sbroker passed via the
broker. On master it is not always sent and fetched only on explicit
request.

By use cases, I rather meant to find out how often it is called and if it
should work without a pool checkout. If it is not very often I will add
get/format_status to DBConnection. This will provide parameters, socket
details and other connection information that will be nicely formatted in
observer and other introspection tools. I planned to add that at some point
but we could use it to fetch parameters without always including them in
the state. It would make the parameter call more expensive though.

It is difficult to abstract using callbacks in two processes to something
that is generic so state is only accessed in one process at a time in
DBConnection.

On Friday, 11 December 2015, Eric Meadows-Jönsson notifications@github.com
wrote:

The parameter map is always sent to the client process and back but rarely
used. Previously they were not sent. What are the use cases for this
function?

Not sure I understand, what was changed and why? The parameters are used
to get connection parameters which can be useful to get information like
the server timezone.

—
Reply to this email directly or view it on GitHub
#128 (comment).

[ericmj]

Sorry, I don't understand this at all. Are you talking about connection parameters?

The full parameter map is passed to the client process on every checkout.

What is the reason for this? Why was it changed from a function doing a GenServer.call like it was previously?

[fishcakez]

What is the reason for this? Why was it changed from a function doing a GenServer.call like it was previously?

There are no GenServer.calls in DBConnection because only one process can hold the state at a time. This means there isn't a good way to handle a call as the state could be checked out in a client when the call arrives. However DBConnection.Connection will log an info message if a handle_info message arrives while the process is checked out. Socket messages don't arrive when checked out because the socket is set to active: false and the message queue flushed for socket messages during the checkout callback.

DBConnection is designed like this so that the state (i.e. Postgrex.Protocol.state) is fetched once at the start of a transaction and then stored in the client process until commit/rollback is handled. The callbacks (e.g. Postgrex.Protocol.execute/3) are all called in the client process and state changes are saved in the pdict. This means all the logic and state is held in one process for transactions and transactions are independent of pooling and disconnect/reconnection attempts. This independence makes the logic much simpler. I think the pdict usage is well worth it.

I should have mentioned this design decision as it turns out to be an optimisation for transactions or prepare/execute/close because state is copied once end the socket only needs to be deactivated once when doing mutplie calls during a pool checkout, such as a transaction. However this PR does not add support for transactions or pooling so I forgot to mention it.

[fishcakez]

I will fix the three issue tomorrow using a newer release of DBConnection and keep the requirement at Elixir 1.0.

• stop/1 will be implemented by writing a custom supervisor to sit on top of a pool which would normally have a Supervisor as the top process.
• Use a global read/write concurrent ets table to store parameters using a (monitor) reference as key stored in protocol state.
• timeout fix as stated in initial comment.

[fishcakez]

I haven't been able to fix stop/1. Even when doing as described there is still a problem. If the named process is not the top level process (i.e. the pid returned by start_link is not the named process by one below is) it is unclear which process should be called. This can not be solved with 1.2 either :(. The other two issues have been resolved.

The ability to use a prepared query prepared by another process has been added in 9e7b93c, which means using prepared queries with a pool just works. The method is efficient in that describing the query is skipped and the type information is reused. However it is inefficient in that a failed execute has to be sent and received to know the query needs to be prepared. This can be made more efficient at a later time. The (query) parameters have been outside the query struct to support this and will be needed for Ecto.

[josevalim]

Can we leave stop support outside of DBConnection and just call it directly
from Ecto? Or can we add it and say it only works with Elixir 1.2?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[fishcakez]

Ecto doesnt need stop because it puts the pool under its own supervisor,
Ecto stops its supervisor using an exit signal from parent

On 16 December 2015 at 20:30, José Valim notifications@github.com wrote:

Can we leave stop support outside of DBConnection and just call it directly
from Ecto? Or can we add it and say it only works with Elixir 1.2?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

—
Reply to this email directly or view it on GitHub
#128 (comment).

[fishcakez]

1.2 has the same issue as above though too

On 16 December 2015 at 20:34, James Fish james@fishcakez.com wrote:

Ecto doesnt need stop because it puts the pool under its own supervisor,
Ecto stops its supervisor using an exit signal from parent

On 16 December 2015 at 20:30, José Valim notifications@github.com wrote:

Can we leave stop support outside of DBConnection and just call it
directly
from Ecto? Or can we add it and say it only works with Elixir 1.2?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

—
Reply to this email directly or view it on GitHub
#128 (comment).

[josevalim]

So why do we need stop? Can't we tell people to supervise it or something
they need it?

On Wednesday, December 16, 2015, James Fish notifications@github.com
wrote:

Ecto doesnt need stop because it puts the pool under its own supervisor,
Ecto stops its supervisor using an exit signal from parent

On 16 December 2015 at 20:30, José Valim <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Can we leave stop support outside of DBConnection and just call it
directly
from Ecto? Or can we add it and say it only works with Elixir 1.2?

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

—
Reply to this email directly or view it on GitHub
#128 (comment).

—
Reply to this email directly or view it on GitHub
#128 (comment).

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

[fishcakez]

@josevalim, @ericmj and I discussed on IRC and its gone.

[josevalim]

Glad to hear @fishcakez! The best way to solve problems ;)

[fishcakez]

Moebius currently uses stop/1 so we may need to provide a simple one for
one pool as well as ownership pool. I am hoping that the pooling will be
acceptable to them though and we don't need to worry about it.

On 17 December 2015 at 18:55, José Valim notifications@github.com wrote:

Glad to hear @fishcakez https://github.com/fishcakez! The best way to
solve problems ;)

—
Reply to this email directly or view it on GitHub
#128 (comment).

[josevalim]

@fishcakez if they start the db_connection inside their own simple_one_for_one supervisor, wouldn't that be enough?

[fishcakez]

@josevalim if they start one like that how will they stop it? Safely?

[fishcakez]

I would like to:

• Fix registered naming poolboy in DBConnection (it crashes)
• Check it works on the moebius test suite. I have 6 failing tests on a branch using sbroker to track down. All related to documents and transactions. I am using Postgrex transactions in strict mode, which behaves differently to the current transactions.
• Benchmark to check no regression (query/3 uses a different message order, which I hope does not effect performance)
• Release new DBConnection and updated mix.exs

[fishcakez]

Fixed a bug where ArgumentErrors weren't being raised and added to_string implementation for Query for concise logging.

[fishcakez]

Using this branch and a DBConnection fork of Ecto reduces basic query (Repo.insert!/1) time by ~15% and transaction overhead (Repo.transaction(fn -> nil end)) time by ~50% using a local postgres server with poolboy.

The large reduction in transaction overheard comes from more efficient use of postgres (removed 2 round trips) and more efficient pooling (removed 2 calls, 2 casts and 2 :inet.setopts/2). It should be possible to make similar gains for cached queries inside transactions that require minimal decoding by using prepared queries.

[josevalim]

This is beautiful! I can't wait to get this merged and start the final push towards Ecto 2.0.

[fishcakez]

This is ready for review.

[josevalim]

Do we care about making this public? Would using a mapper offer any advantage over decode: :manual? If so, maybe we should get rid of decode: :manual altogether?

[fishcakez]

We dont need manual anymore. If we can make this backwards incompatible
change may be able to tidy up encode/decode a bit.

If we were to need it, likely it would make sense to add to db_connection.

On Monday, 4 January 2016, José Valim notifications@github.com wrote:

In lib/postgrex/connection.ex
#128 (comment):

 * `:decode`  - Decode method: `:auto` decodes the result and `:manual` does
 not (default: `:auto`)

• * :decode_mapper - Fun to map each row in the result to a term, see

Do we care about making this public? Would using a mapper offer any
advantage over decode: :manual? If so, maybe we should get rid of decode:
:manual altogether?

—
Reply to this email directly or view it on GitHub
https://github.com/ericmj/postgrex/pull/128/files#r48723821.

[josevalim]

So kill it with 🔥. :)

[fishcakez]

I've cleaned up the encode/decode so the db_connection protocol is always used.