Skip to content

fix(pg): keep the connection usable after an error in a rows-limited query - #3825

Open
official-burak wants to merge 1 commit into
brianc:masterfrom
official-burak:fix/rows-mode-error-sync
Open

official-burak wants to merge 1 commit into
brianc:masterfrom
official-burak:fix/rows-mode-error-sync

Conversation

@official-burak

Copy link
Copy Markdown

What

Fixes #3707 — an error during a query that runs with the rows option permanently wedges the connection. The query rejects correctly, but ReadyForQuery never comes back and every later query on that client queues forever.

Why it happens

With rows set, Query.prepare() drives the extended protocol with Flush, not Sync: _getRows() sends Execute(rows=N) + connection.flush(), and again on every PortalSuspended. The terminating Sync is only sent from handleCommandComplete / handleEmptyQuery. When the server answers with an ErrorResponse instead, it enters ignore-till-sync — it discards everything until a Sync arrives and withholds ReadyForQuery. handleError sent nothing, so the RFQ never arrives and client.readyForQuery stays false.

The // need to sync after error during a prepared statement comment that sat at the top of handleError was a fossil of the pre-pipelining code, which did sync there — the Oct 2020 redesign (d31486f / dd3ce61) moved Sync into _getRows() and dropped the error-path call, correct for the normal prepared path (Sync already pipelined) but not for rows-mode. pg-cursor kept its handleError sync for exactly this reason, which is why cursors recover from errors and the core rows path does not.

The fix

Track the one Sync a rows-limited portal owes, and let whichever handler observes the end of the extended-protocol sequence send it:

  • _getRows() sets _syncOwed in the rows branch (Flush instead of Sync)
  • _sendOwedSync() sends it and clears the flag — called from handleCommandComplete, handleEmptyQuery and now handleError

The flag makes the send exactly once. That matters in two spots where an error is preceded by a CommandComplete:

  • a timed-out query on some backends sends both CommandComplete and ErrorResponse (the case 9c678e1 originally guarded) — handleCommandComplete already synced, so handleError must not send a second
  • a row that throws while being parsed is surfaced from handleReadyForQuery after CommandComplete already synced — same thing

A second Sync would produce a second ReadyForQuery, letting the next queued query settle on the first frame mid-flight — the same class of bug #2420 fixed in pg-cursor.

Two side effects worth calling out, both intentional:

  • queries whose values fail to serialize in prepare() are unaffected: that path already sends Close + Sync before calling handleError, and since _getRows() never ran, _syncOwed is still false — no double send
  • a connection-level error with a rows query in flight now attempts the Sync too, mirroring pg-cursor; Connection._send() no-ops on a non-writable stream so a dead socket is safe

Canceling a rows-limited query (client.cancel() via a second client) also lands on this path — the ErrorResponse for the cancel now gets its Sync, so the client stays usable after a canceled paged query as well.

Tests

Three integration tests in query-error-handling-prepared-statement-tests.js (skipped in native mode — the fix is in the JS Query):

  • mid-portal error: division by zero on row 5 of 7, pages of 2 — the exact reproduction from the issue; asserts the rejection, that a follow-up query resolves on the same client, and that exactly one ReadyForQuery was emitted for the failed sequence
  • error before any row: multi-statement text rejected at Parse with rows set — the same wedge through a different entry point
  • row-parse error: a throwing type parser cancels the query client-side; asserts the error surfaces, the follow-up works and only one Sync went out — pins the double-send guard

Verified locally against PostgreSQL 16: the new tests hang on master (suite timeout kills them — the wedge) and pass with the fix; the full pg unit and integration suites show no regressions (the only local failures — password-callback-tests, ssl-tests, gh-issues/2085 — fail identically on master in this environment).

…query

With the rows option, Query drives the extended protocol with Flush
instead of Sync (_getRows) and only handleCommandComplete /
handleEmptyQuery send the terminating Sync. When the server answers
with an ErrorResponse instead, it discards messages until a Sync
arrives and withholds ReadyForQuery - handleError sent nothing, so
readyForQuery never returned and every later query on the connection
queued forever.

Restore the error-path Sync by tracking the one Sync a rows-limited
portal owes, sent by whichever of handleCommandComplete,
handleEmptyQuery or handleError observes the end of the sequence.
The flag also keeps exactly one Sync from going out when an
ErrorResponse follows a CommandComplete (a timed-out query on some
backends) or when a row-parse error is surfaced on ReadyForQuery,
where a second Sync would produce a second ReadyForQuery and let a
later query settle mid-flight.

Fixes brianc#3707
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error during a rows-limited query permanently wedges the connection — handleError sends no Sync

1 participant