Skip to content

Fix NpgsqlRawCopyStream's dispose and later reads after an export error - #6711

Open
Brar wants to merge 1 commit into
npgsql:mainfrom
Brar:core/raw-copy-error-dispose
Open

Brar wants to merge 1 commit into
npgsql:mainfrom
Brar:core/raw-copy-error-dispose

Conversation

@Brar

@Brar Brar commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

When the server reported an error after CopyDone instead of CommandComplete (from an AFTER trigger such as a foreign key check, from a deferred constraint at commit, or from a cancellation while these run), the raw COPY stream threw it, but it read CommandComplete and ReadyForQuery outside the cleanup that a failed read of a CopyData message gets. The stream stayed ready, although the connector had already read up to the ReadyForQuery, so its dispose, which closing the connection runs too, waited for another ReadyForQuery, and so did a later read: until the command timeout broke the connection (which also cleared the data source's pool, unless Cancellation Timeout is -1), or forever without one. An error from a second statement in the command, instead of ReadyForQuery, did the same. BeginTextExport reads through the raw stream, so its reader had the same problem.

These reads now end the export as a failed read of a CopyData message does: the stream releases the connection at once, so its dispose neither skips nor waits, and the COPY activity records the error instead of the timeout. After an error ended an export at a read (Cancel makes the server send one too), later calls until the dispose throw InvalidOperationException "The COPY operation ended with an error." with the error as the inner exception: on the raw stream Read, ReadAsync, ReadByte, Write, Flush and Cancel, on the text reader its reads and Cancel. Before, they threw ObjectDisposedException without the error, or waited as above. After the dispose they throw ObjectDisposedException, as before. The stream no longer logs that the COPY operation completed after such an error, nor when BeginRawBinaryCopy, BeginTextExport or BeginTextImport fails before the COPY starts, e.g. when the server answers the command with an error for a missing table.

How a real server gets there

PostgreSQL sends CopyDone before EndCopyTo runs ExecutorFinish, which fires the queued AFTER triggers (copyto.c:1118 and :1015, copy.c:358-359, execMain.c:443), and it commits before it reports command completion (postgres.c:1284-1298 and :1337; all REL_18_STABLE).

CREATE TEMP TABLE parent (id int PRIMARY KEY);
CREATE TEMP TABLE child (id int REFERENCES parent);
BEGIN;
COPY (INSERT INTO child VALUES (1) RETURNING id) TO STDOUT;  -- BeginTextExport; BeginRawBinaryCopy with (FORMAT BINARY)

Live on PostgreSQL 18.6, Debug and Release, sync and async, Command Timeout 5 s:

  • Without this change: the read threw 23503 at once; the dispose then waited 5 s, the connection was broken, and the pool's idle connection was gone. The same with a deferred constraint at commit, a plain AFTER ROW trigger, statement_timeout during a statement-level AFTER trigger and a failing second statement, for BeginRawBinaryCopy and BeginTextExport alike.
  • With this change: the read throws the original at once, later calls throw the InvalidOperationException until the dispose, the dispose returns at once, the connection serves ROLLBACK, RESET and SELECT 1, and the pool's idle connection is reused. Also for an error at a row and for Cancel during a long export. The COPY activity records the 23503 instead of the dispose's timeout.
  • Begin failures: a missing table (42P01), a bad column or option, an aborted transaction (25P02), a COPY to or from a file on the server, and pg_terminate_backend during a lock wait no longer log "completed". The exception, the activity and the connection are unchanged.

Tests

  • CopyTests.Raw_export_error, 40 mock cases: raw binary and text export; an error before the first row, before the second, after CopyDone, after CommandComplete, or none; with and without a later read; sync and async.
  • TracingTests.RawBinaryExport_error_after_CopyDone (2 cases), the only test of the catch's TraceSetException.
  • CopyTests.Raw_copy_error_at_start, 16 cases: all four APIs, sync and async, with an ErrorResponse 42P01 instead of the COPY response, or as a control that disposes right after a successful Begin.
  • Red first, against the stream before this change: 34 of the first 42 cases fail, and the 8 no-error controls among them pass, in 3 of 3 runs each in Debug and Release. Of Raw_copy_error_at_start, exactly the 8 error cases fail without the new log condition. Seven mutations of the change were each caught by exactly the expected cases.

For the reviewer

Dependencies and merge order

Fixes #6704

When the server reported an error after CopyDone instead of
CommandComplete (from an AFTER trigger such as a foreign key check, from
a deferred constraint at commit, or from a cancellation while these
run), the raw COPY stream threw it, but it read CommandComplete and
ReadyForQuery outside the cleanup that a failed read of a CopyData
message gets. The stream stayed ready, although the connector had
already read up to the ReadyForQuery, so its dispose, which closing the
connection runs too, waited for another ReadyForQuery, and a later read
for another message: until the command timeout broke the connection
(which also cleared the pool, unless Cancellation Timeout is -1), or
forever without one. An error from a second statement in the command,
instead of ReadyForQuery, did the same. BeginTextExport reads through
the raw stream, so its reader had the same problem.

These reads now end the export as a failed read of a CopyData message
does: the stream releases the connection at once, so its dispose
neither skips nor waits, and the COPY activity records the error
instead of the timeout. After an error ended an export at a read
(Cancel makes the server send one too), later calls until the dispose
throw InvalidOperationException with the error as the inner exception
instead of ObjectDisposedException, and the stream no longer logs that
the COPY operation completed. Nor does it when BeginRawBinaryCopy,
BeginTextExport or BeginTextImport fails before the COPY starts, e.g.
when the server answers the command with an error for a missing table.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Brar
Brar force-pushed the core/raw-copy-error-dispose branch from 9d0d6cc to 3c65a6c Compare November 1, 2026 13:38
Copilot AI balanced review requested due to automatic review settings November 1, 2026 13:38

Copilot AI left a comment

Copy link
Copy Markdown

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 remain for the COPY and text-export error-handling changes.

No open issues 🎉

What changed in this PR

Fixes raw COPY and text export error handling so failures after CopyDone release the connection promptly instead of causing later reads or disposal to wait and potentially break it.

Changes:

  • Handles completion-message errors through the existing cleanup path and preserves the original error for subsequent calls until disposal.
  • Suppresses completion logs when COPY fails during initialization or an export read.
  • Adds regression tests for error timing, subsequent reads, disposal, connection reuse, logging, and tracing.
File Description
test/​Npgsql.Tests/​TracingTests.cs Verifies tracing records the original export error after CopyDone.
test/​Npgsql.Tests/​CopyTests.cs Adds raw and text COPY error cases and successful controls.
src/​Npgsql/​NpgsqlRawCopyStream.cs Extends error cleanup and preserves failure details until disposal.

Review effort: Balanced

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

Labels

None yet

Projects

None yet

2 participants