Re: [PATCH] lock_timeout and common SIGALRM framework

2012-07-11 22:59 keltezéssel, Tom Lane írta:
> I wrote:
>> I'm starting to look at this patch now.
> After reading this further, I think that the "sched_next" option is a
> bad idea and we should get rid of it. AFAICT, what it is meant to do
> is (if !sched_next) automatically do "disable_all_timeouts(true)" if
> the particular timeout happens to fire. But there is no reason the
> timeout's callback function couldn't do that; and doing it in the
> callback is more flexible since you could have logic about whether to do
> it or not, rather than freezing the decision at RegisterTimeout time.
> Moreover, it does not seem to me to be a particularly good idea to
> encourage timeouts to have such behavior, anyway. Each time we add
> another timeout we'd have to look to see if it's still sane for each
> existing timeout to use !sched_next. It would likely be better, in
> most cases, for individual callbacks to explicitly disable any other
> individual timeout reasons that should no longer be fired.

+1

> I am also underwhelmed by the "timeout_start" callback function concept.

It was generalized out of "static TimestampTz timeout_start_time;"
in proc.c which is valid if deadlock_timeout is activated. It is used
in ProcSleep() for logging the time difference between the time when
the timeout was activated and "now" at several places in that function.

> In the first place, that's broken enable_timeout, which incorrectly
> assumes that the value it gets must be "now" (see its schedule_alarm
> call).

You're right, it must be fixed.

> In the second place, it seems fairly likely that callers of
> get_timeout_start would likewise want the clock time at which the
> timeout was enabled, not the timeout_start reference time. (If they
> did want the latter, why couldn't they get it from wherever the callback
> function had gotten it?) I'm inclined to propose that we drop the
> timeout_start concept and instead provide two functions for scheduling
> interrupts:
>
> enable_timeout_after(TimeoutName tn, int delay_ms);
> enable_timeout_at(TimeoutName tn, TimestampTz fin_time);
>
> where you use the former if you want the standard GetCurrentTimestamp +
> n msec calculation, but if you want the stop time calculated in some
> other way, you calculate it yourself and use the second function.
>
> regards, tom lane
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/