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>
Cc: thomas.munro@enterprisedb.com
Cc: eric.cyr@gmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT
Date: Fri, 16 Nov 2018 17:33:17 -0500
Message-ID: <7865.1542407597@sss.pgh.pa.us> (raw)
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>
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
Attachments:
[text/x-diff] sigpipe-might-not-be-an-error-3.patch (6.5K, ../7865.1542407597@sss.pgh.pa.us/2-sigpipe-might-not-be-an-error-3.patch)
download | inline diff:diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.cindex 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.cindex 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.cindex 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();
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, fujita.etsuro@lab.ntt.co.jp, 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: <7865.1542407597@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox