Re: [PATCH] lock_timeout and common SIGALRM framework

2012-06-18 19:46 keltezéssel, Alvaro Herrera írta:
> Excerpts from Boszormenyi Zoltan's message of vie may 11 03:54:13 -0400 2012:
>> Hi,
>>
>> another rebasing and applied the GIT changes in
>> ada8fa08fc6cf5f199b6df935b4d0a730aaa4fec to the
>> Windows implementation of PGSemaphoreTimedLock.
> Hi,
>
> I gave the framework patch a look. One thing I'm not sure about is the
> way you've defined the API. It seems a bit strange to have a nice and
> clean, generic interface in timeout.h; and then have the internal
> implementation of the API cluttered with details such as what to do when
> the deadlock timeout expires. Wouldn't it be much better if we could
> keep the details of CheckDeadLock itself within proc.c, for example?

Do you mean adding a callback function argument to for enable_timeout()
would be better?

> Same for the other specific Check*Timeout functions. It seems to me
> that the only timeout.c specific requirement is to be able to poke
> base_timeouts[].indicator and fin_time. Maybe that can get done in
> timeout.c and then have the generic checker call the module-specific
> checker.

Or instead of static functions, Check* functions can be external
to timeout.c. It seemed to be a good idea to move all the timeout
related functions to timeout.c.

> ... I see that you have things to do before and after setting
> "indicator". Maybe you could split the module-specific Check functions
> in two and have timeout.c call each in turn. Other than that I don't
> see that this should pose any difficulty.
>
> Also, I see you're calling GetCurrentTimestamp() in the checkers; maybe
> more than once per signal if you have multiple timeouts enabled.

Actually, GetCurrentTimestamp() is not called multiple times,
because only the first timeout source in the queue can get triggered.

> Maybe
> it'd be better to call it in once handle_sig_alarm and then pass the
> value down, if necessary (AFAICS if you restructure the code as I
> suggest above, you don't need to get the value down the the
> module-specific code).

But yes, this way it can be cleaner.

>
> As for the Init*Timeout() and Destroy*Timeout() functions, they seem
> a bit pointless -- I mean if they just always call the generic
> InitTimeout and DestroyTimeout functions, why not just define the struct
> in a way that makes the specific functions unnecessary? Maybe you even
> already have all that's necessary ... I think you just change
> base_timeouts[i].timeout_destroy(false); to DestroyTimeout(i, false) and
> so on.

OK, I will experiment with it.

> Minor nitpick: the "Interface:" comment in timeout.c seems useless.
> That kind of thing tends to get overlooked and obsolete over time. We
> have examples of such things all over the place. I'd just rip it and
> have timeout.h be the interface documentation.

OK.

> BTW it doesn't seem that datatype/timestamp.h is really necessary in
> timeout.h.

You are wrong. get_timeout_start() returns TimestampTz and it's
defined in datatype/timestamp.h.

Thanks for the review, I will send the new code soon.

Best regards,
Zoltán Böszörményi

--
----------------------------------
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/