PQputCopyData's return value 0 should be considered fail #7152
Conversation
PQputCopyData returns 0 when it failed to expand the output buffer in nonblock mode. So 0 should be treating as fail. Signed-off-by: Zhao Junwang <zhjwpku@gmail.com>
zhjwpku
force-pushed
the
PutRemoteCopyData_wrong_logic
branch
from
bd18932
to
6f4c8ce
Compare
Codecov Report
@@ Coverage Diff @@
## main #7152 +/- ##
==========================================
+ Coverage 93.40% 93.41% +0.01%
==========================================
Files 272 272
Lines 58989 58991 +2
==========================================
+ Hits 55101 55109 +8
+ Misses 3888 3882 -6 |
| @@ -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) | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
PQputCopyData returns 0 when it failed to expand the output buffer in
nonblock mode. So 0 should be treating as fail.
DESCRIPTION: Fixes a bug that could cause COPY logic to skip data in case of OOM