feat(postgres): take transaction state from the wire, not from parsing SQL
Follow-up to !18 (merged), and a replacement for its approach. Closes #1 (closed) properly.
The problem, reproduced first
A stock idle GlitchTip backend stack on glitchtip-rust 0.6.1 logs exactly 60 WARNING: there is no transaction in progress per minute — measured here before touching any code, matching the report in #1 (closed). The cause was an unconditional cleanup ROLLBACK on every connection release. !18 (merged) made that conditional by inspecting the first SQL keyword of each statement we send.
Why the keyword approach had to go
Review of that parser found false negatives — the direction that returns a connection with an open transaction block to the pool, where the next request (potentially another organisation's) inherits it:
ROLLBACK TRANSACTION TO SAVEPOINT sandCOMMIT WORK AND CHAIN— PG's grammar is{COMMIT|ROLLBACK|END|ABORT} [WORK|TRANSACTION] [AND [NO] CHAIN], and the optional noise word fell through to "block closed".COMMIT AND CHAIN/ROLLBACK AND CHAIN, which close a block and immediately open a fresh one.- Comment-led SQL (
-- setup\nBEGIN),ABORT, PL/pgSQL bodies, deliberately-unparsed batch scripts, 2PC. - Plus two that no parser can fix: a commit-cancellation race, and submission-order vs wire-order when the async backend has statements in flight.
Each of those is a guessing bug. So the guess is gone rather than patched.
What replaces it
psycopg never guesses: libpq caches the ReadyForQuery status byte, psycopg exposes it as Connection.info.transaction_status, and psycopg_pool's putconn switches on it. We now do the same.
vendor/tokio-postgres gains a second patch, txn-status (GT_PATCH.md entry + regenerated diff). One Arc<AtomicU8> per connection, created in connect_raw, written by the driver task as each request completes (codec::BackendMessages::ready_status), read through Client::transaction_status(). Captured in the driver task rather than the consumer on purpose — prepare() and abandoned streams never read past their last useful message, so a consumer-side capture goes stale exactly when it matters. Upstream has no equivalent API; the upstream ask is recorded and is far more natural than the buffer cap, since every pool built on tokio-postgres needs it.
gt-postgres gains TransactionStatus (psycopg's five states, same ordinals) and release_conn as psycopg_pool's _reset_connection:
| status | action |
|---|---|
| idle | recycle, no SQL at all |
| in transaction / failed transaction | ROLLBACK; discard if even that fails |
| indeterminate | discard — deadpool's recycler only checks is_closed() |
A pre_recycle hook applies the same rule at checkout, so paths that never call release_conn — notably the async backend's autocommit statements, which check a connection out and drop it inline — cannot hand on a poisoned connection either. One relaxed atomic load when the connection is clean.
Deleted: tx_transition, skip_leading_noise, note_statement, the in_tx flag, and their tests.
Python gains Connection.info.transaction_status and a TransactionStatus IntEnum matching psycopg.pq's ordinals.
Tests
Four live-postgres regression tests, each verified to fail when the behaviour it guards is disarmed:
| test | disarmed by |
|---|---|
| idle release sends no ROLLBACK | forcing the release to always roll back (pre-!18 behaviour) |
| an open block IS rolled back | forcing it to never roll back |
failed DECLARE leaves no stranded block |
same |
| a dropped pin with an open block is cleaned up | disarming the Drop guard |
The last is the path that actually fires in production — GC of an abandoned connection, an exception above the block, CancelledError — where the release path never runs. Positive controls throughout so none can pass vacuously (assert a pin was taken; assert the stranded idle in transaction (aborted) block really exists before release; assert the re-pin lands on the same backend), and a pytest.skip rather than a misleading failure when the server runs track_activities=off.
The wire harness learned simple-query ('Q') support and now scripts the status byte, so every branch is asserted directly: idle sends no SQL, T and E both roll back, a failed cleanup ROLLBACK detaches, and a connection dropped inline while inside a block is discarded at the next checkout rather than handed on.
Docs
AGENTS.md now records the session/pool contract: the backend decides transaction state (never SQL text again); session state is deliberately not reset between checkouts, as in psycopg_pool, and if that ever changes it must use deadpool's RecyclingMethod::Clean — never DISCARD ALL, which deallocates prepared statements the per-connection cache still hands out (26000). It also states that sync and async share one pool but not one session model, and async_base documents that departure where a reader will hit it.
The exposure table is corrected: the Postgres driver has been the backend's only supported engine since GlitchTip 6.2.2, not the opt-in it claimed to be — which is why this reached users.
Verification
./scripts/check.shgreen: fmt + clippy + tests for the extension and all five core crates. 89 unit + 22 wire tests ingt-postgres.- Full Python suite against live PostgreSQL 17: 240 passed, 15 skipped, 0 failed.
- End-to-end: the scenario script that produced one WARNING per release on
mainnow produces zero, withROLLBACKstill sent on every path that has a block open.
Follow-ups (tracked, not in this MR)
- Port these changes to django-vpg, which was extracted from v0.6.1 and still carries the unconditional
ROLLBACK. - Then have gt_rust depend on vpg and delete its own copy, sharing one tokio runtime via
vpg_core::set_runtime_provider. - Upstream
Client::transaction_status()to tokio-postgres.