Support statement_timeout #1436
Conversation
.gitignore
Outdated
| @@ -5,3 +5,4 @@ build/ | |||
| node_modules/ | |||
| package-lock.json | |||
| *.swp | |||
| /.idea | |||
There was a problem hiding this comment.
You can put this in a global gitignore instead.
There was a problem hiding this comment.
I removed this. I often checkin the webstorm files in my own repos which is why I added it here. Not a problem.
|
|
||
| function getConInfo (override) { | ||
| var newConInfo = {} | ||
| Object.keys(conInfo).forEach(function (k) { |
There was a problem hiding this comment.
return Object.assign(
{},
conInfo,
override
);There was a problem hiding this comment.
My bad, I started with the appname-tests.js file as a starting point. I've updated this function in both files.
package.json
Outdated
| "pg-connection-string": "0.1.3", | ||
| "pg-native": "^2.2.0", |
There was a problem hiding this comment.
Why this addition of pg-native ?
There was a problem hiding this comment.
Ah shit. The tests seem to be adding that when they are run. I'll remove it.
`npm run test` seems to be adding this
|
Ohhhh this is nice! |
| done() | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
It looks like the tests only check for the correct configuration of statement_timeout.
I think we miss a test for checking if the timeout is actually used, by using a query with pg_sleep(whicheverValueAboveTimeout)
There was a problem hiding this comment.
Sure. But isn't that testing Postgres itself? The tests already confirmed that the setting is set on the connection.
There was a problem hiding this comment.
Sure. But isn't that testing Postgres itself? The tests already confirmed that the setting is set on the connection.
You may have a point here, but the test seems pretty simple to write, so IMHO it's better to add it.
Whatsoever @brianc has the final word.
There was a problem hiding this comment.
I've written it. Travis is running so I'm hoping the error response from postgres is the same from all the versions you guys support. I asserted the error code only with the hope that is will be less fragile than expecting a particular error message.
.gitignore
Outdated
| @@ -4,4 +4,4 @@ build/ | |||
| *.log | |||
| node_modules/ | |||
| package-lock.json | |||
| *.swp | |||
| *.swp | |||
There was a problem hiding this comment.
This looks like an unwanted edit.
There was a problem hiding this comment.
I've added a newline to the end of the file to remove this change
|
Once this PR is approved, I'd be be happy to help with documentation. I'll be camping for the next few days so i'll go silent and come back online Saturday afternoon. |
|
Thanks for all the help & work on this! I'll merge it this weekend, push a new minor version, and document it! ❤️ |
…sts with npm 5 Should save some confusion in future pull requests (brianc#1465, brianc#1436, brianc#1363).
We need the ability to set
statement_timeoutfor a given connection so that all queries timeout instead of running to completion. This is really important when running nodejs/postgres on Heroku because they have http connection limits that timeout at 30 seconds. I want my SQL queries to stop at this boundary too since the waiting http connection will have already been closed.I was not able to add this feature to the native side of things because postgres does not seem to support statement timeouts via pgsql connection strings.