fix(postgres): invalidate connection after client-side timeout

[harrykao]

Pull Request Checklist

• Have you added new tests to prevent regressions?
• If a documentation update is necessary, have you opened a PR to the documentation repository?
• Did you update the typescript typings accordingly (if applicable)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Does the name of your PR follow our conventions?

Description Of Change

The query_timeout feature of the pg package helps handle stuck TCP connections more quickly and gracefully by implementing a client-side timeout:

brianc/node-postgres#1713

Sequelize started passing this dialect-specific option through to pg here:

#13258

I believe we also want to invalidate the connection when a client-side timeout occurs. We shouldn't try to reuse the stuck connection because...it's stuck.

This PR updates the error handling code so that the connection is invalidated if the error matches the one thrown from here:

https://github.com/brianc/node-postgres/blob/5538df6b446f4b4f921947b460fe38acb897e579/packages/pg/lib/client.js#L529

[harrykao]

@sincerekamal Since you added the pass-through for the query_timeout option, does this change seem right to you?

[harrykao]

@ephys @WikiRik Sorry to bug you. What's the right way to ask someone to review this?

[WikiRik]

Hi @harrykao
This is fine. We'll take a look at this soon. Thanks for the message

[WikiRik]

Do you think we can make a test with a very low query_timeout and a query that does take a while? Not a high priority but would be nice to see.

Apart from that I think this should not be an issue, but I do have a question about your explanation. See my comment about that

[harrykao]

@WikiRik Thanks for the review. I've added a test.

I'm not sure about coding style here and I'd be happy to do another round to make this more consistent with the rest of the code.

[harrykao]

Hrm, it looks to me like the native bindings don't pass statement_timeout:

https://github.com/brianc/node-pg-native/blob/master/index.js#L14

Or implement query_timeout.

Would it make sense to skip these tests when testing against native?

[WikiRik]

I think it would be fine to skip these on postgres-native. You can just add an if-statement at the top of the describe block that just returns if the dialect is postgres-native. That reduces nesting and is our new best practice

[WikiRik]

One small suggestion, but it's non blocking
I think it's fine having the new test functions here, especially since a developer might not directly expect that getConnectionPid also releases the connection.

[harrykao]

Thanks @WikiRik!

[harrykao]

@WikiRik Could I get a v6 release with this fix?

[WikiRik]

Can you open a new PR with this change towards our v6 branch?