I'm fresh in the code, but this has solved my issues with PQconnect* 
failing when interrupted by signals.  Some of it is sloppy and not to my 
liking yet, but I'm still digging through to see if anything else needs 
touched.  Comments appreciated.

Honestly, I'm a bit surprised that this issue hasn't been encountered 
before.

Summary:
    * changes to connect() sections to handle errno=EINTR.  this solves 
libpq PQconnect* family problems if the connect is interrupted by a 
signal such as SIGALRM.
    * not all read/recv/write/send calls have been updated

David
Thu, 25 Oct 2001 17:08:25 -0400
David Ford <david@blue-labs.org>
[patch] helps fe-connect.c handle -EINTR more gracefully
On 25 Oct 2001 at 17:08 (-0400), David Ford wrote:
| I'm fresh in the code, but this has solved my issues with PQconnect* 
| failing when interrupted by signals.  Some of it is sloppy and not to my 
| liking yet, but I'm still digging through to see if anything else needs 
| touched.  Comments appreciated.

Disclaimer: I may be wrong as hell ;-), but...


I'm not sure this is correct.  I've tried to /make/ a SIGALRM cause
connect to errno==EINTR, but I can't cause this condition.  I suspect
you have another signal being raised that is causing your symptom.
FTR, the textbook definition[1] of EINTR error for connect is:

  The attempt to establish a connection was interrupted by delivery 
  of a signal that was caught; the connection will be established 
  asynchronously.

Please check the attached prog to see if it is representative of your
code relating to the connect error you're seeing.  If it is, please
run it and see if you can get it to cause the EINTR error from connect.
If you can't I'm more certain that you have a problem elsewhere.

cheers.
  brent

1. http://www.opengroup.org/onlinepubs/7908799/xns/connect.html

-- 
"Develop your talent, man, and leave the world something. Records are 
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing."  -- Duane Allman
Thu, 25 Oct 2001 23:13:19 -0400
Brent Verner <brent@rcfile.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Brent Verner <brent@rcfile.org> writes:
> I'm not sure this is correct.  I've tried to /make/ a SIGALRM cause
> connect to errno==EINTR, but I can't cause this condition.

It wouldn't surprise me in the least if this behavior is
platform-dependent.  It may well be that David's kernel will allow
connect() to be interrupted by SIGALRM while yours won't.  (Which
reminds me that neither of you specified what platforms you were
testing on.  For shame.)  Or maybe the difference depends on whether
you are trying to connect to a local or remote server.

Unless someone can point out a situation where retrying connect()
after EINTR is actively bad, my inclination is to accept the patch.
It seems like a net improvement in robustness to me, with no evident
downside other than a line or two more code.

			regards, tom lane

Fri, 26 Oct 2001 00:05:54 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
Many signals may be the cause of -EINTR.  It depends on what the signal 
is as to how it's normally handled.  sigalarm is the most common due to 
it being a timer event.

Generate a timer that expires as fast as possible (not too fast to 
prevent code execution), and you should see things left and right return 
with -EINTR.

I'm very much aware of why SIGALRM is happening, I generate it and I 
catch it.  As per my original message on this thread, my program does 
maintenance on a scheduled basis.  The period of that maintenance is 
many times per second.

Sooo... :)

Now let's get on with the story.

Libpq doesn't deal with system calls being interrupted in the slightest. 
 None of the read/write or socket calls handle any errors.  Even benign 
returns i.e. EINTR are treated as fatal errors and returned.  Not to 
malign, but there is no reason not to continue on and handle EINTR.

David
p.s. you cant use sleep() or alarm() functions and have a timer event as 
well.  The only POSIX compliant function that doesn't trample signal 
timer events is nanosleep().

Brent Verner wrote:

On 25 Oct 2001 at 17:08 (-0400), David Ford wrote:
| I'm fresh in the code, but this has solved my issues with PQconnect* 
| failing when interrupted by signals.  Some of it is sloppy and not to my 
| liking yet, but I'm still digging through to see if anything else needs 
| touched.  Comments appreciated.

Disclaimer: I may be wrong as hell ;-), but...


I'm not sure this is correct.  I've tried to /make/ a SIGALRM cause
connect to errno==EINTR, but I can't cause this condition.  I suspect
you have another signal being raised that is causing your symptom.
FTR, the textbook definition[1] of EINTR error for connect is:

  The attempt to establish a connection was interrupted by delivery 
  of a signal that was caught; the connection will be established 
  asynchronously.

Please check the attached prog to see if it is representative of your
code relating to the connect error you're seeing.  If it is, please
run it and see if you can get it to cause the EINTR error from connect.
If you can't I'm more certain that you have a problem elsewhere.

cheers.
  brent

1. http://www.opengroup.org/onlinepubs/7908799/xns/connect.html


------------------------------------------------------------------------
[snipped]


Fri, 26 Oct 2001 00:26:22 -0400
David Ford <david@blue-labs.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
On 26 Oct 2001 at 00:05 (-0400), Tom Lane wrote:
| Brent Verner <brent@rcfile.org> writes:
| > I'm not sure this is correct.  I've tried to /make/ a SIGALRM cause
| > connect to errno==EINTR, but I can't cause this condition.
| 
| It wouldn't surprise me in the least if this behavior is
| platform-dependent.  It may well be that David's kernel will allow
| connect() to be interrupted by SIGALRM while yours won't.  (Which
| reminds me that neither of you specified what platforms you were
| testing on.  For shame.)  Or maybe the difference depends on whether
| you are trying to connect to a local or remote server.

sorry, I tested the attached prog on linux(2.2/2.4) and freebsd(4.4R)
to both local and remote(slow) servers.

| Unless someone can point out a situation where retrying connect()
| after EINTR is actively bad, my inclination is to accept the patch.
| It seems like a net improvement in robustness to me, with no evident
| downside other than a line or two more code.

  I've found numerous examples where connect() is retried after EINTR,
infact it appears to be fairly common.

cheers,
  brent

-- 
"Develop your talent, man, and leave the world something. Records are 
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing."  -- Duane Allman

Fri, 26 Oct 2001 01:01:42 -0400
Brent Verner <brent@rcfile.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Brent Verner <brent@rcfile.org> writes:
> | Unless someone can point out a situation where retrying connect()
> | after EINTR is actively bad, my inclination is to accept the patch.

>   I've found numerous examples where connect() is retried after EINTR,
> infact it appears to be fairly common.

Perhaps it does work that way on your system, but that's not the point.
On a machine that behaves that way, we'll never see EINTR returned by
connect(), and so our reaction to it is unimportant.  The question is
what we should do if we *do* get EINTR from connect().  AFAICS, the
appropriate response is to retry.  We already do retry after EINTR in
libpq's recv, send, select, etc calls --- perhaps connect got overlooked
because it's usually only done at program startup.

After further thought, though, it's unclear to me why this solves
David's problem.  If he's got a repeating SIGALRM on a cycle short
enough to interrupt a connect(), seems like it'd just fail again
on the next try.

			regards, tom lane

Fri, 26 Oct 2001 10:22:16 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
David Ford writes:

> Libpq doesn't deal with system calls being interrupted in the slightest.
>  None of the read/write or socket calls handle any errors.  Even benign
> returns i.e. EINTR are treated as fatal errors and returned.  Not to
> malign, but there is no reason not to continue on and handle EINTR.

Libpq certainly does deal with system calls being interrupted:  It does
not allow them to be interrupted.  Take a look into the file pqsignal.c to
see why.

If your alarm timer interrupts system calls then that's because you have
installed your signal handler to allow that.  In my mind, a reasonable
behaviour in that case would be to let the PQconnect or equivalent fail
and provide the errno to the application.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter


Fri, 26 Oct 2001 23:04:39 +0200 (CEST)
Peter Eisentraut <peter_e@gmx.net>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Peter Eisentraut <peter_e@gmx.net> writes:
> Libpq certainly does deal with system calls being interrupted:  It does
> not allow them to be interrupted.  Take a look into the file pqsignal.c to
> see why.

???  Are you momentarily confusing backend and frontend libpq?

AFAICT the client-side libpq doesn't (and shouldn't) touch signal
handling at all, except for a couple of places in the print routines
that temporarily block SIGPIPE.

Since we deal happily with EINTR for most of the frontend socket calls,
I don't see a reason not to cope with it for connect() too.  I am
somewhat concerned about what exactly it means for a non-blocking
connect, however.  Maybe it doesn't mean anything, and we could treat
it the same as EINPROGRESS.

			regards, tom lane

Fri, 26 Oct 2001 17:36:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
"The **SA_RESTART**  flag is always set by the underlying system in 
POSIX mode so that interrupted system calls will fail with return value 
of -1 and the *EINTR* error in /errno/ instead of getting restarted." 
 libpq's pqsignal.c doesn't turn off SA_RESTART for SIGALRM.  Further, 
pqsignal.c only handles SIGPIPE and not to mention that other parts of 
libpq do handle EINTR properly.

PQconnect* family does not handle EINTR.  It does not handle the 
possible and perfectly legitimate interruption of a system call. 
 Globally trying to disable system calls from being interrupted is a Bad 
Thing. Having a timer event is common, having a timer event in a daemon 
is often required.  Timers allow for good housekeeping and playing nice 
with the rest of the system.

Your reasonable behavior in the case of EINTR means repeatable and 
mysterious failure.  There isn't a clean way to re-enter PQconnect* 
while maintaining state in the case of signal interruption and no 
guarantee the function won't be interrupted again.

Basically if you have a timer event in your software and you use pgsql, 
then the following will happen.

a) if the timer event always happens outside the PQconnect* call is 
completed your code will function
b) if the timer event always fires during the PQconnect* call, your code 
will never function
c) if your timer event sometimes fires during the PQconnect* call, your 
code will sometimes function

There are no ifs, ands, or buts about it, if a timer fires inside 
PQconnect* as it is now, there is no way to continue. With a suitablly 
long timer period, you can try the PQconnect* call again and if the 
connect succeeds before the timer fires again you're fine.  If not, you 
must repeatedly try.

That said, there are two ways about it.  a) handle it cleanly inside 
PQconnect* like it should be done, or b) have the programmer parse the 
error string for "Interrupted system call" and re-enter PQconnect.  a) 
is clean, short, and simple.  b) wastes a lot of CPU to attempt to 
accomplish the task.  a) is guaranteed and b) is not guaranteed.

David

Peter Eisentraut wrote:

David Ford writes:

>Libpq doesn't deal with system calls being interrupted in the slightest.
> None of the read/write or socket calls handle any errors.  Even benign
>returns i.e. EINTR are treated as fatal errors and returned.  Not to
>malign, but there is no reason not to continue on and handle EINTR.
>

Libpq certainly does deal with system calls being interrupted:  It does
not allow them to be interrupted.  Take a look into the file pqsignal.c to
see why.

If your alarm timer interrupts system calls then that's because you have
installed your signal handler to allow that.  In my mind, a reasonable
behaviour in that case would be to let the PQconnect or equivalent fail
and provide the errno to the application.



Fri, 26 Oct 2001 19:51:48 -0400
David Ford <david@blue-labs.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Tom Lane writes:

> AFAICT the client-side libpq doesn't (and shouldn't) touch signal
> handling at all, except for a couple of places in the print routines
> that temporarily block SIGPIPE.

Which was my point.

> Since we deal happily with EINTR for most of the frontend socket calls,
> I don't see a reason not to cope with it for connect() too.  I am
> somewhat concerned about what exactly it means for a non-blocking
> connect, however.  Maybe it doesn't mean anything, and we could treat
> it the same as EINPROGRESS.

I feel that if the user installed his signal handlers to interrupt system
calls then he probably had a reason for it, possibly because of the timing
aspects of his application.  Thus, it shouldn't be libpq's task to
override that decision.  If the user doesn't want system calls to be
interrupted, then he should install the signal handlers in the proper way.
If he doesn't know how to do that, he needs to educate himself, that's
all.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter


Sun, 28 Oct 2001 20:16:05 +0100 (CET)
Peter Eisentraut <peter_e@gmx.net>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Peter Eisentraut wrote:

>>AFAICT the client-side libpq doesn't (and shouldn't) touch signal
>>handling at all, except for a couple of places in the print routines
>>that temporarily block SIGPIPE.
>>
>
>Which was my point.
>

My patch doesn't affect signal handling, my patch affects the response 
of connect() after it is interrupted by a signal.

>>Since we deal happily with EINTR for most of the frontend socket calls,
>>I don't see a reason not to cope with it for connect() too.  I am
>>somewhat concerned about what exactly it means for a non-blocking
>>connect, however.  Maybe it doesn't mean anything, and we could treat
>>it the same as EINPROGRESS.
>>
>
>I feel that if the user installed his signal handlers to interrupt system
>calls then he probably had a reason for it, possibly because of the timing
>aspects of his application.  Thus, it shouldn't be libpq's task to
>override that decision.  If the user doesn't want system calls to be
>interrupted, then he should install the signal handlers in the proper way.
>If he doesn't know how to do that, he needs to educate himself, that's
>all.
>

Let me ask you how you would handle SIGALRM without interrupting 
syscalls?  Further, how would you guarantee your SIGALRM handler would 
execute within your granular limits?

Yes, the user has a timing aspect.  Libpq isn't overriding it and my 
patch doesn't change that.  My patch adds the EINTR handling.  Currently 
there is no way for libpq to continue processing a connect call if that 
syscall is interrupted and the user is using POSIX defaults.  Please 
refer to the POSIX specification, I povided a quote in a previous message.

POSIX default signal handling sets SA_RESTART which -disables- system 
call restart.

At present, the PQconnect* function treats EINTR as a fatal error.

David



Sun, 28 Oct 2001 19:19:01 -0500
David Ford <david@blue-labs.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Peter Eisentraut <peter_e@gmx.net> writes:
> I feel that if the user installed his signal handlers to interrupt system
> calls then he probably had a reason for it, possibly because of the timing
> aspects of his application.  Thus, it shouldn't be libpq's task to
> override that decision.

How are we "overriding" it?  A more correct description would be that
we are "coping" with it.  We already do so when a send or recv is
interrupted, so I don't see why there's a problem with extending that
policy to connect calls.

What I think you are arguing is that marking signal handlers SA_RESTART
is a sufficient answer for this issue, but I don't really agree, on
two grounds: (a) not everyone has POSIX signals, (b) SA_RESTART is a
blunt instrument because it's per-signal.  A user might well want
SIGALRM to interrupt some operations, but that doesn't mean he wants it
to cause failures inside subroutine libraries.  Look at the backend:
we make SIGALRM non-SA_RESTART, which means we need to retry after
EINTR in most places.  We do it because we want certain specific waits
to be interruptible, not because we want a global policy of "fail if
interrupted".  (Now that I look at it, I wonder whether SIGINT,
SIGTERM, SIGQUIT shouldn't be non-SA_RESTART as well, but that's a
different discussion.)

My quibble with David has been about whether the fix is correct in
detail, not about whether its purpose is correct.

			regards, tom lane

Mon, 29 Oct 2001 12:10:55 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
Tom Lane wrote:

>My quibble with David has been about whether the fix is correct in
>detail, not about whether its purpose is correct.
>

My preference is to spin until ! EINTR as soon as possible after the 
socket operation.  I'm going to seek some advice from kernel people 
regarding the inner workings of sockets.  I suspect that we'll be able 
to get away with handling it like as is done with EINPROGRESS, but 
something is nagging me at the back of my head.

In regards to the other signals, I think there is another subtle 
problem.  Remember a month ago when I had a huge database that I had to 
upgrade, I had no disk space to export it to and when the old version of 
psql ran out of memory it crashed?  The backend continued to push query 
data out the closed pipe until the backend was forcibly closed or the 
query completed.  Naturally this  caused considerable spammage on the 
console.

So I suspect some tuning needs to be done in respect to SIGPIPE w/ the 
backend.

David



Mon, 29 Oct 2001 15:01:34 -0500
David Ford <david@blue-labs.org>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
David Ford <david@blue-labs.org> writes:
> problem.  Remember a month ago when I had a huge database that I had to 
> upgrade, I had no disk space to export it to and when the old version of 
> psql ran out of memory it crashed?  The backend continued to push query 
> data out the closed pipe until the backend was forcibly closed or the 
> query completed.  Naturally this  caused considerable spammage on the 
> console.

This has been discussed before.  Don't bother proposing that backends
should use the default handling of SIGPIPE, because that won't be
accepted.  A safe limited solution would be to keep backend libpq from
emitting multiple consecutive "broken pipe" reports to stderr.
A better-sounding solution that might have unforeseen side effects
is to set QueryCancel as soon as we see a nonrecoverable-looking
send() error.  This is on the TODO list but no one's gotten round to
doing anything about it yet.

			regards, tom lane

Mon, 29 Oct 2001 16:26:28 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
> David Ford <david@blue-labs.org> writes:
> > problem.  Remember a month ago when I had a huge database that I had to 
> > upgrade, I had no disk space to export it to and when the old version of 
> > psql ran out of memory it crashed?  The backend continued to push query 
> > data out the closed pipe until the backend was forcibly closed or the 
> > query completed.  Naturally this  caused considerable spammage on the 
> > console.
> 
> This has been discussed before.  Don't bother proposing that backends
> should use the default handling of SIGPIPE, because that won't be
> accepted.  A safe limited solution would be to keep backend libpq from
> emitting multiple consecutive "broken pipe" reports to stderr.
> A better-sounding solution that might have unforeseen side effects
> is to set QueryCancel as soon as we see a nonrecoverable-looking
> send() error.  This is on the TODO list but no one's gotten round to
> doing anything about it yet.

Guys, can we come to a resolution this so I can mark it as completed?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 11 Nov 2001 22:57:50 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Guys, can we come to a resolution this so I can mark it as completed?

The QueryCancel idea is a neat hack but I don't have enough confidence
in it to throw it in during beta.  I'll add a little bit of code to
pq_flush to suppress duplicate error messages; that should be safe
enough and get rid of the worst aspects of the problem.

			regards, tom lane

Sun, 11 Nov 2001 23:23:02 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully 
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Guys, can we come to a resolution this so I can mark it as completed?
> 
> The QueryCancel idea is a neat hack but I don't have enough confidence
> in it to throw it in during beta.  I'll add a little bit of code to
> pq_flush to suppress duplicate error messages; that should be safe
> enough and get rid of the worst aspects of the problem.

So I will mark it as done.  If there is something for the TODO list
here, please let me know.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 11 Nov 2001 23:41:59 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [patch] helps fe-connect.c handle -EINTR more gracefully