Add maxUses pool config option

[chriskchew]

This PR is intended to help with balancing connections to a growing read replica cluster. Specifically, this scenario came up when adopting AWS Aurora in an environment where the read replica cluster expands and contracts based on weekly traffic cycles.

The PR adds an optional pool configuration maxUses and adds logic to the Pool that removes a connection upon release when it has been acquired and released maxUses number of times.

I expanded the existing "item wrapper" pattern used for IdleItem and added a ClientItem wrapper for the items put into the _clients array. The ClientItem wrapper holds the use count for the client instance so that the release function can have the data it needs in order to determine when the connection has reached the end of its run.

I also added a few changes for issues I encountered while setting up my local dev environment:

• Added a little logic to checkType in the bring-your-own-promise to explicitly fail when unexpected error occurs

• Added some instructions in the main README listing the steps for dev setup

Thank you for considering these changes!

[chriskchew]

Note for reviewer: It took me a while to figure out why this test kept failing with an error about the value being undefined, so I added the new argument so we can differentiate an unexpected error scenario and subsequently report it as a test failure.

[brianc]

nice - thanks. As an aside, I think it's probably time I deprecate BYOP for pg@9.0 as all current supported versions of node include a native promise.

[chriskchew]

@brianc I'm wondering if there is something I can do to make it easier for you to review this PR? Let me know if you have any questions or initial reactions to the proposed changes. Thank you!

[brianc]

sorry for the delay - I'll take a look at this tomorrow morn!

…

On Tue, Apr 7, 2020 at 8:22 AM Chris Chew ***@***.***> wrote: @brianc <https://github.com/brianc> I'm wondering if there is something I can do to make it easier for you to review this PR? Let me know if you have any questions or initial reactions to the proposed changes. Thank you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2147 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIPTUG2MJBW7LKRVCDLRLMSLFANCNFSM4LUUHZOQ> .

[brianc]

The code looks logical to me! Question though...how do you feel about just adding a _useCount variable or something directly to the client instance versus wrapping it? My concern is around a few of the unit tests that had to change because they were accessing pool._clients directly. Now, I know that's a "private" field but w/ the size of the install base of this lib it might still be being used by more than a handful of people for custom workarounds and stuff. This change would break all their code. I think to be safer for a semver minor release it might be better to monkey patch the existing client instances versus having the pool store something that wraps them.

In the nearish future I want to actually wrap all clients in the pool w/ a proxy or monkey-patch over the .end method on pooled clients to just return the clients to the pool or something if you call .end and maybe log a warning or something. Nasty case of bugs where you end the client yourself, return it, and then we didn't notice it was ended. Anyways.... We already monkey the client and attach a .release function to it if its pooled, I think having a _maxPoolCheckoutCount or something prop patched onto the client might be less risk to breaking existing libraries.

[chriskchew]

I appreciate your thoughtful review!

Question though...how do you feel about just adding a _useCount variable or something directly to the client instance versus wrapping it?

This is a great question. My initial thought was similar to yours, that I would add _useCount to the Client and simplify the Pool changes to simply increment that number and close it when the Client is deemed to be "expended".

What drew me away from that approach was partially that I didn't understand if it was safe to simply "patch" that value into the Client instance when it is instantiated in Pool.newClient(), and then also because the "max uses" seemed mostly to be a Pool concern, and thus would be data that is held on the Pool side of the relationship.

In terms of adding that property to client, would the recommended approach simply be to patch that property on the Client instance inside newClient() and increment it in release(), or do I need to more explicitly define a field on the Client and NativeClient prototypes, probably along with accessor methods incrementUseCount() and getUseCount()?

[brianc]

What drew me away from that approach was partially that I didn't understand if it was safe to simply "patch" that value into the Client instance when it is instantiated in Pool.newClient()

Yeah...should be safe! Maybe call the prop something slightly longer like _poolUseCount or something so it's obviously coming from the pool.

the "max uses" seemed mostly to be a Pool concern

That's true, so is monkey patching .release() on a client to some extent. I think its preferable in this case over possibly breaking things by not having pool._clients not actually be clients.

In terms of adding that property to client, would the recommended approach simply be to patch that property on the Client instance inside newClient() and increment it in release()

Yeah that's what I'd do. Just patch it in newClient and increment it in release. You could actually both patch and increment it in release. Something like near on this line:

https://github.com/brianc/node-postgres/blob/master/packages/pg-pool/index.js#L265

client._poolUseCount = (client._poolUseCount || 0) + 1

💭

[chriskchew]

Thanks for the direction. I will probably start with a fresh branch just to keep a cleaner commit log since the bulk of the changes need to be unwound.

[brianc]

haha okay up to you! I usually squash merge anyway so whatever is easiest for you!

…

On Thu, Apr 9, 2020 at 1:26 PM Chris Chew ***@***.***> wrote: Thanks for the direction. I will probably start with a fresh branch just to keep a cleaner commit log since the bulk of the changes need to be unwound. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2147 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHILD4ASZ6XVGPHMDGYDRLYHOXANCNFSM4LUUHZOQ> .

[chriskchew]

Closing this in favor of #2157