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 <alvhe...@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
