Re: [PATCH] lock_timeout and common SIGALRM framework

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/