Re: [PATCH] lock_timeout and common SIGALRM framework

Excerpts from Boszormenyi Zoltan's message of mar jun 19 04:44:35 -0400 2012:
>
> 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?

Well, that's not exactly what I was thinking, but maybe your idea is
better than what I had in mind.

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

Yeah, except that they play with members of the timeout_params struct,
which hopefully does not need to be exported. So if you can just move
(that is, put back in their original places) the portions that do not
touch that strcut, it'd be great.

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

I see.

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

Oh, right.

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