2012-06-26 18:12 keltezéssel, Alvaro Herrera írta:
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.
Currently, TimeoutName (as an index value) and "priority" is the same
semantically.
I would also add an Assert into register_timeout() to avoid double registration
of the same to prevent overriding he callback function pointer. If that's in
place, the TimeoutName value may still work as is: an index into
base_timeouts[].
(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.
Strictly speaking, just as TimeoutName.
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.
OK.
... 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.
This was what I had in mind at first ...
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.
... and realized this as well.
So, should I keep the enum TimeoutName? Are global variables for
keeping dynamically assigned values preferred over the enum?
Currently we have 5 timeout sources in total, 3 of them are used by
regular backends, the remaining 2 are used by replication standby.
We can have a fixed size array (say with 8 or 16 elements) for future use
and this would be plenty.
Opinions?
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).
--
----------------------------------
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/
