Repository navigation
fix: pre-resolve column types on batch/session pooler path (#169) - #172
Merged
Merged
Conversation
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.
aesslinger
added
prerelease:rc
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
|
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. |
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
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
#169 (filed from the #168 pre-handoff review): 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 sametypeinfo_statementmid-stream interleave as #166, on the batch path that neither #164 nor #168 covered.Root cause
run_batch_in_sessioncallsexec_query_on_clientdirectly (bypassingexec_query's routing to the #164 one-shot pinned path), and #168's pre-resolve inexec_query_on_clientwas gated on!is_transaction_pooler, so a pooler connection (is_transaction_pooler=true) skipped it →query_typedfiredtypeinfo_statementmid-stream → deadlock on large non-builtin results.The fix — per-query routing in
run_batch_in_sessionKeyed on
is_transaction_pooler() && returns_result_set(query), with two sub-cases usingin_transaction(notreused):in_transaction(the user'sBEGINhas pinned the backend on this shared connection — either a session-pinned batch reusing a priorBEGIN, or a bare batch whose earlier statement wasBEGIN): a plainprepareinside 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. Runexec_query_on_clientwithpre_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 fix: pin backend for transaction-pooler result queries on non-builtin column types (#160, #163) #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).Why
in_transactionnotreused: a bare batch[BEGIN, CREATE TEMP TABLE t, SELECT * FROM t]on a pooler hasreused=falsebutin_transaction=trueafter the mid-batchBEGIN. Withreused, the SELECT would route to a one-shot connection that can't see the temp table; within_transaction, it stays on the shared client where the temp table is visible.pre_resolve_typesparameterAdds a
pre_resolve_types: boolparameter toexec_query_on_client, separating the pre-resolve gate fromis_transaction_pooler(which stays for error-message phrasing only). This avoids overloadingis_transaction_pooler=falseon a connection that IS a transaction pooler (which would corrupt error formatting). The three callers pass:exec_query(single):!is_transaction_pooler()(the fix: pre-resolve column types on direct connections to stop enum-table hang (#166) #168 direct path)run_pinned_body(fix: pin backend for transaction-pooler result queries on non-builtin column types (#160, #163) #164 one-shot):false(it does its own prepare)run_batch_in_session(this fix):truewhenin_transactionWhat stays unchanged (nothing breaks #166/#164)
exec_query_pooled_txn,run_pinned_body,pin_pinned_txn— untouched.exec_query's routing — untouched.elsebranch passes!is_transaction_pooler(), same as before).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 withpooler_mode=transaction). Hung past 30s on rc.11/fix: pre-resolve column types on direct connections to stop enum-table hang (#166) #168 (confirmed before the fix); returns cleanly with the fix.wide_enum_table_returns_rows_via_non_session_batch_on_pooler— the non-session sub-case (nosession_id, routes to the one-shot path).All existing live tests (direct connection, direct batch, single-query pooler, DDL routing) still pass — no regression.
Verification
Closes #169.