feat: run_analytical_query — long-running reads on an isolated pool - #283
Conversation
run_select is bounded to a short (~30s) timeout on the shared 5-connection pool so an agent can't pin a connection with a runaway query — which made genuine analytical queries infeasible, and MCPG_STATEMENT_TIMEOUT_MS didn't help (it moves Postgres's statement_timeout but not the client-side asyncio cap in SafeSqlDriver). New tool run_analytical_query: a read-only SELECT validated through the SAME pglast allowlist + TenantSqlDriver (SET LOCAL ROLE / RLS) + read-only transaction as run_select, but executed on a DEDICATED connection pool (mcpg/analytical.py::AnalyticalRunner) sized to the concurrency cap and isolated from the main pool — so a slow query can never starve the fast-path tools. The pool size IS the concurrency cap; the per-call timeout_ms (clamped to the max) is the client-side budget, with the pool's statement_timeout as the PG ceiling. Reuses query.run_select / run_select_tuned verbatim, so validation, tenancy, and result shape are identical (verified: pg_sleep/writes rejected, 300ms budget times out a heavy query, clamp works). Boot-time knobs: MCPG_ENABLE_ANALYTICAL_QUERIES (default true; gates the READ tool — set false to withdraw it from an anonymous read-only deployment), MCPG_ANALYTICAL_TIMEOUT_MS (120000), MCPG_ANALYTICAL_MAX_TIMEOUT_MS (600000, clamps per-call), MCPG_ANALYTICAL_MAX_CONCURRENCY (2). A run_select timeout now points the agent at the tool (self-correcting fallback). Primary DB only for now. The analytical runner is built only when the main database isn't injected (production), so lifespan tests that mock the DB don't spin up a real second pool. Tool surface 253 -> 254; both contract snapshots + doc tables regenerated; counts + anchors updated; user-guide + CHANGELOG; new test_analytical.py (clamp, config validation, gating). Full unit+contract suite (2859) + mypy/ruff clean. Advances roadmap row: N/A — reactive capability from user analysis of the statement-timeout ceiling; not a planned roadmap item. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0122yLZLJ8t4W43sdN6BmTZc
There was a problem hiding this comment.
Code Review
This pull request introduces the run_analytical_query tool to execute long-running, read-only analytical queries on an isolated connection pool, preventing slow queries from starving fast-path tools. Feedback suggests broadening the exception check in run_select from 'timed out' to 'timeout' to correctly catch server-side PostgreSQL timeouts and trigger the fallback suggestion. Additionally, it is recommended to refine the error message when the analytical runner is uninitialized to distinguish between disabled queries and an injected mock database.
| # timeout and the analytical path is available, point the agent at | ||
| # it so it needn't predict duration up front. | ||
| settings = ctx.request_context.lifespan_context.settings | ||
| if settings.enable_analytical_queries and "timed out" in str(exc).lower(): |
There was a problem hiding this comment.
The check "timed out" in str(exc).lower() will not match server-side PostgreSQL timeouts, which typically raise an error containing "canceling statement due to statement timeout" (which does not contain the substring "timed out"). Changing this to check for "timeout" ensures that both client-side and server-side timeouts correctly trigger the self-correcting fallback suggestion to use run_analytical_query.
| if settings.enable_analytical_queries and "timed out" in str(exc).lower(): | |
| if settings.enable_analytical_queries and "timeout" in str(exc).lower(): |
| if runner is None: # defensive: tool only registered when enabled | ||
| raise query.QueryError("analytical queries are disabled (MCPG_ENABLE_ANALYTICAL_QUERIES=false)") |
There was a problem hiding this comment.
If analytical_runner is None because a custom or mock database was injected (e.g., during testing or in specific custom environments), raising an error claiming that MCPG_ENABLE_ANALYTICAL_QUERIES=false is misleading and can make debugging difficult. It is better to check the actual setting and raise a more precise error message.
| if runner is None: # defensive: tool only registered when enabled | |
| raise query.QueryError("analytical queries are disabled (MCPG_ENABLE_ANALYTICAL_QUERIES=false)") | |
| if runner is None: | |
| settings = ctx.request_context.lifespan_context.settings | |
| if not settings.enable_analytical_queries: | |
| raise query.QueryError("analytical queries are disabled (MCPG_ENABLE_ANALYTICAL_QUERIES=false)") | |
| raise query.QueryError("analytical runner is not initialized (possibly due to an injected database)") |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
run_selectfallback that checks"timed out" in str(exc).lower()is a bit brittle (and potentially locale-dependent); consider tagging timeout-originatedQueryErrors with a specific attribute/flag or subclass so you can branch on a structured signal instead of string matching. - With an injected
databaseincreate_server,enable_analytical_queries=Truewill still register therun_analytical_querytool but always run withanalytical_runner is Noneand error; if that embedding pattern is expected, it may be worth either wiring an injectableAnalyticalRunneror aligning tool registration with the same condition used when constructing the runner to avoid advertising a non-functional tool.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `run_select` fallback that checks `"timed out" in str(exc).lower()` is a bit brittle (and potentially locale-dependent); consider tagging timeout-originated `QueryError`s with a specific attribute/flag or subclass so you can branch on a structured signal instead of string matching.
- With an injected `database` in `create_server`, `enable_analytical_queries=True` will still register the `run_analytical_query` tool but always run with `analytical_runner is None` and error; if that embedding pattern is expected, it may be worth either wiring an injectable `AnalyticalRunner` or aligning tool registration with the same condition used when constructing the runner to avoid advertising a non-functional tool.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Follow-up hardening for run_analytical_query, addressing two review points on #283 (both bots flagged them independently): 1. Timeouts now raise a typed QueryTimeoutError (a QueryError subclass) detected structurally — the client-side asyncio wall-clock cap (via __cause__) and the server-side statement_timeout (SQLSTATE 57014). The run_select "retry with run_analytical_query" hint branches on the type instead of matching "timed out" in the message, so it now fires reliably and locale-independently for the Postgres-side timeout it previously missed. The hint is shown only when a runner is present. 2. AnalyticalRunner is injectable via create_server(analytical_runner=), and run_analytical_query is registered only when a runner will back it (register_tools gains analytical_available). An injected-database setup no longer advertises a non-functional tool. Docs: fold the analytical env vars into the README env-var table, the T3 (DoS) threat model in security.md, and the pool-overhead budget in scaling.md — spots the original feature PR missed. No tool-surface change (still 254). Adds structured-detection + registration-alignment unit tests; full unit + contract suite green (2866). Roadmap linkage: N/A — follow-up hardening on #283's reactive capability. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0122yLZLJ8t4W43sdN6BmTZc
Follow-up hardening for run_analytical_query, addressing two review points on #283 (both bots flagged them independently): 1. Timeouts now raise a typed QueryTimeoutError (a QueryError subclass) detected structurally — the client-side asyncio wall-clock cap (via __cause__) and the server-side statement_timeout (SQLSTATE 57014). The run_select "retry with run_analytical_query" hint branches on the type instead of matching "timed out" in the message, so it now fires reliably and locale-independently for the Postgres-side timeout it previously missed. The hint is shown only when a runner is present. 2. AnalyticalRunner is injectable via create_server(analytical_runner=), and run_analytical_query is registered only when a runner will back it (register_tools gains analytical_available). An injected-database setup no longer advertises a non-functional tool. Docs: fold the analytical env vars into the README env-var table, the T3 (DoS) threat model in security.md, and the pool-overhead budget in scaling.md — spots the original feature PR missed. No tool-surface change (still 254). Adds structured-detection + registration-alignment unit tests; full unit + contract suite green (2866). Also syncs uv.lock to pyproject's pglast==8.4 (dependabot #281 bumped the pin but the lock stayed at 8.2 on main). SQL-kernel adversarial + fuzz + safety suites pass unchanged under 8.4. Roadmap linkage: N/A — follow-up hardening on #283's reactive capability. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0122yLZLJ8t4W43sdN6BmTZc
Follow-up hardening for run_analytical_query (review points on #283): - Timeouts raise a typed QueryTimeoutError (QueryError subclass), detected structurally by walking the full __cause__/__context__ chain — covers both the client-side asyncio cap and the server-side statement_timeout (SQLSTATE 57014). The run_select "retry with run_analytical_query" hint branches on the type, shown only when a runner is present. - AnalyticalRunner injectable via create_server(analytical_runner=); tool registered only when a runner backs it. MCPG_ENABLE_ANALYTICAL_QUERIES is the authoritative off-switch (wins over an injected runner). - Docs: analytical env vars in README table, T3 (DoS) threat model, scaling pool-overhead budget. Syncs uv.lock to pyproject's pglast==8.4. No tool-surface change (still 254). Full unit + contract suite green (2870).
Summary
A follow-up to the timeout analysis:
run_selectis bounded to a short (~30 s) timeout on the shared 5-connection pool so an agent can't pin a connection with a runaway query — which made genuine analytical queries infeasible, andMCPG_STATEMENT_TIMEOUT_MSdidn't help (it moves Postgres'sstatement_timeoutbut not the client-side asyncio cap inSafeSqlDriver).New tool
run_analytical_query— a read-only SELECT for real analytical work (large aggregations, multi-table joins, window functions) that:pglastallowlist +TenantSqlDriver(SET LOCAL ROLE/ RLS) + read-only transaction asrun_select(reusesquery.run_select/run_select_tunedverbatim), andmcpg/analytical.py::AnalyticalRunner) isolated from the main pool and sized to the concurrency cap — so a slow query can never starve the fast-path tools.The isolated pool size is the concurrency cap; the per-call
timeout_ms(clamped to the max) is the client-side budget, with the pool'sstatement_timeoutas the Postgres ceiling. Optionalwork_mem. Arun_selecttimeout now points the agent at the tool (self-correcting fallback, so it needn't predict duration up front).Boot-time knobs (the "arguments at boot" you asked for)
MCPG_ENABLE_ANALYTICAL_QUERIESfalseto withdraw it from an anonymous read-only deployment)trueMCPG_ANALYTICAL_TIMEOUT_MS120000(2 min)MCPG_ANALYTICAL_MAX_TIMEOUT_MStimeout_msclamped to it600000(10 min)MCPG_ANALYTICAL_MAX_CONCURRENCY2Read-only only (write/DDL bulk paths deferred); primary database only for now.
Verification (against a live PostgreSQL)
generate_serieswithtimeout_ms=300timed out at 0.3 s ✓; clamp9e9 → 600 s✓;pg_sleep/CREATE TABLErejected by the reused allowlist ✓ (so tenancy/safety carry over).tools.md/architecture.mdregenerated; counts + anchors updated. Newtest_analytical.py(clamp, config validation, gating). Full unit + contract suite: 2859 passed;ruff/ruff format/mypy src/mcpgclean.Roadmap linkage
N/A — reactive capability from your analysis of the statement-timeout ceiling; not a planned roadmap item.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by Sourcery
Add a dedicated analytical query path and tool for long-running read-only SELECTs, backed by an isolated connection pool with configurable timeouts and concurrency, and wire it into the server lifecycle, configuration, and documentation.
New Features:
Enhancements:
Tests: