Обсуждение: Spurious standby query cancellations

In ResolveRecoveryConflictWithLock, there is the comment:

    /*

     * If blowing away everybody with conflicting locks doesn't work, after

     * the first two attempts then we just start blowing everybody away until

     * it does work.

But what it does is something different than that.  

At least under some conditions, it will wait patiently for all initially conflicting locks to go away.  If that doesn't work twice (because new lockers joined while we were waiting for the old ones to go away), then it will wait patiently for all transactions throughout the system to go away even if they don't conflict, perhaps not even realizing that the lock has become free in the mean time.

Then when its patience runs out, it kills everything on the system.  But it never tried to blow away just the conflicting locks, instead it just tried to wait them out.

The fact that trying to wait them out didn't work (twice) doesn't mean that killing them wouldn't have worked.

I think that it was intended that num_attempts would get incremented only once WaitExceedsMaxStandbyDelay becomes true, but that is not what happens with the current code.

Currently WaitExceedsMaxStandbyDelay only has one caller.  I think it we should take the sleep code out of that function and move it into the existing call site, and then have ResolveRecoveryConflictWithLock check with WaitExceedsMaxStandbyDelay before incrementing num_attempts.

On 27 August 2015 at 22:55, Jeff Janes <jeff.janes@gmail.com> wrote:

In ResolveRecoveryConflictWithLock, there is the comment:

    /*

     * If blowing away everybody with conflicting locks doesn't work, after

     * the first two attempts then we just start blowing everybody away until

     * it does work.

But what it does is something different than that.  

At least under some conditions, it will wait patiently for all initially conflicting locks to go away.  If that doesn't work twice (because new lockers joined while we were waiting for the old ones to go away), then it will wait patiently for all transactions throughout the system to go away even if they don't conflict, perhaps not even realizing that the lock has become free in the mean time.

Then when its patience runs out, it kills everything on the system.  But it never tried to blow away just the conflicting locks, instead it just tried to wait them out.

The fact that trying to wait them out didn't work (twice) doesn't mean that killing them wouldn't have worked.

I think that it was intended that num_attempts would get incremented only once WaitExceedsMaxStandbyDelay becomes true, but that is not what happens with the current code.

Currently WaitExceedsMaxStandbyDelay only has one caller.  I think it we should take the sleep code out of that function and move it into the existing call site, and then have ResolveRecoveryConflictWithLock check with WaitExceedsMaxStandbyDelay before incrementing num_attempts.

I agree with the problem in the current code, thank you for spotting it. 

I also agree that the proposed solution would work-ish, if we are suggesting backpatching this.

It's now possible to fix this by putting a lock wait on the actual lock request, which wasn't available when I first wrote that, hence the crappy wait loop. Using the timeout handler would now be the preferred way to solve this. We can backpatch that to 9.3 if needed, where they were introduced.

There's an example of how to use lock waits further down on ResolveRecoveryConflictWithBufferPin(). Could you have a look at doing it that way?

We probably also need to resurrect my earlier patch to avoid logging AccessExclusiveLocks on temp tables.

It's now possible to fix this by putting a lock wait on the actual lock request, which wasn't available when I first wrote that, hence the crappy wait loop. Using the timeout handler would now be the preferred way to solve this. We can backpatch that to 9.3 if needed, where they were introduced.

There's an example of how to use lock waits further down on ResolveRecoveryConflictWithBufferPin(). Could you have a look at doing it that way?

It looks like this will take some major surgery.  The heavy weight lock manager doesn't play well with others when it comes to timeouts other than its own.  LockBufferForCleanup is a simple retry loop, but the lock manager is much more complicated than that.

We probably also need to resurrect my earlier patch to avoid logging AccessExclusiveLocks on temp tables.

I couldn't find that, can you provide a link?

Another source of frequent AccessExclusiveLocks is vacuum truncation attempts.  If a table has some occupied pages right at the end which are marked as all-visible, then forward scan doesn't visit them.  Since it didn't visit them, it assumes they might be empty and truncatable and so takes the AccessExclusiveLock, only to immediately see that the last page is not empty.  Perhaps the forward scan could be special-cased to never skip the final block of a table.  That way if it is not empty, the truncation is abandoned before taking the AccessExclusiveLock.

On 14 September 2015 at 12:00, Jeff Janes <jeff.janes@gmail.com> wrote:

 

It's now possible to fix this by putting a lock wait on the actual lock request, which wasn't available when I first wrote that, hence the crappy wait loop. Using the timeout handler would now be the preferred way to solve this. We can backpatch that to 9.3 if needed, where they were introduced.

There's an example of how to use lock waits further down on ResolveRecoveryConflictWithBufferPin(). Could you have a look at doing it that way?

It looks like this will take some major surgery.  The heavy weight lock manager doesn't play well with others when it comes to timeouts other than its own.  LockBufferForCleanup is a simple retry loop, but the lock manager is much more complicated than that.

Not sure I understand this objection. I can't see a reason that my proposal wouldn't work.

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.

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.
>

That is how I've done it.