Message-ID: <5BDC4BA0.7050106@lab.ntt.co.jp>
Date: Fri, 02 Nov 2018 22:05:36 +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: <15449-1cf737dd5929450e@postgresql.org>
 <83525.1540229278@sss.pgh.pa.us> <18397.1540297291@sss.pgh.pa.us>
 <20181029.155817.216002543.horiguchi.kyotaro@lab.ntt.co.jp>
In-Reply-To: <20181029.155817.216002543.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/10/29 15:58), Kyotaro HORIGUCHI wrote:
> At Tue, 23 Oct 2018 13:21:31 +0100, Tom Lane<tgl@sss.pgh.pa.us>  wrote in<18397.1540297291@sss.pgh.pa.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