Allow "fast" recycling like deadpool_postgres to prevent millions of SELECT 1 queries?

[phiresky]

Currently, diesel-async does a "SELECT 1" query for every connection released back to the pool:

diesel_async/src/pooled_connection/deadpool.rs

Line 84 in b95650f

obj.ping().await.map_err(super::PoolError::QueryError)?;

diesel_async/src/pooled_connection/mod.rs

Lines 235 to 236 in b95650f

	/// The default implementation performs a `SELECT 1` query
	async fn ping(&mut self) -> diesel::QueryResult<()>

While benchmarking lemmy, when counting the queries run with pg_stat_statements, this "SELECT 1" query doubles the number of queries run in total (in 2 minutes, 2 million application queries are run and another 2 million "SELECT 1" queries. 48% of all queries run are the ping() calls. Now these queries have a mean execution time of 0.02ms, and I don't know how relevant their impact is, but still.

In deadpool_postgres, the recycling method is configurable: https://docs.rs/deadpool-postgres/latest/deadpool_postgres/enum.RecyclingMethod.html And apparently the plan is to move to "fast" recycling by default in the next version, which does not run any query.

Would it be possible to make the recycling method configurable? Or maybe even to use the "normal" deadpool_postgres connection manager? I don't understand exactly how diesel-async and deadpool interact, sorry.

[weiznich]

These queries are somewhat required to ensure that the connection is actually working. At least that's the only safe way I'm aware of to check whether the connection is actually usable. If someone knows a better way of handling that please comment here.

In deadpool_postgres, the recycling method is configurable: https://docs.rs/deadpool-postgres/latest/deadpool_postgres/enum.RecyclingMethod.html And apparently the plan is to move to "fast" recycling by default in the next version, which does not run any query.

As far as I understand that documentation RecyclingMethod::Fast does just skip the check and pretends that the connection work. I'm not sure if that's a great idea, as you then easily can end up with degraded database connections for whatever reason.

Would it be possible to make the recycling method configurable? Or maybe even to use the "normal" deadpool_postgres connection manager? I don't understand exactly how diesel-async and deadpool interact, sorry.

It is already somewhat configurable as you technically could just provide your own connection manager that implements deadpool::Manage::recycle differently or you could provide your own connection type that implements PoolableConnection differently. Both can be achieved by using the new type pattern.

That written: I might be open to accept a similar interface to that one exported by deadpool_postgres as long as the relevant PR contains documentation and tests for all variants (and support for all pooling solutions).

[phiresky]

Yeah, I understand that's the reason. But it sounds like the deadpool authors don't think it's necessary most of the time? The docs just say Fast: Unless you have special needs this is a safe choice. Maybe @bikeshedder can give some insight?

I only have experience with node-postgres and that doesn't do any query as liveness check either.

[bikeshedder]

In deadpool the Fast RecyclingMethod relies on Client.is_closed which is based on the connection state of the TCP/IP socket. This method can produce false positives in that a connection is reported as non-closed but the first query fails. There are a few cases where the connection is immediately closed and there are some where the connection is seen as "open" yet any communication using it will fail.

When using a unix domain socket the operating system guarantees that the connection is closed when the remote side goes away so relying on that is safe. (I might be wrong here, but I'm not aware of a way that a closed unix domain socket can linger around like a dead TCP/IP connection can.)

For TCP/IP it's a huge can of worms. There are various error cases where a TCP/IP connection is considered connected even though in reality it's long dead.

• When shutting down PostgreSQL (even killing the process) the operating system will usually ensure a FIN frame is sent thus terminating the connection cleanly. The client will get to know that the database server is no longer there and the connection is considered closed. If that FIN frame is lost - for what reason ever - the connection will not be closed and the client won't get to know that there is anything wrong with the server.
• In the case of a server crash, a network cable is unplugged, some intermediate router dies, etc. that FIN frame will never be sent. Therefore the client might assume that the connection is still alive. The next TCP/IP frame sent from the client to the server will either result in an RST frame from the server, it it's reachable, or some intermediate router might answer with an appropriate ICMP packet or some other means of letting the client know that the TCP/IP frame could not be delivered.

That's the entire reason why so many network protocols incorporate a periodic ping or heartbeat. That's the only reliable way to know if a connection is still usable or not. Also it's often necessary if there are NAT gateways in between you and the server so the connection entry in the NAT table doesn't get removed because of inactivity.

---

It really depends on your network if you want to perform proper connection validation or not.

My reasoning behind switching to the Fast recycling method is the following: There is no guarantee that the server didn't close the connection between the connection validation and your queries. So if you really want to be 100% safe you need to handle this kind of error regardless of the recycling method. For a network service it can be a huge performance hit if the underlying servers and network virtually never causes this kind of situation. Therefore Fast has become the new default.

btw. I did change the default to RecyclingMethod::Fast a long time ago. That comment is indeed misleading and an oversight by me. I just fixed the documentation: deadpool-rs/deadpool@153ce83

I haven't received any tickets about that decision ,yet. Either nobody was really affected by it or the few affected users did adapt and specify the recycling method in the config. I can only guess that everything is fine. 🙈

[phiresky]

@weiznich I just tried implementing my own PoolableConnection like this:

pub struct MyAsyncPgConnection(AsyncPgConnection);
impl Deref for MyAsyncPgConnection {
  type Target = AsyncPgConnection;
  fn deref(&self) -> &Self::Target {
    &self.0
  }
}
impl DerefMut for MyAsyncPgConnection {
  fn deref_mut(&mut self) -> &mut Self::Target {
    &mut self.0
  }
}
#[async_trait::async_trait]
impl PoolableConnection for MyAsyncPgConnection {
  async fn ping(&mut self) -> diesel::QueryResult<()>
  where
    for<'a> Self: 'a,
  {
    // NO-OP for perf
    // todo: check self.conn.is_closed() but conn is private
    Ok(())
  }
}

It works at compile time but at runtime it fails with "Cannot directly establish a pooled connection". The reason is that the DerefMut<> impl for AsyncConnection doesn't delegate the establish() function for some reason?

[phiresky]

I have created a benchmark to compare the performance with ping() a noop by monkey-patching diesel-async: https://github.com/phiresky/lemmy-perf-data/blob/main/comparisons/2023-07-04-pool-liveness-check.md

lemmy finishes processing events around 10% faster with the check removed. For some reason the total time reported by pg_stat_statements is higher, I don't know why.

[weiznich]

It works at compile time but at runtime it fails with "Cannot directly establish a pooled connection". The reason is that the DerefMut<> impl for AsyncConnection doesn't delegate the establish() function for some reason?

I think the only way to get it working via that approach is by implementing AsyncConnection for your new type wrapper. Just using DerefMut likely does not work there.