Re: [PATCH] lock_timeout and common SIGALRM framework

2012-06-26 06:59 keltezéssel, Alvaro Herrera írta:
> I cleaned up the framework patch a bit. My version's attached. Mainly,
> returning false for failure in some code paths that are only going to
> have the caller elog(FATAL) is rather pointless -- it seems much better
> to just have the code itself do the elog(FATAL). In any case, I
> searched for reports of those error messages being reported in the wild
> and saw none.

OK. The cleanups are always good.

One nitpick, though. Your version doesn't contain the timeout.h
and compilation fails because of it. :-) You might have forgotten
to do "git commit -a".

> There are other things but they are in the nitpick department. (A
> reference to "->check_timeout" remains that needs to be fixed too).

Yes, it's called ->timeout_func() now.

> There is one thing that still bothers me on this whole framework patch,
> but I'm not sure it's easily fixable. Basically the API to timeout.c is
> the whole list of timeouts and their whole behaviors. If you want to
> add a new one, you can't just call into the entry points, you have to
> modify timeout.c itself (as well as timeout.h as well as whatever code
> it is that you want to add timeouts to). This may be good enough, but I
> don't like it. I think it boils down to proctimeout.h is cheating.
>
> The interface I would actually like to have is something that lets the
> modules trying to get a timeout register the timeout-checking function
> as a callback. API-wise, it would be much cleaner. However, I'm not
> raelly sure that code-wise it would be any cleaner at all. In fact I
> think it'd be rather cumbersome. However, if you could give that idea
> some thought, it'd be swell.

Well, I can make the registration interface similar to how LWLocks
are treated, but that doesn't avoid modification of the base_timeouts
array in case a new internal use case arises. Say:

#define USER_TIMEOUTS 4

int n_timeouts = TIMEOUT_MAX;
static timeout_params base_timeouts[TIMEOUT_MAX + USER_TIMEOUTS];

and register_timeout() adds data after TIMEOUT_MAX.

> I haven't looked at the second patch at all yet.

This is the whole point of the first patch. But as I said above for
registering a new timeout source, it's a new internal use case.
One that touches a lot of places which cannot simply be done
by registering a new timeout source.

--
----------------------------------
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/