Open kjdelisle opened 7 years ago
Use pool.query
and Bluebird resource management.
@charmander Isn't the whole idea of a connection pool that it limits the ability of calling code to arbitrarily create and utilize connections to a database? As I mentioned initially, you can follow best practices to avoid the problem, but wouldn't it make a lot of sense to add something that throws errors when you attempt to use a resource you've given back to the pool?
Additionally, if I want to perform a series of distinct queries/transactions against a database, using pool.query
would add the overhead of releasing connections back to the pool and asking for new ones, which could be especially cumbersome if the pool is currently exhausted and that particular set of actions must now queue up to receive a resource it could've hung onto.
Sorry, to be specific: they’re for the two different cases.
pg.Pool.prototype.connectResource = function () {
return this.connect().disposer(function (client) {
client.release();
});
};
function singleQuery(db, input) {
return db.query('SELECT $1::int AS number', [input]);
}
function multipleQueries(db) {
return Promise.using(db.connectResource(), function (client) {
return Promise.map([1, 2, 3], function (input) {
return singleQuery(client, input);
});
});
}
Your example is a good pattern, but again, it's not about the idea that you can just "code better" to avoid the problem. It's that if you're interested in holding a connection from the pool for the right reasons, and you were to screw up somewhere down a long and potentially convoluted chain of calls that it won't do what a connection pool is meant to do and stop you.
Your example would limit the value of the connection pool to its queuing functionality when the pool is exhausted.
I think I'm going to open a PR for this soon.
It's that if you're interested in holding a connection from the pool for the right reasons
Which reasons?
Maybe I want to run 500 separate transactions in series and don't want to incur the overhead of constantly releasing/acquiring a connection from the pool? It might also be that those transactions aren't just 500 of the same function, running over and over (maybe they're pulled from a unit-of-work queue or something to that effect).
There's any number of potential use-cases for not immediately surrendering your hold on the connection instance after a single query.
Maybe I want to run 500 separate transactions in series and don't want to incur the overhead of constantly releasing/acquiring a connection from the pool?
You might be overestimating the overhead:
'use strict';
var pg = require('./');
var pool = new pg.Pool();
var COUNT = 10000;
function release(client) {
client.release();
}
pool.connect().then(function (client) {
client.release();
var start = process.hrtime();
var i = 0;
(function acquire() {
if (i++ === COUNT) {
var time = process.hrtime(start);
console.log((time[0] + time[1] * 1e-9).toFixed(3));
pool.end();
return;
}
pool.connect()
.then(release)
.then(acquire);
})();
});
10,000 release/acquires, 42 ms. That’s the point of a pool.
(To be clear, I’m not against this being implemented; extra safety is always nice. I do think any instances of this problem are indicative of questionable design on the user’s part, though.)
Actually, I just discovered something interesting while working on my branch...
Subsequent calls to the same client object will fail when using Promises instead of callbacks with TypeError: client.release is not a function
It makes sense because the Promise version is handing back the reference to the object itself, while the callback version is receiving its own distinct reference to the function: https://github.com/brianc/node-pg-pool/blob/master/index.js#L139
So even though there's code in there to delete the client.release
reference, it's only going to work for the Promise version since any calling code will still retain its reference (in the form of the done
parameter).
~So, technically, if you're using Promises, you're already protected from using released connection objects (just not in a way that's crystal clear).~
Sorry, wasn't thinking about it enough. This still lets you run one more query against the now-released resource. It's only on the release the 2nd time around that you run aground.
It makes sense because the Promise version is handing back the reference to the object itself, while the callback version is receiving its own distinct reference to the function:
I believe it makes sense because promises are based on the use of process ticks, i.e. delays, and once a delay is created, the pool gets released, but not immediately.
I fully support @kjdelisle, this library should support simple diagnostics when someone tries to use a released pool connection. This would let you easily spot a fault in your code :wink:
I noticed that if I continue making use of a client reference after calling
done()
, like so:...that it would release the items to the pool, but still make use of them:
I would still expect a user of this library to be careful about reusing connection instances that they've released to the pool, but if you were to use an intermediate object reference that prevents direct access to those connection instances, you could invalidate those references by setting a flag on
client.release()
that prevents people from calling things likeclient.query(...)
.