Client is missing in error handling

[vitaly-t]

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?

[brianc]

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!

[vitaly-t]

@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 😄

[vitaly-t]

@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.

[brianc]

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.

[vitaly-t]

@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.

[sibedge]

@brianc it seems we have the same issue within the core driver, when creating a Client directly, as per this question: https://github.com/brianc/node-postgres/issues/1351