Re: [PATCH] lock_timeout and common SIGALRM framework

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?

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. ... 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. 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).

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.

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.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support