[YoseptF]

Note: I jumped the gun and opened this before raising an issue. I have now raised TabularisDB/tabularis#1110 as CONTRIBUTING.md asks and am keeping this as a draft until it is discussed.

Adds an execute_read_only_query RPC that runs one query inside BEGIN READ ONLY … ROLLBACK, matching what the built-in driver already does for AI/MCP reads. Today plugin connections only get the host's SQL classifier, so a write it misses (a side-effecting function, a data-modifying CTE) reaches the database. With this the database rejects it.

The Tabularis side that calls it (and falls back to execute_query for plugins without it): TabularisDB/tabularis#1109

How to test

# PostgreSQL 16 on 127.0.0.1:54320 (the live-test defaults)
cargo build
P='{"host":"127.0.0.1","port":54320,"username":"postgres","password":"password","database":"testdb"}'
printf '%s\n' \
  "{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"execute_read_only_query\",\"params\":{\"params\":$P,\"query\":\"SHOW transaction_read_only\"}}" \
  "{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"execute_query\",\"params\":{\"params\":$P,\"query\":\"SHOW transaction_read_only\"}}" \
  "{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"execute_read_only_query\",\"params\":{\"params\":$P,\"query\":\"CREATE TABLE t (id int)\"}}" \
  | target/debug/postgresql-plugin

• On main: request 1 and 3 return -32601 (not implemented).
• On this branch: 1 returns on, 2 returns off, 3 fails with cannot execute CREATE TABLE in a read-only transaction.

Live tests: POSTGRES_PLUGIN_BIN=target/debug/postgresql-plugin cargo test --test live_db -- --test-threads=1 (two new execute_read_only_query_* tests).

[github-actions]

Version suggestion

Based on this PR's title (feat) and the prerelease:rc label:

Current	1.0.0-rc.8
Suggested next tag	v1.0.0-rc.9

This is informational only — no tag or release is created automatically yet.

[aesslinger]

Review: changes requested

Verified the gap this closes is real and unsolved, and the port is parity-faithful on the load-bearing points — BEGIN READ ONLY → run → always ROLLBACK, connection-close-on-ROLLBACK-failure via PgClient::take, search_path before BEGIN, no session pinning, -32603 error shape matching execute_query. One change is needed before merge.

---

🔴 Missing InFlight Drop guard — panic between BEGIN and ROLLBACK leaks the read-only txn to the pool

src/handlers/query.rs — exec_read_only_query

The builtin you're porting (tabularis drivers/postgres/mod.rs:1203) wraps the client in an InFlight(Option<Client>) Drop guard whose Drop calls Client::take (close) unless the success path explicitly .take()s it out first. So on any path that doesn't reach a successful ROLLBACK — early return, or panic/unwind between BEGIN and ROLLBACK — the connection is closed, not returned to the pool. The builtin's own comment states the contract: "the read-only transaction never leaks into another caller."

This PR has no such guard. It relies on the explicit drop(PgClient::take(pg_client)) in the if let Err arm only. A panic inside exec_query_on_client (between BEGIN-success and ROLLBACK) drops pg_client normally → returns the connection to the pool with the open read-only transaction. RecyclingMethod::Fast (client.rs:573) resets nothing — it only checks is_closed() — so the open BEGIN READ ONLY survives recycling. The next execute_query on that connection runs inside the leaked read-only txn: its writes fail with "cannot execute INSERT in a read-only transaction", its BEGIN warns about an existing transaction.

Reachability: not theoretical. Cargo.toml sets no panic = "abort" and there is no catch_unwind anywhere in src/, so the default unwind strategy applies — a panic unwinds through exec_read_only_query, skips ROLLBACK, and drops the client normally. Panic sources exist: extract.rs has 20 try_into().unwrap() sites plus an unreachable!() (line 1953) — all length-guarded for well-formed wire data, but the builtin's InFlight exists precisely to cover the malformed-bytes / upstream-library-bug class. This is the first function in the plugin that opens an explicit transaction without a Drop guard; exec_query has no transaction lifecycle to leak, so it's a genuinely new code path.

Severity: availability/ergonomics, not data-integrity — a read-only txn can't commit writes, and a leaked one self-blocks the next caller rather than corrupting data. But it's a real divergence from the safety net the builtin deliberately built, and CLAUDE.md's parity bar is explicit: "behavioral differences are regressions here."

Suggested fix (small, mirrors the builtin):

struct ReadOnlyGuard(Option<PgClient>);
impl Drop for ReadOnlyGuard {
    fn drop(&mut self) {
        if let Some(c) = self.0.take() { drop(PgClient::take(c)); }
    }
}

Acquire the client into the guard, borrow through it like the builtin does, and .take() it out only on the ROLLBACK-Ok path so it returns to the pool cleanly. On every other path (panic, ROLLBACK failure) the guard's Drop closes the connection. Note: the BEGIN-fails early-? path does not need the guard — if BEGIN fails no transaction opened, returning the connection to the pool is clean (arguably more correct than the builtin's defensive close there).

---

🟡 ROLLBACK-failure → PgClient::take cleanup path is untested

tests/live_db.rs

The most dangerous cleanup branch — ROLLBACK fails → drop(PgClient::take(pg_client)) detaches/closes the connection so the aborted txn doesn't leak — has zero coverage. Both new live tests run against a healthy local DB where ROLLBACK always succeeds, so they can't reach it. A future edit that breaks the take (e.g. "simplifies" it to a bare log::warn! + normal drop) would silently recycle aborted-txn connections into the pool and ship green.

Worth at least one test that proves the close path is intact, even if it has to inject a ROLLBACK failure indirectly (or a unit test on the guard once the InFlight-style fix above lands — the guard's Drop is the thing that needs proving).

Also a small test-quality note on test 2: the SHOW transaction_read_only == "off" assertion runs on a fresh pool connection, so it would pass even if ROLLBACK were removed entirely — it's vacuous w.r.t. ROLLBACK. The SELECT count(*) FROM read_only_probe == 0 assertion is the one that actually proves ROLLBACK worked (the row didn't persist). Worth a one-line comment so a future reader knows which assertion is load-bearing.

---

✅ What's solid (verified)

• The gap is real and unsolved. Plugin MCP reads today get only the host's keyword classifier; a classifier-missed write (mutating function, data-modifying CTE) reaches the DB. BEGIN READ ONLY rejects exactly those, server-side — confirmed airtight across DML, DDL, SELECT INTO, setval, data-modifying CTEs, and writes inside SECURITY DEFINER functions.
• -32601 fallback handshake is sound (with companion tabularis#1109). The plugin dispatches unmatched methods to not_implemented → -32601; the host's is_method_not_found matches remote.code == -32601; old plugins fall back to execute_query byte-identically to the pre-PR trait default.
• Multi-statement injection is impossible through the extended-protocol query_typed/execute_typed path (one statement per Parse); the BEGIN READ ONLY/ROLLBACK strings are constants.
• Request/response contract byte-for-byte identical to execute_query's non-session path across plugin, host RpcDriver, and README; session_id omission is deliberate and consistent.
• Backward-compat: purely additive across all four old/new upgrade combinations; no dispatch shadowing (Rust string-literal match).
• Edge cases (empty query, multi-statement, non-u64 request_id, limit=0/page=0) all benign — same as execute_query.

---

Companion + process notes

• tabularis#1109 is required and needs review — it's the host-side wiring that actually calls this method (with the -32601 fallback). Neither PR lights up alone; coordinate the release so a released host never calls a missing method on every MCP read.
• I should be clear that I have not run the same review pass on tabularis#1109 (the host side) — I read it and confirmed the -32601 handshake is sound, but the full ensemble wasn't applied there. It's possible some or all of these same concerns need addressing on that side too (the host's RpcDriver::execute_read_only_query override has its own connection/error paths). Happy to run the same checks on that PR if it's wanted.
• No prerelease:* label yet — CI requires one (Conventional Commits title feat: is fine).
• It's a draft pending tabularis#1110 discussion — your own note; don't merge until the issue is agreed.

---

Summary

One blocking change: add the InFlight-style Drop guard so a panic between BEGIN and ROLLBACK closes the connection instead of leaking the read-only transaction into the pool — that's the parity bar. One follow-up: cover the ROLLBACK-failure cleanup path. Everything else is solid; the port is faithful where it counts.

[aesslinger]

Thanks for this — genuinely nice work. The port is faithful where it counts (BEGIN READ ONLY → run → always ROLLBACK, connection-close on ROLLBACK failure, search_path before BEGIN, -32603 shape matching execute_query), the two live tests are well-constructed (the count(*) == 0 assertion genuinely proving the write didn't persist is a good touch), and the backward-compat story — purely additive, -32601 fallback keeping old plugins working — is exactly right. The gap this closes is real and worth doing, and the scope discipline (one method, reusing exec_query_on_client, no new abstraction) is appreciated. The change requested above is a small parity gap, not a rethink.