fix: honour request cancellation on PostgreSQL, and apply max_rows to comment-prefixed and CTE queries - #399
Closed
Elrendio wants to merge 2 commits into
Closed
Conversation
`SQLRowLimiter.isSelectQuery` classified a statement with
`sql.trim().toLowerCase().startsWith("select")`, so `max_rows` was
silently a no-op for every query that does not open with a bare
`SELECT`. No LIMIT was appended at all — the cap simply did not exist
for these, and nothing surfaced that:
-- reporting job\nSELECT * FROM t -> returned unchanged
/* tag */ SELECT * FROM t -> returned unchanged
WITH x AS (SELECT ...) SELECT * FROM x-> returned unchanged
(SELECT a) UNION (SELECT b) -> returned unchanged
Leading comments are common (query tags/attribution) and CTEs are the
normal shape of an analytical query, so in practice the row cap covered
much less than it appeared to.
Classify on the comment/string-blanked text instead, and accept `WITH`
as row-returning. The SQL handed to the server is never rewritten by the
classification, so an attribution comment survives verbatim. A
data-modifying CTE (`WITH x AS (DELETE ... RETURNING *) SELECT ...`) is
deliberately still not limited: a LIMIT would cap the rows handed back
while the write ran in full, which reads as a cap that isn't one. That
check reuses the read-only classifier's keyword heuristic, now shared as
`hasMutatingKeyword`.
Enabling CTEs exposed a second problem: the LIMIT/TOP helpers matched
the first clause found textually, which on a CTE is the *inner* one.
`WITH x AS (SELECT ... LIMIT 5) SELECT * FROM x JOIN big ON true` would
have had the CTE's own cap rewritten while the statement stayed
uncapped. Every helper now scans with parenthesis-depth tracking (the
mechanism already used for `hasSetOperator` and the ORDER BY hoist) and
reasons only about the statement's own clause; a nested LIMIT/TOP caps
only its branch, so the statement still gets a cap appended. This also
fixes the same pre-existing hole for plain subqueries
(`SELECT * FROM (SELECT * FROM t LIMIT 5) s`).
On SQL Server, TOP now lands on the statement's own SELECT rather than
the first one textually (for a CTE, the final SELECT), and a leading CTE
is kept outside the derived table when a set operation forces a wrap —
T-SQL has no `SELECT ... FROM (WITH ...) AS subq` form.
`SELECT ... UNION ALL SELECT ...` keeps its existing behaviour: it
starts with `select`, gets a trailing LIMIT, and on PostgreSQL that
binds to the whole set operation. Covered by a regression test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The MCP request's AbortSignal reached `execute_sql` (as `extra`) but was only ever read for telemetry. Nothing in `src/` referenced an AbortSignal, and `ExecuteOptions` had no field to carry one, so cancelling a tool call merely stopped us waiting for the answer: the backend went on producing a result set nobody would ever read. That is the same class of bug as bytebase#384 on MySQL — a query outliving the client that asked for it — and this mirrors the fix accepted for it in bytebase#386. MySQL's trigger is its own client-side timeout; here the trigger is the client cancelling. Both end the same way: kill the statement server-side over a second connection, and don't hand the connection back to the pool in a state the next borrower would inherit. It matters most exactly where it hurts most: on read replicas serving agents, where an abandoned analytical query keeps consuming IO for its full runtime and drives replication lag long after the agent has gone. Reproduction (integration test `Query cancellation (options.signal)`): start `SELECT pg_sleep(30)`, abort the signal, then watch `pg_stat_activity`. Before this change the test fails after 30,042ms with the query having *resolved* — the abort changed nothing and the backend slept the full 30s. After it, the statement rejects with SQLSTATE 57014 (query_canceled) within milliseconds and the backend is gone from `pg_stat_activity`. - `ExecuteOptions` gains an optional `signal`; `execute_sql` passes the request's own. Connectors that ignore it are unaffected. - PostgreSQL cancellation is out-of-band: the request must arrive on a different connection, since the one running the query is blocked waiting for it. `cancelBackend` opens a dedicated short-lived connection rather than borrowing from the pool — a pool saturated with the very queries being cancelled has nothing left to lend. `pg`'s own `Client.cancel` is not reusable here: it needs the internal Query object, which the promise-based query API never exposes. - The connection is destroyed instead of released after a cancellation. A CancelRequest races the statement it targets, so it can instead land on the ROLLBACK issued during cleanup and leave the session in an aborted transaction. Mirrors MySQL's `isConnectionPoisoned`. - The abort listener is removed in `finally`, so listeners don't accumulate across the statements of one request. - An already-aborted signal is refused before a connection is taken, and re-checked once one is held: an AbortSignal dispatches "abort" exactly once, so a signal that fired while we waited for the pool would never reach a listener registered afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Elrendio
added a commit
to Elrendio/dbhub
that referenced
this pull request
Aug 10, 2026
Integration branch: for_stockly_main carries every Stockly-facing change on top of upstream main, one merge per upstream PR, so each PR branch stays reviewable on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Split into one PR per bug, as they are independent and easier to review separately:
Each branch is now a single commit on |
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.
Two independent bugs in one PR because they were found together and share a test run. They are two clean commits — happy to split into
fix: apply max_rows to comment-prefixed and CTE queriesandfix(postgres): cancel the running query when the request is abortedif you prefer one concern per PR.Two independent bugs, both of which let a query keep consuming database resources that nobody
asked it to consume. We hit both running DBHub against ~24 production PostgreSQL read replicas
with agents driving
execute_sql; between them they contributed to replication lag of up to10.9 h and one production incident caused by a single agent query that ran for 1 h 30.
Each is its own commit and each stands alone.
1.
max_rowssilently does not apply to many queries —d3694d0SQLRowLimiter.isSelectQuerydecided whether to cap a statement with:so anything not opening with a bare
SELECTgot no LIMIT appended at all. Not a wronglimit — no limit, and nothing anywhere says so. Verified by executing the shipped v1.2.0
module (
applyMaxRows(sql, 100)):-- reporting job\nSELECT * FROM t/* tag: report */ SELECT * FROM tWITH x AS (SELECT * FROM orders) SELECT * FROM x(SELECT id FROM a) UNION (SELECT id FROM b)SELECT * FROM (SELECT * FROM t LIMIT 5) sSELECT * FROM tSELECT * FROM t\nLIMIT 100✅This is not an exotic corner. Leading comments are how queries get attributed
(
-- app=…,/* trace-id */), and a CTE is the ordinary shape of an analytical query — whichis exactly the kind of query a row cap exists to contain. In practice
max_rowscovered farless than it appeared to.
The existing suite pinned the gap rather than catching it: three integration tests were named
should not apply maxRows to CTE queries (WITH clause). This PR inverts them.To be clear about what those tests were pinning:
git log -Sshows thestartsWith('select')guard and those three tests both arrive in the same commit —
4fa886e feat: support max-row limit, the commit that introducedmax_rows. The CTE exclusion was never a separate, deliberatecarve-out; it is a consequence of that one-line classifier, and the tests recorded whatever it
happened to do. (One of the assertions is commented "not limited anymore", which reads as though a
decision had been reversed — the history shows there was no earlier behaviour to reverse.) If the
exclusion is wanted, it deserves to be an explicit rule rather than a side effect of the guard,
and this PR should be redirected to documenting it instead.
Fix. Classify on the comment/string-blanked text, and treat a leading
WITHasrow-returning. The SQL sent to the server is never rewritten by the classification, so an
attribution comment survives verbatim — only the classifier sees the blanked form.
A data-modifying CTE (
WITH x AS (DELETE … RETURNING *) SELECT …) is deliberately stillnot limited: a LIMIT would cap the rows handed back while the write ran in full, which reads
as a cap that isn't one. That check reuses the read-only classifier's own keyword heuristic,
extracted as
hasMutatingKeyword, so both layers agree on what counts as a write hidden insidea
WITH. A false positive there only means the statement keeps today's behaviour.Enabling CTEs exposed a second bug, which is why the diff is larger than "add
withto alist". Every LIMIT/TOP helper matched the first clause found textually, which on a CTE is the
inner one:
Naively enabling CTEs would have rewritten the CTE's own
LIMIT 5and left the statement — thepart that can return millions of rows — uncapped. So the helpers now scan with parenthesis-depth
tracking (the mechanism already in this file for
hasSetOperatorand the ORDER BY hoist) andreason only about the statement's own clause; a nested LIMIT/TOP caps only its branch, so the
statement still gets a cap appended. This also fixes the same pre-existing hole for plain
subqueries (
SELECT * FROM (SELECT * FROM t LIMIT 5) s, last row of the table above).On SQL Server,
TOPnow lands on the statement's ownSELECTrather than the first onetextually (for a CTE, the final
SELECT), and a leading CTE is kept outside the derived tablewhen a set operation forces the
#387wrap — T-SQL has noSELECT … FROM (WITH …) AS subqform.Explicitly preserved:
SELECT … UNION ALL SELECT …starts withselect, still gets atrailing
LIMIT, and on PostgreSQL that binds to the whole set operation. Pinned by aregression test.
2. MCP cancellation is ignored — the query keeps running —
db26b0bsrc/tools/execute-sql.tsreceivesextra, which carries the MCP SDK'sAbortSignal, and usesit only for
trackToolRequest(...). There was noAbortSignal/AbortControlleranywhere insrc/, andExecuteOptionshad no field to carry one. Cancelling a tool call therefore onlystopped us waiting for the answer: the backend went on producing a result set nobody would
ever read.
This is the same class of bug as #384 ("MySQL queries can continue running on the server
after query timeout") and this change mirrors the fix accepted for it in #386. MySQL's
trigger is its own client-side timeout; here the trigger is the client cancelling. Both end the
same way: kill the statement server-side over a second connection, and don't hand the connection
back to the pool in a state the next borrower inherits. PostgreSQL was simply left behind.
It matters most where it hurts most — read replicas serving agents, where an abandoned analytical
query keeps consuming IO for its full runtime and drives replication lag long after the agent has
given up and moved on.
Reproduction, as the added integration test
Query cancellation (options.signal): startSELECT pg_sleep(30), abort the signal, then watchpg_stat_activity.expected null to be an instance of Error— the query resolved. The abort changed nothing and the backend slept the full 30 s.
query_canceled) withinmilliseconds, and the backend is gone from
pg_stat_activity.Fix.
ExecuteOptionsgains an optionalsignal;execute_sqlpasses the request's own. Connectorsthat ignore it are unaffected — MySQL/MariaDB/SQLite/SQL Server behaviour is untouched.
connection, since the one running the query is blocked waiting for it.
cancelBackendopens adedicated short-lived connection rather than borrowing from the pool — a pool saturated with the
very long-running queries being cancelled has nothing left to lend, which is exactly the
situation this runs in.
pg's ownClient.cancel(client, query)is not reusable here: it needs the internalQueryobject (
client.activeQuery === query), which the promise-basedclient.query()API neverexposes.
pg_cancel_backend(pid)over a fresh connection is the same protocol-leveloperation, and matches how
killQuerywas done for MySQL in fix(mysql): kill server-side query on timeout instead of leaking it into the pool #386.the statement it targets, so it can instead land on the
ROLLBACKissued during cleanup andleave the session sitting in an aborted transaction. Mirrors MySQL's
isConnectionPoisoned.Covered by a test asserting the connector still works after a cancellation.
finally, so listeners don't accumulate across the statementsof one request.
held: an
AbortSignaldispatchesabortexactly once, so a signal that fired while we waitedfor the pool would never reach a listener registered afterwards.
Cancellation stays advisory — the backend acts on it at an interrupt point, and a statement that
already finished ignores it — so
cancelBackendis best-effort and logs rather than throwing.Testing
pnpm run test:unit— 983 passed (958 onmain; +25 new).pnpm run test:integration— 339 passed across PostgreSQL, MySQL, MariaDB, SQL Server andSQLite containers.
pnpm run build:backend— clean.the new tests fail after 30 s, as shown above.
New coverage: leading
--//* *//mixed comments; CTE; CTE with an inner LIMIT; CTE with its ownLIMIT; CTE with a parameterized LIMIT; data-modifying CTE (all three of INSERT/UPDATE/DELETE);
parenthesised set operation; nested-subquery LIMIT;
UNION ALLregression; SQL Server CTE/TOPcases; signal forwarding from the tool handler; and four PostgreSQL cancellation integration
tests (plain, inside
BEGIN READ ONLY, pool reuse after cancellation, already-aborted signal).Notes for reviewers
explain_sqlalso runs user SQL and could take the samesignal— one line — but is left outto keep this diff to the reported bugs. Happy to add it.
be a natural follow-up; MySQL already has the timeout-triggered half from fix(mysql): kill server-side query on timeout instead of leaking it into the pool #386.
npx tsc --noEmitreports 132 pre-existing errors onmain; this branch reports the same 132,none in the changed files.