Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit a 'release' event when a connection is released back to the pool #2845

Merged

Conversation

nihonjinrxs
Copy link
Contributor

@nihonjinrxs nihonjinrxs commented Oct 17, 2022

This PR has the Pool object emit a 'release' event upon the release of a client/connection back to the pool.

We're experiencing an issue that feels quite similar to what's described in #2262, and in reading through the comments there, I was interested to see @brianc's suggestion of adding a 'release' event as the corresponding bookend to the 'connect' and 'acquire' events. However, in searching the code, it seems this was never implemented, so I've implemented that here.

@nihonjinrxs nihonjinrxs force-pushed the feature/pg-pool/events/emit-release branch from 7cf4cbe to f77824c Compare October 18, 2022 16:06
@nihonjinrxs nihonjinrxs marked this pull request as ready for review October 18, 2022 16:12
@nihonjinrxs
Copy link
Contributor Author

@brianc let me know if you need anything more from me on this. Happy to help get this merged in whatever way you need. Thanks!

@weyert
Copy link

weyert commented Oct 24, 2022

Nice, looks like this could also be used to count the number of open connections?

@nihonjinrxs
Copy link
Contributor Author

nihonjinrxs commented Oct 30, 2022

Nice, looks like this could also be used to count the number of open connections?

@weyert You could, but I think it's probably easier to use the existing properties totalCount and idleCount to understand connection count in the pool. There's also a waitingCount which counts requests waiting for a connection to be available.

If you wanted to do it via events, there are also existing events emitted:

  • connect when a new connection is made to postgres
  • remove when a connection is dropped from the pool

What this PR adds is the balancing release event for the acquire event which is emitted when an existing connection in the pool is acquired to perform a query.

@nihonjinrxs nihonjinrxs marked this pull request as draft October 30, 2022 05:08
@nihonjinrxs nihonjinrxs marked this pull request as ready for review October 30, 2022 05:08
@brianc
Copy link
Owner

brianc commented Nov 21, 2022

This looks perfectly good to me! Thank you so much! Agree the symmetry here is nice and kinda long standing silly oversight on my part to not do that to begin with. 😄 ❤️

@brianc
Copy link
Owner

brianc commented Nov 21, 2022

okay weirdly....the tests don't seem to have run for this PR? Would you be able to try rebasing on master and see if they run at that point? Since it's a net addition only with like 1 line of change and good test coverage I can also just pull it and run them manually if you don't have time....though I do like every PR to go through CI.

@nihonjinrxs
Copy link
Contributor Author

Hey @brianc -- sorry for the delay in getting back to this. I've synced my fork, and CI seems to be running. Thanks!

@nihonjinrxs
Copy link
Contributor Author

@brianc Any chance we can get this one merged soon?

@nihonjinrxs
Copy link
Contributor Author

Hey, @brianc -- sorry to ping again, but it's been a while. Hoping to get this PR merged soon. We're currently operating off of a local branch incorporating this change, and I'd really like to be able to come back to the origin distribution if possible. Let me know if you need anything from me to make this happen. Thanks!

@nihonjinrxs
Copy link
Contributor Author

@brianc just checking in again -- would love to get this PR merged soon, so we can get off this branch build. Let me know if there's anything more you need here.

@brianc
Copy link
Owner

brianc commented Mar 6, 2023

sorry for delay! Will release it today!

@brianc brianc merged commit 810b125 into brianc:master Mar 6, 2023
@nihonjinrxs nihonjinrxs deleted the feature/pg-pool/events/emit-release branch March 10, 2023 18:35
nihonjinrxs added a commit to nihonjinrxs/DefinitelyTyped that referenced this pull request May 2, 2023
Added in brianc/node-postgres#2845 and released as part of version 8.10.0.
thijs pushed a commit to thijs/node-postgres that referenced this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants