Restartable signals 'n all that

While poking at the vacuum-launcher issue currently under discussion,
I got annoyed again at the behavior we've known for a long while that
on some platforms pg_usleep() won't be interrupted by signals. (In
particular I see this on HPUX, though not on Linux or Darwin. Anyone
know if it happens on any BSDen?) I noticed that with the launcher set
to sleep at most one second between checks for signals, it seemed to
*always* take the full second before shutting down, which seemed awfully
unlucky.

Some more testing and man-page-reading revealed the full truth of what's
going on. The Single Unix Spec's select(2) page says under ERRORS

[EINTR]
The select() function was interrupted before any of the selected events
occurred and before the timeout interval expired. If SA_RESTART has been
set for the interrupting signal, it is implementation-dependent whether
select() restarts or returns with [EINTR].

Since pqsignal() sets SA_RESTART for all trapped signals except SIGALRM,
that means we are exposing ourselves to the implementation dependency.
What I furthermore realized while tracing is that "restart" means
"start counting down the full timeout interval over again". Thus, if
we have told select() to time out after 1 second, and SIGINT arrives
after 0.9 second, we will wait a full second more before responding.

Bad as that is, it gets worse rapidly: each new signal arrival restarts
the cycle. So a continuous flow of signals at a spacing of less than
1 second would prevent the delay from *ever* terminating.

This may be why some kernels reduce the timeout value before returning,
so that a "restart" behavior in userland behaves sanely. But that's
not what's happening for me :-(.

To me, this calls into question whether we should try to avoid using
SA_RESTART at all. The reason for doing it of course is to avoid
unexpected syscall EINTR failures as well as short read/short write
behaviors during disk I/O. However, if that's the plan then what the
heck is pqsignal() doing giving an exception for SIGALRM? As soon as
you have even one non-restartable trapped signal, it seems you need
to handle EINTR everywhere.

I looked into the CVS history and found that we inherited the SIGALRM
exception from Berkeley (in fact it's in the v4r2 sources from 1994).
Back then the system's usage of SIGALRM was pretty darn narrow --- it
was used only to trigger the deadlock checker, which means it applied
only while waiting for a lock, and the range of code in which the
interrupt could occur was just a few lines. Now that we use SIGALRM for
statement_timeout, the interrupt can potentially happen almost anywhere
in the backend code.

So we've got two problems: SA_RESTART is preventing some EINTRs from
happening when we'd like, and yet it seems we are at risk of unwanted
EINTRs anyway.

The only really clean solution I can see is to stop using SA_RESTART
and try to make all our syscalls EINTR-proof. But the probability
of bugs-of-omission seems just about 100%, especially in third party
backend add-ons that we don't get to review the code for.

If we do nothing, anyone using statement_timeout is at risk. The
risk is somewhat ameliorated by the fact that occurrence of the
interrupt means transaction cancellation anyway, so an unexpected
error of some other type isn't really a fatal problem. But it's
still a bit nervous-making. I don't currently see a way to get
corrupted data from an EINTR (bufmgr is fairly robust about write
failures, for instance) but ...

If we decide to live with that, and fix any reported problems, then
one thing we could do to ameliorate the sleep problem is to turn
off SA_RESTART for all activity-cancelling interrupts, in particular
SIGINT/SIGTERM/SIGQUIT. This wouldn't make it safe for bgwriter
and friends to go back to long sleep intervals, because they are
watching for other interrupts too that don't represent reasons to
cancel transactions. But it would at least solve the problem of
slow response to shutdown requests.

Comments? I sure hope someone has a better idea.

regards, tom lane

On Sun, Jul 01, 2007 at 12:11:26PM -0400, Tom Lane wrote:
> To me, this calls into question whether we should try to avoid using
> SA_RESTART at all. The reason for doing it of course is to avoid
> unexpected syscall EINTR failures as well as short read/short write
> behaviors during disk I/O.

Hrm, is there even a list of syscalls that are considered
restartable/interruptable?

> The only really clean solution I can see is to stop using SA_RESTART
> and try to make all our syscalls EINTR-proof. But the probability
> of bugs-of-omission seems just about 100%, especially in third party
> backend add-ons that we don't get to review the code for.

Not all syscalls can return EINTR and most of the ones that can are not
ones likely to be used by most backend addons. The problem is there
would be a transistion, which would suck. (All the calls that could
return EINTR have to be checked for other errors anyway, but still).

One other possiblity is using something like nanosleep():

Compared to sleep(3) and usleep(3), nanosleep() has the
advantage of not affecting any signals, it is standardized by
POSIX, it provides higher timing resolution, and it allows to
continue a sleep that has been interrupted by a signal more
easily.

But I don't know if that would help on your HPUX box though...

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> One other possiblity is using something like nanosleep():
> But I don't know if that would help on your HPUX box though...

Doesn't help ... the function exists but it seems to have the identical
restarting behavior to select :-(

I had previously tried poll(), noting that its man page didn't say
anything about being restartable, but no luck there either.

I'm prepared to write off HPUX as being broken in this regard, if
no one else reports similar behaviors from their platforms; but we
still have to think about whether statement_timeout isn't putting
us at risk due to SIGALRM not being marked SA_RESTART.

regards, tom lane

This has been saved for the 8.4 release:

http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Tom Lane wrote:
> While poking at the vacuum-launcher issue currently under discussion,
> I got annoyed again at the behavior we've known for a long while that
> on some platforms pg_usleep() won't be interrupted by signals. (In
> particular I see this on HPUX, though not on Linux or Darwin. Anyone
> know if it happens on any BSDen?) I noticed that with the launcher set
> to sleep at most one second between checks for signals, it seemed to
> *always* take the full second before shutting down, which seemed awfully
> unlucky.
>
> Some more testing and man-page-reading revealed the full truth of what's
> going on. The Single Unix Spec's select(2) page says under ERRORS
>
> [EINTR]
> The select() function was interrupted before any of the selected events
> occurred and before the timeout interval expired. If SA_RESTART has been
> set for the interrupting signal, it is implementation-dependent whether
> select() restarts or returns with [EINTR].
>
> Since pqsignal() sets SA_RESTART for all trapped signals except SIGALRM,
> that means we are exposing ourselves to the implementation dependency.
> What I furthermore realized while tracing is that "restart" means
> "start counting down the full timeout interval over again". Thus, if
> we have told select() to time out after 1 second, and SIGINT arrives
> after 0.9 second, we will wait a full second more before responding.
>
> Bad as that is, it gets worse rapidly: each new signal arrival restarts
> the cycle. So a continuous flow of signals at a spacing of less than
> 1 second would prevent the delay from *ever* terminating.
>
> This may be why some kernels reduce the timeout value before returning,
> so that a "restart" behavior in userland behaves sanely. But that's
> not what's happening for me :-(.
>
> To me, this calls into question whether we should try to avoid using
> SA_RESTART at all. The reason for doing it of course is to avoid
> unexpected syscall EINTR failures as well as short read/short write
> behaviors during disk I/O. However, if that's the plan then what the
> heck is pqsignal() doing giving an exception for SIGALRM? As soon as
> you have even one non-restartable trapped signal, it seems you need
> to handle EINTR everywhere.
>
> I looked into the CVS history and found that we inherited the SIGALRM
> exception from Berkeley (in fact it's in the v4r2 sources from 1994).
> Back then the system's usage of SIGALRM was pretty darn narrow --- it
> was used only to trigger the deadlock checker, which means it applied
> only while waiting for a lock, and the range of code in which the
> interrupt could occur was just a few lines. Now that we use SIGALRM for
> statement_timeout, the interrupt can potentially happen almost anywhere
> in the backend code.
>
> So we've got two problems: SA_RESTART is preventing some EINTRs from
> happening when we'd like, and yet it seems we are at risk of unwanted
> EINTRs anyway.
>
> The only really clean solution I can see is to stop using SA_RESTART
> and try to make all our syscalls EINTR-proof. But the probability
> of bugs-of-omission seems just about 100%, especially in third party
> backend add-ons that we don't get to review the code for.
>
> If we do nothing, anyone using statement_timeout is at risk. The
> risk is somewhat ameliorated by the fact that occurrence of the
> interrupt means transaction cancellation anyway, so an unexpected
> error of some other type isn't really a fatal problem. But it's
> still a bit nervous-making. I don't currently see a way to get
> corrupted data from an EINTR (bufmgr is fairly robust about write
> failures, for instance) but ...
>
> If we decide to live with that, and fix any reported problems, then
> one thing we could do to ameliorate the sleep problem is to turn
> off SA_RESTART for all activity-cancelling interrupts, in particular
> SIGINT/SIGTERM/SIGQUIT. This wouldn't make it safe for bgwriter
> and friends to go back to long sleep intervals, because they are
> watching for other interrupts too that don't represent reasons to
> cancel transactions. But it would at least solve the problem of
> slow response to shutdown requests.
>
> Comments? I sure hope someone has a better idea.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Added to TODO:

* Research use of signals and sleep wake ups

http://archives.postgresql.org/pgsql-hackers/2007-07/msg00003.php

---------------------------------------------------------------------------

Tom Lane wrote:
> While poking at the vacuum-launcher issue currently under discussion,
> I got annoyed again at the behavior we've known for a long while that
> on some platforms pg_usleep() won't be interrupted by signals. (In
> particular I see this on HPUX, though not on Linux or Darwin. Anyone
> know if it happens on any BSDen?) I noticed that with the launcher set
> to sleep at most one second between checks for signals, it seemed to
> *always* take the full second before shutting down, which seemed awfully
> unlucky.
>
> Some more testing and man-page-reading revealed the full truth of what's
> going on. The Single Unix Spec's select(2) page says under ERRORS
>
> [EINTR]
> The select() function was interrupted before any of the selected events
> occurred and before the timeout interval expired. If SA_RESTART has been
> set for the interrupting signal, it is implementation-dependent whether
> select() restarts or returns with [EINTR].
>
> Since pqsignal() sets SA_RESTART for all trapped signals except SIGALRM,
> that means we are exposing ourselves to the implementation dependency.
> What I furthermore realized while tracing is that "restart" means
> "start counting down the full timeout interval over again". Thus, if
> we have told select() to time out after 1 second, and SIGINT arrives
> after 0.9 second, we will wait a full second more before responding.
>
> Bad as that is, it gets worse rapidly: each new signal arrival restarts
> the cycle. So a continuous flow of signals at a spacing of less than
> 1 second would prevent the delay from *ever* terminating.
>
> This may be why some kernels reduce the timeout value before returning,
> so that a "restart" behavior in userland behaves sanely. But that's
> not what's happening for me :-(.
>
> To me, this calls into question whether we should try to avoid using
> SA_RESTART at all. The reason for doing it of course is to avoid
> unexpected syscall EINTR failures as well as short read/short write
> behaviors during disk I/O. However, if that's the plan then what the
> heck is pqsignal() doing giving an exception for SIGALRM? As soon as
> you have even one non-restartable trapped signal, it seems you need
> to handle EINTR everywhere.
>
> I looked into the CVS history and found that we inherited the SIGALRM
> exception from Berkeley (in fact it's in the v4r2 sources from 1994).
> Back then the system's usage of SIGALRM was pretty darn narrow --- it
> was used only to trigger the deadlock checker, which means it applied
> only while waiting for a lock, and the range of code in which the
> interrupt could occur was just a few lines. Now that we use SIGALRM for
> statement_timeout, the interrupt can potentially happen almost anywhere
> in the backend code.
>
> So we've got two problems: SA_RESTART is preventing some EINTRs from
> happening when we'd like, and yet it seems we are at risk of unwanted
> EINTRs anyway.
>
> The only really clean solution I can see is to stop using SA_RESTART
> and try to make all our syscalls EINTR-proof. But the probability
> of bugs-of-omission seems just about 100%, especially in third party
> backend add-ons that we don't get to review the code for.
>
> If we do nothing, anyone using statement_timeout is at risk. The
> risk is somewhat ameliorated by the fact that occurrence of the
> interrupt means transaction cancellation anyway, so an unexpected
> error of some other type isn't really a fatal problem. But it's
> still a bit nervous-making. I don't currently see a way to get
> corrupted data from an EINTR (bufmgr is fairly robust about write
> failures, for instance) but ...
>
> If we decide to live with that, and fix any reported problems, then
> one thing we could do to ameliorate the sleep problem is to turn
> off SA_RESTART for all activity-cancelling interrupts, in particular
> SIGINT/SIGTERM/SIGQUIT. This wouldn't make it safe for bgwriter
> and friends to go back to long sleep intervals, because they are
> watching for other interrupts too that don't represent reasons to
> cancel transactions. But it would at least solve the problem of
> slow response to shutdown requests.
>
> Comments? I sure hope someone has a better idea.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:

> So we've got two problems: SA_RESTART is preventing some EINTRs from
> happening when we'd like, and yet it seems we are at risk of unwanted
> EINTRs anyway.
>
> The only really clean solution I can see is to stop using SA_RESTART
> and try to make all our syscalls EINTR-proof. But the probability
> of bugs-of-omission seems just about 100%, especially in third party
> backend add-ons that we don't get to review the code for.

Did we do anything about this? I see we have it on TODO ...

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> So we've got two problems: SA_RESTART is preventing some EINTRs from
>> happening when we'd like, and yet it seems we are at risk of unwanted
>> EINTRs anyway.
>>
>> The only really clean solution I can see is to stop using SA_RESTART
>> and try to make all our syscalls EINTR-proof. But the probability
>> of bugs-of-omission seems just about 100%, especially in third party
>> backend add-ons that we don't get to review the code for.

> Did we do anything about this? I see we have it on TODO ...

No, I haven't done anything about it.

(I'm not entirely convinced that there's a real problem on any modern
platforms.)

regards, tom lane