(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
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e835e4b..7329281 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
*************** dynamic_library_path = 'C:\tools\postgre
*** 5266,5272 ****
practice. On a heavily loaded server you might want to raise it.
Ideally the setting should exceed your typical transaction time,
so as to improve the odds that a lock will be released before
! the waiter decides to check for deadlock.
</para>
<para>
--- 5266,5273 ----
practice. On a heavily loaded server you might want to raise it.
Ideally the setting should exceed your typical transaction time,
so as to improve the odds that a lock will be released before
! the waiter decides to check for deadlock. Only superusers can change
! this setting.
</para>
<para>
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 92391ed..48ffe95 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_int ConfigureNamesI
*** 1532,1539 ****
},
{
! /* This is PGC_SIGHUP so all backends have the same value. */
! {"deadlock_timeout", PGC_SIGHUP, LOCK_MANAGEMENT,
gettext_noop("Sets the time to wait on a lock before
checking for deadlock."),
NULL,
GUC_UNIT_MS
--- 1532,1539 ----
},
{
! /* This is PGC_SUSET to prevent hiding from log_lock_waits. */
! {"deadlock_timeout", PGC_SUSET, LOCK_MANAGEMENT,
gettext_noop("Sets the time to wait on a lock before
checking for deadlock."),
NULL,
GUC_UNIT_MS
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
