Re: [PATCH] lock_timeout and common SIGALRM framework

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support