Re: deadlock_timeout at < PGC_SIGHUP?

A few years ago, this list had a brief conversation on $SUBJECT:
http://archives.postgresql.org/message-id/1215443493.4051.600.camel@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.

nm

On Tue, Mar 29, 2011 at 1:38 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> A few years ago, this list had a brief conversation on $SUBJECT:
> http://archives.postgresql.org/message-id/1215443493.4051.600.camel@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

On Tue, Mar 29, 2011 at 1:26 PM, Robert Haas <robertmhaas(at)gmail(dot)com> 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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Mar 29, 2011 at 1:38 AM, Noah Misch <noah(at)leadboat(dot)com> 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

On Tue, Mar 29, 2011 at 2:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

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 <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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

On Tue, Mar 29, 2011 at 2:29 PM, Noah Misch <noah(at)leadboat(dot)com> 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

On Wed, Mar 30, 2011 at 04:48:26PM -0400, Robert Haas wrote:
> On Tue, Mar 29, 2011 at 2:29 PM, Noah Misch <noah(at)leadboat(dot)com> 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.

(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

2011/6/17 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (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