Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT

I wrote:
> I just had a thought about that: suppose we add a flag to CopyState
> to indicate whether we reached EOF while reading.  ...
> Then the logic in ClosePipeToProgram could be "if we did not reach
> EOF, then a SIGPIPE termination is unsurprising.  If we did reach
> EOF, then SIGPIPE is an error."  If the called program gets SIGPIPE
> for some reason other than us closing the pipe early, then we would
> see EOF next time we try to read, and correctly report that there's
> a problem.

Concretely, something like the attached.

I'm not quite sure whether the reached_eof test should be
"if (bytesread == 0)" or "if (bytesread < maxread)".  The former
seems more certain to indicate real EOF; are there other cases where
the fread might return a short read?  On the other hand, if we
support in-band EOF indicators (\. for instance) then we might
stop without having made an extra call to CopyGetData that would
see bytesread == 0.  On the third hand, if we do do that it's
not quite clear to me whether SIGPIPE is ignorable or not.

            regards, tom lane

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 6588ebd..8975446 100644
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
*************** typedef struct CopyStateData
*** 114,120 ****
      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 */
      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? */
--- 114,122 ----
      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        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? */
*************** CopyGetData(CopyState cstate, void *data
*** 575,580 ****
--- 577,584 ----
                  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:

*************** CopyGetData(CopyState cstate, void *data
*** 595,601 ****
              bytesread = minread;
              break;
          case COPY_NEW_FE:
!             while (maxread > 0 && bytesread < minread && !cstate->fe_eof)
              {
                  int            avail;

--- 599,605 ----
              bytesread = minread;
              break;
          case COPY_NEW_FE:
!             while (maxread > 0 && bytesread < minread && !cstate->reached_eof)
              {
                  int            avail;

*************** CopyGetData(CopyState cstate, void *data
*** 623,629 ****
                              break;
                          case 'c':    /* CopyDone */
                              /* COPY IN correctly terminated by frontend */
!                             cstate->fe_eof = true;
                              return bytesread;
                          case 'f':    /* CopyFail */
                              ereport(ERROR,
--- 627,633 ----
                              break;
                          case 'c':    /* CopyDone */
                              /* COPY IN correctly terminated by frontend */
!                             cstate->reached_eof = true;
                              return bytesread;
                          case 'f':    /* CopyFail */
                              ereport(ERROR,
*************** ProcessCopyOptions(ParseState *pstate,
*** 1050,1055 ****
--- 1054,1061 ----
      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 */
*************** ClosePipeToProgram(CopyState cstate)
*** 1727,1737 ****
--- 1733,1755 ----
                  (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))));
+     }
  }

  /*
*************** BeginCopyFrom(ParseState *pstate,
*** 3169,3175 ****
      oldcontext = MemoryContextSwitchTo(cstate->copycontext);

      /* Initialize state variables */
!     cstate->fe_eof = false;
      cstate->eol_type = EOL_UNKNOWN;
      cstate->cur_relname = RelationGetRelationName(cstate->rel);
      cstate->cur_lineno = 0;
--- 3187,3193 ----
      oldcontext = MemoryContextSwitchTo(cstate->copycontext);

      /* Initialize state variables */
!     cstate->reached_eof = false;
      cstate->eol_type = EOL_UNKNOWN;
      cstate->cur_relname = RelationGetRelationName(cstate->rel);
      cstate->cur_lineno = 0;
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 2d75773..6bc0244 100644
*** a/src/backend/storage/file/fd.c
--- b/src/backend/storage/file/fd.c
*************** OpenTransientFilePerm(const char *fileNa
*** 2271,2281 ****
--- 2271,2287 ----
   * 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 and SIGUSR2 processing, rather than the SIG_IGN settings 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));
*************** OpenPipeStream(const char *command, cons
*** 2293,2300 ****
  TryAgain:
      fflush(stdout);
      fflush(stderr);
      errno = 0;
!     if ((file = popen(command, mode)) != NULL)
      {
          AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];

--- 2299,2313 ----
  TryAgain:
      fflush(stdout);
      fflush(stderr);
+     pqsignal(SIGPIPE, SIG_DFL);
+     pqsignal(SIGUSR2, SIG_DFL);
      errno = 0;
!     file = popen(command, mode);
!     save_errno = errno;
!     pqsignal(SIGPIPE, SIG_IGN);
!     pqsignal(SIGUSR2, SIG_IGN);
!     errno = save_errno;
!     if (file != NULL)
      {
          AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];

*************** TryAgain:
*** 2307,2314 ****

      if (errno == EMFILE || errno == ENFILE)
      {
-         int            save_errno = errno;
-
          ereport(LOG,
                  (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
                   errmsg("out of file descriptors: %m; release and retry")));
--- 2320,2325 ----
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a3b9757..cfa416a 100644
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
*************** PostgresMain(int argc, char *argv[],
*** 3769,3774 ****
--- 3769,3778 ----
       * handler in the postmaster to reserve the signal. (Of course, this isn't
       * an issue for signals that are locally generated, such as SIGALRM and
       * SIGPIPE.)
+      *
+      * Also note: signals that are set to SIG_IGN here should be reset in
+      * OpenPipeStream, so that exec'd programs see a standard signal
+      * environment.
       */
      if (am_walsender)
          WalSndSignals();