Repository: go
Issue: database/sql: rate limit creation of new connections
Labels: Performance,NeedsInvestigation

I would like to proposal a little change on the connection pool implementation for the `database/sql` the change wants to address some issues with the current implementation which can **in some situations overwhelm the database during the event of multiple create connections**, the change wants to protect the underlying database from a connection stampede situation.

There is a draft of the change proposal that can be reviewed here [1], is just a draft and needs to be improved by adding all of the proper test coverage, but before going forward I would like to see if there is any chance that this proposal could be accepted.

### Connection stampede issue

Current implementation allows to create as many new connections are necessary while there is still room for it when new DB operations are handled and there are no new connections available, see code here [2]. By allowing this the database can be overwhelmed by having to handle multiple connection created concurrently, which impacts also the latency of the other operations that are already inflight.

I have observed that this situation sky-rockets the CPU of the database quickly to 100% impacting the latency (AVG and P99), see following screenshot for understanding the impact of this situation.

The following screenshot shows a synthetic workload of 1500 ops/sec for a simple `select value from table where key = key`, returning at most 1 row per query. For that test we configured the pool for having at most 32 open connections and leaving the max idle connections to the default 2 value, having 8 instances of a Golang service accessing to a Postgres DB.

<img width="1259" alt="current_pool_2_max_idle" src="https://user-images.githubusercontent.com/1226121/206929628-71a57bc4-ba33-46f6-bdac-1e2c2afbd3e0.png">

This issue presented, can happen at any time, not only during unexpected spike of traffics, also when the traffic is "stable", since the fluctuation of outstanding operations can vary due to different reasons - GC pauses, etc. In the previous example the traffic was sustained at 1500 ops/sec but it had enough instability for triggering the undesired behaviour, multiple connections created at once and later one having most of them removed because there were "enough" idle connections, **leaving the DB the full time in a partial outage** 

A workaround for avoiding this situation is having enough idle connections configured beforehand, where the connection stampede problem is circumvented since enough connections should be already in place, however it forces to have the system running with a lot of spare connections which is not a cheap resource for databases like Postgres.

I have repeated the same test with 6 and 12 max idle connections, and found that with 6 the DB was still overwhelmed and with 12 the system was able operate with an AVG P99 of 32ms and with the CPU operating at the expected levels

### Avoiding the create connection stampede

Im proposing a small change [1] in the current implementation that prevents to create multiple connections at once, and allowing to create a connection one by one while there is still necessity - there are still operations queued and the system did no reach the maximum number of opened connections.

Following screenshot shows how the same workload that before overwhelmed the DB now allows the system to keep working, still with a noticeable impact with the latencies but being able to keep responding the operations.

<img width="1259" alt="new_pool_2_max_idle" src="https://user-images.githubusercontent.com/1226121/206929240-fd2dbaa1-67e6-4d52-acbd-2a80e89d55f3.png">


When we increase the number of idle connections to 6 - previous example had 2 - we can see how the system can operate normally, with the latency expected in whats our SLO (20 ms) and most important the CPU usage of the DB is under 40%, with the current code even having 6 idle connections we were still overwhelming the DB

<img width="1259" alt="new_pool_6_max_idle" src="https://user-images.githubusercontent.com/1226121/206929270-09a2a86b-4b4b-46e7-addd-6f91e524a883.png">

One of the benefits of not allowing the creation of multiple connections is that operations that were queued can quickly start using connections release back to the pool, while in the current implementation this does not happen when there is still room in the pool. 

### Other interesting changes

The adoption of this new change could come with also other changes that could be beneficial

#### Configuring the maximum number of inflight create connections operations

Current proposal limits this to 1 max inflight create connection operation [4], this can be configured. Indeed we could achieve backwards compatibility with the current code by configuring it into the number of max open connections, or 0 if there were no max open connections configure.

#### Allowing also to configure the min idle connections

While we do have a configuration for the max idle connections, the current pool does not have the way to configure the pool in such a way that there are always some spare idle connections and it proactively makes sure that the pool has this spare resources ready to be used in case of connection spikes, with the proposed implementation we can "easily" add this new condition when `maybeOpenNewConnectionsDBLocked` [5] is called.

#### Minimum time for considering a connection idle

Current implementation considers a connection as idle from the moment that is released back to the pool, and if the number of connections considered free is larger than max idle connections they are immediately discarded. This kind of solution does not play well when there are fluctuations in short period of times. The connection pool could consider a connection as idle after some period of time - for example 60 seconds by default - without receiving traffic, this will allow the pool to evict unnecessary connections only after some period of time.

#### New metric for knowing how much connection have been opened

Today there is no such metric, for troubleshooting issues like this or for just undestanding how many connections are we creating at some point this metric will definitely help. Today this can be circumvented by using the one regarding the close connection, that can be an approx proxy metric.


[1] https://github.com/pfreixes/go/commit/506ae655a35d3ec27cd168c9e469c39f221b5379
[2] https://github.com/golang/go/blob/master/src/database/sql/sql.go#L1387
[3] https://pkg.go.dev/database/sql#DB.SetMaxIdleConns
[4] https://github.com/pfreixes/go/commit/506ae655a35d3ec27cd168c9e469c39f221b5379#diff-5a8509ddb343ed538034d07d446b921410102a666e7e9715f13137c0e8690dbaR779
[5] https://github.com/pfreixes/go/commit/506ae655a35d3ec27cd168c9e469c39f221b5379#diff-5a8509ddb343ed538034d07d446b921410102a666e7e9715f13137c0e8690dbaR1201