GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Read COPY data from the connector's current read buffer by Brar · Pull Request #6712 · npgsql/npgsql

Conversation

@Brar

@Brar Brar commented Oct 25, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

A backend message longer than the read buffer (e.g. a notice over 8 KB) makes the connector switch to a bigger buffer. NpgsqlRawCopyStream, which BeginTextExport also reads through, and NpgsqlBinaryExporter kept the buffer they found when they were created. After such a message they read the following data from the old buffer, which the switch had emptied, i.e. directly from the socket, bypassing what the connector had already received into the new buffer. From there they misread the stream, or waited for data that had already arrived. If an earlier message had already made the connector switch, the next switch returned that bigger buffer's array to the ArrayPool, and the reader went on reading socket data into it, even after the pool had handed it out again (shown with a mock probe).

The readers now take the connector's current buffer whenever they read a CopyData message, like the data reader does for each row. The raw stream also takes it when it reads the response to the COPY command: after an oversize message before that response, its ReadTimeout (which the text reader's and writer's Timeout set) went to the old buffer, so asynchronous reads ignored it. #6721 fixes the same pattern in the pgoutput decoder.

How a real server gets there

Notices may come between CopyData messages (protocol.sgml:1341-1345), and PostgreSQL flushes each one at once (elog.c:3695; references to REL_18_STABLE). Sources of a notice longer than the read buffer during COPY ... TO STDOUT, each run live:

  • a PL/pgSQL RAISE NOTICE of a large value in a function the COPY query calls;
  • client_min_messages = log with debug_print_plan = on, both user-settable, which print an ordinary join's plan of 11 to 13 KB (postgres.c:954): at a row when a function plans an SPI query there, or before CopyOutResponse for the COPY query itself;
  • an IMMUTABLE function with constant arguments, which the planner runs before CopyOutResponse.

The binary header goes out inside the first row's CopyData, so a notice raised while the first row is computed lands between CopyOutResponse and the header. These triggers are realistic but uncommon; with the default Command Timeout the failures below take 30 s, and with 0 they never end.

Live on 18.6 with Command Timeout 5 s, COPY (SELECT f(i) FROM generate_series(1, 3) i) TO STDOUT with f raising a 20000-character notice for each row: without this change, BeginRawBinaryCopy returned 29 bytes of a notice's text as COPY data and then failed with "Unknown message code: 80", BeginBinaryExport failed with "Invalid COPY binary signature at beginning!", and BeginTextExport timed out and left the connection broken. With it, all three read the same data as with a 10-character notice.

The tests' scenarios, live on 18.6 and 14.24 with Command Timeout 5 s:

  • A raw binary export with a 20 KB notice at row 2, or the debug_print_plan output: main returned the header and row 1, then timed out and broke the connection; with this change, all 51 bytes.
  • A binary export with a notice before the header or before a later row: main read no row, or only row 1, and timed out (with Command Timeout 0 it was still waiting after 10 s); with this change, every row.
  • The raw export's ReadTimeout after a notice before CopyOutResponse (debug_print_plan, ReadTimeout = 500 ms, a 3-s stall at the first row): main timed out after 3.5 s, this change after 0.5 s. Only async reads are affected; sync reads use the socket's timeout. The import's variant, with a long notice before CopyInResponse (e.g. raised by an IMMUTABLE function in the default of a column the COPY doesn't list), behaves the same, but that trigger is contrived.

Tests

Three mock tests in CopyTests with 10 cases, each oversize case with a 10-character-notice control; they use the smallest read buffer (4096 bytes), so that the mock can write an oversize notice in one piece:

  • Raw_binary_export_with_notice_between_CopyData_messages;
  • Raw_binary_copy_timeout_with_notice_before_copy_response (export and import);
  • Binary_export_with_notice_before_row (before the header, or before a later row).

Red first: without the source change, the 5 oversize cases fail in Debug and Release (NpgsqlException "Exception while reading from stream" at the reads on the old buffer, or the timeout assertion), and the 5 controls pass. The read cases fail the same way with main's COPY code. Removing any one of the four lookups fails exactly its own cases.

For the reviewer

  • A core change in NpgsqlRawCopyStream and NpgsqlBinaryExporter, with no API change. The buffer is looked up per message, not per use.
  • The exporter's Init doesn't take the buffer: its only effect would be on the error path that Fix NpgsqlBinaryExporter's dispose after an error or a connection break #6713 fixes.
  • Imports aren't affected otherwise: the import stream and NpgsqlBinaryImporter read only messages, through the connector.
  • The bug is on main and, by reading, on the 10.0 hotfix branch. Backport: planned for it.

Dependencies and merge order

  • It depends on no other PR.
  • Conflicts textually with Fix NpgsqlBinaryExporter's dispose after an error or a connection break #6713, in NpgsqlBinaryExporter (the ReadHeader and StartRow read lines) and in CopyTests.cs (new tests at the same place). Merge this PR first; Fix NpgsqlBinaryExporter's dispose after an error or a connection break #6713 is then rebased keeping both changes: its read line followed by this PR's two lines, and both test blocks.

Fixes #6702

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Brar
Brar requested review from roji and vonzshik as code owners October 25, 2026 21:39
This was referenced Oct 25, 2026
Fix NpgsqlBinaryExporter's dispose after an error or a connection break #6713
Open
Fix NpgsqlRawCopyStream's dispose and later reads after an export error #6711
Open
Add copy-both and replication support to the test server mock #6689
Merged
@Brar @claude
Read COPY data from the connector's current read buffer
fc71698
A backend message bigger than the read buffer (e.g. a notice longer
than 8 KB) makes the connector switch to a bigger buffer.
NpgsqlRawCopyStream, which BeginTextExport also reads through, and
NpgsqlBinaryExporter kept the buffer they found when they were
created. After such a message they read the following data from the
old buffer, which the switch had emptied, i.e. directly from the
socket, bypassing what the connector had already received into the
new buffer. From there they misread the stream, or waited for data
that had already arrived. With a function that raises a long notice
for each row of COPY (SELECT ...) TO STDOUT, the raw stream returned
part of a notice as COPY data and then failed with an unknown message
code, the binary exporter failed with "Invalid COPY binary signature",
and the text export timed out. If an earlier message had already made
the connector switch, the next switch returned that bigger buffer's
array to the ArrayPool, and the reader went on reading socket data into
it, even after the pool had handed it out again.

The readers now take the connector's current buffer whenever they
read a CopyData message, like the data reader does for each row. The
raw stream also takes it when it reads the response to the COPY
command: after such a message before that response, its ReadTimeout
(which the text reader's and writer's Timeout set) went to the old
buffer, so asynchronous reads ignored it. The pgoutput decoder had the
same problem ("Read pgoutput messages from the connector's current
read buffer").

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@Brar
Brar force-pushed the core/copy-read-buffer branch from 2933ff0 to fc71698 Compare November 1, 2026 13:38
Copilot AI balanced review requested due to automatic review settings November 1, 2026 13:38
Copilot started reviewing on behalf of Brar November 1, 2026 13:40
View session
Copilot AI reviewed Nov 1, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues are blocking the COPY read-buffer refresh changes.

No open issues 🎉

What changed in this PR

Fixes #6702 by making COPY readers refresh their cached read buffer after messages that may cause the connector to replace it. This prevents stale-buffer reads and preserves timeout handling without changing public APIs.

Changes:

  • Refresh the raw COPY stream’s buffer after the COPY response and at each CopyData message.
  • Refresh the binary exporter’s buffer before reading the header and subsequent rows.
  • Add 10 mock test cases covering oversized notices, short-notice controls, and timeout propagation.
File Description
test/​Npgsql.Tests/​CopyTests.cs Adds regression tests for buffer switches during COPY and before COPY responses.
src/​Npgsql/​NpgsqlRawCopyStream.cs Refreshes the buffer for COPY data reads and timeout handling.
src/​Npgsql/​NpgsqlBinaryExporter.cs Refreshes the buffer before reading binary headers and rows.

Review effort: Balanced

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
vonzshik
vonzshik reviewed Nov 7, 2026
View reviewed changes
Comment thread test/Npgsql.Tests/CopyTests.cs
.WriteReadyForQuery()
.FlushAsync();
// Reading beyond the end of the response fails instead of waiting
server.Close();

@vonzshik vonzshik Nov 7, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do we really need to explicitly close the server here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Brar Brar Nov 7, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Only when the fix breaks, but then it's what makes the test fail rather than hang.
Without the fix, the reads that go to the stale buffer wait for data that has already arrived, and the tests run with Command Timeout=0: on CI that lasts until --blame-hang-timeout kills the test host after 30 s, which aborts the rest of the run, and a local run without the blame flags never ends. With the close, those reads fail at once with "Exception while reading from stream".
Passing runs don't notice it, since the mock closes only after writing the whole response.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Brar Brar Nov 7, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I played around with it a bit more and looked into where exactly it gets stuck. It isn't waiting for a message that the mock doesn't send: the mock sends the whole response up to ReadyForQuery, and without the fix the reads wait on the socket for data that would come after it.

The long notice makes the connector switch to a bigger buffer, which takes over the unread bytes and leaves the old one empty (NpgsqlConnector.cs:1537-1544, NpgsqlReadBuffer.cs:403-404). Without the fix, the raw stream and the binary exporter keep the old buffer (NpgsqlRawCopyStream.cs:73, NpgsqlBinaryExporter.cs:62), find it empty and read the socket (NpgsqlReadBuffer.cs:300-302), while what they need is already in the connector's new buffer:

Test case Waits in Already in the connector's buffer
Binary_export_with_notice_before_row(True,True) BeginBinaryExportAsync, reading the header (NpgsqlBinaryExporter.cs:124) 72 bytes, from the PGCOPY\n\377\r\n\0 signature on
Binary_export_with_notice_before_row(False,True) StartRowAsync for the second row (NpgsqlBinaryExporter.cs:188) the second row, the trailer, CopyDone, CommandComplete, ReadyForQuery
Raw_binary_export_with_notice_between_CopyData_messages(True) ReadAsync (NpgsqlRawCopyStream.cs:317) def, CopyDone, CommandComplete, ReadyForQuery

The tests run with Command Timeout=0, so that read never times out. A real server normally doesn't send anything after ReadyForQuery either until the next query, so there the read waits until the command timeout breaks the connection.

I checked this on this branch with the fix switched off, logging every read from a buffer that isn't the connector's current one, and tried what else the mock could do after the response:

After the response, the mock … Without the fix
closes the connection right away (as in the PR) the three cases fail within 50 ms
closes it after 5 s each read waits until the close
sends a ParameterStatus, then closes after 5 s it lands in the connector's buffer too, and the reads still wait until the close
sends the ParameterStatus 2 s later, while the reads wait, then closes after 5 s the reads take it for COPY data: the binary export fails with "Invalid COPY binary signature at beginning!", and the raw stream returns it as data, then takes the d of def and the next four bytes for the header of a 1.7 GB CopyData message and waits again; each case still runs until the close
closes after 5 s, and the test sets Command Timeout=1 the reads fail after 1 s, the second row's after 3 s, since StartRow first tries a cancel and then waits for the 2 s Cancellation Timeout

With the fix, all six cases of the two tests pass in every variant. So nothing the mock sends ends the wait cleanly; only closing the connection does, and a passing run doesn't notice the close, since the client never reads past ReadyForQuery.

Most tests don't need this, because when they break, they fail on bytes that did arrive, while these reads go past them to the socket. Where main closes the mock's connection, it either breaks the connection on purpose (CommandTests.cs:1430, CopyTests.cs:1342, MultipleHostsTests.cs:685, ReaderTests.cs:1322) or, in the newer mock tests, ends a read that still waits, in a finally (PgServerMockTests.cs:40, ReplicationMockTests.cs:95, ReplicationCommandMockTests.cs:128). The close here does the latter, only unconditionally.

If it helps, I can make the comment say where the reads wait.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/Npgsql.Tests/CopyTests.cs

const string command = "COPY SomeTable TO STDOUT (FORMAT BINARY)";
var streamTask = conn.BeginRawBinaryCopyAsync(command);
await server.ExpectSimpleQuery(command).WaitAsync(TimeSpan.FromSeconds(10));

@vonzshik vonzshik Nov 7, 2026

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I would probably just remove WaitAsync here and below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Brar Brar Nov 7, 2026

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

As long as everything works as intended, they are superfluous, yes.
On the other hand they are a cheap insurance against regressions outside what these tests check.
If a change ever stops the client from sending the query, e.g. by throwing before the write, ExpectSimpleQuery waits forever, and with WaitAsync the test fails after 10 s instead of hanging until CI's blame timeout kills the test host. That's why the mock tests in my other PRs have them, as do the ones from #6689 and #6706 already on main (e.g. the test right above these). But if you'd rather not have them, I'll drop them.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vonzshik vonzshik vonzshik left review comments
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@roji roji Awaiting requested review from roji roji is a code owner

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

COPY TO STDOUT misreads data after a notice longer than the read buffer

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@Brar @vonzshik
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.