(2011/06/12 6:43), Noah Misch wrote:
> On Wed, Mar 30, 2011 at 04:48:26PM -0400, Robert Haas wrote:
>> Me neither. If making the deadlock timeout PGC_SUSET is independently
>> useful, I don't object to doing that first, and then we can wait and
>> see if anyone feels motivated to do more.
>
> Here's the patch for that. Not much to it.
I've reviewed the patch following the article in the PostgreSQL wiki.
It seems fine except that it needs to be rebased, so I'll mark this
"Ready for committers'. Please see below for details of my review.
Submission review
=================
The patch is in context diff format, and can be applied with shifting
a hunk. I attached rebased patch.
The patch fixes the document about deadlock_timeout. Changing GUC
setting restriction would not need test.
Usability review
================
The purpose of the patch is to allow only superusers to change
deadlock_timeout GUC parameter. That seems to fit the conclusion of the
thread:
http://archives.postgresql.org/pgsql-hackers/2011-03/msg01727.php
Feature test
============
After applying the patch, non-superuser's attempt to change
deadlock_timeout is rejected with proper error:
ERROR: permission denied to set parameter "deadlock_timeout"
But superusers still can do that.
The fix for the document is fine, and it follows the wording used for
similar cases.
This patch doesn't need any support of external tools such as pg_dump
and psql.
Performance review
==================
This patch would not cause any performance issue.
Coding review
=============
The patch follows coding guidelines, and seems to have no portability
issue. It includes proper comment which describes why the parameter
should not be changed by non-superuser.
The patch produces no compiler warning for both binaries and documents.
Architecture review
===================
AFAICS, this patch adopts the GUC parameter's standards.
Regards,
--
Shigeru Hanada