Re: lock_timeout GUC patch - Review

The Monday 02 August 2010 13:59:59, Boszormenyi Zoltan wrote :
> >
> > Also, I made sure that only one or two timeout causes (one of
> > deadlock_timeout
> > and lock_timeout in the first case or statement_timeout plus one of the
> > other two)
> > can be active at a time.
>
> A little clarification is needed. The above statement is not entirely true.
> There can be a case when all three timeout causes can be active, but only
> when deadlock_timeout is the smallest of the three. If the fin_time value
> for the another timeout cause is smaller than for deadlock_timeout then
> there's no point to make deadlock_timeout active. And in case
> deadlock_timeout
> triggers and CheckDeadLock() finds that there really is a deadlock then the
> possibly active lock_timeout gets disabled to avoid calling
> RemoveFromWaitQueue() twice. I hope the comments in the code explain it
> well.
>
> > Previously I was able to trigger a segfault
> > with the default
> > 1sec deadlock_timeout and lock_timeout = 999 or 1001. Effectively, the
> > system's
> > clock resolution makes the lock_timeout and deadlock_timeout equal and
> > RemoveFromWaitQueue() was called twice. This way it's a lot more robust.
> >
>
Ok, I've tested this new version:

This time, it's this case that doesn't work :

Session 1 : lock a table

Session 2 : set lock_timeout to a large value, just to make it obvious (10 seconds).
It times out after 1 s (the deadlock timeout), returning 'could not obtain lock'.
Of course, it should wait 10 seconds.

I really feel that the timeout framework is the way to go here. I know what
you said about it before, but I think that the current code is getting
too complicated, with too many special cases all over.

As this is only my second review and I'm sure I am missing things here, could
someone with more experience have a look and give advice ?

Cheers

Marc