Re: Refactoring in lock.c

Alvaro Herrera <alvherre@surnet.cl> writes:
> Here is a small patch to refactor common functionality out of
> LockRelease and LockReleaseAll, creating a new static function
> RemoveProcLock.
> (This comes from Heikki's two-phase commit patch, where it is used more.)

I was just looking at this code in the context of Heikki's patch, and
it seemed to have some issues: specifically that the code
        if (wakeupNeeded)
            ProcLockWakeup(lockMethodTable, lock);
was formerly run only if the lock still had nRequested > 0.  Since the
case where nRequested == 0 causes the lock to be physically removed,
it would not be merely inefficient but actually a use of a dangling
pointer to call ProcLockWakeup when the lock's been removed.  However
the patched code now does the above unconditionally.  Can you prove
that wakeupNeeded will never be true when nRequested == 0?

It might be safer to migrate the ProcLockWakeup call inside
RemoveProcLock.

FWIW, I agree with turning the WARNINGs into ERRORs and removing the
useless return value from LockRelease et al.

            regards, tom lane