Re: bug in fast-path locking

On Mon, 2012-04-09 at 13:32 -0400, Robert Haas wrote:
> I looked at this more. The above analysis is basically correct, but
> the problem goes a bit beyond an error in LockWaitCancel(). We could
> also crap out with an error before getting as far as LockWaitCancel()
> and have the same problem. I think that a correct statement of the
> problem is this: from the time we bump the strong lock count, up until
> the time we're done acquiring the lock (or give up on acquiring it),
> we need to have an error-cleanup hook in place that will unbump the
> strong lock count if we error out. Once we're done updating the
> shared and local lock tables, the special handling ceases to be
> needed, because any subsequent lock release will go through
> LockRelease() or LockReleaseAll(), which will do the appropriate
> clenaup.
>
> The attached patch is an attempt at implementing that; any reviews appreciated.
>

This path doesn't have an AbortStrongLockAcquire:

if (!(proclock->holdMask & LOCKBIT_ON(lockmode)))
{
...
elog(ERROR,...)

but other similar paths do:

if (!proclock)
{
AbortStrongLockAcquire();

I don't think it's necessary outside of LockErrorCleanup(), right?

I think there could be some more asserts. There are three sites that
decrement FastPathStrongRelationLocks, but in two of them
StrongLockInProgress should always be NULL.

Other than that, it looks good to me.

Regards,
Jeff Davis