Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
2011/6/17 Shigeru Hanada : > (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'. OK, committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
(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. --- 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. 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
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Wed, Mar 30, 2011 at 04:48:26PM -0400, Robert Haas wrote: > On Tue, Mar 29, 2011 at 2:29 PM, Noah Misch wrote: > >> It's actually not > >> clear to me what the user could usefully do other than trying to > >> preserve his transaction by setting a high deadlock_timeout - what is > >> the use case, other than that? > > > > The other major use case is reducing latency in deadlock-prone > > transactions. ?By > > reducing deadlock_timeout for some or all involved transactions, the error > > will > > arrive earlier. > > Good point. > > >> Is it worth thinking about having an explicit setting for deadlock > >> priority? ?That'd be more work, of course, and I'm not sure it it's > >> worth it, but it'd also provide stronger guarantees than you can get > >> with this proposal. > > > > That is a better UI for the first use case. ?I have only twice wished to > > tweak > > deadlock_timeout: once for the use case you mention, another time for that > > second use case. ?Given that, I wouldn't have minded a rough UI. ?If you'd > > use > > this often and assign more than two or three distinct priorities, you'd > > probably > > appreciate the richer UI. ?Not sure how many shops fall in that group. > > 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. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 3981969..f94782f 100644 *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** *** 5258,5264 dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' 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. --- 5258,5265 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. diff --git a/src/backend/utils/index 92391ed..48ffe95 100644 *** a/src/backend/utils/misc/guc.c --- b/src/backend/utils/misc/guc.c *** *** 1532,1539 static struct config_int ConfigureNamesInt[] = }, { ! /* 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
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Tue, Mar 29, 2011 at 2:29 PM, Noah Misch wrote: >> It's actually not >> clear to me what the user could usefully do other than trying to >> preserve his transaction by setting a high deadlock_timeout - what is >> the use case, other than that? > > The other major use case is reducing latency in deadlock-prone transactions. > By > reducing deadlock_timeout for some or all involved transactions, the error > will > arrive earlier. Good point. >> Is it worth thinking about having an explicit setting for deadlock >> priority? That'd be more work, of course, and I'm not sure it it's >> worth it, but it'd also provide stronger guarantees than you can get >> with this proposal. > > That is a better UI for the first use case. I have only twice wished to tweak > deadlock_timeout: once for the use case you mention, another time for that > second use case. Given that, I wouldn't have minded a rough UI. If you'd use > this often and assign more than two or three distinct priorities, you'd > probably > appreciate the richer UI. Not sure how many shops fall in that group. 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. (Of course, we're speaking of 9.2.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Tue, Mar 29, 2011 at 08:26:44AM -0400, Robert Haas wrote: > I'd be inclined to think that PGC_SUSET is plenty. Agreed. A superuser who would have liked PGC_USERSET can always provide a SECURITY DEFINER function. > It's actually not > clear to me what the user could usefully do other than trying to > preserve his transaction by setting a high deadlock_timeout - what is > the use case, other than that? The other major use case is reducing latency in deadlock-prone transactions. By reducing deadlock_timeout for some or all involved transactions, the error will arrive earlier. > Is it worth thinking about having an explicit setting for deadlock > priority? That'd be more work, of course, and I'm not sure it it's > worth it, but it'd also provide stronger guarantees than you can get > with this proposal. That is a better UI for the first use case. I have only twice wished to tweak deadlock_timeout: once for the use case you mention, another time for that second use case. Given that, I wouldn't have minded a rough UI. If you'd use this often and assign more than two or three distinct priorities, you'd probably appreciate the richer UI. Not sure how many shops fall in that group. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
Excerpts from Greg Stark's message of mar mar 29 11:15:50 -0300 2011: > Alternatively we could have the deadlock timer reset all the time and > fire repeatedly. Then we could just have all backends ignore the > deadlock if they're not the lowest priority session in the cycle. But > this depends on everyone knowing everyone else's priority (and having > a consistent view of it). Presumably it'd be published in MyProc before going to sleep, so it'd be available for everyone and always consistent. Not sure if this requires extra locking, though. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Tue, Mar 29, 2011 at 2:20 PM, Tom Lane wrote: > IIRC, the > current deadlock detector always kills the process that detected the > deadlock, but I *think* that's just a random choice and not an essential > feature. If so, it'd be pretty easy to instead kill the lowest-priority > xact among those involved in the deadlock. I think it was just easier. To kill yourself you just exit with an error. To kill someone else you have to deliver a signal and hope the other process exits cleanly. There are a bunch of things to wonder about too. If you don't kill yourself then you might still be in a deadlock cycle so presumably you have to reset the deadlock timer? What if two backends both decide to kill the same other backend. Does it handle getting a spurious signal late cleanly? How does it know which transaction the signal was for? Alternatively we could have the deadlock timer reset all the time and fire repeatedly. Then we could just have all backends ignore the deadlock if they're not the lowest priority session in the cycle. But this depends on everyone knowing everyone else's priority (and having a consistent view of it). -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
Robert Haas writes: > On Tue, Mar 29, 2011 at 1:38 AM, Noah Misch wrote: >> What is notable/surprising about the behavior when two backends have >> different >> values for deadlock_timeout? > I'd be inclined to think that PGC_SUSET is plenty. It's actually not > clear to me what the user could usefully do other than trying to > preserve his transaction by setting a high deadlock_timeout - what is > the use case, other than that? Yeah, that was my reaction too: what is the use case for letting different backends have different settings? It fails to give any real guarantees about who wins a deadlock, and I can't see any other reason for wanting session-specific settings. I don't know how difficult a priority setting would be. IIRC, the current deadlock detector always kills the process that detected the deadlock, but I *think* that's just a random choice and not an essential feature. If so, it'd be pretty easy to instead kill the lowest-priority xact among those involved in the deadlock. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Tue, Mar 29, 2011 at 1:26 PM, Robert Haas wrote: > Is it worth thinking about having an explicit setting for deadlock > priority? That'd be more work, of course, and I'm not sure it it's > worth it, but it'd also provide stronger guarantees than you can get > with this proposal. Priority makes better sense, I think. That's what we're trying to control after all. But you would need to change the way the deadlock detector works... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout at < PGC_SIGHUP?
On Tue, Mar 29, 2011 at 1:38 AM, Noah Misch wrote: > A few years ago, this list had a brief conversation on $SUBJECT: > http://archives.postgresql.org/message-id/1215443493.4051.600.ca...@ebony.site > > What is notable/surprising about the behavior when two backends have different > values for deadlock_timeout? After sleeping to acquire a lock, each backend > will scan for deadlocks every time its own deadlock_timeout elapses. Some > might > be surprised that a larger-deadlock_timeout backend can still be the one to > give > up; consider this timeline: > > Backend Time Command > A N/A SET deadlock_timeout = 1000 > B N/A SET deadlock_timeout = 100 > A 0 LOCK t > B 50 LOCK u > A 100 LOCK u > B 1050 LOCK t > (Backend A gets an ERROR at time 1100) > > More generally, one cannot choose deadlock_timeout values for two sessions > such > that a specific session will _always_ get the ERROR. However, one can drive > the > probability rather high. Compare to our current lack of control. > Is some other behavior that only arises when backends have different > deadlock_timeout settings more surprising than that one? > > If we could relax deadlock_timeout to a GucContext below PGC_SIGHUP, it would > probably need to stop at PGC_SUSET for now. Otherwise, an unprivileged user > could increase deadlock_timeout to hide his lock waits from log_lock_waits. > One > could remove that limitation by introducing a separate log_lock_waits timeout, > but that patch would be significantly meatier. Some might also object to > PGC_USERSET on the basis that a user could unfairly preserve his transaction > by > setting a high deadlock_timeout. However, that user could accomplish a > similar > denial of service by idly holding locks or trying deadlock-prone lock > acquisitions in subtransactions. I'd be inclined to think that PGC_SUSET is plenty. It's actually not clear to me what the user could usefully do other than trying to preserve his transaction by setting a high deadlock_timeout - what is the use case, other than that? Is it worth thinking about having an explicit setting for deadlock priority? That'd be more work, of course, and I'm not sure it it's worth it, but it'd also provide stronger guarantees than you can get with this proposal. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout
Tom Lane wrote: > Simon Riggs <[EMAIL PROTECTED]> writes: > > Why is deadlock_timeout set at SIGHUP? > > Because it's not clear what the behavior would be like if different > backends had different settings ... except that it'd probably be > surprising. I have added a code comment explaining this. -- Bruce Momjian <[EMAIL PROTECTED]>http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout
On Mon, 2008-07-07 at 11:16 -0400, Tom Lane wrote: > Simon Riggs <[EMAIL PROTECTED]> writes: > > Why is deadlock_timeout set at SIGHUP? > > Because it's not clear what the behavior would be like if different > backends had different settings ... except that it'd probably be > surprising. Yeh, agreed. I was thinking to set the deadlock_timeout the same for all people running the deadlock-prone transactions and set it higher for others. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] deadlock_timeout
Simon Riggs <[EMAIL PROTECTED]> writes: > Why is deadlock_timeout set at SIGHUP? Because it's not clear what the behavior would be like if different backends had different settings ... except that it'd probably be surprising. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers