Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support statement_timeout #1436

Merged
merged 7 commits into from Sep 3, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Expand Up @@ -4,4 +4,4 @@ build/
*.log
node_modules/
package-lock.json
*.swp
*.swp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unwanted edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a newline to the end of the file to remove this change

3 changes: 3 additions & 0 deletions lib/client.js
Expand Up @@ -272,6 +272,9 @@ Client.prototype.getStartupConf = function () {
if (params.replication) {
data.replication = '' + params.replication
}
if (params.statement_timeout) {
data.statement_timeout = String(parseInt(params.statement_timeout, 10))
}

return data
}
Expand Down
1 change: 1 addition & 0 deletions lib/connection-parameters.js
Expand Up @@ -64,6 +64,7 @@ var ConnectionParameters = function (config) {

this.application_name = val('application_name', config, 'PGAPPNAME')
this.fallback_application_name = val('fallback_application_name', config, false)
this.statement_timeout = val('statement_timeout', config, false)
}

// Convert arg to a string, surround in single quotes, and escape single quotes and backslashes
Expand Down
5 changes: 4 additions & 1 deletion lib/defaults.js
Expand Up @@ -60,7 +60,10 @@ module.exports = {
application_name: undefined,
fallback_application_name: undefined,

parseInputDatesAsUTC: false
parseInputDatesAsUTC: false,

// max milliseconds any query using this connection will execute for before timing out in error. false=unlimited
statement_timeout: false
}

var pgTypes = require('pg-types')
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -19,8 +19,8 @@
"main": "./lib",
"dependencies": {
"buffer-writer": "1.0.1",
"packet-reader": "0.3.1",
"js-string-escape": "1.0.1",
"packet-reader": "0.3.1",
"pg-connection-string": "0.1.3",
"pg-pool": "~2.0.3",
"pg-types": "~1.12.1",
Expand Down
9 changes: 1 addition & 8 deletions test/integration/client/appname-tests.js
Expand Up @@ -7,14 +7,7 @@ var suite = new helper.Suite()
var conInfo = helper.config

function getConInfo (override) {
var newConInfo = {}
Object.keys(conInfo).forEach(function (k) {
newConInfo[k] = conInfo[k]
})
Object.keys(override || {}).forEach(function (k) {
newConInfo[k] = override[k]
})
return newConInfo
return Object.assign({}, conInfo, override )
}

function getAppName (conf, cb) {
Expand Down
62 changes: 62 additions & 0 deletions test/integration/client/statement_timeout-tests.js
@@ -0,0 +1,62 @@
'use strict'
var helper = require('./test-helper')
var Client = helper.Client

var suite = new helper.Suite()

var conInfo = helper.config

function getConInfo (override) {
return Object.assign({}, conInfo, override )
}

function getStatementTimeout (conf, cb) {
var client = new Client(conf)
client.connect(assert.success(function () {
client.query('SHOW statement_timeout', assert.success(function (res) {
var statementTimeout = res.rows[0].statement_timeout
cb(statementTimeout)
client.end()
}))
}))
}

if (!helper.args.native) { // statement_timeout is not supported with the native client
suite.test('No default statement_timeout ', function (done) {
getConInfo()
getStatementTimeout({}, function (res) {
assert.strictEqual(res, '0') // 0 = no timeout
done()
})
})

suite.test('statement_timeout integer is used', function (done) {
var conf = getConInfo({
'statement_timeout': 3000
})
getStatementTimeout(conf, function (res) {
assert.strictEqual(res, '3s')
done()
})
})

suite.test('statement_timeout float is used', function (done) {
var conf = getConInfo({
'statement_timeout': 3000.7
})
getStatementTimeout(conf, function (res) {
assert.strictEqual(res, '3s')
done()
})
})

suite.test('statement_timeout string is used', function (done) {
var conf = getConInfo({
'statement_timeout': '3000'
})
getStatementTimeout(conf, function (res) {
assert.strictEqual(res, '3s')
done()
})
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But isn't that testing Postgres itself? The tests already confirmed that the setting is set on the connection.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

4 changes: 3 additions & 1 deletion test/unit/connection-parameters/creation-tests.js
Expand Up @@ -22,6 +22,7 @@ var compare = function (actual, expected, type) {
assert.equal(actual.host, expected.host, type + ' host')
assert.equal(actual.password, expected.password, type + ' password')
assert.equal(actual.binary, expected.binary, type + ' binary')
assert.equal(actual.statement_timout, expected.statement_timout, type + ' statement_timeout')
}

test('ConnectionParameters initializing from defaults', function () {
Expand Down Expand Up @@ -60,7 +61,8 @@ test('ConnectionParameters initializing from config', function () {
host: 'yo',
ssl: {
asdf: 'blah'
}
},
statement_timeout: 15000
}
var subject = new ConnectionParameters(config)
compare(subject, config, 'config')
Expand Down