]> granicus.if.org Git - postgresql/commitdiff
Handle EPIPE more sanely when we close a pipe reading from a program.
authorTom Lane <tgl@sss.pgh.pa.us>
Mon, 19 Nov 2018 22:02:25 +0000 (17:02 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Mon, 19 Nov 2018 22:02:39 +0000 (17:02 -0500)
Previously, any program launched by COPY TO/FROM PROGRAM inherited the
server's setting of SIGPIPE handling, i.e. SIG_IGN.  Hence, if we were
doing COPY FROM PROGRAM and closed the pipe early, the child process
would see EPIPE on its output file and typically would treat that as
a fatal error, in turn causing the COPY to report error.  Similarly,
one could get a failure report from a query that didn't read all of
the output from a contrib/file_fdw foreign table that uses file_fdw's
PROGRAM option.

To fix, ensure that child programs inherit SIG_DFL not SIG_IGN processing
of SIGPIPE.  This seems like an all-around better situation since if
the called program wants some non-default treatment of SIGPIPE, it would
expect to have to set that up for itself.  Then in COPY, if it's COPY
FROM PROGRAM and we stop reading short of detecting EOF, treat a SIGPIPE
exit from the called program as a non-error condition.  This still allows
us to report an error for any case where the called program gets SIGPIPE
on some other file descriptor.

As coded, we won't report a SIGPIPE if we stop reading as a result of
seeing an in-band EOF marker (e.g. COPY BINARY EOF marker).  It's
somewhat debatable whether we should complain if the called program
continues to transmit data after an EOF marker.  However, it seems like
we should avoid throwing error in any questionable cases, especially in a
back-patched fix, and anyway it would take additional code to make such
an error get reported consistently.

Back-patch to v10.  We could go further back, since COPY FROM PROGRAM
has been around awhile, but AFAICS the only way to reach this situation
using core or contrib is via file_fdw, which has only supported PROGRAM
sources since v10.  The COPY statement per se has no feature whereby
it'd stop reading without having hit EOF or an error already.  Therefore,
I don't see any upside to back-patching further that'd outweigh the
risk of complaints about behavioral change.

Per bug #15449 from Eric Cyr.

Patch by me, review by Etsuro Fujita and Kyotaro Horiguchi

Discussion: https://postgr.es/m/15449-1cf737dd5929450e@postgresql.org

src/backend/commands/copy.c
src/backend/storage/file/fd.c

index 7e5249e1e20399dd79470af4c706fa47de02a7b8..b956d2a5d3e93e99c73d497549cd708a951cc49f 100644 (file)
@@ -114,7 +114,9 @@ typedef struct CopyStateData
        FILE       *copy_file;          /* used if copy_dest == COPY_FILE */
        StringInfo      fe_msgbuf;              /* used for all dests during COPY TO, only for
                                                                 * dest == COPY_NEW_FE in COPY FROM */
-       bool            fe_eof;                 /* true if detected end of copy data */
+       bool            is_copy_from;   /* COPY TO, or COPY FROM? */
+       bool            reached_eof;    /* true if we read to end of copy data (not
+                                                                * all copy_dest types maintain this) */
        EolType         eol_type;               /* EOL type of input */
        int                     file_encoding;  /* file or remote side's character encoding */
        bool            need_transcoding;       /* file encoding diff from server? */
@@ -575,6 +577,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
                                ereport(ERROR,
                                                (errcode_for_file_access(),
                                                 errmsg("could not read from COPY file: %m")));
+                       if (bytesread == 0)
+                               cstate->reached_eof = true;
                        break;
                case COPY_OLD_FE:
 
@@ -595,7 +599,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
                        bytesread = minread;
                        break;
                case COPY_NEW_FE:
-                       while (maxread > 0 && bytesread < minread && !cstate->fe_eof)
+                       while (maxread > 0 && bytesread < minread && !cstate->reached_eof)
                        {
                                int                     avail;
 
@@ -623,7 +627,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
                                                        break;
                                                case 'c':       /* CopyDone */
                                                        /* COPY IN correctly terminated by frontend */
-                                                       cstate->fe_eof = true;
+                                                       cstate->reached_eof = true;
                                                        return bytesread;
                                                case 'f':       /* CopyFail */
                                                        ereport(ERROR,
@@ -1050,6 +1054,8 @@ ProcessCopyOptions(ParseState *pstate,
        if (cstate == NULL)
                cstate = (CopyStateData *) palloc0(sizeof(CopyStateData));
 
+       cstate->is_copy_from = is_from;
+
        cstate->file_encoding = -1;
 
        /* Extract options from the statement node tree */
@@ -1727,11 +1733,23 @@ ClosePipeToProgram(CopyState cstate)
                                (errcode_for_file_access(),
                                 errmsg("could not close pipe to external command: %m")));
        else if (pclose_rc != 0)
+       {
+               /*
+                * If we ended a COPY FROM PROGRAM before reaching EOF, then it's
+                * expectable for the called program to fail with SIGPIPE, and we
+                * should not report that as an error.  Otherwise, SIGPIPE indicates a
+                * problem.
+                */
+               if (cstate->is_copy_from && !cstate->reached_eof &&
+                       WIFSIGNALED(pclose_rc) && WTERMSIG(pclose_rc) == SIGPIPE)
+                       return;
+
                ereport(ERROR,
                                (errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
                                 errmsg("program \"%s\" failed",
                                                cstate->filename),
                                 errdetail_internal("%s", wait_result_to_str(pclose_rc))));
+       }
 }
 
 /*
@@ -3194,7 +3212,7 @@ BeginCopyFrom(ParseState *pstate,
        oldcontext = MemoryContextSwitchTo(cstate->copycontext);
 
        /* Initialize state variables */
-       cstate->fe_eof = false;
+       cstate->reached_eof = false;
        cstate->eol_type = EOL_UNKNOWN;
        cstate->cur_relname = RelationGetRelationName(cstate->rel);
        cstate->cur_lineno = 0;
index 827a1e2620b4a1b4a5ec0045e784533415ca6fc5..9e596e7868b12064b716a65a392ce28602aec82f 100644 (file)
@@ -2279,11 +2279,16 @@ OpenTransientFilePerm(const char *fileName, int fileFlags, mode_t fileMode)
  * Routines that want to initiate a pipe stream should use OpenPipeStream
  * rather than plain popen().  This lets fd.c deal with freeing FDs if
  * necessary.  When done, call ClosePipeStream rather than pclose.
+ *
+ * This function also ensures that the popen'd program is run with default
+ * SIGPIPE processing, rather than the SIG_IGN setting the backend normally
+ * uses.  This ensures desirable response to, eg, closing a read pipe early.
  */
 FILE *
 OpenPipeStream(const char *command, const char *mode)
 {
        FILE       *file;
+       int                     save_errno;
 
        DO_DB(elog(LOG, "OpenPipeStream: Allocated %d (%s)",
                           numAllocatedDescs, command));
@@ -2301,8 +2306,13 @@ OpenPipeStream(const char *command, const char *mode)
 TryAgain:
        fflush(stdout);
        fflush(stderr);
+       pqsignal(SIGPIPE, SIG_DFL);
        errno = 0;
-       if ((file = popen(command, mode)) != NULL)
+       file = popen(command, mode);
+       save_errno = errno;
+       pqsignal(SIGPIPE, SIG_IGN);
+       errno = save_errno;
+       if (file != NULL)
        {
                AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];
 
@@ -2315,12 +2325,9 @@ TryAgain:
 
        if (errno == EMFILE || errno == ENFILE)
        {
-               int                     save_errno = errno;
-
                ereport(LOG,
                                (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
                                 errmsg("out of file descriptors: %m; release and retry")));
-               errno = 0;
                if (ReleaseLruFile())
                        goto TryAgain;
                errno = save_errno;