Re: [PATCH] lock_timeout and common SIGALRM framework

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.

I am also underwhelmed by the "timeout_start" callback function concept.
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). 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