Re: lock_timeout GUC patch - Review

The Thursday 29 July 2010 13:55:38, Boszormenyi Zoltan wrote :
> I fixed this by adding CheckLockTimeout() function that works like
> CheckStatementTimeout() and ensuring that the same start time is
> used for both deadlock_timeout and lock_timeout if both are active.
> The preference of errors if their timeout values are equal is:
> statement_timeout > lock_timeout > deadlock_timeout

As soon as lock_timeout is bigger than deadlock_timeout, it doesn't
work, with this new version.

Keeping the deadlock_timeout to 1s, when lock_timeout >= 1001,
lock_timeout doesn't trigger anymore.

>
> > * Consider the changes to the code in the context of the project as a whole:
> > * Is everything done in a way that fits together coherently with
> > other features/modules?
> > I have a feeling that
> > enable_sig_alarm/enable_sig_alarm_for_lock_timeout tries to solve a
> > very specific problem, and it gets complicated because there is no
> > infrastructure in the code to handle several timeouts at the same time
> > with sigalarm, so each timeout has its own dedicated and intertwined
> > code. But I'm still discovering this part of the code.
> >
>

> This WIP patch is also attached for reference, too. I would prefer
> this way, but I don't have more time to work on it and there are some
> interdependencies in the signal handler when e.g. disable_sig_alarm(true);
> means to disable ALL timers not just the statement_timeout.
> The specifically coded lock_timeout patch goes with the flow and doesn't
> change the semantics and works. If someone wants to pick up the timer
> framework patch and can make it work, fine. But I am not explicitely
> submitting it for the commitfest. The original patch with the fixes works
> and needs only a little more review.

Ok, understood. But I like the principle of this framework much more (the rest
of the code seems simpler to me as a consequence of this framework).

But it goes far beyond the initial intent of the patch.