Skip to content

fix: sync rows query after error - #3759

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

fix: sync rows query after error#3759
foredstudio wants to merge 1 commit into
brianc:masterfrom
foredstudio:foredstudio/fix-rows-error-sync

Conversation

@foredstudio

Copy link
Copy Markdown

Fixes #3707.

When a query uses the rows option, _getRows sends Flush instead of pipelining Sync. If PostgreSQL returns an ErrorResponse, it ignores subsequent messages until a Sync arrives, so the client can remain permanently stuck waiting for ReadyForQuery.

This sends Sync from handleError only for rows-mode queries, matching the existing rows-mode handling in handleCommandComplete and handleEmptyQuery. Normal queries keep their existing behavior.

Tests added for both rows-mode and normal-query error paths.

Validation:

  • node test/unit/client/query-error-sync-tests.js
  • make test-unit
  • eslint packages/pg/lib/query.js packages/pg/test/unit/client/query-error-sync-tests.js

@foredstudio

Copy link
Copy Markdown
Author

CI is green across the full matrix. This is ready for review when convenient; the change is limited to sending Sync after rows-mode query errors, with regression coverage for rows-mode and normal-query error paths.

@brianc brianc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for this! I'm out of town for another week so I'll be slow on responding but this looks reasonable. I have a question tho before merging this - is there a way to write an integration test that could also replicate the error? I assume issuing an invalid query with rows: 1 on the config could trigger the backend to send an error response and trip the previously offending code path? Unit tests are always nice, but in this case due to the interplay of backend and frontend I'd love to see an integration test covering the behavior. Is that something you'd be willing to include? 🙏

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

2 participants