improve is_closed effectiveness #41
Conversation
|
To fix clippy: #42 |
|
Thanks for looking into it! |
There was a problem hiding this comment.
Between compute_ctl and postgres, a heartbeat-like message seems unnecessary. Can you merely check if the socket is still open? Within the same host, you shouldn't get into a situation where the client thinks that the connection is still open even though the server has already closed it.
| /// Check the connection is alive and wait for the confirmation. | ||
| pub async fn check_connection(&self) -> Result<(), Error> { | ||
| // sync is a very quick message to test the connection health. | ||
| query::sync(self.inner()).await | ||
| } |
There was a problem hiding this comment.
The Sync message affects the protocol state, it's not a simple "no-op" message. Per docs:
At completion of each series of extended-query messages, the frontend should issue a Sync message. This parameterless message causes the backend to close the current transaction if it's not inside a BEGIN/COMMIT transaction block (“close” meaning to commit if no error, or roll back if error). Then a ReadyForQuery response is issued. The purpose of Sync is to provide a resynchronization point for error recovery.
So we'll have to be careful to only call it when not in the middle of processing a query.
There was a problem hiding this comment.
That's true, fortunately the query encoding on the client is always atomic
It reproduces with my new test locally if I comment this line out https://github.com/neondatabase/neon/pull/11346/files#diff-31e2bc8fcc7da75eeda79b330b11c902f0031b77ba28df20874a45b71a53f541R121. I think that we currently do not reconnect at all if connection to postgres gets closed I was pretty sure that it's some |
@ololobus had some issues in compute_ctl with
is_closed()not detecting postgres connections being closed. There seemed to be two general problems. More details in sfackler#1229 but this is an early patch in before we can get it in upstream.