Re: Another bug introduced by fastpath patch - Mailing list pgsql-hackers

On Wed, Nov 27, 2013 at 8:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> We could still do this if we were willing to actually reject requests
>> for session level locks on fast-path-eligible lock types.  At the moment
>> that costs us nothing really.  If anyone ever did have a use case, we
>> could consider adding the extra logic to support it.
>
> Nope, that *still* doesn't work, because in non-allLocks mode the main
> loop won't clear any locks that have been promoted from fastpath to
> regular.  Sigh.  For the moment I'm proposing that we just re-fetch
> the list header after acquiring the lock.  The attached patch is slightly
> more verbose than that, because I took the opportunity to reformulate the
> while() loop as a for() loop and thereby eliminate some goto's.

This approach looks good to me.  When I originally worked on this, I
struggled quite a bit with the idea that LockReleaseAll() might really
mean LockReleaseSome().  I thought I'd come up with an approach that
was adequate, but evidently not entirely.

In terms of making this more robust, one idea - along the lines you
mentioned in your original post - is to have a separate code path for
the case where we're releasing *all* locks.  Such a mode could simply
acquire backendLock, clear fpLockBits and fpVXIDLock, and release the
backendLock.  After that, we can just walk through all the PROCLOCK
lists (nothing can be getting added concurrently, since we no longer
have any fast-path locks for someone else to transfer) and nuke
everything we find there.  This code could be used not only in the
allLocks = true case but also whenever we don't hold any session
locks, which is nearly all the time.  (I'm here assuming that we have
some cheap way of knowing whether that's the case, but that shouldn't
be too hard to contrive.)

Of course, that doesn't solve the problem of what to do when we do
hold session locks and want to release everything else.  I'm not sure
there's an easy option that's any better than what we have now.  The
problem of releasing all of our transaction-lifespan locks while
holding on to session-lifespan locks seems related to the problem of
releasing locks for an aborted subtransaction while holding onto the
parent's locks, but those are handled in completely different ways.
We could rip out the non-allLocks case from LockReleaseAll()
altogether and rely on retail lock release in all cases where we are
not releasing *everything*, but that would probably be significantly
worse in cases where we hold one session lifespan lock plus a large
number of transaction-lifespan locks.  That's probably a fairly rare
case, but I'm not sure the benefits in maintainability are enough to
justify such a change.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company