Re: Minor fix in lwlock.c

"Qingqing Zhou" <zhouqq(at)cs(dot)toronto(dot)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