Concerns about statement-timeout patch

I've been trying to do some code review of the recent statement-timeout
feature addition, and I've got some fairly serious concerns about it.

One problem that needs discussion is that the enable_sig_alarm and
disable_sig_alarm calls were dropped into postgres.c at rather randomly
chosen places. The disable in particular is wrong because in the normal
case it will occur after we have done transaction commit. This creates
a window between committing a command and reaching the disable call
wherein the timeout interrupt could happen. If it does happen, the
net result will be that the client receives an error message, making
it look like the command failed --- when in fact it was committed.

The simplest solution would be to move the calls into start_xact_command
and finish_xact_command respectively. However that would affect the
semantics a little, in that for a querystring containing BEGIN and/or
COMMIT commands, the timeout would be measured across subsets of the
query string, not the whole string as now. I am not sure if this is
a problem or not; the existing semantics don't exactly match my idea
of a "statement" timeout anyway.

Another possible objection is that end-of-transaction cleanup would
not be counted in the statement timeout. This does not bother me,
since the cleanup should be quick, but maybe it would bother someone.
(I would place the disable after DeferredTriggerEndQuery(), so that
RI triggers are run before we disable the timeout.)

I am also concerned about the fact that the feature requires assuming
that setitimer(ITIMER_REAL) plays nicely with sleep(). The documents
I have say that on some platforms sleep() destroys any pending
ITIMER_REAL setting. We could perhaps fix that by adding a call to
reset the end-of-statement timeout after every sleep() in the backend
... but that's obviously a fragile way to proceed.

Comments?

regards, tom lane