|From:||Boszormenyi Zoltan <zb(at)cybertec(dot)at>|
|To:||Marc Cousin <cousinmarc(at)gmail(dot)com>|
|Subject:||Re: lock_timeout GUC patch - Review|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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)
the interactions are yet to be discovered. Setting it globally is not
> * 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
ordered by their fin_time but it doesn't survive "make check", it
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.
|Next Message||Robert Haas||2010-07-29 13:10:27||Re: page corruption on 8.3+ that makes it to standby|
|Previous Message||Oleg Bartunov||2010-07-29 11:03:32||Re: [GENERAL] Incorrect FTS result with GIN index|