Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 6, 2023
Merged

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

merged 2 commits into from
Mar 6, 2023
+38 −0

Conversation

@nihonjinrxs
Copy link
Contributor

@nihonjinrxs nihonjinrxs commented Oct 17, 2022 •
edited
Loading

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

nihonjinrxs commented Oct 24, 2022

@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 •
edited
Loading

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

nihonjinrxs commented Dec 7, 2022

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

nihonjinrxs commented Dec 28, 2022

@brianc Any chance we can get this one merged soon?
@nihonjinrxs
Copy link
Contributor Author

nihonjinrxs commented Jan 24, 2023

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

nihonjinrxs commented Feb 15, 2023

@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
@shleeable shleeable mentioned this pull request Mar 8, 2023
Closed
@nihonjinrxs nihonjinrxs deleted the feature/pg-pool/events/emit-release branch March 10, 2023 18:35
@nihonjinrxs nihonjinrxs mentioned this pull request Mar 10, 2023
Merged
@dholms dholms mentioned this pull request Apr 19, 2023
Merged
nihonjinrxs added a commit to nihonjinrxs/DefinitelyTyped that referenced this pull request May 2, 2023
@nihonjinrxs
Add 'release' event type to pg Pool types
2791804 
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
@nihonjinrxs
Emit a 'release' event when a connection is released back to the pool (…
eb2dfdf 
…brianc#2845)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@nihonjinrxs @weyert @brianc