From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alvaro Herrera <alvherre(at)surnet(dot)cl> |
Cc: | Patches <pgsql-patches(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Neil Conway <neilc(at)samurai(dot)com> |
Subject: | Re: Refactoring in lock.c |
Date: | 2005-05-18 22:16:37 |
Message-ID: | 25935.1116454597@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
Alvaro Herrera <alvherre(at)surnet(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Neil Conway | 2005-05-19 01:40:32 | Re: patches for items from TODO list |
Previous Message | Tom Lane | 2005-05-18 21:53:08 | Re: numeric precision when raising one numeric to another. |