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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
