8000 Clean up error cases in psql's COPY TO STDOUT/FROM STDIN code. · xiaom/postgres@2573f08 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2573f08

Browse files
tglsfdctglsfdc
committed
Clean up error cases in psql's COPY TO STDOUT/FROM STDIN code.
Adjust handleCopyOut() to stop trying to write data once it's failed one time. For typical cases such as out-of-disk-space or broken-pipe, additional attempts aren't going to do anything but waste time, and in any case clean truncation of the output seems like a better behavior than randomly dropping blocks in the middle. Also remove dubious (and misleadingly documented) attempt to force our way out of COPY_OUT state if libpq didn't do that. If we did have a situation like that, it'd be a bug in libpq and would be better fixed there, IMO. We can hope that commit fa4440f took care of any such problems, anyway. Also fix longstanding bug in handleCopyIn(): PQputCopyEnd() only supports a non-null errormsg parameter in protocol version 3, and will actively fail if one is passed in version 2. This would've made our attempts to get out of COPY_IN state after a failure into infinite loops when talking to pre-7.4 servers. Back-patch the COPY_OUT state change business back to 9.2 where it was introduced, and the other two fixes into all supported branches.
1 parent 8439ee4 commit 2573f08

File tree

1 file changed

+37
-38
lines changed

1 file changed

+37
-38
lines changed

‎src/bin/psql/copy.c

Lines changed: 37 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -363,15 +363,15 @@ handleCopyOut(PGconn *conn, FILE *copystream)
363363
ret = PQgetCopyData(conn, &buf, 0);
364364

365365
if (ret < 0)
366-
break; /* done or error */
366+
break; /* done or server/connection error */
367367

368368
if (buf)
369369
{
370-
if (fwrite(buf, 1, ret, copystream) != ret)
370+
if (OK && fwrite(buf, 1, ret, copystream) != ret)
371371
{
372-
if (OK) /* complain only once, keep reading data */
373-
psql_error("could not write COPY data: %s\n",
374-
strerror(errno));
372+
psql_error("could not write COPY data: %s\n",
373+
strerror(errno));
374+
/* complain only once, keep reading data from server */
375375
OK = false;
376376
}
377377
PQfreemem(buf);
@@ -392,29 +392,18 @@ handleCopyOut(PGconn *conn, FILE *copystream)
392392
}
393393

394394
/*
395-
* Check command status and return to normal libpq state. After a
396-
* client-side error, the server will remain ready to deliver data. The
397-
* cleanest thing is to fully drain and discard that data. If the
398-
* client-side error happened early in a large file, this takes a long
399-
* time. Instead, take advantage of the fact that PQexec() will silently
400-
* end any ongoing PGRES_COPY_OUT state. This does cause us to lose the
401-
* results of any commands following the COPY in a single command string.
402-
* It also only works for protocol version 3. XXX should we clean up
403-
* using the slow way when the connection is using protocol version 2?
395+
* Check command status and return to normal libpq state.
404396
*
405-
* We must not ever return with the status still PGRES_COPY_OUT. Our
406-
* caller is unable to distinguish that situation from reaching the next
407-
* COPY in a command string that happened to contain two consecutive COPY
408-
* TO STDOUT commands. We trust that no condition can make PQexec() fail
409-
* indefinitely while retaining status PGRES_COPY_OUT.
397+
* If for some reason libpq is still reporting PGRES_COPY_OUT state, we
398+
* would like to forcibly exit that state, since our caller would be
399+
* unable to distinguish that situation from reaching the next COPY in a
400+
* command string that happened to contain two consecutive COPY TO STDOUT
401+
* commands. However, libpq provides no API for doing that, and in
402+
* principle it's a libpq bug anyway if PQgetCopyData() returns -1 or -2
403+
* but hasn't exited COPY_OUT state internally. So we ignore the
404+
* possibility here.
410405
*/
411-
while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_OUT)
412-
{
413-
OK = false;
414-
PQclear(res);
415-
416-
PQexec(conn, "-- clear PGRES_COPY_OUT state");
417-
}
406+
res = PQgetResult(conn);
418407
if (PQresultStatus(res) != PGRES_COMMAND_OK)
419408
{
420409
psql_error("%s", PQerrorMessage(conn));
@@ -457,7 +446,9 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
457446
/* got here with longjmp */
458447

459448
/* Terminate data transfer */
460-
PQputCopyEnd(conn, _("canceled by user"));
449+
PQputCopyEnd(conn,
450+
(PQprotocolVersion(conn) < 3) ? NULL :
451+
_("canceled by user"));
461452

462453
OK = false;
463454
goto copyin_cleanup;
@@ -578,29 +569,37 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary)
578569
if (ferror(copystream))
579570
OK = false;
580571

581-
/* Terminate data transfer */
572+
/*
573+
* Terminate data transfer. We can't send an error message if we're using
574+
* protocol version 2.
575+
*/
582576
if (PQputCopyEnd(conn,
583-
OK ? NULL : _("aborted because of read failure")) <= 0)
577+
(OK || PQprotocolVersion(conn) < 3) ? NULL :
578+
_("aborted because of read failure")) <= 0)
584579
OK = false;
585580

586581
copyin_cleanup:
587582

588583
/*
589-
* Check command status and return to normal libpq state
584+
* Check command status and return to normal libpq state.
590585
*
591-
* We must not ever return with the status still PGRES_COPY_IN. Our
592-
* caller is unable to distinguish that situation from reaching the next
593-
* COPY in a command string that happened to contain two consecutive COPY
594-
* FROM STDIN commands. XXX if something makes PQputCopyEnd() fail
595-
* indefinitely while retaining status PGRES_COPY_IN, we get an infinite
596-
* loop. This is more realistic than handleCopyOut()'s counterpart risk.
586+
* We do not want to return with the status still PGRES_COPY_IN: our
587+
* caller would be unable to distinguish that situation from reaching the
588+
* next COPY in a command string that happened to contain two consecutive
589+
* COPY FROM STDIN commands. We keep trying PQputCopyEnd() in the hope
590+
* it'll work eventually. (What's actually likely to happen is that in
591+
* attempting to flush the data, libpq will eventually realize that the
592+
* connection is lost. But that's fine; it will get us out of COPY_IN
593+
* state, which is what we need.)
597594
*/
598595
while (res = PQgetResult(conn), PQresultStatus(res) == PGRES_COPY_IN)
599596
{
600597
OK = false;
601598
PQclear(res);
602-
603-
PQputCopyEnd(pset.db, _("trying to exit copy mode"));
599+
/* We can't send an error message if we're using protocol version 2 */
600+
PQputCopyEnd(conn,
601+
(PQprotocolVersion(conn) < 3) ? NULL :
602+
_("trying to exit copy mode"));
604603
}
605604
if (PQresultStatus(res) != PGRES_COMMAND_OK)
606605
{

0 commit comments

Comments
 (0)
0