Re: Minor fix in lwlock.c

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> /* Unlock semaphores first */
> while (extraWaits-- > 0)
>     PGSemaphoreUnlock(&proc->sem);

> /* Add the lock into my list then.
>  * If a process is in exiting status, it could use the reserved lwlocks
>  */
> reserved = proc_exit_inprogress? 0 : NUM_RESERVED_LWLOCKS;
> if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS - reserved)
>     elog(ERROR, "too many LWLocks taken");
> held_lwlocks[num_held_lwlocks++] = lockid;

But if the MAX_SIMUL_LWLOCKS - NUM_RESERVED_LWLOCKS limit is reached,
you elog without having recorded the lock you just took ... which is a
certain loser since nothing will ever release it.  Also,
proc_exit_inprogress is not the appropriate thing to test for unless
you're going to use an elog(FATAL).

I think it would work to record the lock, unwind the extraWaits, and
*then* elog if we're above the allowable limit.  Something like

 if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
     elog(PANIC, "too many LWLocks taken");
 held_lwlocks[num_held_lwlocks++] = lockid;

 while (extraWaits-- > 0)
     PGSemaphoreUnlock(&proc->sem);

 if (!InError && num_held_lwlocks >= MAX_SIMUL_LWLOCKS - NUM_RESERVED_LWLOCKS)
     elog(ERROR, "too many LWLocks taken");

except we don't have the InError flag anymore so there'd need to be some
other test for deciding whether it should be OK to go into the reserved
locks.

But I think this is too much complexity for a case that shouldn't ever
happen.

            regards, tom lane