Message-ID: <5BE2B874.3070504@lab.ntt.co.jp>
Date: Wed, 07 Nov 2018 19:03:32 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: BUG #15449: file_fdw using program cause exit code error when
 using LIMIT
References: <5BDC4BA0.7050106@lab.ntt.co.jp>
 <20181106.125304.193575333.horiguchi.kyotaro@lab.ntt.co.jp>
 <5BE18409.2070004@lab.ntt.co.jp>
 <20181107.092245.117036769.horiguchi.kyotaro@lab.ntt.co.jp>
In-Reply-To: <20181107.092245.117036769.horiguchi.kyotaro@lab.ntt.co.jp>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: tgl@sss.pgh.pa.us, eric.cyr@gmail.com, pgsql-bugs@lists.postgresql.org,
        pgsql-hackers@lists.postgresql.org
Precedence: bulk

(2018/11/07 9:22), Kyotaro HORIGUCHI wrote:
> At Tue, 06 Nov 2018 21:07:37 +0900, Etsuro Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote in<5BE18409.2070004@lab.ntt.co.jp>
>> (2018/11/06 12:53), Kyotaro HORIGUCHI wrote:
>>> At Fri, 02 Nov 2018 22:05:36 +0900, Etsuro
>>> Fujita<fujita.etsuro@lab.ntt.co.jp>  wrote
>>> in<5BDC4BA0.7050106@lab.ntt.co.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