PoolInner::acquire() does not try the idle queue after a transient connection failure

[abonander]

While thinking about potential sources of the infamous PoolTimedOut error, I realized that there's an interesting failure mode to acquire().

Once it decides to open a new connection, that's all it tries to do:

sqlx/sqlx-core/src/pool/inner.rs

Lines 283 to 284 in e1ac388

	// Attempt to connect...
	return self.connect(deadline, guard).await;

If a nonfatal connection error happens, it just continues in the backoff loop in connect() and never touches the idle queue again:

sqlx/sqlx-core/src/pool/inner.rs

Line 348 in e1ac388

Ok(Err(Error::Database(error))) if error.is_transient_in_connect_phase() => (),

It will continue to do this until the timeout if the transient error does not resolve itself.

Right now, only the Postgres driver overrides DatabaseError::is_transient_in_connect_phase(), but one of the error codes it considers transient is the "too many connections" error:

sqlx/sqlx-postgres/src/error.rs

Lines 192 to 195 in e1ac388

	// too_many_connections
	// This may be returned if we just un-gracefully closed a connection,
	// give the database a chance to notice it and clean it up.
	"53300",

This means that if the max_connections of the pool exceeds what is currently available on the server, tasks can get stuck in a loop trying to open new connections despite there being idle connections available, leading to surprising PoolTimedOut errors.

This is potentially the cause of some such issues being reported, although it's only likely to occur with the Postgres driver.

[abonander]

I forgot to mention how I think this should be solved.

The logic of acquire() should be changed to race between acquiring a connection from the idle queue and opening a new connection.

To avoid wasting connection attempts, the connection process should be spawned as a separate task so that if the acquire() call wins the race, the connection is still allowed to complete. If the return type of the task is crate::Result<PoolConnection> then it should automatically return to the pool if the task completes after its handle is dropped.

The acquire() logic should wait a short amount of time before spawning the connection task to avoid growing the pool too eagerly or stampeding the server. This could even be configurable (#2854).