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

Wed, 30 Oct 2002 14:43:36 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Concerns about statement-timeout patch

Added to TODO:

 * Research interaction of setitimer() and sleep() used by statement_timeout

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

Tom Lane wrote:
> 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
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
> http://www.postgresql.org/users-lounge/docs/faq.html
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Thu, 7 Nov 2002 23:29:18 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Concerns about statement-timeout patch