[jameshartig]

Instead of closing connections before creating new ones this changes the health checker to create connections before closing them. It also will not close idle connections if that would put the pool below minimum connections.

I also added more stats to provide better visibility into the churn of the pool.

Fixes #1164

[jackc]

There's a lot going here -- more changes at once than I'm comfortable with for such a critical part of the code. At the very least the stat tracking and behavior changes should be considered separately.

---

It appears that creating connections before closing them could allow the pool to temporarily exceed MaxConns. It could potentially double MaxConns if MinConns == MaxConns. I don't think that is desirable. We already have had issues with that. See #679 and 39b096d.

If we are going try to reduce cases where there is a drastic change such as many connections being created or closed at once I think we should take a different approach. My thought is the health check should have a loop that executes as long as there are any changes required to restore health -- but each iteration of that loop only makes one change. i.e. each loop iteration will create or close one connection. If creation happened before close that would ensure that there were no cases where the pool was suddenly emptied or where there was suddenly way more than MaxConns.

---

As far as the stat changes go, ConstructingCount is already part of the underlying stats -- and it means something different. The other stats seem only nominally useful ... not sure it is worth adding another lock to every connection and health check.

[jameshartig]

At the very least the stat tracking and behavior changes should be considered separately.

The stat changes felt related because they are touching the same code and were useful in testing to understand what is actually going on rather than just testing what the resulting number of connections are. They're also critical for us in production to understand how often the pool is changing and why its changing. If connections are consistently being closed for being idle maybe we can lower MinConns or if we're consistently having to make connections maybe we need to raise MinConns, for example. I can separate them out to a new PR if that's better?

It appears that creating connections before closing them could allow the pool to temporarily exceed MaxConns.

I was concerned about that but since it would only be for a second or so I thought it might be okay. Definitely understand needing to respect that.

My thought is the health check should have a loop that executes as long as there are any changes required to restore health

I actually started with that approach but thought it was needlessly complicated. I can go back to that pretty easily and push up a new commit. What I had done is guarantee we always stay under MaxConns by closing a connection first then creating a connection but only doing one per loop. The downside is that it required a LOT more calls to AcquireAllIdle which is why I originally abandoned it. Now I think a compromise would be to try to do as much as we can each loop while still respecting MaxConns. That might mean creating and destroying 3 conns if the difference between MaxConns and MinConns is 3. But if MaxConns == MinConns then nothing can get done each loop. In that case should I ignore MaxConns or MinConns?

As far as the stat changes go, ConstructingCount is already part of the underlying stats -- and it means something different.

There's ConstructingConns but that's how many are constructing right now. I tried to keep the format the same as things like AcquireCount and CanceledAcquireCount. I'm happy to rename it if there's a better option. Maybe NewConnsCount?

The other stats seem only nominally useful ... not sure it is worth adding another lock to every connection and health check.

I think it says something with the stdlib has a stat given the scrutiny of the changes there. Knowing why connections were closed is extremely important in tweaking the lifetimes. Otherwise you have no idea why things are closing. The health check is the only other place locking that mutex and it only runs every minute, by default, so the lock contention should be almost nonexistent.

[jameshartig]

I updated the PR with the suggested changes. I also tweaked some of the tests to check in AfterConnect that the pool is what we expect. That was the best way I could think of to test the state of the pool in the middle of the health check loop

[jackc]

I just spent a while looking over the code and I do not see any obvious problems. But as the old saying goes, there is a big difference between no obvious problems and obviously no problems.

checkHealth() is significantly more complicated than the current production checkIdleConnsHealth() and checkMinConns() were. We have atomics, explicit goroutines, and wait groups all in one function. Now of course the current code has the "reset everything all at once" problem and the new code doesn't, but we are giving up a lot of simplicity to solve that problem.

I know that the async res.Destroy is one of the complicating factors, but I still can't help but think that it should be possible to solve the "reset everything all at once" problem in a simpler fashion.

Here's more of what I was originally picturing.

First, checkIdleConnsHealth() and checkMinConns() are changed to only create or destroy one connection each call -- and they also return true if they did anything and false if they didn't. Then loop until nothing changes.

for p.checkMinConns() || p.checkIdleConnsHealth()  {
	time.Sleep(500 * time.Millisecond)
}

The sleep gives some time for the async Destroy to finish. If we wanted to get really clever the destructor passed into puddle.NewPool() could signal when the conn finished closing. Though I'm not a fan of getting unnecessarily clever so I would probably go with the sleep.

Also, I noticed that Release() now checks for max conn lifetime exceeded. I'm not sure that's a good idea. If all the connections were created at the same time and they are all in heavy use then they would all get killed at about the same time.

[jameshartig]

I apologize for the complexity. I was trying to satisfy all of the conditions (stay above min conns and below max conns, ignore idle conns, etc). The only issue I see with your proposal is that checkMinConns won't know how many connections to create since checkIdleConnsHealth is the one calculating how many to remove. I tried to separate the creating and destroying but they're too linked now. If we're at minConns then checkMinConns won't create any and then checkIdleConnsHealth could destroy the only connection left and still leave the pool empty. I could make checkMinConns determine how many are going to be destroyed but then that'll be another call to AcquireAllIdle each loop.

Also, I noticed that Release() now checks for max conn lifetime exceeded.

I think you might have this backwards. I removed that code from Release to simplify where connections are getting destroyed and to prevent what you described.

[jackc]

The only issue I see with your proposal is that checkMinConns won't know how many connections to create since checkIdleConnsHealth is the one calculating how many to remove.

I don't think it would need to though. At least if we allow it to go one below min conns for a brief window -- and I think that is acceptable.

Here's what I picture happening. Assume the pool has min conns = 4 and it currently has 4 connections and all of them are too old. Our pool contents are the following:

(conn 1 - too old), (conn 2 - too old), (conn 3 - too old), (conn 4 - too old)

In the first iteration of the loop p.checkMinConns() does nothing and p.checkIdleConnsHealth() removes one connection.

(conn 2 - too old), (conn 3 - too old), (conn 4 - too old)

Next iteration: p.checkMinConns() creates a conn so p.checkIdleConnsHealth() is never called.

(conn 2 - too old), (conn 3 - too old), (conn 4 - too old), (conn 5 - okay)

Next iteration: p.checkMinConns() does nothing and p.checkIdleConnsHealth() removes one connection.

(conn 3 - too old), (conn 4 - too old), (conn 5 - okay)

Next iteration: p.checkMinConns() creates a conn so p.checkIdleConnsHealth() is never called.

(conn 3 - too old), (conn 4 - too old), (conn 5 - okay), (conn 6 - okay)

Next iteration: p.checkMinConns() does nothing and p.checkIdleConnsHealth() removes one connection.

(conn 4 - too old), (conn 5 - okay), (conn 6 - okay)

Next iteration: p.checkMinConns() creates a conn so p.checkIdleConnsHealth() is never called.

(conn 4 - too old), (conn 5 - okay), (conn 6 - okay), (conn 7 - okay)

Next iteration: p.checkMinConns() does nothing and p.checkIdleConnsHealth() removes one connection.

(conn 5 - okay), (conn 6 - okay), (conn 7 - okay)

Next iteration: p.checkMinConns() creates a conn so p.checkIdleConnsHealth() is never called.

(conn 5 - okay), (conn 6 - okay), (conn 7 - okay), (conn 8 - okay)

Last iteration: Neither p.checkMinConns() nor p.checkIdleConnsHealth() does anything so the loop ends.

It does briefly fall below min conns, but it never flushes the whole pool either. I think it is reasonable behavior.

Also, I noticed that Release() now checks for max conn lifetime exceeded.

I think you might have this backwards. I removed that code from Release to simplify where connections are getting destroyed and to prevent what you described.

Ah you're right. Though looking back on it again I think I see the reason why that code was there. The pool health check only runs occasionally and only can look at idle conns. If the pool was under heavy usage a conn might never actually be visible to the health check and thereby never actually be closed. Maybe the check should remain in Release() but it should automatically create a new conn if it would fall below min conns.

[jameshartig]

At least if we allow it to go one below min conns for a brief window -- and I think that is acceptable.

I would prefer to air on the side of going above max versus going below min but I'll start with what you outlined and see if anything stands out as a way to solve it. If there is more than 1 connection above MinConns is it okay for multiple to be closed? I would think p.checkIdleConnsHealth() should try to close as many as it can to reduce calls to AcquireAllIdle().

Maybe the check should remain in Release() but it should automatically create a new conn if it would fall below min conns.

Should it block Release or should it just make the connection in a goroutine? I wasn't sure if it would be okay for Release to take multiple seconds making a new connection (obviously that's a worst case).

Overall, I appreciate the back-and-forth and I hope my next push is much closer to what you had in mind.

[jackc]

I would prefer to air on the side of going above max versus going below min

I prefer to error on the side of going below min conns because the worst that happens below min conns is an Acquire has to wait for the connection to be established. But if it goes over max conns the worst that happens is this or another application attempts and fails to establish a connection because the PostgreSQL server reaches its limit.

If there is more than 1 connection above MinConns is it okay for multiple to be closed? I would think p.checkIdleConnsHealth() should try to close as many as it can to reduce calls to AcquireAllIdle().

I'm not sure it's necessary. AcquireAllIdle() should not be that expensive. If benchmarks prove otherwise we could change it. But doing 1 conn at a time with a sleep between each one also has the advantage of gradually spreading the connection lifetimes out so they don't all die at the same time.

Should it block Release or should it just make the connection in a goroutine? I wasn't sure if it would be okay for Release to take multiple seconds making a new connection (obviously that's a worst case).

Definitely should not block. Probably the most elegant solution would be to use a channel to do a non-blocking send to the backgroundHealthCheck loop. It would just trigger the normal health check.

[jameshartig]

I prefer to error on the side of going below min conns because the worst that happens below min conns is an Acquire has to wait for the connection to be established.

Makes sense. I made sure to only preemptively create a new connection if we're under MaxConns.

Definitely should not block. Probably the most elegant solution would be to use a channel to do a non-blocking send to the backgroundHealthCheck loop. It would just trigger the normal health check.

The only issue I ran into with that was that the health check did nothing since Destroy is async and when the health check ran, the TotalConns was unchanged. I ended up making a specific channel for when release destroys so the health check could preemptively create a new connection.

The other thing I ran into was: What to do if checkMinConns failed because we can't create new connections for some reason? This wasn't handled previously and I'm not sure of the approach you want. I just decided to have it retry a couple times before giving up but I can remove that if you think its too complex.

[jameshartig]

The TestPoolBackgroundChecksMaxConnLifetime and TestConnReleaseDestroysClosedConn tests seemed flaky (see latest master: https://github.com/jackc/pgx/runs/6718683623?check_suite_focus=true). They were randomly failing for me too in this PR and I think I fixed them by adding a for loop. For some reason the Destroy would sometimes take more than 500ms. Not sure if other tests need to be adjusted too or if there's a more elegant solution.

[jackc]

Sorry to keep nitpicking this PR, but I still think this is too complicated for the benefit gained.

We want to avoid all the connections in the pool dying at once when the all hit their max lifetime at once, but it's not worth worrying about it briefly going under by a small amount. MinConns == 10 and actual conns == 0 is a problem. MinConns == 10 and actual conns == 8 or 9 for 1 or 2 seconds is not.

There seems to be a lot of code dedicated to carefully preventing actual conns from going under min conns and minimizing the amount of time when it does. I'm willing and prefer to trade a little less rigor in maintaining MinConns for less code.

Obviously, I haven't thought through the edge cases in the detail you have (in particular -- good catch on needing to handle closeChan in the loop) , but my guess is that this whole change excluding tests should only need to be ~30 LOC.

For example, given the previously suggested change of checkIdleConnsHealth() and checkMinConns() only making one change and returning true if they did anything, and given a buffered healthCheckChan with a len of 1 -- what about a backgroundHealthCheck() like this:

func (p *Pool) backgroundHealthCheck() {
	ticker := time.NewTicker(p.healthCheckPeriod)
	defer ticker.Stop()

	go func() {
		for {
			select {
			case <-p.closeChan:
				return
			case <-ticker.C:
				p.healthCheckChan <- struct{}{}
			}
		}
	}()

	for {
		select {
		case <-p.closeChan:
			return
		case <-p.healthCheckChan:
			if p.checkIdleConnsHealth() || p.checkMinConns() {
				select {
				case p.healthCheckChan <- struct{}{}:
				default:
				}
			}

			time.Sleep(500 * time.Millisecond)
		}
	}
}

*Conn.Release would do something like this when it destroyed a connection:

go func() {
	time.Sleep(time.Second) // give connection time to completely close before triggering health check.
	select {
		case c.p.healthCheckChan <- struct{}{}:
		default:
	}
}()

The sleep should be long enough for the destroy to finish before the health check is triggered -- and if occasionally its not enough it really shouldn't matter.

---

The other thing I ran into was: What to do if checkMinConns failed because we can't create new connections for some reason? This wasn't handled previously and I'm not sure of the approach you want. I just decided to have it retry a couple times before giving up but I can remove that if you think its too complex.

Creating the connection should be done asynchronously (not sure if that's happening in this new version). So we won't have any way of knowing if the connection was established successfully -- all we know is we started a connection process.

---

The TestPoolBackgroundChecksMaxConnLifetime and TestConnReleaseDestroysClosedConn tests seemed flaky (see latest master: https://github.com/jackc/pgx/runs/6718683623?check_suite_focus=true). They were randomly failing for me too in this PR and I think I fixed them by adding a for loop. For some reason the Destroy would sometimes take more than 500ms. Not sure if other tests need to be adjusted too or if there's a more elegant solution.

Yeah, it's annoying dealing with timing in tests like that. I wouldn't worry about it for now.

[jameshartig]

We want to avoid all the connections in the pool dying at once when the all hit their max lifetime at once, but it's not worth worrying about it briefly going under by a small amount.

You'd know better than I how common MinConns = 10 is versus MinConns = 1. We have MinConns = 1 in many of our production services. Granted I could make it 2 or 3 but to have more connections sitting around all the time just to prevent an empty pool when we might need it seems like a waste of resources. We might be an edge case but connection setup for us takes a few seconds per connection since we have to prime the pg catalog in AfterConnect (which is a separate issue in itself). It is actually more common than you'd think, multiple times an hour, that we'd see calls taking multiple seconds and fsil SLO because they're waiting in Acquire for a new connection since the pool was emptied. Hense why I started this PR anyways.

what about a backgroundHealthCheck() like this:

The extra goroutine to listen on the ticker just to write to the check channel seems unnecessary. Moving the actual health check if or loop into a separate function (or even duplicating the code since it's so small in your example) and having 2 branches call it (on ticker or channel recv) seems a lot simpler for that code. My current commit's backgroundHealthCheck() is basically:

for {
		select {
		case <-p.closeChan:
			return
		case <-p.releaseDestroyChan:
			p.checkHealth(true)
		case <-ticker.C:
			p.checkHealth(false)
		}
	}
}

Which is a lot easier to understand in my opinion especially if I rename releaseDestroyChan because I have to give up caring about going under MinConns and get rid of the boolean argument to checkHealth.

I couldn't find a use for checkMinConns returning anything other than an error since really the only thing that should keep the health check going is if there are more connections to remove.

*Conn.Release would do something like this when it destroyed a connection:

That seems fine assuming we're okay with the pool potentially not having any connections for at least a whole second. Again, not sure how common that is or how unexpected that is.

Also, even with MinConns=10, the initial connections are all made at the same time so they'll all expire at the same time. So you could still end up with an empty pool if you're using all 10 connections heavily and each time you Release() they're destroyed. If you had 10 RPS and MinConns=10 then your pool would be empty in a second before we even try to make new ones.

Would you be open to the max lifetime having some configurable jitter? Maybe time.Minute? So we would randomly add to the lifetime when checking up to the "max jitter".

Creating the connection should be done asynchronously

I'm using the existing createIdleResources method which does make them asynchronously but waits until they're done to return an error. If you have MaxConns = 10 but there's an transient error making new connections without checking an error you could end up draining the whole pool rather than keeping some around until you can make new connections again. I can remove the retries if you think that's too complex and maybe just bail if checkMinConns ever errors?

An example that happened to us with CockroachDB was that the leader was overloaded so new connections failed because they timed out trying to authenticate but existing connections were already authenticated so they were fine. Our applications kept trying to make new connections after killing existing ones which further overloaded the leader and the only solution was for us to stop all of our applications and start them up one-by-one.

[jackc]

Okay, I think I see why now we keep coming to different solutions -- we've been looking at very different, almost opposite problems.

The problem I was looking at is:

• A busy server
• With lots of connections
• Where establishing a connection is expensive enough to avoid when possible, but not so much as to be catastrophic if it occasionally happens while servicing a request
• The primary concern when replacing old connections is never going over max connections

And the problem you were looking at is:

• A mostly idle server
• With one connection
• Where establishing a connection is extremely expensive: too expensive to ever happen when servicing a request
• The primary concern when replacing old connections is never going below min connections

---

It may not be possible to solve both problems at the same time without a more complicated solution than I was hoping for. 😮‍💨

---

My insistence on handling one connection at a time with a delay in between cycles was aimed at the avoiding the thundering herd problem on a busy server with many connections. But I see now that that approach is unhelpful or even harmful to the problem you are trying to solve...

---

It appears to me that the cause of most of the confusion I had with this PR was the attempt to work around the asynchronous, fire-and-forget nature of closing and creating connections. When a connection is created we know that we started the connection process but we don't get a signal when it is ready for use. And when a connection is destroyed we don't get a signal when it is finished and there is now room in the pool.

If we had that signaling mechanism we could reliably handle cases like the following:

When connection is too old:

• If there is room in the pool to create the replacement, mark old connection as to-be-closed but still active, then create the replacement, wait until the replacement signals it is ready, then close the old connection.
• If there is not room in the pool to create the replacement, close the old connection, listen for the signal of its close completion, then immediately create the new connection.

That would still be a significant increase in complexity, but maybe it is unavoidable. Or maybe one of the approaches in this PR is enough... IDK.

[jameshartig]

Updated the PR and removed a lot of the complexity. Getting something merged it better than nothing.

I did add a "jitter" which can help reduce the chance for the pool to be emptied via Release()'s because the jitter should help spread out the lifetimes.

I had to remove a bunch of the tests I added because without a guarantee of staying above MinConns a lot of the assert.EqualValues(t, 1, stats.TotalConns()) were failing because it happened to be 0 at the time of the assert since we might've been in the middle of the health checker running.

[jameshartig]

Okay, I think I see why now we keep coming to different solutions -- we've been looking at very different, almost opposite problems.

I was trying my best to find a middle ground between both solutions which is why there was a lot of complexity.

If we had that signaling mechanism we could reliably handle cases like the following

I was thinking about that and I thought puddle could add something like MarkToClose which would mark the connection as "should-be-closed-later" but still make it available in the pool. Then later pgxpool could call CloseMarkedResources after it was sure the new conns were created. CloseMarkedResources would try to close all the resources that were previously marked but any resources that are Acquired would be instead closed when they returned to the pool. This would require another field on the resource to track the Marked, CloseUponReturn, None state of the resource.

That was just my first thought but I didn't have any experience with puddle and I wasn't sure where else it was used so I was hesitant to add that complexity if it was just used by pgxpool, which is why I tried to add the complexity here too. You are correct though, things would be a lot easier in pgxpool if we could get some better control of when things are in and leaving the pool.

[jackc]

I was trying my best to find a middle ground between both solutions which is why there was a lot of complexity.

Yeah, I agree. This is probably the best we can do without a signaling mechanism.

That was just my first thought but I didn't have any experience with puddle and I wasn't sure where else it was used so I was hesitant to add that complexity if it was just used by pgxpool, which is why I tried to add the complexity here too.

Puddle was designed with pgx in mind, but is intended to be entirely generic resource pool. It's goal is to encapsulate all the scary concurrent parts of a context.Context aware pool -- and nothing else that can be implemented by a wrapper. My experience building and maintaining that library is part of why I was so instinctively against adding complexity. It is a tricky bit of code (see https://github.com/jackc/puddle/blob/4e959849469a78bd060ffa7531102bfca85bf125/puddleg/pool.go#L264 for example).

But I don't think it would be necessary to modify puddle. A puddle.Pool has constructor and destructor functions (provided by pgxpool here:

pgx/pgxpool/pool.go

Line 196 in a814153

p.p = puddle.NewPool(

). Those functions could signal just before returning to indicate a that a connection or disconnection was complete.

[jameshartig]

Those functions could signal just before returning to indicate a that a connection or disconnection was complete.

One issue is that the destructor seems to be called before it leaves the pool: https://github.com/jackc/puddle/blob/4e959849469a78bd060ffa7531102bfca85bf125/puddleg/pool.go#L495

Is it possible for the connection to be destructed while it's still in the pool? It looks like the lock is after the deconstructor so is it possible for something to still acquire it from the pool between deconstructor and before it's removed?

[jackc]

It is destructed before it leaves the pool, but that is what we want. destroyAcquiredResource() is only called on an acquired resource. So nothing else can acquire it while it is being destroyed.