Repository navigation
fix: pin backend for transaction-pooler result queries on non-builtin column types (#160, #163) - #164
Merged
Merged
Conversation
, #163) rc.9 (#149) switched user queries to the unnamed-statement protocol (`query_typed`: Parse("")+Bind+Describe+Execute+Sync in one batch). On the non-empty-rows path, tokio-postgres 0.7.18 resolves each result column's type *inside* the response-drain loop (src/query.rs:102, `get_type`), and for a non-builtin OID (custom enum/composite/domain/range/extension type) that's a cache miss → an interleaved named `typeinfo_statement` round-trip (src/prepare.rs:144-146) fired mid-stream. A transaction-mode pooler (Supavisor/PgBouncer) doesn't expect an out-of-order named statement inside a draining unnamed batch and deadlocks — the query hangs until the host's 120s RPC timeout (`Plugin call 'execute_query' timed out after 120s`). rc.8's named path resolved all column types during a separate pre-Execute `prepare` Describe, in a clean window before any Bind/Execute — so it never interleaved and never hung. Tables with only built-in column types never hit the cache miss (`get_type` returns from `Type::from_oid`), which is why only some tables hang (#163: one table hangs while another works on the same connection). Fix: run a best-effort `prepare(&final_query)` before the unnamed `query_typed` on the non-empty-rows path. The prepare's Parse+Describe+Sync resolves every result-column type via `typeinfo_statement` *before* the data fetch drains the stream, restoring rc.8's clean pre-Execute window. A prepare failure (26000 on a bare transaction pooler with max_prepared_statements=0 — the original #1097 orphan) is swallowed at debug and the unnamed `query_typed` still runs: it succeeds for built-in columns, or surfaces the 26000 that #158's `format_query_error` rewrites to actionable guidance. Worst case on the worst-configured pooler is a clear error instead of a hang — strictly better than today. Nothing is reverted: #149's unnamed data fetch stays (pooler-safe per #1097), #158's prepare()-skip on empty result sets and 26000/42P05 error rewrite stay, and #158's pooler_mode plumbing stays (the pre-resolve is unconditional, so it helps users on hosts without pooler_mode too — #160's reporter on 0.28.0, #163 on 0.29.0). Adds a live regression test (client_live_tests.rs): creates a custom enum type + table and runs SELECT * through the full execute_query path, asserting rows return within 30s. The test validates the fix on a direct connection; the bug itself needs a transaction-mode pooler fixture (not in this repo's CI), documented as manual verification in the PR. Co-authored-by: Adam J. Esslinger <14128427+aesslinger@users.noreply.github.com>
aesslinger
added
the
prerelease:rc
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
Bump rustls-webpki 0.103.15 -> 0.103.16 (RUSTSEC-2026-0382) and webpki-root-certs 1.0.9 -> 1.0.10 (RUSTSEC-2026-0383). Both advisories are CVE-2026-111943 (emSign intermediate distrust). Lockfile-only update; the current rustls 0.23.46 and rustls-platform-verifier 0.7.1 floors already accept the patched versions, so no Cargo.toml change. cargo audit now reports zero vulnerabilities (only the pre-existing chacha20 yanked-crate warning remains). Build and 417 unit tests pass.
Replace the pre-warm-prepare approach with a pinned-transaction path for result-set queries on connections marked pooler_mode=transaction. The prepare-warming added another named statement and still collided on PgBouncer (42P05 s1, same as rc.10's s0); it did not solve the root cause. Root cause is two orthogonal pooler bugs: - Problem A (#1097): named user statements orphaned across the prepare/Bind-Execute Sync gap. #149's unnamed query_typed fixed it and stays the data-fetch protocol. - Problem B (#160/#163): tokio-postgres's internal cached named typeinfo_statement, fired for non-builtin result column OIDs, collides with stale server-side statements on reassigned pooler backends, or hangs on Supavisor. The pinned path fixes B without forking tokio-postgres: - one-shot client from a transient pool (never cached/returned), so its private typeinfo cache dies with the connection after each call - BEGIN pins the backend; DEALLOCATE ALL clears stale statements - prepare resolves column types while pinned, before the data fetch - unnamed query_typed runs the data fetch (#149 preserved) - DEALLOCATE ALL + COMMIT end the pin; the one-shot client closes - bounded by a 90s timeout so a Supavisor stall is an actionable error, not the host's opaque 120s timeout format_query_error now gives the max_prepared_statements + pooler_mode guidance even when pooler_mode is unset (conditional phrasing), so users on older hosts that can't send the field still get the hint. Verified locally against pg-tabularis-test + PgBouncer transaction mode: - direct Postgres: enum + builtin tables pass - PgBouncer txn, max_prepared_statements=0, pooler_mode=transaction: enum table 10/10 clean (rc.10 42P05s on the 2nd call) - PgBouncer txn, max_prepared_statements=100: passes with and without pooler_mode (pooler-side PS tracking fixes the old path too) Co-Authored-By: Claude <noreply@anthropic.com>
aesslinger
changed the title
…#163) format_query_error's new is_transaction_pooler=false branch (conditional phrasing) had zero unit-test coverage — a grep for format_query_error across all test files returned no hits. Extract the pure phrasing selection into pooler_error_lead(bool) so the false branch — the PR's new behavior — is unit-testable without a live tokio_postgres::Error (whose DbError/SQLSTATE can't be constructed without a real server response). The SQLSTATE-gated assembly stays covered by the live client_live_tests error-message tests. Also trim pooler_mode before the transaction comparison, matching how startup_script is trimmed in build_pool: a hand-edited connections.json with a trailing space ("transaction ") would otherwise silently bypass the pinned path and the user would hit the #160/#163 hang on the shared cached client (the very thing this PR fixes) with no diagnostic pointing at the whitespace. Not reachable through the host's dropdown UI, but free and consistent with the sibling field's treatment.
…ft (#160, #163) apply_search_path_local inlined the same snapshot-read/fallback/SET construction as apply_search_path, differing only in SET LOCAL vs SET, instead of reusing the existing build_search_path_sql — the two had already diverged (apply_search_path calls build_search_path_sql; apply_search_path_local inlined format!). Parameterize build_search_path_sql with a `local: bool` so both call it, restoring a single source of truth for the search_path construction. Add 5 build_search_path_sql_local_* unit tests mirroring the existing SET cases so the SET LOCAL path's pure logic is covered without a live DB (the live tests omit the schema param, so apply_search_path_local was previously untested). Also fix three doc comments that drifted from the code: - exec_query_pooled_txn step 3 named prepare(&final_query) but the code calls prepare(&query) (the original user query, before pagination); LIMIT/OFFSET doesn't change result column types so the warmed cache still applies. - pin_pooled_txn said "every error path after BEGIN best-effort rolls back" but the COMMIT-failure path doesn't (PostgreSQL auto-rollbacks on COMMIT failure; the one-shot client drop covers it regardless). - oneshot_pool_client said "the caller drops both the client and the pool" but the pool local is dropped at the end of the function, not by the caller (only the Object is returned).
#160, #163) Add pooled_txn_ddl_stays_off_the_pinned_path live test: a non-result-set statement (CREATE TABLE) through execute_query with pooler_mode=transaction must stay on the shared cached client, NOT the pinned one-shot path. The guard is is_transaction_pooler() && returns_result_set(query); if the returns_result_set half were removed, this DDL would route to exec_query_pooled_txn -> prepare("CREATE TABLE") and fail (DDL has no prepared-statement form), so a successful CREATE proves the guard's FALSE side holds. Previously the only test setting pooler_mode=transaction sent a SELECT, exercising only the TRUE branch. Also correct the misleading comment in pooled_txn_enum_column_returns_rows: its 2-run loop claimed to "prove the one-shot client discard works," but on direct Postgres reusing the client also succeeds (the stale typeinfo_statement collision is pooler-only — it needs a backend reassignment between the prepare and a later reuse). The comment now states the 2-run loop proves the pinned path is repeatable on direct PG (no leaked BEGIN/uncleared DEALLOCATE/stale handle breaks run 2), and explicitly notes it does NOT prove the discard property — that is covered by the manual PgBouncer matrix in the PR body.
, #163) oneshot_pool_client called build_pool per query (never cached), so a transaction-pooler result-set query with a startup script paid for the preflight (a throwaway connection that runs the script in a rolled-back txn, purely to validate syntax) PLUS the real connection — 2 connections and 2 script executions per query, vs once-amortized on the cached path. The preflight is redundant on the one-shot path: the post_create hook already runs the script on the real connection and surfaces the same error shape (format_pool_error unwrapping PostCreateHook(Backend)), so a bad script fails cleanly either way. Split build_pool into build_pool (cached path, preflight on) and build_oneshot_pool (one-shot path, preflight off) via a shared build_pool_impl(skip_preflight). A bad startup script now fails at pool.get() via the hook with the same message — one connection per query instead of two, no behavior change on the failure path. Bounded by STARTUP_SCRIPT_TIMEOUT_MS (30s) either way, so this only removes a redundant connection, not a safety bound. Only affects the pooler_mode=transaction AND startup_script intersection; a transaction- pooler connection without a startup script already paid just 1 connection.
aesslinger
marked this pull request as ready for review
October 22, 2026 15:49
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
This was referenced Oct 23, 2026
aesslinger
added a commit
that referenced
this pull request
Oct 23, 2026
…e hang (#166) (#168) * fix: pre-resolve column types on direct connections to stop enum-table hang (#166) rc.11 (#164) only pre-resolved result column types when Pooler Mode = "Transaction", so direct (non-pooler) connections still hung on a table with a non-builtin column type (custom enum, composite, extension type) once the result was larger than a few KB. The same tokio-postgres interleave that #164 fixed for poolers deadlocks on any connection: query_typed resolves non-builtin column types (get_type → the internal named typeinfo_statement) inside its RowDescription loop, mid-stream over the same channel delivering the query's DataRows. The typeinfo reply is queued behind the pending rows, but query_typed won't drain the rows until get_type returns → deadlock once the row data fills the bounded responses channel + socket buffer (the size threshold — small results pass, large ones hang). This is not pooler-specific; the pooler additionally orphans the statement across backend reassignment (42P05 on PgBouncer), but the hang mechanism is the interleave, not the reassignment. Fix: pre-resolve result column types via prepare(query) on the shared cached client before the query_typed data fetch, for direct connections only (gated on !is_transaction_pooler in exec_query_on_client, which covers single, batch, and explain queries — the batch path calls exec_query_on_client directly, bypassing exec_query). prepare runs Parse+Describe+Sync with no Execute/no DataRows, so its get_type interleave has an empty channel and completes cleanly, warming CachedTypeInfo.types on the shared InnerClient. The subsequent query_typed then hits that cache (get_type's client.type_(oid) returns early) → no typeinfo_statement mid-drain → no deadlock. This is exactly rc.8's behavior, which worked. The pooler path (#164's exec_query_pooled_txn pinned one-shot transaction) is untouched — a plain prepare would collide across backend reassignment (the s0→s1 shift that killed the removed pre-warm, ca33b98), so poolers keep the pinned path. Direct connections have no reassignment hazard, so the plain prepare is safe and cheaper (preserves pool reuse; measured ~39ms/call vs ~37ms baseline, zero overhead on builtin types). The pre-resolve is non-fatal: a prepare failure is swallowed (.ok()) and query_typed runs anyway — for builtin types get_type is always a Type::from_oid cache hit (no hang), and a direct prepare failure on a valid result-set query is rare. The prepared Statement is kept alive so its warmed cache persists into query_typed and its .columns() is reused by the empty-rows branch (eliminating the double-prepare that the pre-#166 empty-rows column-discovery would otherwise incur). Also corrects the #164 framing in exec_query's routing comment: Problem B is the typeinfo_statement interleave during Row deserialization (deadlocks on any connection once the response is large enough), not just pooler reassignment — the pooler adds the reassignment orphan on top, but the hang mechanism is the interleave. Adds a live regression test (wide_enum_table_returns_rows_on_direct_ connection): 600 rows × 500-byte pad + an enum column, fetched at limit 50 (≈25 KB, enough to fill the channel buffer) on a direct connection, asserting it returns within 30s. rc.11 hangs past 120s on this; the existing small-enum test uses a single tiny row and passes on the broken path, which is the coverage gap that let #166 ship. Verified locally against pg-tabularis-test (direct, no pooler): - wide enum table, limit 50: returns 50 rows in 0.34s (rc.11 hung) - wide enum table, limit 500: returns 500 rows (rc.11 hung) - plain text table, limit 500: returns 500 rows (no regression) - SHOW search_path: returns 1 row (no regression) - pooler path (pooler_mode=transaction), enum limit 500: returns 500 rows (no regression) * fix: bump ctutils 0.4.2 -> 0.4.5 to clear RUSTSEC-2026-0385 A new advisory published 2026-10-23 (RUSTSEC-2026-0385) flags ctutils 0.4.2 for non-constant-time mask generation and u128 comparisons (a crypto-failure / timing-side-channel category). ctutils is a transitive dep (pulled via the RustCrypto utils chain through rustls/ring/aws-lc-rs), not a direct dependency, so `cargo update -p ctutils` bumps it within the same minor to 0.4.5 (satisfies the >=0.4.4 patch). cargo audit now reports zero vulnerabilities (only the pre-existing chacha20 yanked-crate warning remains, as on rc.10/rc.11). Build and 425 unit tests pass. This is independent of the #166 fix in this PR — the advisory dropped after rc.11's audit passed yesterday. * docs: fix pre-resolve comment imprecision (Statement lifetime, rc.8 claim) The pre-handoff review (4 independent agents) flagged two comment imprecisions in the #166 pre-resolve block: - Line 724: said the Statement is kept alive "so its warmed cache persists into the query_typed" — but tokio-postgres's CachedTypeInfo.types lives on InnerClient, not Statement; Statement::Drop sends a server Close but does not clear the cache. The Statement is actually kept alive for .columns() reuse in the empty-rows branch. A maintainer reading the old comment might avoid dropping the Statement early under the false belief it would clear the cache and re-introduce the hang. - Line 713: said "This is exactly rc.8's behavior" — rc.8 used the named query() path (types resolved in prepare's Describe); this uses a separate prepare + the unnamed query_typed. The outcome is equivalent (types resolved before the data fetch, no mid-stream interleave) but the code path is not identical. No code change — comments only. * test: add batch-path wide-enum regression test (#166) The pre-handoff review found the batch path (run_batch_in_session → exec_query_on_client) with a wide enum table on a direct connection was untested — the single-query test covers exec_query → exec_query_on_client, but the batch caller is a distinct path with its own connection handling. If the pre-resolve were removed from the batch path specifically, every existing test would still pass. Add wide_enum_table_returns_rows_via_batch_on_direct_connection: execute_query_batch (no session_id, the non-session batch path) with the same 600-row × 500-byte-pad enum table at limit 50, asserting rows return within 30s. Mirrors the single-query test's fixture, just routed through the batch RPC (run_batch_in_session → exec_query_on_client).
aesslinger
added a commit
that referenced
this pull request
Oct 23, 2026
* fix: pre-resolve column types on batch/session pooler path (#169) The #166 fix (#168) and the #164 pinned one-shot path both only cover non-session single result-set queries. A batch or interactive transaction session on a transaction-mode pooler (pooler_mode=transaction) still deadlocked on a table with a non-builtin column type (custom enum, composite, extension type) once the result was large enough to fill the channel buffer — the same typeinfo_statement mid-stream interleave as #166, on the batch path that neither #164 nor #168 covered. run_batch_in_session calls exec_query_on_client directly (bypassing exec_query's routing to the #164 one-shot pinned path), and #168's pre-resolve in exec_query_on_client was gated on !is_transaction_pooler, so a pooler connection (is_transaction_pooler=true) skipped it → query_typed fired typeinfo_statement mid-stream → deadlock. Fix: per-query routing in run_batch_in_session, keyed on is_transaction_pooler() && returns_result_set(query), with two sub-cases: * in_transaction (the user's BEGIN has pinned the backend on this shared connection — either a session-pinned batch reusing a prior BEGIN, or a bare batch whose earlier statement was BEGIN): a plain prepare inside that transaction is safe (no reassignment can orphan the named statement) and keeps the SELECT on the same connection where transaction-local state (temp tables, SET variables) is visible. Run exec_query_on_client with pre_resolve_types=true. * Not in_transaction (no open transaction pins the backend): route to the one-shot pinned path (exec_query_pooled_txn, the proven #164 mechanism). The one-shot client dies with its stale client-side type cache; the shared batch client sits idle for that one query (acceptable: when there's no transaction there's no transaction-local state to lose by using a different connection). Using in_transaction (not reused) as the routing condition is the key correctness point: a bare batch [BEGIN, CREATE TEMP TABLE t, SELECT * FROM t] on a pooler has reused=false but in_transaction=true after the mid-batch BEGIN. With reused, the SELECT would route to a one-shot connection that can't see the temp table; with in_transaction, it stays on the shared client where the temp table is visible. Also adds a pre_resolve_types: bool parameter to exec_query_on_client, separating the pre-resolve gate from is_transaction_pooler (which stays for error-message phrasing only). This avoids overloading is_transaction_pooler=false on a connection that IS a transaction pooler (which would corrupt error formatting). The three callers pass: - exec_query (single): !is_transaction_pooler() (the #168 direct path) - run_pinned_body (#164 one-shot): false (it does its own prepare) - run_batch_in_session (this fix): true when in_transaction No changes to exec_query_pooled_txn, run_pinned_body, pin_pinned_txn, or exec_query's routing. The #166 direct-connection fix and the #164 single-query pooler fix are untouched. Adds two live regression tests: - wide_enum_table_returns_rows_via_session_pinned_batch_on_pooler: the reporter's actual scenario (BEGIN → SELECT wide enum → ROLLBACK in a session-pinned batch with pooler_mode=transaction). Hung past 30s on rc.11/#168; returns cleanly with the fix. - wide_enum_table_returns_rows_via_non_session_batch_on_pooler: the non-session sub-case (no session_id, routes to the one-shot path). Verified locally: 425 unit tests + 10 live tests pass, clippy clean. * fix: stop one-shot routing from evicting the batch cancel guard (#172) The #169 batch/session pooler fix routed a non-in_transaction result-set query to exec_query_pooled_txn from inside run_batch_in_session's loop. The one-shot registers its own CancelGuard under the SAME request_id as the batch guard; cancel::insert_action is a HashMap::insert (overwrite), and the one-shot guard's Drop removes the key entirely — so the batch's shared-client guard was evicted, leaving subsequent batch queries with no registered cancel handle (a host cancel(request_id) became a no-op, defeating #126's "a late cancel is never a no-op for an in-flight query" guarantee). On main the batch never called the one-shot, so this is a new regression introduced by #172. Fix: hold the guard as Option<CancelGuard>, take+drop it around each one-shot routing (so the one-shot owns the map entry while it runs), and re-register for the shared client before the next iteration. Also fixes pre-handoff-review findings on #172: - E1: the pre-resolve block comment still said "Gated to direct connections / poolers keep the one-shot path" after the gate changed to pre_resolve_types (now true for session-pinned pooler in_transaction too). Rewrote to match the gate. - E4: exec_query's comment said "the batch path stays on the shared cached client / owns its own transaction" — now wrong for the no-tx pooler one-shot routing and the bare-batch-no-BEGIN case. Rewrote. - E3: two test comments named reused (not in_transaction) as the routing key; the code keys on in_transaction. Corrected. - Added empty_result_returns_columns_via_session_pinned_batch_on_pooler to guard the empty-rows column-discovery fallback (pre_resolve_types gate), which #169 made reachable for session-pinned pooler batches but no test exercised. Follow-ups filed: #173 (bare [SET/CREATE TEMP, SELECT] batches on a pooler lose session-local state on the one-shot path), #170 updated (the .ok() swallow's 25P02 transaction-abort cascade on the new pooler in_transaction path). Verified: 425 unit + 11 live tests pass, clippy clean.
aesslinger
added a commit
that referenced
this pull request
Oct 23, 2026
…n a transaction pooler (#173) (#174) * fix: keep bare [SET/CREATE TEMP, SELECT] batch on the shared client on a transaction pooler (#173) PR #172 (#169 fix) routed a non-`in_transaction` result-set query in a batch to the one-shot pinned path (`exec_query_pooled_txn`) on a separate connection. A bare batch (no `session_id`, no `BEGIN`) that sets session-local state in an earlier non-result-set statement — a session `SET` (not `SET LOCAL`) or a `CREATE TEMP TABLE` outside a transaction — then runs a result-set `SELECT` silently loses that state, because the SELECT runs on a fresh one-shot connection that never saw the `SET`/temp table (the one-shot applies only the host's `schema` via `SET LOCAL` and does not replay prior session state). Result: the SELECT returns the wrong precision (GUC case) or `ERROR: relation "t" does not exist` (temp-table case). No error surfaces about the lost state. On `main` (rc.11, before #172), `run_batch_in_session` ran every query via `exec_query_on_client` on one shared `pg_client`, so a `SET` then `SELECT` in a single bare batch reliably shared state (same connection, same backend). #172 broke that for the pooler `!in_transaction` case. Fix: detect session-local state set earlier in the bare batch and keep the result-set SELECT on the shared batch client with a column-type pre-resolve, instead of the one-shot. A new pure classifier `sets_session_local_state` matches a session `SET` (without `LOCAL`) or a `CREATE TEMP[ORARY] TABLE`; transaction-scoped forms (`SET LOCAL`) and permanent `CREATE TABLE` are excluded — the `in_transaction` branch already keeps the former on the shared client, and the latter is visible to any connection. A sticky `session_state_set` flag in `run_batch_in_session` flips after such a statement, and the routing condition `in_transaction || session_state_set` reuses the existing `in_transaction` pre-resolve path verbatim. No regression to #166/#168/#160/#163/#169: the fix only adds a third routing condition that routes to the same `exec_query_on_client( pre_resolve_types=true)` path the `in_transaction` branch already uses. It never alters the direct-connection #166 path (`!is_transaction_pooler()`), the #164 one-shot path, or the #169 `in_transaction` path. Warming `CachedTypeInfo.types` via `prepare` means `query_typed`'s `get_type` is a cache hit and `typeinfo_statement` never fires mid-drain on any connection — the identical mechanism #166/#168 and the #169 `in_transaction` branch rely on, so no new hang/deadlock is introduced. Scope: restores the direct-PostgreSQL + `pooler_mode=transaction` config (one TCP connection = one backend, held) and the temp-table case — the clearest *new* regressions #173 identifies. On a real PgBouncer transaction pooler, a session `SET` between autocommit statements is already lost to backend reassignment on `main`, so the pure-GUC case is pre-existing there (not a new regression) and stays out of scope; the existing session-pinned `BEGIN`/`COMMIT` path (covered by `wide_enum_table_returns_rows_via_session_ pinned_batch_on_pooler`) remains the PgBouncer-canonical workaround. Adds 9 unit tests for `sets_session_local_state` and two live regression tests: `temp_table_visible_in_bare_batch_on_pooler` (the temp-table case) and `session_set_visible_in_bare_batch_on_pooler` (probed via `SELECT current_setting('extra_float_digits')` returning "3" vs the lost-SET default "1"). Verified: 434 unit + 13 live `client_live_tests` (incl. all #166/#168/#160/ #163/#169 guards) pass against a live PostgreSQL; 38/39 `live_db` end-to-end (the one failure is `extension "vector" is not available`, a pre-existing fixture gap needing the pgvector-test container). clippy clean, fmt applied. * fix: address pre-handoff-review findings on #173 classifier and routing Pre-handoff ensemble review of #174 surfaced four verified findings: 1. `CREATE [GLOBAL|LOCAL] TEMP[ORARY] TABLE` was a false negative. PostgreSQL accepts an optional deprecated `GLOBAL`/`LOCAL` qualifier before `TEMP`/`TEMPORARY` (both still create a session-scoped temp table — verified live), but the 3-keyword lookahead classified `CREATE LOCAL TEMP TABLE ...` as non-session-local, so a later `SELECT * FROM t` in the bare batch still hit the exact #173 `relation does not exist` regression. Extend the `CREATE` arm to 4 keywords and skip the `GLOBAL`/`LOCAL` noise word (mirrors how `transaction_effect` skips optional `WORK`/`TRANSACTION`). 2. `SET TRANSACTION` / `SET CONSTRAINTS` were false positives — transaction-control verbs, not GUCs, that set no session state a later SELECT depends on. Exclude them alongside `SET LOCAL`. 3. `session_state_set` flipped on the query text alone, ignoring `outcome.is_ok()` — a `SET`/`CREATE TEMP TABLE` the server rejected established no session state but still steered a later SELECT onto the shared client. Gate on success, mirroring the sibling `in_transaction` update two lines above. 4. Docs drift: the `exec_query_on_client` pre-resolve comment listed "two cases where a plain `prepare` is safe" and said the no-open-transaction pooler path "can't safely `prepare`" — but #173 now routes that path to the shared-client pre-resolve. Rewrote the comment to list three cases and document the #173 case's real-PgBouncer tradeoff honestly (the prepare is non-fatal; safe on the direct+`pooler_mode=transaction` config the PR targets). Also fixed "two cases"→"three cases" in the routing comment and the `exec_query` batch-path description, and corrected the `sets_session_local_state` doc (it listed `SET SESSION AUTHORIZATION` as "may be missed" but the classifier catches it). Adds a unit test for the `GLOBAL`/`LOCAL` qualifier and folds `SET TRANSACTION`/`SET CONSTRAINTS` into the existing exclusion test. Verified: 435 unit + 13 live `client_live_tests` (incl. all #166/#168/#160/ #163/#169 guards) pass against a live PostgreSQL; clippy clean; fmt applied.
aesslinger
added a commit
that referenced
this pull request
Oct 23, 2026
…n't starve them (#167) (#176) * fix: route ping/connect/cancel to dedicated lanes so stuck queries don't starve them (#167) The plugin had 4 workers fed by a single shared queue; every RPC type shared them. When 4 calls were stuck (any slow query up to the host's 120s call timeout), every subsequent RPC queued behind them — a `ping` or `test_connection` to a *different* database, even the `cancel` notification that's meant to relieve the stuck workers. The module comment claimed "a slow query on one connection does not block a `ping`/metadata call on another," but that isolation was by-worker-slot, and it broke at saturation (>=4 stuck calls). Worst: the `cancel` notification routed through the same 4-worker queue, so 4 stuck workers deadlocked the relief valve — only quitting Tabularis freed them. Fix: route by method into three lanes before a worker pulls the line. - cancel lane (1 worker): `cancel` notifications run on a dedicated worker that bypasses the general queue. When 4 general workers are stuck, the cancel still fires `pg_cancel_backend` so the stuck workers' awaits resolve and free up — instead of queuing behind them. The relief valve is no longer stuck behind what it relieves. - lifecycle lane (2 workers): `ping`/`test_connection`/`initialize`/ `shutdown` run on a dedicated lane, so a health check or new connect to database B isn't blocked by 4 slow queries against database A. Two workers (not one) so a single stuck `ping` (a `SELECT 1` against an unreachable DB) doesn't starve the lane. - general lane (4 workers): unchanged — metadata, execute_query, CRUD, DDL, BLOB. `run_reader` peeks the method (`rpc::peek_method`) and routes; `handle_line` stays the single source of truth for parse + dispatch + response. The `SHUTDOWN_GRACE` drain (#155) now drains all lanes' worker handles the same way (timeout_at + abort()), so the orphan-on-quit behavior is preserved. A cancel-driven worker-abort (Layer 3: `select!`-on-cancel dropping the in-flight `handle_line` future) was implemented and tested, then reverted. The `cancel_notification_stops_the_server_side_statement` live test proved a cancelled query MUST return its `57014` error response (the documented `#126` contract — the host reads it); the worker-abort dropped the future before it could produce that response, suppressing it and hanging the host. The "server ignores `pg_cancel_backend`" gap is left as a documented pre-existing limit (matching the builtin) rather than traded for a contract break — see `cancel::cancel`'s doc comment. Adds 6 `peek_method` unit tests and 2 `tests/worker_starvation.rs` integration tests: a `ping` to another DB returns past 4 hung general queries (Layer 2); a `cancel` notification isn't queued behind 4 hung general workers (Layer 1). Verified empirically against a live PostgreSQL: 13/13 `client_live_tests` (the full #160->#173 chain — #166/#168 direct enum pre-resolve, #164/#160 pooler one-shot, #169 batch pooler, #173/#174 bare-batch session state — all intact); 3/3 `tests/shutdown.rs` (#155 drain); 2/2 `tests/worker_starvation.rs`; 38/39 `tests/live_db` (the one failure is `extension "vector" is not available`, a pre-existing pgvector fixture gap needing the 54321 container — not a regression). 441 unit tests pass; clippy clean; fmt applied. * fix: remove stale Layer 3 worker-abort docs, make cancel-lane test non-vacuous (#167) A pre-handoff ensemble review (11 dimensions + /code-review) found the module doc and test-file doc still described the reverted Layer 3 worker-abort (select!/CancellationToken) as live and tested, when run_worker runs handle_line unwrapped with no such mechanism. Five findings fixed: - src/main.rs module doc + spawn-site comments: rewrite to describe Layers 1-2 only, with an honest 'worker-abort was considered but not added' paragraph pointing to cancel::cancel's doc for the rationale. Remove 'Cancellable (Layer 3)' inline comments. - tests/worker_starvation.rs module doc: rewrite to describe Layers 1-2 only; remove the dangling 'asserted in the next test' comment (there was no next test). - Cancel test rewritten as cancel_runs_on_its_own_lane_*: send cancel as a request (id 200, not a notification) and assert a stdout response within 10s — a misrouted cancel queues behind 4 stuck workers and never responds. Mutation-tested: misrouting cancel->General fails the test with the right message; the old test passed on that regression. - Add 5 direct lane_for unit tests to main.rs (the routing decision the whole fix depends on had no direct test). Add PartialEq/Eq/Debug to Lane for assert_eq!. - Remove dead _cancellable: bool param on run_worker and the unused cancel_notification test helper (YAGNI — their doc comments were the stale ones). - Document the cancel-lane single-worker tradeoff at the spawn site (no connect_timeout on cancel_query; a stuck cancel blocks the lane). - Reword peek_method doc: the double parse is an accepted cost of keeping handle_line's signature unchanged, not 'avoids a second parse'. Verified: 441 unit tests pass (incl. 5 new lane_for), 2/2 worker_starvation integration tests pass, clippy -D warnings clean, fmt clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
#160 and #163: querying a table with a non-builtin PostgreSQL type (custom
enum, composite, domain, range, or extension type likecitext/ Supabaseauth.uid) hangs or errors behind a transaction-mode pooler. rc.8 works; rc.9/rc.10 hang. The health-check disconnect in #163 is downstream fallout from repeated hung/cancelled queries, not an independent bug.Root cause (two orthogonal pooler bugs)
prepare/Bind-ExecuteSync gap when a transaction-mode pooler reassigns the backend between them →26000. fix: use unnamed-statement protocol for pooler (PgBouncer) safety #149's unnamedquery_typedpath fixed this and stays the data-fetch protocol.typeinfo_statement— fired for non-builtin result-column OIDs — collides with stale server-side prepared statements on reassigned pooler backends (42P05on PgBouncer) or hangs (Supavisor). fix: use unnamed-statement protocol for pooler (PgBouncer) safety #149 does not fix this.Why the previous draft's prepare-warming was wrong
The earlier commit on this branch added an unconditional
preparebeforequery_typed. Local PgBouncer testing showed it only adds another named statement and moves the collision (s0→s1); it does not solve Problem B. That approach is removed.The fix (no dependency fork)
For connections marked
pooler_mode = "transaction", result-set queries take a pinned one-shot transaction path:post_createhook already validates the script on the real connection with the same error shape, so the preflight would just open a second throwaway connection per query for nothing).BEGIN— pins the backend for the whole sequence.DEALLOCATE ALL— clears stale server-side prepared statements on the pinned backend.prepare(query)— resolves every result column's type via tokio'stypeinfo_statementwhile pinned, before the data fetch drains anything, so the named statement can't be orphaned mid-resolution. Uses the original userquery(before pagination); the appendedLIMIT/OFFSETon the data fetch doesn't change result column types, so the warmed cache applies to the paginated fetch too.query_typed(&final_query, &[])— the fix: use unnamed-statement protocol for pooler (PgBouncer) safety #149 unnamed data fetch (Problem A stays fixed). BinaryRowdecoding /extract_valueunchanged.DEALLOCATE ALL+COMMIT.The transient pool's client is discarded after each call, so tokio-postgres's private cached
typeinfo_statementhandle (whichDEALLOCATE ALLcan't clear) dies with the connection — no stale client-side type cache is ever reused.The whole sequence is bounded by a 90s plugin-side timeout (below the host's 120s call timeout), so a Supavisor-style stall surfaces as an actionable error instead of the opaque host timeout.
Non-result-set statements (DDL, writes) and the batch path stay on the shared cached client —
execute_typedis unnamed and needs no pinning.pooler_modeis trimmed before the transaction comparison (matching howstartup_scriptis normalized inbuild_pool), so a hand-editedconnections.jsonwith surrounding whitespace can't silently bypass the pinned path.User guidance
format_query_errornow gives themax_prepared_statements > 0+pooler_modeguidance even whenpooler_modeis unset (conditional phrasing: "if this connection goes through a transaction-mode pooler…"), so users on older hosts that can't send the field still see the hint. Whenpooler_mode=transactionis set, the guidance is definitive.The best pooler-side fix remains
max_prepared_statements > 0for PgBouncer-style poolers. For poolers without that setting, marking the connection's Pooler Mode as "Transaction" in Tabularis activates the pinned path.Verified locally (pg-tabularis-test + PgBouncer transaction mode)
max_ps=0,pooler_mode=transaction(new pinned path)max_ps=0, nopooler_mode(old path)max_ps=100,pooler_mode=transactionmax_ps=100, nopooler_modeNote: plain PgBouncer gives a clean
42P05, not the Supavisor hang. The pinned path removes the same underlying backend-reassignment/named-statement hazard, but Supavisor-specific confirmation still needs a real Supavisor repro or a reporter retest.What stays (no existing work lost)
query_typeddata fetch (Problem A covered).prepare()-skip on empty result sets for pooler connections.format_query_errorrewrite +pooler_modeplumbing.Row/extract_valuedecoding surface (no simple-query rewrite, no parity risk).Out of scope / follow-up
tokio-postgres— explicitly avoided.pool.get()(the 90s bound above wraps only the pinned query phase;pool.get()stays unbounded, as it is on the cached path) — tracked separately.Verification