Re: lock_timeout GUC patch - Review

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: Marc Cousin <cousinmarc(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: lock_timeout GUC patch - Review
Date: 2010-07-29 11:55:38
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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

Attachment Content-Type Size
5-pg91-locktimeout-18-ctxdiff.patch text/x-patch 36.5 KB
pg91-experimental-lock-framework.patch text/x-patch 60.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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