semiocast
Added a connection_timeout option and the related test case
Related to https://github.com/semiocast/pgsql/issues/49. I don't know if the lists:keytake/3 is necessary here might rewrite it with a lists:keyfind/3 instead.
When you call gen_server:start_link and hit a timeout, does the server get cleaned up properly? Like, if you hit the timeout because you're waiting in line for PG, does the connection drop, or does Postgres see a client until it reaches TCP timeout?
I will see if I can find a way to test this. The current test case is only there to test that the option is valid. I noticed that depending at which point in the initialization process the timeout append, postgres may throw an "incomplete startup packet" error.
I have been inserting timer:sleep/1 at different stages of the connection to trigger the timeout, but it is not very reliable. Do you have any suggestion?
Yeah, it seems like too much bother to make an automated test for it. I'm just not familiar enough with the inner details of gen_server to know exactly what happens in this case. But it seems relevant to your use-case, because if PG is overloaded this could just make it more overloaded. (Though it seems promising that in your own case making this change worked out well.)
@waisbrot Sorry haven't had time to work on this. Can I keep the PR open and I update it once I get an answer to your question?
I don't have merge powers. Just fond of this library and hoping it's helpful if I review a bit.
@pguyot seems at least somewhat active, but this is a pretty quiet repository so I doubt it's a big deal to leave it open. Especially when you're so diligent that you check in every couple weeks!
I understand the issue is that the supervisor's mailbox is full because opening connections takes too much time. Adding a timeout on start_link will not fully fix the problem: each pgsql_connection process that fails to open a connection within the allocated timeout will nevertheless stress the server (and the local operating system) and potentially slow down the next processes.
What do you think about moving the open logic out of init/1 handler and open the connection asynchronously by self-sending a specific 'open' message? The supervisor will be able to handle as many new children as needed (init/1 currently doesn't do much except opening the connection).
A connection timeout option seems to be a different feature and is compatible with this approach. Also, such an option probably should be applied to both the first connection and reconnections if reconnect option is also provided.
@pguyot Sounds good to me. As I mentioned above, I might not be able to start right way but if nobody else has an immediate need for this fix I would be interested to work on it.
I did some modifications already and I am wondering if I should keep the timeout where it is or move it down to the tcp:connect/3 in pgsql_open/1. It would automatically apply to connections and reconnections. But in an other hand I am afraid that, in some cases, the connection hang higher up, during the communication with postgres not when the tcp connection is being established.