Repository navigation
fix: keep bare [SET/CREATE TEMP, SELECT] batch on the shared client on a transaction pooler (#173) - #174
Merged
Conversation
…n 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.
aesslinger
added
bug
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. |
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.
Summary
PR #172 (#169 fix) routed a non-
in_transactionresult-set query in a batch to the one-shot pinned path (exec_query_pooled_txn) on a separate connection. A bare batch (nosession_id, noBEGIN) that sets session-local state in an earlier non-result-set statement — a sessionSET(notSET LOCAL) or aCREATE TEMP TABLEoutside a transaction — then runs a result-setSELECTsilently loses that state, because the SELECT runs on a fresh one-shot connection that never saw theSET/temp table (the one-shot applies only the host'sschemaviaSET LOCALand 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. Onmain(rc.11, before #172),run_batch_in_sessionran every query viaexec_query_on_clienton one sharedpg_client, so aSETthenSELECTin a single bare batch reliably shared state (same connection, same backend). #172 broke that for the pooler!in_transactioncase.Closes #173.
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.
sets_session_local_state(src/handlers/query.rs): matches a sessionSET(withoutLOCAL) or aCREATE TEMP[ORARY] TABLE. Transaction-scoped forms (SET LOCAL) and permanentCREATE TABLEare excluded — thein_transactionbranch already keeps the former on the shared client, and the latter is visible to any connection.session_state_setflag inrun_batch_in_sessionflips after such a statement; the routing condition becomesin_transaction || session_state_set, reusing the existingin_transactionpre-resolve path verbatim.Why 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 thein_transactionbranch already uses. It never alters:!is_transaction_pooler()),in_transactionpath.Warming
CachedTypeInfo.typesviapreparemeansquery_typed'sget_typeis a cache hit andtypeinfo_statementnever fires mid-drain on any connection — the identical mechanism #166/#168 and the #169in_transactionbranch rely on, so no new hang/deadlock is introduced.Scope
Restores the direct-PostgreSQL +
pooler_mode=transactionconfig (one TCP connection = one backend, held) and the temp-table case — the clearest new regressions #173 identifies. On a real PgBouncer transaction pooler, a sessionSETbetween autocommit statements is already lost to backend reassignment onmain, so the pure-GUC case is pre-existing there (not a new regression) and stays out of scope; the existing session-pinnedBEGIN/COMMITpath (covered bywide_enum_table_returns_rows_via_session_pinned_batch_on_pooler) remains the PgBouncer-canonical workaround.Tests
sets_session_local_state(src/handlers/query_tests.rs).src/client_live_tests.rs):temp_table_visible_in_bare_batch_on_poolerandsession_set_visible_in_bare_batch_on_pooler(probed viaSELECT current_setting('extra_float_digits')returning"3"vs the lost-SET default"1").Verification
client_live_tests(incl. all 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/fix: pre-resolve column types on direct connections to stop enum-table hang (#166) #168/Bug: Regression in plugin v1.0.0-rc.10 table hangs while opening but works in v1.0.0-rc.8 #160/Query timeout followed by health-check disconnect after repeated cancellations #163/Batch/session path on a transaction pooler still deadlocks on non-builtin column types #169 guards) pass against a live PostgreSQL.live_dbend-to-end (the one failure isextension "vector" is not available, a pre-existing fixture gap needing the pgvector-test container on port 54321 — unrelated to this change).cargo clippy --all-targets -- -D warningsclean;cargo fmt --allapplied.