Re: lock_timeout GUC patch - Review

Hi, I've been reviewing this patch for the last few days. Here it is :

* Submission review * Is the patch in context diff format?
Yes
 * Does it apply cleanly to the current CVS HEAD?
Yes
 * Does it include reasonable tests, necessary doc patches, etc?
Doc patches are there.
There are no regression tests. Should there be ?

* Usability review * Does the patch actually implement that?
Yes
 * Do we want that?
I think so. At least I'd like to have this feature :)
 * Do we already have it?
No
 * Does it follow SQL spec, or the community-agreed behavior?
I didn't see a clear conclusion from the -hackers thread on this (GUC
vs SQL statement extension)
 * Does it include pg_dump support (if applicable)?
Not applicable. Or should pg_dump and/or pg_restore put lock_timeout to 0 ?
 * Are there dangers?
As it is a guc, it could be set globally, is that a danger ?
 * Have all the bases been covered?
I honestly don't know. It touches alarm signal handling.

* Apply the patch, compile it and test: * Does the feature work as advertised?
I only tested it with Linux. The code is very OS-dependent. It works
as advertised with Linux. I found only one corner case (see below)
 * 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().
 * Are there any assertion failures or crashes?
No


* Performance review * Does the patch slow down simple tests?
No
 * If it claims to improve performance, does it?
Not applicable

* Does it slow down other things?
No. Maybe alarm signal handling and enabling will be slower, as there
is more work done there  (for instance, a GetCurrentTimestamp, that
was only done when log_lock_waits was activated until now. But I
couldn't measure a slowdown.

* Read the changes to the code in detail and consider: * Does it follow the project coding guidelines?
I think so
 * Are there portability issues?
It seems to have already been adressed, from the previous discussion
in the thread.
 * Will it work on Windows/BSD etc?
It should. I couldn't test it though. Infrastructure is there.

* Are the comments sufficient and accurate?
Yes

* Does it do what it says, correctly?
Yes

* Does it produce compiler warnings?
No

* Can you make it crash?
No

* Consider the changes to the code in the context of the project as a whole: * Is everything done in a way that fits
togethercoherently 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.
 * Are there interdependencies that can cause problems?
I don't think so.