search

dwyl / hapi-postgres-connection

:zap: Creates a PostgreSQL Connection (Pool) available anywhere in your Hapi App
GNU General Public License v2.0
40 stars 13 forks source link

The pool itself should be exposed, not an array of clients #36

Closed finnhodgkin closed 1 year ago

finnhodgkin commented 6 years ago

The pg module api has been updated since hapi-postgres-connection was put together. It's no longer necassary to push individual clients to an array. This isn't just an improved api, it also massively effects database response times - after swapping to a direct connection, I found that on certain database heavy routes response times go from 30 seconds to 5.

I believe that instead of exposing an array of clients, we should be exporting the pool directly. Ideally I'm guessing we should aim to keep the api as similar as possible, but currently it's accessed through HPC.getCon().client.query/request.pg.client.query - a little misleading when what should really be happening (assuming HPC is now the pool) is HPC.query/request.pg.query or for transactional queries, HPC.client.query.

If the api was kept as-is but the user needed to make a transactional query then it would become HPC.getCon().client.client.query - definitely doesn't look right to me :smile:

My recommendation would be for a fairly major rewrite including breaking changes to the api :+1:

Happy to make a PR if people think it's appropriate.

roryc89 commented 6 years ago

My understanding is that the problem that hapi-postgres-connection was solving (handling the connecting and releasing of clients) is no longer an issue with the client pool. Is it worth deprecating this module now so DWYL does not have to maintain it?

roryc89 commented 6 years ago

Of course it may also make sense to expose the pool so current users of the module have better performance.

nelsonic commented 6 years ago

@finnhodgkin thanks for opening this issue and sharing your feedback regarding perf. As @roryc89 says, this module is a "relic" of the time when pg did not have the feature and we had to manually shut-down connections in our tests etc.

If there's a good "official solution" I'm 100% happy to "deprecate" this module. 👍 It does not have that many users so we won't be "upsetting" too many people. https://www.npmjs.com/package/hapi-postgres-connection image Note: There hasn't been much "maintenance" required ...

Feed free to prepare a PR along the lines of: "This module is no longer required if you are using the latest pg and hapi ..."

Thanks.