Client is missing in error handling
Closed this issue · 6 comments
I am trying to adopt the use of node-postgres
6.x and the new pool within pg-promise
, and I encountered one critical issue that stopped me from being able to finish it properly.
When it comes to the error handling for failed connections, up till version 5.1 of node-postgres
we used the following error handler:
pg.on('error', function(err, client){
});
i.e. we would always get the Client
object for which the connection failed.
With the new version 6.x of the driver this still works only if we attach the handler in the old way, via pg.on
. However, the new approach suggests that we do pool.on('error', handler)
, which fires the event without passing in the Client
object.
It is thus inconsistent, but more importantly, it is a big issue when trying to bind the Client
context with broken connections that's currently at the very core of pg-promise
error handling.
Could we, please, have the same behavior within the pool, to provide the Client
object?
Yah that makes total sense & was likely an oversight on my part.
Just to make sure I'm clear: what you're saying is you want this:
const Pool = require('pg').Pool
const pool = new Pool()
pool.on('error', (error, client) => {
// client should be the client that threw the error
// but it's currently null
})
right?
If that's what you're proposing that should be an easy fix!
@brianc Further investigation just showed me that when we get that connection-related error, err
is an error object that also happens to have property client
set to the right Client
object.
That kind of takes care of it, in a sense that I can use it that way, not that it is the best way to do it, because:
- throwing it into a console or log creates a much larger output, with the
Client
rendered as extra - still inconsistent with how the event is fired in the older way that still works
right?
Yep 😄
@brianc While you are here, may I ask a related question?
As within pg-promise
the new pool is used automatically, do you think the following strategy is the best one?
For each new triplet of host
+ port
+ database
I automatically create a new Pool
object, and that's how the new pools are being used.
Yeah I think that strategy is good! You might want to add user
and password
to the uniqueness of a given pool...but I'm not 100% sure about that. It seems like if I wanted to connect to the same database w/ different users I'd expect their connections to be different, but maybe pg-promise
takes care of that.
I'm going to leave this issue open so I can be reminded to add the client
instance as the 2nd parameter to the error callback. While it is on the error
parameter as a property, I agree with you its nicer to be its own thing.
@brianc Thanks, Brian! I do not use user name + password, on the assumption that a separate connection pool on such a granular level would become too fragmented in terms of resources, and thus uncontrollable, i.e. it is better to tweak the size of the pool on the database level, and leave it at that, as far as the generic solutions go. You still get a good connectivity boost and isolation when using different servers / databases.
To make it better, you would have to manually control the pool creation strategy, which In my case isn't suitable, as the connectivity in pg-promise
is fully automatic.
P.S. Please make sure that if you move the Client
into a parameter, it requires a new release of node-postgres
, so I can upgrade the library accordingly, with the code, without it ending up broken silently because the error object no longer has property client
.
@brianc it seems we have the same issue within the core driver, when creating a Client
directly, as per this question: brianc/node-postgres#1351