(2018/10/29 15:58), Kyotaro HORIGUCHI wrote:
At Tue, 23 Oct 2018 13:21:31 +0100, Tom Lane<t...@sss.pgh.pa.us> wrote
in<18397.1540297...@sss.pgh.pa.us>
After a bit of thought, the problem here is blindingly obvious:
we generally run the backend with SIGPIPE handing set to SIG_IGN,
and evidently popen() allows the called program to inherit that,
at least on these platforms.
So what we need to do is make sure the called program inherits SIG_DFL
handling for SIGPIPE, and then special-case that result as not being
a failure. The attached POC patch does that and successfully makes
the file_fdw problem go away for me.
Thanks for working on this!
It's just a POC because there are some things that need more thought
than I've given them:
1. Is it OK to revert SIGPIPE to default processing for *all* programs
launched through OpenPipeStream? If not, what infrastructure do we
need to add to control that? In particular, is it sane to revert
SIGPIPE for a pipe program that we will write to not read from?
(I thought probably yes, because that is the normal Unix behavior,
but it could be debated.)
Forcing to ignore SIGPIPE lets the program to handle EPIPE, which
might not be handled poperly since it would be unexpected by the
program.
If we don't read from the pipe (that is, we are writing to it),
anyway we shouldn't change the behavior since SIGPIPE can come
from another pipe.
I'm a bit confused here. Horiguchi-san, are you saying that the called
program that we will read from should inherit SIG_DFL for SIGPIPE, as
proposed in the POC patch, but the called program that we will write to
should inherit SIG_IGN as it currently does?
ISTM that it would be OK to inherit SIG_DFL in both cases, because I
think it would be the responsibility of the called program to handle
SIGPIPE properly (if necessary) in both cases. Maybe I'm missing
something, though.
2. Likewise, is it always OK for ClosePipeToProgram to ignore a
SIGPIPE failure? (For ordinary COPY, maybe it shouldn't, since
we don't intend to terminate that early.)
Since the SIGPIPE may come from another pipe, I think we
shouldn't generally.
Agreed; if ClosePipeToProgram ignores that failure, we would fail to get
a better error message in CopySendEndOfRow if the called program
(inheriting SIG_DFL for SIGPIPE) was terminated on SIGPIPE:
if (cstate->is_program)
{
if (errno == EPIPE)
{
/*
* The pipe will be closed automatically on
error at
* the end of transaction, but we might get a
better
* error message from the subprocess' exit code
than
* just "Broken Pipe"
*/
ClosePipeToProgram(cstate);
/*
* If ClosePipeToProgram() didn't throw an
error, the
* program terminated normally, but closed the pipe
* first. Restore errno, and throw an error.
*/
errno = EPIPE;
}
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not write to COPY program:
%m")));
}
But iff we are explicitly stop reading from
the pipe before detecting an error, it can be ignored since we
aren't interested in further failure.
You mean that we should ignore other failures of the called program when
we stop reading from the pipe early?
Thus ClosePipeToProgram
might need an extra parameter or CopyState may need an additional
flag named, say, "explicit_close" (or
"ignore_sigpipe_on_pclose"..) in CopyState which will be set in
EndCopy.
3. Maybe this should be implemented at some higher level?
I'm not sure, but it seems to me not needed.
ISTM that it's a good idea to control the ClosePipeToProgram behavior by
a new member for CopyState.
4. Are there any other signals we ought to be reverting to default
behavior before launching a COPY TO/FROM PROGRAM?
All handlers other than SIG_DEF and IGN are reset on exec().
Only SIGUSR2 seems to be set to SIG_IGN, but it's not likely to
harm anything. Perhaps it would be safer against future changes
if we explicitly reset all changed actions to SIG_DEF, but it
might be too-much..
Not sure, but reverting SIGPIPE to default would be enough as a fix for
the original issue, if we go with the POC patch.
Best regards,
Etsuro Fujita
