Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8dc49a8

Browse files
committed
Handle EPIPE more sanely when we close a pipe reading from a program.
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
1 parent 923f9c2 commit 8dc49a8

File tree

2 files changed

+33
-8
lines changed

2 files changed

+33
-8
lines changed

src/backend/commands/copy.c

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,9 @@ typedef struct CopyStateData
102102
FILE *copy_file; /* used if copy_dest == COPY_FILE */
103103
StringInfo fe_msgbuf; /* used for all dests during COPY TO, only for
104104
* dest == COPY_NEW_FE in COPY FROM */
105-
bool fe_eof; /* true if detected end of copy data */
105+
bool is_copy_from; /* COPY TO, or COPY FROM? */
106+
bool reached_eof; /* true if we read to end of copy data (not
107+
* all copy_dest types maintain this) */
106108
EolType eol_type; /* EOL type of input */
107109
int file_encoding; /* file or remote side's character encoding */
108110
bool need_transcoding; /* file encoding diff from server? */
@@ -566,6 +568,8 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
566568
ereport(ERROR,
567569
(errcode_for_file_access(),
568570
errmsg("could not read from COPY file: %m")));
571+
if (bytesread == 0)
572+
cstate->reached_eof = true;
569573
break;
570574
case COPY_OLD_FE:
571575

@@ -586,7 +590,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
586590
bytesread = minread;
587591
break;
588592
case COPY_NEW_FE:
589-
while (maxread > 0 && bytesread < minread && !cstate->fe_eof)
593+
while (maxread > 0 && bytesread < minread && !cstate->reached_eof)
590594
{
591595
int avail;
592596

@@ -614,7 +618,7 @@ CopyGetData(CopyState cstate, void *databuf, int minread, int maxread)
614618
break;
615619
case 'c': /* CopyDone */
616620
/* COPY IN correctly terminated by frontend */
617-
cstate->fe_eof = true;
621+
cstate->reached_eof = true;
618622
return bytesread;
619623
case 'f': /* CopyFail */
620624
ereport(ERROR,
@@ -1040,6 +1044,8 @@ ProcessCopyOptions(ParseState *pstate,
10401044
if (cstate == NULL)
10411045
cstate = (CopyStateData *) palloc0(sizeof(CopyStateData));
10421046

1047+
cstate->is_copy_from = is_from;
1048+
10431049
cstate->file_encoding = -1;
10441050

10451051
/* Extract options from the statement node tree */
@@ -1717,11 +1723,23 @@ ClosePipeToProgram(CopyState cstate)
17171723
(errcode_for_file_access(),
17181724
errmsg("could not close pipe to external command: %m")));
17191725
else if (pclose_rc != 0)
1726+
{
1727+
/*
1728+
* If we ended a COPY FROM PROGRAM before reaching EOF, then it's
1729+
* expectable for the called program to fail with SIGPIPE, and we
1730+
* should not report that as an error. Otherwise, SIGPIPE indicates a
1731+
* problem.
1732+
*/
1733+
if (cstate->is_copy_from && !cstate->reached_eof &&
1734+
WIFSIGNALED(pclose_rc) && WTERMSIG(pclose_rc) == SIGPIPE)
1735+
return;
1736+
17201737
ereport(ERROR,
17211738
(errcode(ERRCODE_EXTERNAL_ROUTINE_EXCEPTION),
17221739
errmsg("program \"%s\" failed",
17231740
cstate->filename),
17241741
errdetail_internal("%s", wait_result_to_str(pclose_rc))));
1742+
}
17251743
}
17261744

17271745
/*
@@ -3033,7 +3051,7 @@ BeginCopyFrom(ParseState *pstate,
30333051
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
30343052

30353053
/* Initialize state variables */
3036-
cstate->fe_eof = false;
3054+
cstate->reached_eof = false;
30373055
cstate->eol_type = EOL_UNKNOWN;
30383056
cstate->cur_relname = RelationGetRelationName(cstate->rel);
30393057
cstate->cur_lineno = 0;

src/backend/storage/file/fd.c

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2438,11 +2438,16 @@ OpenTransientFilePerm(const char *fileName, int fileFlags, mode_t fileMode)
24382438
* Routines that want to initiate a pipe stream should use OpenPipeStream
24392439
* rather than plain popen(). This lets fd.c deal with freeing FDs if
24402440
* necessary. When done, call ClosePipeStream rather than pclose.
2441+
*
2442+
* This function also ensures that the popen'd program is run with default
2443+
* SIGPIPE processing, rather than the SIG_IGN setting the backend normally
2444+
* uses. This ensures desirable response to, eg, closing a read pipe early.
24412445
*/
24422446
FILE *
24432447
OpenPipeStream(const char *command, const char *mode)
24442448
{
24452449
FILE *file;
2450+
int save_errno;
24462451

24472452
DO_DB(elog(LOG, "OpenPipeStream: Allocated %d (%s)",
24482453
numAllocatedDescs, command));
@@ -2460,8 +2465,13 @@ OpenPipeStream(const char *command, const char *mode)
24602465
TryAgain:
24612466
fflush(stdout);
24622467
fflush(stderr);
2468+
pqsignal(SIGPIPE, SIG_DFL);
24632469
errno = 0;
2464-
if ((file = popen(command, mode)) != NULL)
2470+
file = popen(command, mode);
2471+
save_errno = errno;
2472+
pqsignal(SIGPIPE, SIG_IGN);
2473+
errno = save_errno;
2474+
if (file != NULL)
24652475
{
24662476
AllocateDesc *desc = &allocatedDescs[numAllocatedDescs];
24672477

@@ -2474,12 +2484,9 @@ OpenPipeStream(const char *command, const char *mode)
24742484

24752485
if (errno == EMFILE || errno == ENFILE)
24762486
{
2477-
int save_errno = errno;
2478-
24792487
ereport(LOG,
24802488
(errcode(ERRCODE_INSUFFICIENT_RESOURCES),
24812489
errmsg("out of file descriptors: %m; release and retry")));
2482-
errno = 0;
24832490
if (ReleaseLruFile())
24842491
goto TryAgain;
24852492
errno = save_errno;

0 commit comments

Comments
 (0)