brianc / node-pg-pool

A connection pool for node-postgres
MIT License
180 stars 64 forks source link

Dropping generic-pool's FIFO option is a regression #74

Closed jgwmaxwell closed 7 years ago

jgwmaxwell commented 7 years ago

It's great to see the flurry of activity around node-pg at the moment, and we're big fans of the progress being made. However, dropping out generic-pool leaves one significant problem with the pool in its current form.

PG pools should, for maximum performance, tend to the lowest number of most active clients. Generic Pool's FIFO option allowed you to minimise the number of connections, helping you to ensure that you keep them hot, reducing load on the main server or pgbouncers, and for a somewhat sleepy application improving performance.

Would you consider a PR to restore a stack-based functionality?

charmander commented 7 years ago

It’s already a stack. index.js:42, index.js:111

'use strict';

const pg = require('pg');

const pool = new pg.Pool({
  host: '/var/run/postgresql',
});

(async () => {
  const firstClient = await pool.connect();
  const secondClient = await pool.connect();

  firstClient.release();
  secondClient.release();

  console.log(pool.totalCount);

  const newClient = await pool.connect();

  console.log(newClient === secondClient);

  newClient.release();
  pool.end();
})().catch(error => {
  process.nextTick(() => {
    throw error;
  });
});
jgwmaxwell commented 7 years ago

I must apologise - I read this comment as the pool being FIFO, rather than the client being FIFO - I agree, actually reading the code it is indeed a stack. My bad. Closing due to non-issue.