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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
