Re: Spurious standby query cancellations - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Spurious standby query cancellations
Date
Msg-id CAA4eK1Jk7ax6YdYVgSJ80p=9Gqyi6C-AJ=S_ZHT5As1vV7+LyA@mail.gmail.com
Whole thread Raw
In response to Re: Spurious standby query cancellations  (Jeff Janes <jeff.janes@gmail.com>)
List pgsql-hackers
On Fri, Dec 25, 2015 at 1:45 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>
> On Wed, Dec 23, 2015 at 9:40 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
> > On Wed, Sep 23, 2015 at 11:33 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
> >>
> >> On further thought, neither do I.  The attached patch inverts
> >> ResolveRecoveryConflictWithLock to be called back from the lmgr code so that
> >> is it like ResolveRecoveryConflictWithBufferPin code.  It does not try to
> >> cancel the conflicting lock holders from the signal handler, rather it just
> >> loops an extra time and cancels the transactions on the next call.
> >>
> >> It looks like the deadlock detection is adequately handled within normal
> >> lmgr code within the back-ends of the other parties to the deadlock, so I
> >> didn't do a timeout for deadlock detection purposes.
> >
> > I was testing that this still applies to git HEAD.  And it doesn't due
> > to the re-factoring of lock.h into lockdef.h.  (And apparently it
> > never actually did, because that refactoring happened long before I
> > wrote this patch.  I guess I must have done this work against
> > 9_5_STABLE.)
> >
> > standby.h cannot include lock.h because standby.h is included
> > indirectly by pg_xlogdump.  But it has to get LOCKTAG which is only in
> > lock.h.
> >
> > Does this mean that standby.h also needs to get parts spun off into a
> > new standbydef.h that can be included from front-end code?
>
>
> That is how I've done it.
>
> The lock cancel patch applies over the header split patch.
>

Review Comments -

1.
/*
  * If somebody wakes us between LWLockRelease and WaitLatch, the latch
--- 1081,1103 ----
  * If 
LockTimeout is set, also enable the timeout for that.  We can save a
  * few cycles by enabling both 
timeout sources in one call.
  */
! if (!InHotStandby)
  {
! if (LockTimeout > 0)
!
{
! EnableTimeoutParams timeouts[2];
!
! timeouts[0].id = 
DEADLOCK_TIMEOUT;
! timeouts[0].type = TMPARAM_AFTER;
! timeouts
[0].delay_ms = DeadlockTimeout;
! timeouts[1].id = LOCK_TIMEOUT;
!
timeouts[1].type = TMPARAM_AFTER;
! timeouts[1].delay_ms = LockTimeout;
!
enable_timeouts(timeouts, 2);
! }
! else
! enable_timeout_after
(DEADLOCK_TIMEOUT, DeadlockTimeout);
  }

If you are enabling the timeout only in non-hotstandby mode, then
later in code (in same function) it should be disabled under the same
condition, otherwise it will lead to assertion failure.

2.
+ /*
+ * Clear any timeout requests established above.  We assume here that the
+ * Startup 
process doesn't have any other outstanding timeouts than what 
+ * this function
+ * uses.  If 
that stops being true, we could cancel the timeouts
+ * individually, but that'd be slower.
+ */

Comment seems to be misaligned.

3.
+ void
+ StandbyLockTimeoutHandler(void)
+ {
+ /* forget any pending STANDBY_DEADLOCK_TIMEOUT request */
+
disable_timeout(STANDBY_DEADLOCK_TIMEOUT, false);
+ }

Is there any reason to disable deadlock timeout when it is not
enabled by ResolveRecoveryConflictWithLock()?



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

pgsql-hackers by date:

Previous
From: Craig Ringer
Date:
Subject: Re: [JDBC] JDBC behaviour
Next
From: Filip Rembiałkowski
Date:
Subject: Re: proposal: make NOTIFY list de-duplication optional