Releasing dropped connections

[vitaly-t]

Following up on this issue, it may be the case that we should stop releasing dropped connections back to the pool.

This requires further investigation.

[vitaly-t]

As per my comment there, we need to identify a way for the client to detect dropped connections.

Only then we can try to formulate a solution.

[vitaly-t]

The issue has received some useful follow up, and so based on the current findings, I have created branch connections, to start working on this.

[vitaly-t]

Released within 9.1.0-Beta.0. You can install it with:

$ npm i pg-promise@beta

Now for every query that errors, we check the type of error, and if it relates to connectivity, we mark the client for release from the pool.

@jmacmahon, @johanneswuerbach Please give it a go, guys, and let me know ;)

[vitaly-t]

Help is needed to review the update and to field-test the heck out of it.

[vitaly-t]

@jmacmahon, @johanneswuerbach guys, it's been 5 days since I gave you the update you wanted. Any chance you can test 9.1.0-Beta.0, and provide feedback?

[johanneswuerbach]

@vitaly-t we still haven't upgrade to v9 so that takes more time then anticipated. I've probably a result for you tomorrow (CET).

[jmacmahon]

I'll test it on our dev environment tomorrow (BST). Thanks for the work!

[johanneswuerbach]

Our tests pass, so this change seems good to go from our perspective 🚢 👍

[vitaly-t]

@johanneswuerbach Can you, please, tell a little how you tested, and what difference manifested itself?

[johanneswuerbach]

We have a fairly extensive test suite around testing connection failures between transaction, in the middle of transactions, when the instance just goes away, etc. as we use pgbouncer running inside kubernetes internally and have a lot of connection churn.

In our tests I was able to remove our monkey patch of brianc/node-pg-pool#119 as in at least all those cases broken clients are not being re-added to the pool.

We would still keep that patch as an additional safety net though.

[vitaly-t]

Thanks @johanneswuerbach! I think even with that check removed it should be good, because we now check for any broken connection after every query that failed. In fact, I'm not sure if that patch doesn't counteract what I have done. Your test should be done without that patch, or else the test has no value to us.

Sorry, I mean, yes, that's what you did, so again thank you! :)

I'll just wait for @jmacmahon to get back with the results today, and if they are positive, then I will release 9.1.0 with all the latest changes.

[jmacmahon]

I have been unable to test the DB disconnect behaviour as there seems to be a regression introduced regarding the type parsing.

We have configured:

const stringToNumber = (val: unknown) => parseInt(val as string || '', 10)
const passThroughAsString = (val: string) => val

  pgTypes.setTypeParser(pgDataTypes.int8, stringToNumber)
  pgTypes.setTypeParser(pgDataTypes.numeric, stringToNumber)
  pgTypes.setTypeParser(pgDataTypes.date, passThroughAsString)

But when querying a table with an integer row, these values are coming back as strings. This is causing our acceptance tests to fail and therefore I am unable to deploy the service consuming the beta library to our development environment.

[vitaly-t]

@jmacmahon Check out my answer (bottom update in it).

In theory, it all should be the same. And at least in the past Id 20 was sufficient to use.

That should correspond to the new INT8. While I'm not sure where that pgDataTypes.int8 is coming from...

[vitaly-t]

I have released another beta - 9.1.0-Beta.1, now merged into the master with all the latest TypeScript updates.

@jmacmahon Please try again, now using this update, see if you have any issues, and if so, use that StackOverflow reference I gave above. Cheers!

[jmacmahon]

Apologies for not including our pgTypes dictionary. It's a hangover from the days of v7 which (I believe) didn't include the TypeID enum. You are correct that pgDataTypes.int8 is set to 20.

In any case, the new 9.1.0-Beta.1 still has this regression.

[vitaly-t]

@jmacmahon Are you going to fix this on your end? Because it seems nothing can be done on this end anymore.

[vitaly-t]

Ok, this is a bit too slow as a follow-up. I am releasing 9.1.0 now, as it seems to be working as expected. If any issue should arise related to the change, it'll be addressed in due course.

[vitaly-t]

Feature #675 is an extension on this.