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

(2018/10/29 15:58), Kyotaro HORIGUCHI wrote:
> At Tue, 23 Oct 2018 13:21:31 +0100, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in<18397(dot)1540297291(at)sss(dot)pgh(dot)pa(dot)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