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

(2018/11/07 9:22), Kyotaro HORIGUCHI wrote:
> 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:
>>>>> 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?

Yeah, thanks for the explanation!

> 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).

For the SIGSEGV case, I think it would be better that we don't rely on
the output data, IMO, because I think there might be a possibility that
the program have generated that data incorrectly/unexpectedly.

>>> 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.

OK, I understand your idea. Thanks for the patch!

> 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

I think this would be contrary to users expectations: if the SELECT
command works for limit 1, they would expect that the command would work
for limit 2 as well. So, I think it would be better to error out that
command for limit 1 as well, as-is.

Best regards,
Etsuro Fujita