Skip to content

perf: turn pool_pre_ping off by default and rely on postgres_retry - #54

Merged
lesnik512 merged 2 commits into
mainfrom
chore/pre-ping-off
Oct 16, 2026
Merged

lesnik512 merged 2 commits into
mainfrom
chore/pre-ping-off

Conversation

@lesnik512

Copy link
Copy Markdown
Member

pool_pre_ping is now off by default, and postgres_retry handles a pooled connection that died while idle. This removes one round trip on every connection checkout. The setting is still DB_POOL_PRE_PING, so a deployment can turn pre-ping back on.

Why this is safe now

  • db-retry 0.7.1 retries a DBAPIError that SQLAlchemy marks connection_invalidated. That covers asyncpg's InterfaceError: connection is closed, which a dead idle connection raises on its first use.
  • db-retry 0.8.0 replaced tenacity with a plain loop. The decorator's success-path cost dropped from about 22 µs to 0.14 µs per call. Before that, the retry cost more than the pre-ping round trip it replaces. pyproject.toml now requires db-retry>=0.8.0, and the lock drops tenacity.

Fix: three use cases queried outside the Transaction

A retry only recovers if the retried function owns its whole transaction. create_chat, create_message and mark_read ran reads before async with self.transaction:

  • create_chat: fetch_direct_by_key for direct chats.
  • create_message: is_member and fetch_by_idempotency_key.
  • mark_read: fetch_member and get_one_or_none.

When one of those reads hit a lost connection, the session was left in an invalid transaction that nothing rolled back, so the retry failed with PendingRollbackError. This was already true with pre-ping on, for a connection lost mid-request. With pre-ping off it also happens on the first statement after an idle connection dies.

Those reads now run inside the block. An early return commits first, so __aexit__ only closes the session and the returned row stays loaded, as in the read use cases since #51. The direct-key validation in create_chat moved to _direct_key to keep __call__ under ruff's complexity limit.

Tests

tests/use_cases/test_connection_loss.py:

  • New lost_pooled_connection fixture: it returns a connection to the pool, then terminates its backend. This is the case pre-ping used to cover. The existing lost_connection fixture kills a connection the session already holds, which pre-ping never sees.
  • The read use cases and the membership-gated use cases, now including create_message and mark_read, run against both fixtures. A new test covers the create_chat direct-key lookup.
  • With DB_RETRY_RETRIES_NUMBER=1, the pooled read tests fail with InterfaceError: connection is closed, so the default retry is what makes them pass.
  • Before the fix, the create_chat, create_message and mark_read cases failed with PendingRollbackError.
  • The kill logic shared by both fixtures is in _watch_backend and _terminate_backend.

tests/test_settings.py checks the new default.

Lint and ty pass, and all 141 tests pass with 100% coverage.

Not done

I didn't rerun the benchmark against chat-app's endpoints. The gain comes from the db-retry measurements: a one-row session.get took 183 to 193 µs per request with pre-ping on, and 173 to 181 µs with pre-ping off plus postgres_retry and Transaction. The gap grows with network latency between the app and Postgres.

Requires db-retry 0.8.0, whose retry costs about 0.14 us per call, and adds a test that a connection terminated while idle in the pool is recovered by the retry.
…ide the Transaction

A lost connection on a read before the block left the session in an invalid transaction that nothing rolled back, so the retry failed with PendingRollbackError. With pool_pre_ping off, an idle connection that died in the pool hits this on the first statement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant