cc @bradfitz @kardianos

@pfreixes The preferred way to handle this is to wrap the Connector with a one that rate limits the Connect method. Is this possible for you to implement?

Yes definitely we can use our own Connector for rate limit the connection creation, this will solve the main issue presented.

However, there are still some other benefits that by overriding the connector we wont benefit, like for example for those requests that did not found a connection but there was still room and asked for a new connection while they are waiting already connections opened could be added back to the pool, these at connector level wont have the chance to reuse them. The proposed solution applies the rate limit while allows the system to reuse connections that were put back to the pool.

Also notice that this a situation that anyone that uses this connection pooling implementation might suffer, so I would not expect to have to ask about implementing their own Connector implementation for apply the proper throttling. Yeps a third package can be created outside of the standard library that everybody can use, but it's still weird having to do that. And still thinking that the trotting + reusing connections returned back to the pool is a good combo that would need to go together.

Not sure if there is any way to more forward with this proposal without creating too much friction, or do you think that the benefits of the proposal will not justify a change in the current implementation?

On the other hand, the other improvements suggested could be added on top of the current implementation also, so maybe its worth it to consider them regardless of the original discussion, talking about having a way to configure the minimum idle connections, time for considering a connection idle, counter for telling number of connections opened.

WDYT?

Yes definitely we can use our own Connector for rate limit the connection creation, this will solve the main issue presented.

Great.

However, there are still some other benefits that by overriding the connector we wont benefit, like for example for those requests that did not found a connection but there was still room and asked for a new connection while they are waiting already connections opened could be added back to the pool, these at connector level wont have the chance to reuse them. The proposed solution applies the rate limit while allows the system to reuse connections that were put back to the pool.

I don't understand what you are trying to communicate in this paragraph.

Yeps a third package can be created outside of the standard library that everybody can use, but it's still weird having to do that.

I have heard you that this feels weird to you. This doesn't feel weird to me.

Not sure if there is any way to more forward with this proposal without creating too much friction,

I don't understand what you are trying to communicate here. We agree that limiting the connection opening can be done in the Connector. Now I just need:

What I need to proceed

Please explain what your desired outcome for interacting with database/sql is. Separate out different concerns.

I don't understand what you are trying to communicate in this paragraph.

@kardianos Suppose goroutine A needs a database connection, but all the ones in the pool are in use. So it asks the Connector for a new one. But (as per your suggestion), Connector.Connect itself is going to block in the middle via time.Sleep or similar in order to rate limit new connections. Meanwhile, goroutine B finishes what it was doing and returns its connection to the pool. Ideally goroutine A should take that connection instead of continuing to wait for no reason.

If database/sql were to cancel the context it passes to Connector.Connect when this happens then it would probably work. Based on a cursory evaluation it doesn't appear to do so. Another option is to allow Connect to return a channel or duration or something (perhaps wrapped in some error type) telling database/sql when to try again. Then database/sql can honor that while still accounting for things getting returned to the pool, but does not have to get into the details of how that specific delay was calculated.

@rittneje Thank you. However this example assumes that Max Connections limit hasn't been reached yet. If you set it at some reasonable limit, it will just wait for the B connection to finish and use that. If it hasn't reached it, then the most expedite thing would be indeed to open a new connection. If you truly apply a rate limit in the Connector, then under normal operation there should be no sleep anyway. The only time a rate limit would apply is to prevent a stampede which is a very different situation then what you presented in this example.

So while I believe I understand what you are saying, a rate limiter in said situation wouldn't actually take effect, because we are using a rate limit, not a standard delay. Unless everyone needed a new connection, in which the first few would be issued right away, and the others would need to wait and would wait even as existing connections returned. But in this case what I would want would NOT be a rate limit per say, but a limit on the number of active openers at a time that could be adjusted. I wouldn't want to serialize it totally; when staring a new application I would like to open MANY new connections at the same time, even if that number is bounded; further, I would want the number of open connection requests open to actually be a fractional amount of existing connections, compared to some max, so the closer it is to the max, the more likely it is to reuse a connection.

But...

Are you sure that the database connection rate limit is your problem? I would be given to think you are trying to address the wrong thing: perhaps you are too aggressively closing database connections? In the charts you posted, the number of connections closed every minute is way too high. 10 connections are open at a time but you have several thousand connections closed per minute. Maybe I'm misreading those charts.

@rittneje thanks for helping me to clarify, better an example that thousands of words, regarding this

If database/sql were to cancel the context it passes to Connector.Connect when this happens then it would probably work. Based on a cursory evaluation it doesn't appear to do so. Another option is to allow Connect to return a channel or duration or something (perhaps wrapped in some error type) telling database/sql when to try again. Then database/sql can honor that while still accounting for things getting returned to the pool, but does not have to get into the details of how that specific delay was calculated.

Not sure if i understand this properly, but having the feeling that would be better to keep the throttling complexity under the connection pooling - as it does the POC that I shared [1], by doing that the Connector can remain as it is Today without having to be bothered with this complexity.

@kardianos regarding this

Are you sure that the database connection rate limit is your problem? I would be given to think you are trying to address the wrong thing: perhaps you are too aggressively closing database connections? In the charts you posted, the number of connections closed every minute is way too high. 10 connections are open at a time but you have several thousand connections closed per minute. Maybe I'm misreading those charts.

Thats a different problem, which also happens, not sure if you have read my first post or maybe I did not explain myself in a right way. So let me try it again.

As part of the connection stampede problem, which is still an issue by itself, the implementation Today is too eager on closing connections, meaning that any connection that is returning back to the pool which is accounted as a connection that is > than MaxConnectionsIdle is automatically removed. So for avoiding this we are being forced to configure the MaxConnectionsIdle ~ MaxConnections, but this is IMO another workaround.

As part of the proposal for solving the connection stampede issue also Im proposing other changes, and one of them is related on making the closing of idle connections less eager. This would be done by allowing to configure the minimum time before a connection is considered idle, for example by default 60 seconds, where until this time is not reached the connection can be still be reused as a free connection and would not be considered idle. Together with this, the system can also prevent to remove all idle connections in a single shot, why? when a traffic of spike happens, what use to happen is that all connections are created in a short period of window, so without any safe guard all of them will be expired together. When a minimum time for considering a connection idle + not removing all of them at the same time is used, the connection pool has better chances to cope with traffic that has some variances in short period of times.

Another option that Im proposing, is adding a min idle connections option, this would help on having some room on the pool for unexpected traffic, for example while you can have a max connections of 32, you can have a min idle of lets say 10%, 3/4 connections that are just sitting there in case of new traffic comes.

The overall proposal is multi factorial and is composed by the following

• Avoid the connection stampede issue (what is in he POC)
• Avoid having the system purging all connections immediately
• Allow to configure a minimum room of idle connections

And regarding this

So while I believe I understand what you are saying, a rate limiter in said situation wouldn't actually take effect, because we are using a rate limit, not a standard delay. Unless everyone needed a new connection, in which the first few would be issued right away, and the others would need to wait and would wait even as existing connections returned. But in this case what I would want would NOT be a rate limit per say, but a limit on the number of active openers at a time that could be adjusted. I wouldn't want to serialize it totally; when staring a new application I would like to open MANY new connections at the same time, even if that number is bounded; further, I would want the number of open connection requests open to actually be a fractional amount of existing connections, compared to some max, so the closer it is to the max, the more likely it is to reuse a connection.

In my original post Im already mentioning that the the current constant [2] of a maximum 1 inflight create connection can be moved to a config parameter where the user can tell how many create connections would be allowed to have inflight, where 2 could be totally acceptable value

[1] pfreixes@506ae65
[2] pfreixes@506ae65#diff-5a8509ddb343ed538034d07d446b921410102a666e7e9715f13137c0e8690dbaR779

To rephrase, you are proposing:

1. Limit the number of in-flight database connections that can be initiated concurrently.
2. Add a minimum time duration to wait before a connection is considered idle (for the connection closer).
3. Add a minimum number of idle connection parameter, to automatically open idle connection in the background as more connections are used.

Is this correct?

yes, good summary @kardianos