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 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
1 participant
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