Fix two libpq resource-handling bugs on COPY error paths - #555
Merged
Conversation
PostgresBinaryReader::FetchNextBuffer() leaked the PQgetCopyData buffer when the message was shorter than the tuple count it has to contain. PQgetCopyData allocates even when it hands back a length we cannot use, and buffer was only assigned after the check, so FreeBuffer() never saw it. Assign it first and let the existing ownership do the freeing. The three COPY error paths passed a possibly null PGresult to PQresultErrorMessage(). libpq returns an empty string rather than crashing, which is how "Failed to copy data: " with no reason after it gets produced. A null result means the connection failed, so report PQerrorMessage() in that case - the same pattern ExecuteQueries() and PostgresQueryBind() already use. Reported as Snowflake-Labs/pg_lake#415.
This was referenced Aug 21, 2026
staticlibs
approved these changes
Aug 21, 2026
staticlibs
left a comment
staticlibs
left a comment
Member
There was a problem hiding this comment.
Thanks for the PR! Looks good to me! I will add it to 1.5 branch shortly.
staticlibs
pushed a commit
that referenced
this pull request
Aug 21, 2026
This is a backport of the PR #555 to `v1.5-variegata` stable branch. PostgresBinaryReader::FetchNextBuffer() leaked the PQgetCopyData buffer when the message was shorter than the tuple count it has to contain. PQgetCopyData allocates even when it hands back a length we cannot use, and buffer was only assigned after the check, so FreeBuffer() never saw it. Assign it first and let the existing ownership do the freeing. The three COPY error paths passed a possibly null PGresult to PQresultErrorMessage(). libpq returns an empty string rather than crashing, which is how "Failed to copy data: " with no reason after it gets produced. A null result means the connection failed, so report PQerrorMessage() in that case - the same pattern ExecuteQueries() and PostgresQueryBind() already use. Reported as Snowflake-Labs/pg_lake#415.
sfc-gh-dachristensen
pushed a commit
to Snowflake-Labs/pg_lake
that referenced
this pull request
Aug 21, 2026
Four libpq resource-handling bugs reported in #415, patched against the pinned duckdb-postgres submodule. PGConnect() threw without calling PQfinish(). PQconnectdb allocates the PGconn even when the connection fails, so every failed postgres_scan or connection pool expansion leaked one. pgduck_server is long lived, so a retry loop against an unreachable source Postgres grows it without bound: 2000 failed scans took RSS from 78.8 MB to 116.5 MB, and the next 2000 added the same again. With the patch RSS is flat over 6000. ExecuteQueries() threw on the first bad result and left the rest of the libpq pipeline queued. The results themselves are freed when the connection closes, but the connection goes back to the pool still busy, and the next PQsendQuery on it fails with "another command is already in progress" instead of running. It now drains every result and throws the first error afterwards. LoadEntries() sends five concatenated statements over one connection, so it is the caller that hits this. PostgresBinaryReader::Next() leaked the PQgetCopyData buffer when the message was shorter than the tuple count it must contain. The buffer is now handed to the reader before the length is validated, so Reset() frees it on the way out. The three COPY error paths passed a possibly null PGresult to PQresultErrorMessage(). Current libpq returns an empty string rather than crashing, which is how "Failed to copy data: " with no reason gets produced - a null result means the connection died, so they now report PQerrorMessage() instead. The first two are already fixed upstream and this patch can be dropped once the submodule pin moves past them. The other two are upstream in duckdb/duckdb-postgres#555. Signed-off-by: Marco Slot <marco.slot@snowflake.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Two libpq resource-handling bugs on the COPY paths, found while looking into Snowflake-Labs/pg_lake#415.
PostgresBinaryReader::FetchNextBuffer()leaks thePQgetCopyDatabuffer when the message is shorter than the 2-byte tuple count it has to contain.PQgetCopyDataallocates even when it returns a length we cannot use, andbufferwas only assigned after the length check, soFreeBuffer()never saw it. Assigning it first hands it to the existing ownership, which frees it on the way out of the throw.The three COPY error paths pass a possibly null
PGresulttoPQresultErrorMessage(). That does not crash - libpq returns an empty string - but it is howFailed to copy data:with nothing after it gets produced, and a null result there means the connection failed, so the reason is on the connection.ExecuteQueries()andPostgresQueryBind()already do theresult ? PQresultErrorMessage(result) : PQerrorMessage(conn)thing, so this just makes the COPY paths match.The same report also covered a missing
PQfinish()inPGConnect()and an undrained pipeline inExecuteQueries(); both are already fixed on main, so they are not in this PR. For what it is worth, thePQfinish()one measured at about 19 kB per failed connection attempt in a long-lived process, so it was worth having.No test - neither path is reachable from a well-behaved server.