bench: benchmark-suite plan + Phase 1 perf harness foundation (19)

[devopam]

Summary

Kicks off the public, evidence-driven benchmark suite (roadmap 19). Two parts:

1. The plan — docs/plans/benchmark-suite.md (+ roadmap §19). The thesis, the non-negotiable framing rule (we never claim MCPg out-executes Postgres — the perf tier exists to disarm that objection and prove the overhead is sub-ms, which earns credibility for the real token win in v2), the methodology, TPC-H scale, the break-even model, the honest-caveats table, and the locked v1 decisions (v1 = performance + static HTML dashboard; SF10 ceiling).

2. Phase 1 foundation — the performance harness, designed by the Opus architect grounded in MCPg's real APIs:

• benchmarks/perf/paths.py — NativeRunner (a persistent psycopg connection replicating MCPg's exact BEGIN READ ONLY … ROLLBACK envelope, minus all MCPg overhead — the fair floor, not psql -c) and ServerSideRunner (the real in-process run_select over the real SafeSqlDriver + pool, keeping the genuine per-call allocation + serialization).
• benchmarks/perf/queries.py — the two-axis query set (compute × result-size), heavy tier from TPC-H (Q1/Q3/Q5/Q6).
• benchmarks/perf/stats.py — percentiles + seeded bootstrap median CI + warm-up drop (pure, unit-tested).
• benchmarks/perf/schema.py — structured JSON result schema (provenance passed in).
• benchmarks/perf/runner.py — orchestrates paths × queries (cold + warm) → JSON, gc-disabled around timed regions.
• benchmarks/datasets/ — TPC-H schema/index DDL + a DuckDB→COPY loader (SF1/SF10; data regenerated locally, never committed).
• pyproject.toml — dev-only bench group (duckdb); pytest pythonpath += "." so the benchmark package is importable in its tests.

Next phases (own PRs): the end-to-end transport paths, the overhead-decomposition waterfall (the t_db == native gate), the concurrency sweep, and the self-contained HTML dashboard — then the v1 writeup.

Verification: 8 new unit tests pass; full suite 2813 passed; ruff check ., ruff format --check ., mypy src/mcpg all clean. The DB-touching harness is an operator tool (runs against a live PostgreSQL), not unit-tested — matching the existing benchmarks/bench.py.

Roadmap linkage

Advances roadmap row: 19.1 (and lands the §19 plan).

Checklist

• Tests added/updated first (TDD); suite passes locally
• ruff, ruff format, and mypy src/mcpg pass
• CHANGELOG.md — n/a (no shipped-surface change; benchmarks are an operator tool)
• Roadmap row cited above (19.1)
• No hand-edits to src/mcpg/_vendor/ (the vendor tree no longer exists — 18.1)

🤖 Generated with Claude Code

https://claude.ai/code/session_0122yLZLJ8t4W43sdN6BmTZc

---

Generated by Claude Code

Summary by Sourcery

Introduce a reproducible performance benchmarking harness and documented plan for the MCPg benchmark suite, including initial native vs server-side latency measurements on TPC-H workloads.

Enhancements:

• Add a structured performance benchmarking harness with native and server-side runners, query taxonomy, statistics aggregation, and JSON result schema under a new benchmarks package.
• Provide scripts and SQL to generate and load a TPC-H dataset into PostgreSQL for local benchmark runs, without committing data files.

Build:

• Define a dev-only bench dependency group (DuckDB) and adjust pytest pythonpath so the benchmarks package is importable in its tests.

Documentation:

• Document the benchmark-suite roadmap and methodology, including performance and token-efficiency objectives, datasets, and phasing in a new benchmark plan and feature-shortlist entry.

Tests:

• Add unit tests covering benchmark statistics, query coverage across compute and result-size axes, and serialization of performance run results.

[gemini-code-assist-2]

Code Review

This pull request introduces Phase 1 of the MCPg benchmark suite, which includes a reproducible performance harness comparing native PostgreSQL execution against MCPg server-side paths using TPC-H datasets, statistical aggregation utilities, and associated unit tests. The review feedback highlights two critical robustness improvements: ensuring a ROLLBACK is always executed in the native runner's transaction envelope using a try...finally block to prevent aborted transaction states on persistent connections, and wrapping database and runner initializations in nested try...finally blocks to prevent connection pool leaks in case of setup failures.

[gemini-code-assist-2]

Since the connection is created with autocommit=True and is persistent across multiple benchmark iterations, manually executing BEGIN TRANSACTION READ ONLY without a try...finally block to guarantee ROLLBACK is highly risky. If cur.execute(sql) or cur.fetchall() raises an exception, the ROLLBACK statement will be skipped, leaving the persistent connection in an aborted transaction state. This will cause all subsequent queries in the benchmark run to fail with InFailedSqlTransaction errors. Wrap the execution in a try...finally block to ensure ROLLBACK is always executed.

Suggested change

	async def run_once(self, sql: str, *, max_rows: int) -> int:
	conn = self._conn
	start = time.perf_counter_ns()
	async with conn.cursor(row_factory=dict_row) as cur:
	await cur.execute("BEGIN TRANSACTION READ ONLY")
	await cur.execute(sql)
	rows = await cur.fetchall()
	# Match run_select's max_rows truncation + dict materialization so
	# the serialization work compared is like-for-like.
	_ = [dict(r) for r in rows[:max_rows]]
	await cur.execute("ROLLBACK")
	return time.perf_counter_ns() - start
	async def run_once(self, sql: str, *, max_rows: int) -> int:
	conn = self._conn
	start = time.perf_counter_ns()
	async with conn.cursor(row_factory=dict_row) as cur:
	await cur.execute("BEGIN TRANSACTION READ ONLY")
	try:
	await cur.execute(sql)
	rows = await cur.fetchall()
	# Match run_select's max_rows truncation + dict materialization so
	# the serialization work compared is like-for-like.
	_ = [dict(r) for r in rows[:max_rows]]
	finally:
	await cur.execute("ROLLBACK")
	return time.perf_counter_ns() - start

[gemini-code-assist-2]

If NativeRunner.connect raises an exception, the Database connection pool (which has already successfully connected) will never be closed, leaking active database connections. To prevent resource leaks, wrap the runner initialization and benchmark execution in nested try...finally blocks to guarantee that both the database pool and the native connection are always closed.

    database = Database(settings)
    await database.connect()
    try:
        native = await NativeRunner.connect(args.database_url)
        try:
            results: list[ResultRow] = []
            pg = await database.driver().execute_query(
                "SELECT current_setting('server_version') AS v, current_setting('server_version_num')::int AS num",
                force_readonly=True,
            )
            pg_meta = {"version_string": pg[0].cells["v"], "server_version_num": pg[0].cells["num"]} if pg else {}

            for query in all_queries():
                for label, runner in (("native", native), ("server_side", ServerSideRunner(database))):
                    warm, cold = await _sample_path(runner, query, args.iterations)
                    results.append(_row(label, query, "warm", warm))
                    results.append(_row(label, query, "cold", [cold]))
        finally:
            await native.close()
    finally:
        await database.close()

[sourcery-ai]

Hey - I've found 3 security issues, 3 other issues, and left some high level feedback:

Security issues:

• Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
• Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
• Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)

General comments:

• In bootstrap_median_ci you resample 10,000 times on every summarize call; given this runs for each (path, query, temperature) combination, consider making the resample count configurable or lowering the default to avoid the CI computation dominating runtime for larger suites.
• In _sample_path you disable GC around warm iterations but not for the initial cold measurement; if you care about isolating cold-path DB effects from GC pauses, consider applying the same GC disable/enable pattern to the cold sample or explicitly documenting that GC noise is part of the cold path.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `bootstrap_median_ci` you resample 10,000 times on every summarize call; given this runs for each (path, query, temperature) combination, consider making the resample count configurable or lowering the default to avoid the CI computation dominating runtime for larger suites.
- In `_sample_path` you disable GC around warm iterations but not for the initial cold measurement; if you care about isolating cold-path DB effects from GC pauses, consider applying the same GC disable/enable pattern to the cold sample or explicitly documenting that GC noise is part of the cold path.

## Individual Comments

### Comment 1
<location path="benchmarks/perf/runner.py" line_range="87-55" />
<code_context>
+    )
+    database = Database(settings)
+    await database.connect()
+    native = await NativeRunner.connect(args.database_url)
+    results: list[ResultRow] = []
+    try:
+        pg = await database.driver().execute_query(
+            "SELECT current_setting('server_version') AS v, current_setting('server_version_num')::int AS num",
+            force_readonly=True,
+        )
+        pg_meta = {"version_string": pg[0].cells["v"], "server_version_num": pg[0].cells["num"]} if pg else {}
+
+        for query in all_queries():
+            for label, runner in (("native", native), ("server_side", ServerSideRunner(database))):
+                warm, cold = await _sample_path(runner, query, args.iterations)
+                results.append(_row(label, query, "warm", warm))
+                results.append(_row(label, query, "cold", [cold]))
+    finally:
+        await native.close()
+        await database.close()
</code_context>
<issue_to_address>
**issue (bug_risk):** Initialize `pg_meta` before the `try` block to avoid masking errors with `UnboundLocalError` if the metadata query fails.

Because `pg` is fetched inside the `try`, any exception before `pg_meta` is assigned will still run the `finally` and then continue to construct `metadata`. At that point `pg_meta` is undefined and raises `UnboundLocalError`, obscuring the original exception. Pre-initializing `pg_meta: dict[str, Any] = {}` before the `try:` prevents this masking while keeping later access safe.
</issue_to_address>

### Comment 2
<location path="benchmarks/perf/runner.py" line_range="153" />
<code_context>
+def main(argv: list[str] | None = None) -> int:
+    parser = argparse.ArgumentParser(description="MCPg performance benchmark (native vs server-side).")
+    parser.add_argument("--database-url", required=True, help="PostgreSQL DSN (a TPC-H-loaded database).")
+    parser.add_argument("--iterations", type=int, default=50, help="Warm iterations per pathxquery (>= 20).")
+    parser.add_argument("--scale-factor", type=int, default=1, help="TPC-H scale factor the DB was loaded at.")
+    parser.add_argument("--output", type=Path, required=True, help="Path to write the result JSON.")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Validate a minimum `--iterations` value so warm measurements are non-empty and match the CLI contract.

The help text promises `>= 20`, but the code doesn’t enforce any minimum. With `_WARMUP = 5`, values like `--iterations 0` or other small values yield an empty warm array, so `summarize()` returns all-zero stats, which is misleading but looks valid. Consider validating `iterations >= _WARMUP + 1` (or at least `>= 1`) at argument parsing or at the start of `_run`, and failing fast with a clear error message.

Suggested implementation:

```python
    parser.add_argument(
        "--iterations",
        type=int,
        default=50,
        help=f"Warm iterations per pathxquery (>= {_WARMUP + 1}).",
    )

```

```python
    args = parser.parse_args(argv)

    if args.iterations < _WARMUP + 1:
        parser.error(f"--iterations must be >= {_WARMUP + 1} (warm measurements would be empty).")

    run = asyncio.run(_run(args))

```
</issue_to_address>

### Comment 3
<location path="docs/plans/benchmark-suite.md" line_range="19" />
<code_context>
+
+| Clause | Evidence |
+|---|---|
+| *dramatically more token-efficient* | 40–70 %+ fewer tokens per database task vs a bare `run_select` tool (the headline) |
+| *more reliable* | fewer error→retry loops, higher task-completion rate |
+| *negligible performance cost* | sub-millisecond, decomposed overhead; ~0 % on heavy queries; cache **beats** native on repeat reads |
</code_context>
<issue_to_address>
**issue (typo):** The `40–70 %+` sequence looks like a typo in the percentage notation.

Please rephrase `40–70 %+` to something like `40–70%+` or `40–70%` so the percentage notation is unambiguous.

```suggestion
| *dramatically more token-efficient* | 40–70% fewer tokens per database task vs a bare `run_select` tool (the headline) |
```
</issue_to_address>

### Comment 4
<location path="benchmarks/datasets/load_tpch.py" line_range="51" />
<code_context>
            duck.execute(f"CALL dbgen(sf={scale_factor})")
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

*Source: opengrep*
</issue_to_address>

### Comment 5
<location path="benchmarks/datasets/load_tpch.py" line_range="54" />
<code_context>
                duck.execute(f"COPY {table} TO '{csv_path}' (FORMAT csv, HEADER false)")
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

*Source: opengrep*
</issue_to_address>

### Comment 6
<location path="benchmarks/datasets/load_tpch.py" line_range="68" />
<code_context>
            await conn.execute(f"VACUUM (ANALYZE) {table}")
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

*Source: opengrep*
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

Source: opengrep

[sourcery-ai]

security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

Source: opengrep

[sourcery-ai]

security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.

Source: opengrep