Support connection maximum lifetime to recycle connections regularly

[kdubb]

Adds maxLifetime to PoolOptions and alters PooledConnection to honor the maximum lifetime as well as the idle timeout.

Motivation:

A maxLifetime option has been added to PoolOptions that determines the maximum amount of time a connection will stay connected; after this amount of time it will be closed at the next eviction check.

The "expiration" naming was changed to "eviction" to represent the more general check that is performed. The idle timeout refreshing was also change to start immediately after connection and update accordingly to make the idle timeout more predictable and match the max life time check.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

[kdubb]

There is a follow on PR that adds a min-size attribute to ensure a minimum # of connections are always available. Will create after this is merged.

[kdubb]

@vietj Fixed testNoConnectionLeaks test. It was expecting pg_terminate_backend to return an integer but it returns a boolean.

I also added a test for maxLifetime similar to the test for idleTimeout which I also altered to explicitly ensure maxLifetime is not participating.

[kdubb]

@vietj Can we get this PR moving forward? I'm assuming this is easier than #1297 and is more important to fixing the Quarkus extensions.

[vietj]

A couple of very minor changes, well done.

Please provide a PR to backport to 4.x branch

[kdubb]

Requested changes are completed. Should be good to go.

[tsegismont]

Thank you @kdubb, LGTM

A few minor comments and proposed changes.

[kdubb]

Updated with requested changes and squashed into two distinct commits. The second one is about fixing tests unrelated to the feature, so kept it as separate commit.

[tsegismont]

@kdubb can you please rebase?

[tsegismont]

@kdubb have you found any issue while rebasing or haven't found time to do it?

[tsegismont]

I went ahead and merged the PR manually, thank you @kdubb

[kdubb]

Apologies I was out for the last week. Thanks for merging!

[tsegismont]

No worries

[chandramouli-r]

There is a follow on PR that adds a min-size attribute to ensure a minimum # of connections are always available. Will create after this is merged.

Hi @kdubb , thanks a ton for this PR. Coincidentally, I posted a similar request on the group.

I had a 2 follow-up ideas on this feature, namely:

1. Adding a jitter to the maxLifetime
2. Marking IN_USE connections that are ready to be evicted with a flag, since the current approach skips in_use connections that are ready for eviction (this flag will prevent "Acquire" from returning those connections as part of getConnection()).

I can work on the above, but I was wondering if you are working on the min-size PR to ensure min # of connections (since you mentioned it earlier)? I think my follow-ups will work well if we can ensure connections are replenished in the background as soon as they are marked for eviction.

Thanks again.

[chandramouli-r]

(FYI both my ideas are borrowed from here)

[kdubb]

@chandramouli-r Hikari is a great connection pool for JDBC but not everything translates to the reactive pool (which I think can be much simpler).

WRT "jitter", I really don't see the need for it given that each connection is only recycled after it's done being used. From observing this PR actually in use, even a set of connections that starts off in synchronized recycling (because they were started by a min-size parameter) becomes out of sync fairly quickly because of different query times used on each connection.

Flagging "in use" is likewise not entirely needed. I looked at this quite a bit during implementation. First, much of the "standard" usage acquires and releases a connection from the pool without explicitly obtaining a connection and releasing it (by client code). Second, even in the case of client code that does explicitly acquire connections they are for short lived queries and (if properly written) will close the connection soon after.

This boils down to, connections are constantly getting a chance to be closed within a reasonable amount of time and the "jitter" is basically built in due to the difference in queries. I would let this sit and see if there is anything more that is actually needed before investing time time in adding features that my not be explicitly needed.

[kdubb]

As far as min-size I'll be opening that PR today.

[tsegismont]

WRT "jitter", I really don't see the need for it given that each connection is only recycled after it's done being used.

I find the idea interesting because, if many connections are created around the same time, they will be evicted around the same time too (when max lifetime has been reached).

[kdubb]

Yes. My point was though that after about a couple hours of running the application the evictions are happening seconds apart and they only get further as time goes on.

[kdubb]

That was with a 30 minute eviction time.

[chandramouli-r]

@kdubb Thanks for working on the min-size PR.
I'm fine with using the 2 changes: maxLifeTime + min-size and checking (at least for my workload) if there are connections that last well beyond their lifetime.

I agree that jitter may be naturally added since some connections will be idle during the eviction check, and some might be in use, thus delaying their eviction.

The reason I proposed the second idea (adding an "evicted" flag) is to handle the case where a busy pool is always using some connections in steady state, and the cleaner may be racing with the connection usage to evict it. I can do some bookkeeping in my application to check how long connections last beyond their lifetime, and based on that, maybe it could be worth adding an "evicted" flag.

[kdubb]

The max lifetime is a hard check after every return to the pool. The only time a connection will last well beyond its lifetime is if it is acquired and held for a period of time well beyond maxLifetime.

I've used PostgreSQL extensively and one use case like this that exists for acquiring extremely long lived connections is notifications. Applications that use notifications tend to acquire a single connection and keep it open as long as possible (i.e. forever). An "in use" flag would do nothing in this case as it would just be marked "in use" and without a forceful closure it would stay marked.

I really don't know of another reason an application would not be regularly returning connections back to the pool after every query or transaction; neither of which should come any where close to a maxLifetime.

[chandramouli-r]

I think this is where my confusion lies:

The max lifetime is a hard check after every return to the pool

I didn't see a check for hasLifetimeExpired on the "return to the pool path" in the PR (it is only invoked during evict() which is called by the Cleaner).

(Maybe I'm just looking at the wrong place).

[kdubb]

Actually, this might be part of the minSize changes. I'll be looking tonight.

[tsegismont]

I agree with @chandramouli-r .

We could have a predicate in the Recycle implementation. For SqlConnection pool, it would consist in verifying maxLifetimeTimestamp is in the future. If it's not, the connection should be removed from the pool instead of being recycled.

Actually, this might be part of the minSize changes. I'll be looking tonight.

@kdubb please create separate PRs if possible.

[chandramouli-r]

Hi @kdubb - just wanted to check if you got time to look at the minSize changes (I can help out if you have a working branch). Thanks.

[kdubb]

@chandramouli-r minSize for 5.x is in PR #1319.

The "hard check" I referred to had to be removed. Support is needed in the ConnectionPool and Lease in vertx-core

[kdubb]

@chandramouli-r I've opened #4680 in the core repo to request the needed change. With this (or something similar) it's an easy hasLifetimeExpired check in the PooledConnection.cleanup method to ensure maxLifetime is honored.