feat: cancel the server-side statement on a host cancel notification (#126) - #127
Merged
Merged
Conversation
…126) When a host plugin-call timeout fires today, the host abandons its pending JSON-RPC slot but the Postgres backend keeps running the statement server-side — an orphaned query holding locks. tabularis#832 will fix the host half by sending a fire-and-forget cancel notification; this is the plugin-side implementation, ready to wire up the moment that lands. Until then a cancel never arrives and this is a safe no-op addition — no existing RPC's behavior or response shape changes. Design (agreed in the tabularis#832 thread: host owns *when*, plugin owns *how*): each in-flight execute_query/execute_query_batch/explain_query registers a tokio_postgres::CancelToken keyed by request id before running the statement, and deregisters it when the query resolves. On a cancel notification for a known id, CancelToken::cancel_query opens a fresh matching-TLS connection and sends pg_cancel_backend; the in-flight query future then resolves via its normal Err path ("canceling statement due to user request") and flows out as a normal error response for the original call. An unknown id (already finished, or arrived after cleanup) is a no-op, never an error, so a late/duplicate cancel can't disrupt a later query that reused the id. - src/cancel.rs (new): a CancelGuard RAII registry. register builds a boxed cancel action from the live CancelToken + ConnectionParams; drop deregisters on every return path (Ok or Err) so the map can't leak. cancel(id) runs the one-shot action; run_cancel picks NoTls vs make_tls_connect via needs_tls, matching the pool's two-branch shape. - src/handlers/query.rs: cancel handler; CancelGuard threaded into run_batch_in_session (session + non-session paths) and exec_query. - src/rpc.rs: "cancel" dispatch; handle_line returns Option<Value>, None for a notification (no top-level id field) so no response is written. Notification-ness is decided by absence of the id field, not by method (id: null is a request whose id is null, not a notification). - src/main.rs: run_worker skips the stdout write for None — a stray response line for a notification would corrupt the protocol stream. - src/client.rs: pub(crate) make_tls_connect + needs_tls. Fixed a real bug the helper's doc had claimed: it always builds a TLS connector via build_tls_connector with no NoTls fallback, so callers must check needs_tls first and use NoTls directly when false (the pool already does this two-branch shape). cancel::run_cancel does the same. Envelope (per the issue's proposed shape, since tabularis#832 hasn't fixed it yet): {"method":"cancel","params":{"id":<request-id>}} as a true id-less notification. If the host settles on a different shape, only rpc.rs's dispatch string + the cancel handler's param read change — the registry and the query-handler threading are envelope-agnostic. TDD: 5 unit tests for the registry's bookkeeping (register/cancel/remove, one-shot action, unknown-id no-op, CancelGuard drop-deregisters, no-id guard is a no-op) using a fake boxed action — a real CancelToken needs a live connection. A #[ignore]-gated live-database test sends pg_sleep(30), cancels it, and confirms via pg_stat_activity that the backend stops; run with --include-ignored. Verified: cargo build --release; 361 unit + 30 live-DB (2 ignored: the new cancel test + the pgvector one) against the local pg-tabularis-test podman container; the cross-repo 83-test byte-for-byte parity suite (POSTGRES_PLUGIN_BIN against tabularis' src-tauri/tests/postgres_integration parity*) passes 83/83; cargo clippy --all-targets -D warnings; cargo fmt --all --check; npx markdownlint CHANGELOG.md — all clean. Dormant until tabularis#832 ships. Ahead of the builtin for this one behavior (the builtin's cancel_query_impl only aborts its local tokio task, never calls pg_cancel_backend) — flagged per the repo's "don't improve on the builtin silently" rule, but scoped to a bug the reporter explicitly asked to fix.
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. |
…r-side-statement # Conflicts: # CHANGELOG.md
aesslinger
added a commit
that referenced
this pull request
Sep 30, 2026
Ships the two PRs merged since 1.0.0-rc.5: the NULL column-default filter parity fix (#125, #122 — case-insensitive to match the builtin) and the cancel server-side statement notification handler (#127, #126 — dormant until tabularis#832 ships the host-side notification). Verified: cargo build --release; 366 unit + 30 live-DB against the local pg-tabularis-test podman container; the cross-repo 83-test byte-for-byte parity suite (POSTGRES_PLUGIN_BIN against tabularis' src-tauri/tests/postgres_integration parity*) passes 83/83; cargo clippy --all-targets -D warnings; cargo fmt --all --check; npx markdownlint CHANGELOG.md — all clean.
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
Implements the plugin-side half of #126 — cancel the server-side statement when the host sends a
cancelnotification. Companion to TabularisDB/tabularis#832 (still OPEN), which will ship the host half: sending a fire-and-forgetcancelnotification when its plugin-call timeout fires. Today a timed-outexecute_queryabandons the JSON-RPC slot but leaves the Postgres backend running the statement — an orphaned query holding locks. This PR makes the plugin stop it.Design (agreed in the tabularis#832 thread — host owns when, plugin owns how): each in-flight
execute_query/execute_query_batch/explain_queryregisters atokio_postgres::CancelTokenkeyed by request id, and on acancelnotification for a known id, sendspg_cancel_backendvia a fresh matching-TLS connection. The in-flight query then resolves through its normalErrpath ("canceling statement due to user request") as a normal error response for the original call. An unknown id (already finished, or arrived after cleanup) is a no-op, never an error.Changes
src/cancel.rs(new) —CancelGuardRAII registry.registerbuilds a boxed cancel action from the liveCancelToken+ConnectionParams;dropderegisters on every return path (Ok or Err) so the map can't leak.cancel(id)runs the one-shot action.run_cancelpicksNoTlsvsmake_tls_connectvianeeds_tls, matching the pool's two-branch shape; errors are logged and swallowed (fire-and-forget).src/handlers/query.rs—cancelhandler;CancelGuardthreaded intorun_batch_in_session(session + non-session paths) andexec_query.src/rpc.rs—"cancel"dispatch;handle_linereturnsOption<Value>,Nonefor a notification (no top-levelidfield) so no response is written. Notification-ness is decided by absence of theidfield, not by method (id: nullis a request whose id is null, not a notification).src/main.rs—run_workerskips the stdout write forNone— a stray response line for a notification would corrupt the protocol stream.src/client.rs—pub(crate) make_tls_connect+pub(crate) needs_tls. Also fixes a real bug the helper's doc had:make_tls_connectalways builds a TLS connector with noNoTlsfallback, so callers must checkneeds_tlsfirst (the pool already does this two-branch shape);cancel::run_canceldoes the same.src/bin/test_plugin.rs— handles the newOption<Value>return.src/cancel_tests.rs, fake boxed action — a realCancelTokenneeds a live connection) + an#[ignore]-gated live test that sendspg_sleep(30), cancels it, and confirms viapg_stat_activitythe backend stops.Envelope
Per the issue's proposed shape (tabularis#832 hasn't fixed it yet):
{"method":"cancel","params":{"id":<request-id>}}as a true id-less notification. If the host settles on a different shape, onlyrpc.rs's dispatch string + thecancelhandler's param read change — the registry and the query-handler threading are envelope-agnostic.Backward compatibility & dormancy
No host sends
canceltoday (verified: nocancelwiring onupstream/main). The new dispatch is never hit;CancelGuardregister/drop is two cheapHashMapops per query with no output change — byte-identical responses for every existing RPC. Dormant until tabularis#832 ships; safe to merge now (zero breakage) and lights up when #832 lands.Ahead of the builtin for this one behavior (the builtin's
cancel_query_implonly aborts its local tokio task, never callspg_cancel_backend) — flagged per the repo's "don't improve on the builtin silently" rule, but scoped to a bug the reporter explicitly asked to fix.Verification
cargo build --releasePOSTGRES_PLUGIN_BINagainsttabularis'src-tauri/tests/postgres_integration parity*): 83/83 pass — no existing behavior regressed.--include-ignored).cargo clippy --all-targets -- -D warnings;cargo fmt --all --check;npx markdownlint CHANGELOG.md— all clean.Closes #126.