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

At Tue, 06 Nov 2018 21:07:37 +0900, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <5BE18409(dot)2070004(at)lab(dot)ntt(dot)co(dot)jp>
> (2018/11/06 12:53), Kyotaro HORIGUCHI wrote:
> > At Fri, 02 Nov 2018 22:05:36 +0900, Etsuro
> > Fujita<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote
> > in<5BDC4BA0(dot)7050106(at)lab(dot)ntt(dot)co(dot)jp>
> >> (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>
> >>>> 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.)
>
> >> 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.
> >
> > So, I think we *should* (not just OK to) restore SIGPIPE to
> > SIG_DFL in any case here to prevent undetermined situation for
> > the program.
>
> OK
>
> >>>> 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")));
> >> }
> >
> > Mmm, that's EPIPE of fwrite, not SIGNALLED&SIGPIPE of called
> > program's exit status. So it is irrelevant to called program's
> > SIGPIPE setup. It requires SIGPIPE to be kept to SIG_IGN on the
> > backend side.
>
> My explanation might not be enough. Let me explain. If the called
> program that inherited SIG_DFL for SIGPIPE terminated on SIGPIPE for
> some reason, ClosePipeToProgram *as-is* would create an error message
> from that program's exit code. But if we modify ClosePipeToProgram
> like the original POC patch, that function would not create that
> message for that termination. To avoid that, I think it would be
> better for ClosePipeToProgram to ignore the SIGPIPE failure only in
> the case where the caller is a COPY FROM PROGRAM that is allowed to
> terminate early. (Maybe we could specify that as a new argument for
> BeginCopyFrom.)

I understood. Thanks for the explanation. I agree to that.

> >>> 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?
> >
> > Yes, we have received sufficient data from the pipe then closed
> > it successfully. The program may want write more but we don't
> > want it. We ragher should ignore SIGPIPE exit code of the program
rather
> > since closing our writing end of a pipe is likely to cause it and
>
> I think so too.
>
> > even if it comes from another pipe, we can assume that the
> > SIGPIPE immediately stopped the program before returning any
> > garbage to us.
>
> Sorry, I don't understand this.

Mmm. It looks confusing, sorry. In other words:

We don't care the reason for program's ending if we received
enough data from the program and we actively (= before receiving
EOF) stop reading. On the other hand if SIGPIPE (on another pipe)
or something fatal happened on the program before we end reading,
we receive EOF just after that and we are interested in the cause
of the program's death.

# Does the above make sense?

In the sense of "We don't care the reason", negligible reasons
are necessariry restricted to SIGPIPE, evan SIGSEGV could be
theoretically ignored safely. "theoretically" here means it is
another issue whether we rely on the output from a program which
causes SEGV (or any reason other than SIGPIPE, which we caused).

> >>>> 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.
> >
> > Agreed. I wonder why there's no API that resets all handlers to
> > SIG_DFL at once or some flag telling to exec() that it should
> > start with default signal handler set.
>
> Such API would be an improvement, but IMO I think that would go beyond
> the scope of this fix.

Ah, I wanted OS or standard library to provide it. Not us. Anyway
it is not relevant to the topic here.

> > Finally in the attached PoC, I set cstate->eof_reached on failure
> > of NextCopyFromRawFields then if eof_reached we don't ingore
> > SIGPIPE. For a program like
> > "puts("test1");fflush(stdout);kill(getpid(), SIGPIPE);", I had
> > the following behavior. (I got an error without the fflush() but
> > it is inevitable).
> >
> > =# select * from ft2 limit 1;
> > a
> > -------
> > test1
> >
> > =# select * from ft2 limit 2;
> > ERROR: program "/home/horiguti/work/exprog" failed
> > DETAIL: child process was terminated by signal 13: Broken pipe
> >
> > For the original case:
> >
> > =# select * from ft1 limit 1;
> > a
> > ------
> > test
> > =# select * from ft1 limit 2;
> > a
> > ------
> > test
> > (1 row)
> >
> >
> > I didn't confirmed whether it is complete.
>
> Sorry, I don't understand this fully, but the reason to add the
> eof_reached stuff is to avoid ignoring the SIGPIPE failure in normal
> COPY FROM PROGRAM cases?

Yes, if we have received EOF, the program ended before we
finished reading from the pipe thus any error should be
reported. Elsewise we don't care at least SIGPIPE. In the
attached patch all kind of errors are ignored when pipe is closed
from reading-end on backends.

As the result it doesn't report an error for SELECT * FROM ft2
LIMIT 1 on "main(void){puts("test1"); return 1;}".

=# select * from ft limit 1;
a
-------
test1
(1 row)

limit 2 reports the error.

=# select * from ft limit 2;
ERROR: program "/home/horiguti/work/exprog" failed
DETAIL: child process exited with exit code 1

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center