voxpelli / node-pg-pubsub

A Publish/Subscribe implementation on top of PostgreSQL NOTIFY/LISTEN
https://www.npmjs.com/package/pg-pubsub
MIT License
236 stars 16 forks source link

accumulated connections -> use Pool() instead of Client() ? #82

Closed filipposisti closed 3 years ago

filipposisti commented 4 years ago

Hello,

i found that for high uptime (e.g. 2 -3 months) connections tend to accumulate (pgsql 9.6). Is would be possible to use a connection pool instead of:

const db = new pg.Client(this.conObject);

Or could you suggest a policy for managing connections? I need to keep the listener up and running 24/7.

Kind regards

voxpelli commented 4 years ago

A pool is not feasible as this module uses just a single connection for all its listening and only opens a new connection whenever that first connection is closed.

So in a way one could say that this module implements it’s own single connection pool.

If connections accumulates then that would indicate some bug in this modules own connection “pool”

filipposisti commented 4 years ago

ok

So in a way one could say that this module implements it’s own single connection pool.

that's clear.

So, as workaround, could i schedule a .close() and re-run .addChannel() (for each channel that was previously opened)? This action should close all active database connections and clear duplicated idle connections. Active channels are persisted in application configuration (pg-pubsub has no memory).

Thank you!

bostrom commented 4 years ago

We're experiencing the same issue: LISTEN connections build up in postgres after a while (sometimes even after only one day).

What could be the reason for the abandoned connections? @filipposisti did you find a remedy?

cdaringe commented 4 years ago

It seems to me that the proposed suggests a solution without addressing a root cause. if this pkg is infact leaking connections, we should find it. In my alt impl (not promoting, just sharing) you can see pretty clearly a hedge against new connections:

if (this._db) return this._db, https://github.com/cdaringe/pg-subscribe/blob/master/src/index.ts#L46

how confident are you that your own source doesn't redundantly init a new instance of the exported class?

bostrom commented 4 years ago

how confident are you that your own source doesn't redundantly init a new instance of the exported class?

Thanks. Yes, I was thinking of that too and it's something we need to investigate.

filipposisti commented 4 years ago

Hello,

@filipposisti did you find a remedy?

well now i am not experiencing this issue. I need to investigate what happens when db server reboots due to o.s. maintenance and the app server running node-pg-pubsub is still running. I know that this component will try to reconnect but i am not sure that old connections will be closed.

I think that we could add a process to properly manage the connections, i am posting my last advise:

So, as workaround, could i schedule a .close() and re-run .addChannel() (for each channel that was previously opened)? This action should close all active database connections and clear duplicated idle connections. Active channels are persisted in application configuration (pg-pubsub has no memory).

if .close() will effectively drop all active db connections, this could be a good way to do some housekeeping with stale / old / duplicated connections (due to wathever reason) and re-establish a fresh connection.

voxpelli commented 4 years ago

@filipposisti So adding a .close() here? https://github.com/voxpelli/node-pg-pubsub/blob/bb6de616aaf5aee879ead621ff6bb38ff717b6e8/index.js#L59-L64

voxpelli commented 4 years ago

@filipposisti and others: Can you test https://github.com/voxpelli/node-pg-pubsub/releases/tag/v0.5.1-0 + review https://github.com/voxpelli/node-pg-pubsub/commit/9d460dd17b81ee1dd31a5b5d8a610dc24f7d4b98 ?

abolognino commented 4 years ago

Restarting the database triggers this issue quite often on our machines. We switched to https://github.com/andywer/pg-listen

voxpelli commented 4 years ago

The pg module is a bit vague on what state a connection is in when the database client emits an 'error' event, so the 0.5.1-0 release adds the suggested db.end() and hopefully that will stop the behavior you were seeing @abolognino

voxpelli commented 3 years ago

This is now part of the released 0.6.0