GeezerVison is showing github.com — Read COPY data from the connector's current read buffer by Brar · Pull Request #6712 · npgsql/npgsql
Conversation
Brar
commented
Oct 25, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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
Fixes #6702
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Oct 25, 2026
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]>
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.
|
|
.WriteReadyForQuery()
|
|
|
.FlushAsync();
|
|
|
// Reading beyond the end of the response fails instead of waiting
|
|
|
server.Close();
|
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.
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.
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.
|
|
|
|
|
const string command = "COPY SomeTable TO STDOUT (FORMAT BINARY)";
|
|
|
var streamTask = conn.BeginRawBinaryCopyAsync(command);
|
|
|
await server.ExpectSimpleQuery(command).WaitAsync(TimeSpan.FromSeconds(10));
|
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.
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.
This branch has not been deployed
No deployments
Reviewers
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
Milestone
No milestone
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.
|