The chance that num_held_lwlocks is beyond MAX_SIMUL_LWLOCKS is similar to
the chance that failed to grasp a spinlock in 1 minute, so they should be
treated in the same way. This is mainly to prevent programming error (e.g.,
forget to release the LWLocks).

Regards,
Qingqing

---

Index: lwlock.c
================================================== =================
RCS file: /projects/cvsroot/pgsql/src/backend/storage/lmgr/lwlock.c,v
retrieving revision 1.25
diff -c -r1.25 lwlock.c
*** lwlock.c 31 Dec 2004 22:01:05 -0000 1.25
--- lwlock.c 8 Apr 2005 02:19:31 -0000
***************
*** 328,334 ****
SpinLockRelease_NoHoldoff(&lock->mutex);

/* Add lock to list of locks held by this backend */
! Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
held_lwlocks[num_held_lwlocks++] = lockid;

/*
--- 328,335 ----
SpinLockRelease_NoHoldoff(&lock->mutex);

/* Add lock to list of locks held by this backend */
! if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
! elog(FATAL, "Too many LWLocks");
held_lwlocks[num_held_lwlocks++] = lockid;

/*
***************
*** 397,403 ****
else
{
/* Add lock to list of locks held by this backend */
! Assert(num_held_lwlocks < MAX_SIMUL_LWLOCKS);
held_lwlocks[num_held_lwlocks++] = lockid;
}

--- 398,405 ----
else
{
/* Add lock to list of locks held by this backend */
! if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
! elog(FATAL, "Too many LWLocks");
held_lwlocks[num_held_lwlocks++] = lockid;
}

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> The chance that num_held_lwlocks is beyond MAX_SIMUL_LWLOCKS is similar to
> the chance that failed to grasp a spinlock in 1 minute, so they should be
> treated in the same way. This is mainly to prevent programming error (e.g.,
> forget to release the LWLocks).

Hmm ... yeah, it's not too hard to imagine a bug leading to trying to
grab content locks on more than 100 buffers, for example. Patch
applied, although I reduced the severity from FATAL to ERROR. I don't
see any reason to think we'd be unable to recover normally from such a
bug --- do you?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

"Tom Lane" <tgl@sss.pgh.pa.us> writes
> I don't see any reason to think we'd be unable to recover normally from
such a
> bug --- do you?
>

I guess the problem is here:

/*
* Fix the process wait semaphore's count for any absorbed wakeups.
*/
while (extraWaits-- > 0)
PGSemaphoreUnlock(&proc->sem);

elog(ERROR) won't recover semaphore count.

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> I guess the problem is here:

> /*
> * Fix the process wait semaphore's count for any absorbed wakeups.
> */
> while (extraWaits-- > 0)
> PGSemaphoreUnlock(&proc->sem);

Mmm. Could be a problem, but the chances of having extraWaits>0 is
really pretty small. In any case, FATAL doesn't fix this, because
it will still try to go through normal backend exit cleanup which
requires having working LWLock support. If you take the above risk
seriously then you need a PANIC error.

The alternative would be to move the Unlock loop in front of the
addition of the LWLock to held_lwlocks[], but I think that cure
is probably worse than the disease --- the chance of an error during
Unlock seems nonzero.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 8: explain analyze is your friend

"Tom Lane" <tgl@sss.pgh.pa.us> writes>
> The alternative would be to move the Unlock loop in front of the
> addition of the LWLock to held_lwlocks[], but I think that cure
> is probably worse than the disease --- the chance of an error during
> Unlock seems nonzero.
>

Another alternative might use PG_TRY/PG_CATCH to make sure that the
semaphore is released. But seems this costs too much ...

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> Another alternative might use PG_TRY/PG_CATCH to make sure that the
> semaphore is released. But seems this costs too much ...

I agree. LWLockAcquire is a hot-spot already.

Maybe we *should* make it a PANIC. Thoughts?

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"Tom Lane" <tgl@sss.pgh.pa.us> writes
>
> Maybe we *should* make it a PANIC. Thoughts?
>

Reasonable. Since this should *never* happen. Once happened, that's means we
have a serious bug in our design/coding.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> writes
>> Maybe we *should* make it a PANIC. Thoughts?

> Reasonable. Since this should *never* happen. Once happened, that's means we
> have a serious bug in our design/coding.

Plan C would be something like

if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
{
release the acquired lock;
elog(ERROR, "too many LWLocks taken");
}

But we couldn't just call LWLockRelease, since it expects the lock to
be recorded in held_lwlocks[]. We'd have to duplicate a lot of code,
or split LWLockRelease into multiple routines, neither of which seem
attractive answers considering that this must be a can't-happen
case anyway.

PANIC it will be, unless someone thinks of a reason why not by
tomorrow...

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/docs/faq

"Tom Lane" <tgl@sss.pgh.pa.us> writes
> Plan C would be something like
>
> if (num_held_lwlocks >= MAX_SIMUL_LWLOCKS)
> {
> release the acquired lock;
> elog(ERROR, "too many LWLocks taken");
> }
>
> But we couldn't just call LWLockRelease, since it expects the lock to
> be recorded in held_lwlocks[]. We'd have to duplicate a lot of code,
> or split LWLockRelease into multiple routines, neither of which seem
> attractive answers considering that this must be a can't-happen
> case anyway.

We can reserve some LWLocks for elog(FATAL) since the shmem_exit() would
need it (Seems elog(ERROR) does not need it). So even if ERROR is upgraded
to FATAL in some cases (e.g., PGSemaphoreUnlock() fails), we could still
exit gracefully. The code will be like this:

---
/* 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;
---

Since this is a should-not-happen case, so the fix could be reserved for
tomorrow when we need PG to grasp more LWLocks than now.

Regards,
Qingqing

"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

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly