Connect to database lazily with configurable idle timeout

[tispratik]

Summary

Establish the connection pool on first tool use instead of at server startup, and let idle connections be reaped. This makes it practical to define many database MCP configs without every server holding an open Postgres connection from session start.

Motivation

When a user defines a large number of postgres-mcp servers (one per database), the previous behavior opened and held a live Postgres connection for every configured server as soon as the MCP client started — even for databases that are never queried in a session. That hogs connection slots on the database side.

Changes

• server.py: store the database URL without connecting eagerly. The lazy path already present in SqlDriver.execute_query opens the pool on first use, so the startup pool_connect call was redundant.
• sql/sql_driver.py: the pool now uses min_size=0 with a max_idle reaper, so an unused/idle server releases its connection(s) back to zero.
• Configurable idle timeout: new --max-idle flag and DATABASE_MAX_IDLE env var (default 300s). Invalid values (non-numeric, zero, negative) fall back to the default via a validating max_idle property — a single chokepoint covering the constructor, CLI flag, and env var.
• README: documents lazy connections and the idle timeout with config examples.

Behavior

• No DB connection is opened at server start; the first tool call that needs the database establishes it.
• A connection idle past max_idle is reaped and transparently re-established on the next query (the idle timer restarts per connection).
• The happy path (default 300s) is unchanged.

Testing

All existing unit tests pass (tests/unit/test_transport.py, test_access_mode.py, tests/unit/sql/), and the max_idle validation/fallback paths were verified manually.

🤖 Generated with Claude Code

[szmania]

Would really love to see this feature implemented. Getting database timeouts after about 15 minutes of idle time

[jssmith]

Review: Lazy database connections with configurable idle timeout

Verdict: Approve with minor comments.

Assessment

Good practical improvement — deferring pool creation to first tool use eliminates unnecessary connections when users configure many database servers. The max_idle reaper with min_size=0 means an idle server holds zero connections.

Strengths

1. Lazy connect is well-implemented. Removing the eager pool_connect from main() and relying on the existing lazy path in SqlDriver.execute_query is the minimal change.
2. max_idle validation is thorough. The property setter rejects None, zero, and negative values with a warning + fallback to default. Covers CLI flag, env var, and constructor.
3. README documentation includes both flag and env var examples with clear explanation of the tradeoff.

Comments

1. Removed obfuscate_password import from server.py. This import was used in the deleted error-logging block for the eager connect. Verify no other code paths in server.py reference obfuscate_password — if not, the removal is clean.

2. db_connection.connection_url = database_url is set directly without going through the constructor. This works because connection_url is a plain attribute, but it means the URL is stored on the pool object before any validation happens. Acceptable since validation occurs on first pool_connect.

3. The max_idle reaper + min_size=0 interaction with PR #195's asyncio.Lock should be considered if both PRs merge. PR #195 adds a _connect_lock to pool_connect; this PR changes min_size and adds max_idle. They touch the same code and will need a merge resolution, but no logical conflict.

4. No test for the lazy connect path itself — the PR description says "verified manually". A unit test confirming that main() does not call pool_connect would be valuable.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.

[jssmith]

Review — concise

Verdict: Approve with comments. Good feature with thoughtful defaults and validation. Minor concerns around error visibility and min_size=0 behavior.

Strengths

• Lazy connection: db_connection.connection_url = database_url instead of pool_connect() at startup. Server starts without holding a Postgres connection.
• max_idle property with validation — rejects None, zero, and negative values, falls back to DEFAULT_MAX_IDLE (300s).
• --max-idle CLI flag takes precedence over DATABASE_MAX_IDLE env var. Clear precedence chain.
• min_size=0 on AsyncConnectionPool means idle pool holds zero connections. Combined with lazy connect, a configured-but-unused server keeps zero Postgres connections.
• Non-numeric env var value logs a warning and falls back to default rather than crashing.
• README documentation with both CLI flag and env var examples.

Issues to address

1. Connection errors now silent until first tool call (non-blocking, by design)
The old code called pool_connect() at startup and logged connection errors with obfuscate_password. Now, errors only surface on the first tool call. This is intentional (lazy connect), but users who rely on startup logs to verify connectivity won't see errors. The logger.info("Database URL configured; connection will be established on first use (lazy)") message helps, but consider adding a note in the startup log that connection errors will appear on first use.

2. obfuscate_password import removed but still used elsewhere (non-blocking)
The diff removes from .sql import obfuscate_password from server.py. Verify no remaining references in the file. The old startup error handling used it; if no other code path in server.py uses it, the removal is correct.

3. min_size=0 may cause connection latency on first query (non-blocking)
With min_size=0, the first query must establish a new connection. For frequently-used databases, this adds latency on the first call after idle reaping. The README mentions this trade-off ("higher values keep connections warm"). Acceptable, but worth noting for production deployments.

4. No test for max_idle validation (non-blocking)
The property setter logic (rejecting invalid values) isn't tested. A simple unit test verifying that None, 0, -1 fall back to DEFAULT_MAX_IDLE would be valuable.

5. max_idle set before connection_url is validated (non-blocking)
db_connection.max_idle is set in the argument parsing section, before the database URL is checked. If the URL is missing, the max_idle setting is applied to a pool that will never connect. Harmless, but the ordering is slightly confusing.

This review was created by an AI agent (OpenHands) on behalf of @jssmith.