Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PQputCopyData's return value 0 should be considered fail #7152

Merged
merged 1 commit into from
Aug 29, 2023
Merged

PQputCopyData's return value 0 should be considered fail #7152

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/backend/distributed/connection/remote_commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,14 +716,14 @@ PutRemoteCopyData(MultiConnection *connection, const char *buffer, int nbytes)
Assert(PQisnonblocking(pgConn));

int copyState = PQputCopyData(pgConn, buffer, nbytes);
if (copyState == -1)
if (copyState <= 0)
Copy link
Member

Choose a reason for hiding this comment

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

From docs:

The result is 1 if the data was queued, zero if it was not queued because of full buffers (this will only happen in nonblocking mode),

this does not seem like a failure

however, the current code does seem suspect

in practice, we might always end up in FinishConnectionIO below (?), but maybe we should force it to go in there if copyState == 0

Copy link
Member

Choose a reason for hiding this comment

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

hm, ok, on inspecting the code, the 0 seems to happen only case of out of memory.

so there's a choice here between failing immediately (current PR) or trying a FinishConnectionIO, but current code sems risky

Copy link
Contributor Author

@zhjwpku zhjwpku Aug 29, 2023 •
edited

Choose a reason for hiding this comment

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

From docs:

The result is 1 if the data was queued, zero if it was not queued because of full buffers (this will only happen in nonblocking mode),

this does not seem like a failure

however, the current code does seem suspect

in practice, we might always end up in FinishConnectionIO below (?), but maybe we should force it to go in there if copyState == 0

IMHO FinishConnectionIO may not help here, it tries to send all pending data(which is hold in conn->outBuffer) for the connection, but when copyState == 0 the data PQputCopyData try to send even not added to conn->outBuffer, we should either treat this as a failure or try PQputCopyData again, but should not treat this as a success.

I checked postgres upstream, all PQputCopyData are used with <= 0, the result 0 is returned when failed to enlarge conn->outBuffer.

{
return false;
}

/*
* PQputCopyData may have queued up part of the data even if it managed
* to send some of it succesfully. We provide back pressure by waiting
* to send some of it successfully. We provide back pressure by waiting
* until the socket is writable to prevent the internal libpq buffers
* from growing excessively.
*
Expand Down