fix(db) postgres connector keepalive errors on load

[bungle]

Summary

Postgres strategy (new model) could result error like the below when running multiple subsequent Admin API calls rapidly (or parallel):

[error] 27206#0: *3351 lua coroutine: runtime error: /usr/local/share/lua/5.1/pgmoon/init.lua:294: bad request

[thibaultcha]

Even if pre-existing prior to this PR, such logging message really should have some context prepended to them for users to understand their meaning.

[thibaultcha]

Not following the self as a first argument convention - maybe we should?

[thibaultcha]

I also notice we made this function a local, but the only place it is being called is in _mt:query() - it seems like unnecessary abstraction?

[thibaultcha]

Wouldn't the fix be more concise if it simply added checks for self.connection.sock in the places you just added it? I'm not quite sure making setkeepalive() and connect() local functions really helps? It seems like it just makes the logic more difficult to follow: we sometimes call setkeepalive(), and some other times call self:setkeepalive() - the latter just unsetting self.connection, but that seems unnecessary since the former will already unset self.connection.sock, and we now check for this value in query()?)

[thibaultcha]

I still struggle to see the difference before and after the patch, a lot of it seems to be re-factoring. What is the part that fixes it exactly, and what was the root cause for the bug? The PR description is very vague on details.

[bungle]

The biggest change is that when you don't have self.connection the self:query doesn't set one either, it just creates connection, uses it and sets keepalive, but it doesn't touch this self.connection in any way.

[bungle]

But I agree, if user ever calls :connect I think we might have a problem, with both cassandra and pg. That's why I would prefer that something in our system makes connection available per request and closes/keepalives it after. Something other that this strategy instance field, which may be shared between single worker on several requests.

[thibaultcha]

Gotcha. The :connect() method was intended for users (likely, said users are us) who know they will need to make several queries at once. It is fine in the CLI or init contexts, but not at runtime indeed, since the DAO is shared between all requests, one of those queries could yield and another request could use the DAO, which by then has this connection object as an attribute...

Let's merge this for now, and consider what to do with :connect() later!