From: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Marc Cousin <cousinmarc(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Hans-Juergen Schoenig <hs(at)cybertec(dot)at>, Ants Aasma <ants(at)cybertec(dot)at>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | Re: [PATCH] lock_timeout and common SIGALRM framework |
Date: | 2012-07-13 13:45:10 |
Message-ID: | 50002666.7010802@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2012-07-13 14:52:43 | Re: Synchronous Standalone Master Redoux |
Previous Message | Atri Sharma | 2012-07-13 13:43:14 | Re: Regarding installation of FDW on Windows |