Re: lock_timeout GUC patch - Review

Hi,

Marc Cousin írta:
> Hi, I've been reviewing this patch for the last few days. Here it is :
>
...
>   * Are there dangers?
> As it is a guc, it could be set globally, is that a danger ?
>

I haven't added any new code covering supplemental (e.g. autovacuum)
processes,
the interactions are yet to be discovered. Setting it globally is not
recommended.

>   * Are there corner cases the author has failed to consider?
> The feature almost works as advertised : it fails when lock_timeout =
> deadlock_timeout. Then the lock_timeout isn't detected. I think this
> case isn't considered in handle_sig_alarm().
>

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

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

I tried to create a framework that could potentially handle any number
timeouts
ordered by their fin_time but it doesn't survive "make check", it
reliably stalls
at the test in parallel_schedule below:

# ----------
# Another group of parallel tests
# ----------
test: select_into select_distinct select_distinct_on select_implicit
select_having subselect union case join aggregates transactions random
portals arrays btree_index hash_index update namespace prepared_xacts delete

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.

Best regards,
Zoltán Böszörményi