From: Tom Lane <tgl@sss.pgh.pa.us>
To: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
        thomas.munro@enterprisedb.com, eric.cyr@gmail.com,
        pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #15449: file_fdw using program cause exit code error when
 using LIMIT
In-reply-to: <31355.1542389631@sss.pgh.pa.us>
References: <5BE4318F.4040002@lab.ntt.co.jp>
 <20181109.143931.243136889.horiguchi.kyotaro@lab.ntt.co.jp>
 <5BE552ED.4040304@lab.ntt.co.jp>
 <20181112.185214.76711162.horiguchi.kyotaro@lab.ntt.co.jp>
 <5BE966F4.1040702@lab.ntt.co.jp> <5BEA9E82.1080709@lab.ntt.co.jp>
 <30744.1542388851@sss.pgh.pa.us> <31355.1542389631@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 16 Nov 2018 12:33:51 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <7801.1542407515.0@sss.pgh.pa.us>
Date: Fri, 16 Nov 2018 17:33:17 -0500
Message-ID: <7865.1542407597@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <7801.1542407515.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="sigpipe-might-not-be-an-error-3.patch";
	charset="us-ascii"
Content-ID: <7801.1542407515.2@sss.pgh.pa.us>
Content-Description: sigpipe-might-not-be-an-error-3.patch
Content-Transfer-Encoding: quoted-printable

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 =3D=3D COPY_FILE */
  	StringInfo	fe_msgbuf;		/* used for all dests during COPY TO, only for
  								 * dest =3D=3D 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 =3D=3D COPY_FILE */
  	StringInfo	fe_msgbuf;		/* used for all dests during COPY TO, only for
  								 * dest =3D=3D 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 =3D=3D 0)
+ 				cstate->reached_eof =3D true;
  			break;
  		case COPY_OLD_FE:
  =

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

--- 599,605 ----
  			bytesread =3D 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 =3D true;
  							return bytesread;
  						case 'f':	/* CopyFail */
  							ereport(ERROR,
--- 627,633 ----
  							break;
  						case 'c':	/* CopyDone */
  							/* COPY IN correctly terminated by frontend */
! 							cstate->reached_eof =3D true;
  							return bytesread;
  						case 'f':	/* CopyFail */
  							ereport(ERROR,
*************** ProcessCopyOptions(ParseState *pstate,
*** 1050,1055 ****
--- 1054,1061 ----
  	if (cstate =3D=3D NULL)
  		cstate =3D (CopyStateData *) palloc0(sizeof(CopyStateData));
  =

+ 	cstate->is_copy_from =3D is_from;
+ =

  	cstate->file_encoding =3D -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 !=3D 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) =3D=3D 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 =3D MemoryContextSwitchTo(cstate->copycontext);
  =

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

  	/* Initialize state variables */
! 	cstate->reached_eof =3D false;
  	cstate->eol_type =3D EOL_UNKNOWN;
  	cstate->cur_relname =3D RelationGetRelationName(cstate->rel);
  	cstate->cur_lineno =3D 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 OpenPipeStrea=
m
   * 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 defau=
lt
+  * SIGPIPE and SIGUSR2 processing, rather than the SIG_IGN settings the
+  * backend normally uses.  This ensures desirable response to, eg, closi=
ng
+  * 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 =3D 0;
! 	if ((file =3D popen(command, mode)) !=3D NULL)
  	{
  		AllocateDesc *desc =3D &allocatedDescs[numAllocatedDescs];
  =

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

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

  	if (errno =3D=3D EMFILE || errno =3D=3D ENFILE)
  	{
- 		int			save_errno =3D 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 is=
n'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();

------- =_aaaaaaaaaa0--