Repository navigation
fix: pre-resolve column types on direct connections to stop enum-table hang (#166) - #168
Merged
Merged
Conversation
…e 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)
aesslinger
added
prerelease:rc
|
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. |
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
1 similar comment
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
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.
This was referenced Oct 23, 2026
…laim) 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.
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
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 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.
aesslinger
added a commit
that referenced
this pull request
Oct 23, 2026
Bumps version to 1.0.0-rc.12 in Cargo.toml, Cargo.lock, .tabularium, and CHANGELOG.md. ### Included since rc.11 **Fixed:** - Direct-connection result-set queries on non-builtin column types (custom enum/composite/domain/range/extension types) no longer hang on large results (#166, #168) — pre-resolves column types via `prepare` on the shared cached client before the `query_typed` data fetch (the rc.11 fix only covered the transaction-pooler path) - Batch / interactive-transaction-session result-set queries on a transaction-mode pooler no longer deadlock on wide non-builtin-typed tables (#169, #172) — `run_batch_in_session` now makes the same per-query routing decision as the single-query path (in_transaction pre-resolves on the shared client; no open transaction routes to the one-shot pinned path) - A bare batch (no `session_id`, no `BEGIN`) that sets session-local state earlier (a session `SET` or `CREATE TEMP[ORARY] TABLE`) then runs a result-set `SELECT` no longer silently loses that state on a `pooler_mode = transaction` connection (#173, #174) — the rc.11 #172 fix routed the SELECT to a fresh one-shot that never saw the SET/temp table; the fix keeps the SELECT on the shared batch client with a pre-resolve, restoring main's within-batch guarantee (handles GLOBAL/LOCAL TEMP, excludes SET LOCAL / SET TRANSACTION / SET CONSTRAINTS) - A slow or hung query no longer starves `ping`/`test_connection`/ `initialize`/`shutdown` and the `cancel` notification for every connection (#167, #176) — RPCs are routed by method into three lanes before a worker pulls the line: a dedicated cancel lane (the relief valve, never queued behind the workers it relieves), a 2-worker lifecycle lane, and the existing 4-worker general lane; the SHUTDOWN_GRACE drain (#155) covers all lanes' worker handles **Known limitations (tracked as follow-ups, not regressions vs. the builtin):** - A bare [SET, SELECT] / [CREATE TEMP TABLE, SELECT] batch on a *real* PgBouncer (pool_mode = transaction) can still lose session state between autocommit statements and, in rare cases with non-builtin types under reassignment during the pre-resolve prepare, can hang (#175) — wrap such batches in BEGIN/COMMIT; the direct + pooler_mode=transaction flag config (the common use) is unaffected - A composite type ALTER TYPE-ed (same OID, new field layout) on a recycled pooled connection can read a stale cached Type and produce wrong JSON for the composite's fields (#171) — pre-existing (tokio-postgres/deadpool cache-invalidation limitation); the rc.11/#168 pre-resolve widened the cache-warming window but did not introduce the bug Verified: 441 unit tests pass; cargo clippy --all-targets -- -D warnings clean; markdownlint clean on CHANGELOG.md.
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
#166: a
SELECT *on a table with a non-builtin column type (customenum, composite, extension type) and wide rows hangs on a direct (non-pooler) connection once the result is larger than a few KB. The rc.11 fix (#164) only pre-resolved column types when Pooler Mode = "Transaction", so direct connections still used the old path.Root cause
tokio-postgres's
query_typedresolves non-builtin column types (get_type→ the internal namedtypeinfo_statement) inside itsRowDescriptionloop, mid-stream over the same channel delivering the query'sDataRows. The typeinfo reply is queued behind the pending rows, butquery_typedwon't drain the rows untilget_typereturns → deadlock once the row data fills the boundedresponseschannel + socket buffer (hence the size threshold — small results pass, large ones hang). This is not pooler-specific — it deadlocks on any connection. The pooler additionally orphans the statement across backend reassignment (42P05on PgBouncer), but the hang mechanism is the interleave, not the reassignment. The #164 framing ("Problem B = pooler reassignment") was incomplete.The fix
Pre-resolve result column types via
prepare(query)on the shared cached client before thequery_typeddata fetch, for direct connections only (gated on!is_transaction_poolerinexec_query_on_client).prepareruns Parse+Describe+Sync with no Execute/no DataRows, so itsget_typeinterleave has an empty channel and completes cleanly, warmingCachedTypeInfo.typeson the sharedInnerClient. The subsequentquery_typedthen hits that cache (get_type'sclient.type_(oid)returns early) → notypeinfo_statementmid-drain → no deadlock. This is exactly rc.8's behavior, which worked.exec_query_on_client(notexec_query) — it's the single convergence point for single, batch, and explain queries (the batch path calls it directly, bypassingexec_query).preparewould collide across backend reassignment (thes0→s1shift that killed the removed pre-warm,ca33b98); direct connections have no reassignment hazard, so the plainprepareis safe and cheaper.preparefailure is swallowed (.ok()) andquery_typedruns anyway — for builtin typesget_typeis always aType::from_oidcache hit (no hang), and a direct prepare failure on a valid result-set query is rare.Statementis kept alive so its.columns()is reused by the empty-rows branch (which previously did its ownpreparefor column discovery on direct connections).Measured against a live direct Postgres (podman, no pooler): ~39ms/call (cached path) vs ~37ms baseline, with no type-resolution overhead for builtin types (the common case —
get_typeis aType::from_oidcache hit, notypeinfo_statement; theprepareround-trip is still incurred, matching rc.8). Pool reuse is preserved (no per-query connection build, unlike the one-shot pooler path).What stays (no existing work lost)
query_typeddata fetch — stays the data-fetch protocol.prepare()-skip on empty result sets for pooler connections — stays.Also in this PR
exec_query's routing comment now describes Problem B as thetypeinfo_statementinterleave (deadlocks on any connection once the response is large enough), not just pooler reassignment.wide_enum_table_returns_rows_on_direct_connection— 600 rows × 500-byte pad + an enum column, fetched at limit 50 (≈25 KB) on a direct connection, asserting <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 rc.11 still hangs on direct (non-pooler) connections for tables with enum columns; 4 stuck workers then block ping/connect for every connection #166 ship.Out of scope / follow-up
WORKER_POOL_SIZEand blockping/connect for every connection. The hang fix removes the trigger, but the structural isolation gap is tracked in its own issue.Verification
Verified locally against pg-tabularis-test (direct, no pooler):
SHOW search_pathpooler_mode=transaction), enum