Excerpts from Boszormenyi Zoltan's message of mar jun 26 03:59:06 -0400 2012: > > 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".
Oops. Attached. It's pretty much the same you had, except some "bools" are changed to void. > > 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. Well, I don't expect that we're going to have many external uses. My point about registration is so that internal callers use it as well. I was thinking we could do something like xact.c does, which is to have somewhere in proc.c a call like this: TimeoutRegister(DEADLOCK_TIMEOUT, 1, true, CheckDeadLock, GetCurrentTimestamp); at process startup (the magic value 1 is the priority. Maybe there's a better way to handle this). That way, timeout.c or timeout.h do not need to know anything about proc.c at all; we don't need proctimeout.h at all. The only thing it (the timeout module) needs to know is that there is a symbolic constant named DEADLOCK_TIMEOUT. Similarly for statement timeout, etc. When you call enable_timeout you first have to ensure that DEADLOCK_TIMEOUT has been registered; and if it's not, die on an Assert(). That way you ensure that there are no bugs where you try to enable a timeout that hasn't been registered. (If we later find the need to let external modules add timeouts, which I find unlikely, we can have TimeoutRegister return TimeoutName and have this value be dynamically allocated. But for now I think that would be useless complication). The difference with lwlocks is that each lwlock is just a number. The lwlock.c module doesn't need to know anything about how each lwlock is actually used. It's not the same thing here -- which is why I think it would be better to have all modules, even the hardcoded ones, use a registering interface. ... Now, it could be argued that it would be even better to have TimeoutRegister not take the TimeoutName argument, and instead generate it dynamically and pass it back to the caller -- that way you don't need the enum in timeout.h. The problem I have with that idea is that it would force the caller modules to have a global variable to keep track of that, which seems worse to me. > > I haven't looked at the second patch at all yet. > > This is the whole point of the first patch. I know that. But I want to get the details of the framework right before we move on to add more stuff to it. > 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. Sure. That's going to be the case for any other timeout we want to add (which is why I think we don't really need dynamic timeouts). -- Álvaro Herrera <alvhe...@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
1-timeout-framework-v9a.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
