Repository navigation
fix: route ping/connect/cancel to dedicated lanes so stuck queries don't starve them (#167) - #176
Merged
Merged
Conversation
…n'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.
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. |
…n-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
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/test_connectionto a different database, even thecancelnotification that's meant to relieve the stuck workers. The module comment claimed "a slow query on one connection does not block aping/metadata call on another," but that isolation was by-worker-slot and broke at saturation (>=4 stuck calls). Worst: thecancelnotification routed through the same 4-worker queue, so 4 stuck workers deadlocked the relief valve — only quitting Tabularis freed them.Closes #167.
Fix
Route by method into three lanes before a worker pulls the line:
cancelnotifications run on a dedicated worker that bypasses the general queue. When 4 general workers are stuck, the cancel still firespg_cancel_backendso the stuck workers' awaits resolve and free up — instead of queuing behind them. The relief valve is no longer stuck behind what it relieves.ping/test_connection/initialize/shutdownrun 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 stuckpingdoesn't starve the lane.execute_query, CRUD, DDL, BLOB.run_readerpeeks the method (rpc::peek_method) and routes;handle_linestays the single source of truth for parse + dispatch + response. TheSHUTDOWN_GRACEdrain (#155) now drains all lanes' worker handles the same way (timeout_at+abort()), so the orphan-on-quit behavior is preserved.Layer 3 (cancel-driven worker-abort) — implemented, tested, reverted
A
select!-on-cancel that drops the in-flighthandle_linefuture on a hostcancel(so a worker frees even when the server ignorespg_cancel_backend) was implemented and tested, then reverted. Thecancel_notification_stops_the_server_side_statementlive test proved a cancelled query MUST return its57014error response (the documented#126contract — the host reads it); the worker-abort dropped the future before it could produce that response, suppressing it and hanging the host. The "server ignorespg_cancel_backend" gap is left as a documented pre-existing limit (matching the builtin) rather than traded for a contract break — seecancel::cancel's doc comment. (Whether a Layer 3 mechanism that frees a worker without suppressing the response is feasible is an open follow-up question; the naive drop-on-cancel approach is not.)Tests
peek_methodunit tests (src/rpc_tests.rs) — routing classification (cancel/lifecycle/general, notifications, parse errors).lane_forunit tests (src/main.rs) — the routing decision the whole fix depends on, pinned directly (cancel→Cancel, the 4 lifecycle methods→Lifecycle, general/unknown/None→General).tests/worker_starvation.rsintegration tests: apingto another DB returns past 4 hung general queries (Layer 2); acancelruns on its own lane, not behind 4 hung general workers (Layer 1) — the cancel is sent as a request (with anid) so the test reads its stdout response, making the routing observable (a misrouted cancel queues behind 4 stuck workers and never responds).Verification
Empirical proof against a live PostgreSQL that the #166 chain stays intact:
client_live_tests— the full Bug: Regression in plugin v1.0.0-rc.10 table hangs while opening but works in v1.0.0-rc.8 #160->Bare [SET/CREATE TEMP, SELECT] batch on a transaction pooler loses session-local state on the one-shot path #173 chain (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 direct enum pre-resolve, fix: pin backend for transaction-pooler result queries on non-builtin column types (#160, #163) #164/Bug: Regression in plugin v1.0.0-rc.10 table hangs while opening but works in v1.0.0-rc.8 #160 pooler one-shot, Batch/session path on a transaction pooler still deadlocks on non-builtin column types #169 batch pooler, Bare [SET/CREATE TEMP, SELECT] batch on a transaction pooler loses session-local state on the one-shot path #173/fix: keep bare [SET/CREATE TEMP, SELECT] batch on the shared client on a transaction pooler (#173) #174 bare-batch session state) all intact.tests/shutdown.rs— Bug: Plugin process never exits after Tabularis quits if an RPC is in flight #155 drain with the new multi-lane worker handles.tests/worker_starvation.rs— Layers 1 + 2.tests/live_db— the one failure isextension "vector" is not available(a pre-existing pgvector fixture gap needing the 54321 container, not a regression); criticallycancel_notification_stops_the_server_side_statementpasses (the cancel contract the Layer 3 revert preserved).lane_forbin tests);cargo clippy --all-targets -- -D warningsclean;cargo fmt --allapplied.