Permalink
Browse files

postgres' pools will now work correctly.

  • Loading branch information...
durango committed Jul 27, 2013
1 parent db577dd commit ff57af63c2eb395b4828a5984a22984acdc2a5e1
Showing with 83 additions and 58 deletions.
  1. +72 −49 lib/dialects/postgres/connector-manager.js
  2. +11 −9 lib/dialects/postgres/query.js
@@ -6,101 +6,118 @@ module.exports = (function() {
var pgModule = config.dialectModulePath || 'pg'
this.sequelize = sequelize
this.client = null
this.config = config || {}
this.config.port = this.config.port || 5432
this.pooling = (!!this.config.poolCfg && (this.config.poolCfg.maxConnections > 0))
this.pg = this.config.native ? require(pgModule).native : require(pgModule)
this.client = null
this.config = config || {}
this.config.port = this.config.port || 5432
this.pooling = (!!this.config.pool && (this.config.pool.maxConnections > 0))
this.pg = this.config.native ? require(pgModule).native : require(pgModule)
this.poolIdentifier = null
// Better support for BigInts
// https://github.com/brianc/node-postgres/issues/166#issuecomment-9514935
this.pg.types.setTypeParser(20, String);
// set pooling parameters if specified
if (this.pooling) {
this.pg.defaults.poolSize = this.config.poolCfg.maxConnections
this.pg.defaults.poolIdleTimeout = this.config.poolCfg.maxIdleTime
this.pg.defaults.poolSize = this.config.pool.maxConnections
this.pg.defaults.poolIdleTimeout = this.config.pool.maxIdleTime
}
this.disconnectTimeoutId = null
this.pendingQueries = 0
this.maxConcurrentQueries = (this.config.maxConcurrentQueries || 50)
process.on('exit', function() {
this.disconnect()
}.bind(this))
}
Utils._.extend(ConnectorManager.prototype, require("../connector-manager").prototype)
var isConnecting = false
var isConnected = false
ConnectorManager.prototype.query = function(sql, callee, options) {
ConnectorManager.prototype.endQuery = function() {
var self = this
if (this.client === null) {
this.connect()
if (!self.pooling && self.pendingQueries === 0) {
setTimeout(function() {
self.pendingQueries === 0 && self.disconnect.call(self)
}, 100)
}
var query = new Query(this.client, this.sequelize, callee, options || {})
self.pendingQueries += 1
return query.run(sql)
.success(function() { self.endQuery.call(self) })
.error(function() { self.endQuery.call(self) })
}
ConnectorManager.prototype.endQuery = function() {
ConnectorManager.prototype.query = function(sql, callee, options) {
var self = this
self.pendingQueries -= 1
if (self.pendingQueries == 0) {
setTimeout(function() {
self.pendingQueries == 0 && self.disconnect.call(self)
}, 100)
}
self.pendingQueries++
return new Utils.CustomEventEmitter(function(emitter) {
self.connect()
.on('error', function(err) {
emitter.emit('error', err)
})
.on('success', function(done) {
var query = new Query(self.client, self.sequelize, callee, options || {})
done = done || null
query.run(sql, done)
.success(function(results) { emitter.emit('success', results); self.endQuery.call(self) })
.error(function(err) { emitter.emit('error', err); self.endQuery.call(self) })
.on('sql', function(sql) { emitter.emit('sql', sql) })
})
}).run().complete(function() { self.pendingQueries-- })
}
ConnectorManager.prototype.connect = function() {
ConnectorManager.prototype.connect = function(callback) {
var self = this
var emitter = new (require('events').EventEmitter)()
// in case database is slow to connect, prevent orphaning the client
if (this.isConnecting) {
return
emitter.emit('success')
return emitter
}
this.isConnecting = true
this.isConnected = false
var uri = this.sequelize.getQueryInterface().QueryGenerator.databaseConnectionUri(this.config)
var connectCallback = function(err, client) {
var connectCallback = function(err, client, done) {
self.isConnecting = false
if (!!err) {
switch(err.code) {
case 'ECONNREFUSED':
emitter.emit('error', 'Failed to authenticate for PostgresSQL. Please double check your settings.')
break
case 'ENOTFOUND':
case 'EHOSTUNREACH':
case 'EINVAL':
emitter.emit('error', 'Failed to find PostgresSQL server. Please double check your settings.')
break
default:
emitter.emit('error', err)
// release the pool immediately, very important.
done && done(err)
if (err.code) {
switch(err.code) {
case 'ECONNREFUSED':
emitter.emit('error', new Error("Failed to authenticate for PostgresSQL. Please double check your settings."))
break
case 'ENOTFOUND':
case 'EHOSTUNREACH':
case 'EINVAL':
emitter.emit('error', new Error("Failed to find PostgresSQL server. Please double check your settings."))
break
default:
emitter.emit('error', err)
break
}
}
} else if (client) {
client.query("SET TIME ZONE 'UTC'")
.on('end', function() {
client.query("SET TIME ZONE 'UTC'").on('end', function() {
self.isConnected = true
this.client = client
self.client = client
emitter.emit('success', done)
});
} else {
this.client = null
self.client = null
emitter.emit('success', done)
}
}
if (this.pooling) {
// acquire client from pool
this.pg.connect(uri, connectCallback)
this.poolIdentifier = this.pg.pools.getOrCreate(this.sequelize.config)
this.poolIdentifier.connect(connectCallback)
} else {
//create one-off client
this.client = new this.pg.Client(uri)
@@ -111,8 +128,14 @@ module.exports = (function() {
}
ConnectorManager.prototype.disconnect = function() {
var self = this
if (this.client) this.client.end()
if (this.poolIdentifier) {
this.poolIdentifier.destroyAllNow()
}
if (this.client) {
this.client.end()
}
this.client = null
this.isConnecting = false
this.isConnected = false
@@ -18,16 +18,17 @@ module.exports = (function() {
}
Utils.inherit(Query, AbstractQuery)
Query.prototype.run = function(sql) {
Query.prototype.run = function(sql, done) {
this.sql = sql
var self = this
if (this.options.logging !== false) {
this.options.logging('Executing: ' + this.sql)
}
var receivedError = false
, query = this.client.query(sql)
, rows = []
var receivedError = false
, query = this.client.query(sql)
, rows = []
query.on('row', function(row) {
rows.push(row)
@@ -39,13 +40,14 @@ module.exports = (function() {
}.bind(this))
query.on('end', function() {
done && done()
this.emit('sql', this.sql)
if (receivedError) {
return
}
onSuccess.call(this, rows)
onSuccess.call(this, rows, sql)
}.bind(this))
return this
@@ -55,11 +57,11 @@ module.exports = (function() {
return 'id'
}
var onSuccess = function(rows) {
var onSuccess = function(rows, sql) {
var results = []
, self = this
, isTableNameQuery = (this.sql.indexOf('SELECT table_name FROM information_schema.tables') === 0)
, isRelNameQuery = (this.sql.indexOf('SELECT relname FROM pg_class WHERE oid IN') === 0)
, isTableNameQuery = (sql.indexOf('SELECT table_name FROM information_schema.tables') === 0)
, isRelNameQuery = (sql.indexOf('SELECT relname FROM pg_class WHERE oid IN') === 0)
if (isTableNameQuery || isRelNameQuery) {
if (isRelNameQuery) {
@@ -70,7 +72,7 @@ module.exports = (function() {
}
})
} else {
results = rows.map(function(row) { return Utils._.values(row) })
results = rows.map(function(row) { return Utils._.values(row) })
}
return this.emit('success', results)
}

17 comments on commit ff57af6

@joshm

This comment has been minimized.

joshm replied Aug 5, 2013

I can't for the life of me find the ticket this commit was tied to, but that said, I'm not positive but shouldn't this really be doing this:

https://gist.github.com/joshm/6160030

The pg module manages this for you with those methods, calling getOrCreate directly can cause issues, especially if sequelize is not your only connection to pg.

@durango

This comment has been minimized.

Member

durango replied Aug 5, 2013

Why are you not positive about it? The tests in node-postgres even do this.. getOrCreate can also be used for multiple sequelize instances with different host:port options.

can cause issues, especially if sequelize is not your only connection to pg

Is there a source for this claim? I'd be really interested...

@durango

This comment has been minimized.

Member

durango replied Aug 6, 2013

@joshm I'll definitely look into this.. thanks :)

@durango

This comment has been minimized.

Member

durango replied Aug 6, 2013

Do you know of anyway to thoroguhly test? I'm not seeing one iota of a difference when looking at pg_activity, but I'd also like to know if there's a way to see how many times we've actually connected/handshaked vs just acquiring from the pool.

@joshm

This comment has been minimized.

joshm replied Aug 6, 2013

Well, you may be correct, we were having an issue where we had setup a direct connection with pg and setup a connection to pg through sequelize using md5 setting in the hba conf. When I did this calling those methods it seemed to fail to pull the correct pool client out and would try to use the ENV vars to make the connection rather than the supplied config. When I switched this to use the base index calls for connection, the issue went away.

It may be something unique to our setup, I suppose using the exposed calls off of index might be a good thing in case the connection/end methodology ever changes.

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

Hey @joshm so I talked to brianc on IRC a few days ago:

2:58 PM brianc: durango: 1) looks good to me
... [peer reseted for him]
3:02 PM brianc_: durango: 2) monkey-patch the client object with a "._$sequalizeTimeZoneSet = true" property or something [we were discussing something else in regards to SET TIMEZONE]
....
3:07 PM brianc_: calling getOrCreate is fine

So it seems like everything is fine on Sequelize's end BUT if you could.. could you provide me some examples of what you're doing, what error you're getting exactly? I'd love to help, but I can't fly blind :D

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

Well, one thing to try is to just change your setup to use a user other than 'postgres'. I reviewed this a bit more and I agree, it probably is not an incorrect way to do this. But sequelize is trying to use username in the config and if you don't have a 'postgres' user the connection will try to use the 'env' variables as the 'user' (not username) and fail to connect.

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

Are you referring to the tests? Cause we listen for SEQ_USER SEQ_PASS and SEQ_DB in the tests. Typically in an application you'd also explicitly declare usernames and passwords (this is traditionally used universally in every framework/orm/language) when creating a new Sequelize instance. I guess I'm having difficulty in understanding your application's architecture.

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

When you set your settings to not use 'trust' and use md5, if you don't have a postgres user, it won't matter what username is in your sequelize configuration (tests or not)

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

{ database: 'fakedb',
username: 'md5test',
password: 'md5testpass',
host: 'localhost',
port: 5432,
pool: {},
protocol: 'tcp',
queue: true,
native: false,
replication: false,
dialectModulePath: null,
maxConcurrentQueries: undefined }
FIXME: I want to be supported in this dialect as well :-(

Executing: SELECT 'hello'
q

It worked fantastically...

host all md5test 127.0.0.1/32 md5

I tried alter role md5test with password and with encrypted password...

connection:

var fake = new Sequelize('fakedb', 'md5test', 'md5testpass', {dialect: 'postgres'})

Connection worked flawlessly under different circumstances (even used 127.0.0.1 as host). Perhaps check your iptables/firewall/pg_hba.cong?

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

Did you turn on pooling?

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

var fake = new Sequelize('fakedb', 'md5test', 'md5testpass', {
  dialect: 'postgres',
  pooling: { maxConnections: 5, maxIdleTime: 3000 }})

fake.query('SELECT \'hello\'', null, {raw: true})
.error(function(e) {
  console.log('e', e);
})
.success(function(q) {
  console.log('q', q);
})
  Executing: SELECT 'hello'
q [ { '?column?': 'hello' } ]

Works flawlessly, again, please check your iptables/firewall/pg_hba.conf,

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

It is most likely not our iptables/firewall, we have tried this amongst several machines local and remote. Our conf is probably considerably different though when this issue pops up. On the remote machine here is our setup:

TYPE DATABASE USER ADDRESS METHOD

Database administrative login by Unix domain socket

local all postgres peer

"local" is for Unix domain socket connections only

local all all peer

IPv4 local connections:

host all all 127.0.0.1/32 md5

IPv6 local connections:

host all all ::1/128 md5

We are not using the native pg either.

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

Wait wait, ... are you guys on #master, 1.7.0-alpha2, 2.0.0-alpha2, or what?

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

This is us testing 1.7 alpha

@joshm

This comment has been minimized.

joshm replied Aug 9, 2013

Sorry don't mean to make you do a bunch of work. We keep our own branch of sequelize. When/if we get this more nailed down we will submit more information.

@durango

This comment has been minimized.

Member

durango replied Aug 9, 2013

Ah I see, yeah there might be a legitimate problem in 1.7.0-alpha2, to be honest I got completely fed up with the connector-manager in 1.7.0-alpha2 When we moved from Jasmine to BusterJS that's when I first saw that the postgres pooling never actually worked as in... at all so that's when I convinced the team to move to Mocha + Chai (cause tests are faster including npm install time / I get tired of waiting for Travis) and remodeling the connector-manager was pretty much the first thing major thing I did for #master other than shifting to two test frameworks.

Anyway, no problem dude, just glad we're pinpointing the problem, but please do give #master a try and let me know how it goes. I'm confident that the pooling will work for you, although, it was my first time ever diving into the connector-manager code haha no disclaimers ;)

But seriously, please do keep me up-to-date :) If I find anything I'll let you know as well.

Please sign in to comment.