fix: don't hand clients a connection Postgres already closed - #1318
Conversation
Healthchecks only run every healthcheck_interval (30s by default), so a backend terminated by pg_terminate_backend or an admin restart stayed in the pool and was handed to clients unchecked until the interval elapsed. Adds a non-blocking liveness peek on every checkout. An idle pooled connection should have nothing waiting to be read, so pending bytes or a closed socket disqualify it: the connection is force-closed and the existing retry loop fetches another. Pending data is treated as fatal without parsing it. That also discards connections that merely received an asynchronous NoticeResponse, which costs a reconnect but never correctness, and keeps the check cheap enough to run on every checkout. Uses a distinct Error::ServerClosed rather than HealthcheckError so a dead connection does not toggle pool health. The check runs far more often than a healthcheck, and one terminated backend must not ban the pool or trigger failover. Closes pgdogdev#614
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Fantastic! Let me benchmark this locally to double check that this doesn't have a performance impact! |
|
Strangely I see absolutely no performance regression here..which is good of course, but I expected this to make an extra syscall on each checkout! I'm going to wait for GH actions to come back to life and probably merge this as-is. Great work! |
Makes sense, |
2 participants
Healthchecks only run every
healthcheck_interval(30s by default), so a backend terminated bypg_terminate_backendor anadminrestart stayed in the pool and was handed to clients unchecked until the interval elapsed.Adds a non-blocking liveness peek on every checkout. An idle pooled connection should have nothing waiting to be read, so pending bytes or a closed socket disqualify it: the connection is force-closed and the existing retry loop fetches another.
Pending data is treated as fatal without parsing it. That also discards connections that merely received an asynchronous
NoticeResponse, which costs a reconnect but never correctness, and keeps the check cheap enough to run on every checkout.Uses a distinct
Error::ServerClosedrather than HealthcheckError so a dead connection does not toggle pool health. The check runs far more often than a healthcheck, and one terminated backend must not ban the pool or trigger failover.Closes #614