From: | Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | tgl(at)sss(dot)pgh(dot)pa(dot)us, eric(dot)cyr(at)gmail(dot)com, pgsql-bugs(at)lists(dot)postgresql(dot)org, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: BUG #15449: file_fdw using program cause exit code error when using LIMIT |
Date: | 2018-11-07 10:03:32 |
Message-ID: | 5BE2B874.3070504@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs pgsql-hackers |
(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
From | Date | Subject | |
---|---|---|---|
Next Message | Arthur Zakirov | 2018-11-07 11:20:55 | Re: BUG #15488: Unexpected behaviour of to_tsverctor and ts_query |
Previous Message | PG Bug reporting form | 2018-11-07 08:30:20 | BUG #15491: index on function not being used for full text search when querying through a view |
From | Date | Subject | |
---|---|---|---|
Next Message | REIX, Tony | 2018-11-07 10:21:22 | RE: Issue with v11.0 within jsonb_plperl tests in 32bit on AIX |
Previous Message | Alexey Kondratov | 2018-11-07 09:58:11 | Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line |