Add assertion on held AddinShmemInitLock in GetNamedLWLockTranche()
Hi all,
While digging into the LWLock code, I have noticed that
GetNamedLWLockTranche() assumes that its caller should hold the LWLock
AddinShmemInitLock to prevent any kind of race conditions when
initializing shmem areas, but we don't make sure that's the case.
The sole caller of GetNamedLWLockTranche() in core respects that, but
out-of-core code may not be that careful. How about adding an
assertion based on LWLockHeldByMeInMode() to make sure that the
ShmemInit lock is taken when this routine is called, like in the
attached?
Thanks,
--
Michael
Attachments:
lwlock-assert.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 315a78cda9..263b483251 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -599,6 +599,8 @@ GetNamedLWLockTranche(const char *tranche_name)
int lock_pos;
int i;
+ Assert(LWLockHeldByMeInMode(AddinShmemInitLock, LW_EXCLUSIVE));
+
/*
* Obtain the position of base address of LWLock belonging to requested
* tranche_name in MainLWLockArray. LWLocks for named tranches are placed
On Fri, Jul 28, 2023 at 8:54 AM Michael Paquier <michael@paquier.xyz> wrote:
Hi all,
While digging into the LWLock code, I have noticed that
GetNamedLWLockTranche() assumes that its caller should hold the LWLock
AddinShmemInitLock to prevent any kind of race conditions when
initializing shmem areas, but we don't make sure that's the case.The sole caller of GetNamedLWLockTranche() in core respects that, but
out-of-core code may not be that careful. How about adding an
assertion based on LWLockHeldByMeInMode() to make sure that the
ShmemInit lock is taken when this routine is called, like in the
attached?
+1 for asserting that the caller holds AddinShmemInitLock to prevent
reads while someone else is adding their LWLocks.
+ Assert(LWLockHeldByMeInMode(AddinShmemInitLock, LW_EXCLUSIVE));
Why to block multiple readers (if at all there exists any), with
LWLockHeldByMeInMode(..., LW_EXCLUSIVE)? I think
Assert(LWLockHeldByMe(AddinShmemInitLock)); suffices in
GetNamedLWLockTranche.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Fri, Jul 28, 2023 at 11:07:49AM +0530, Bharath Rupireddy wrote:
Why to block multiple readers (if at all there exists any), with
LWLockHeldByMeInMode(..., LW_EXCLUSIVE)? I think
Assert(LWLockHeldByMe(AddinShmemInitLock)); suffices in
GetNamedLWLockTranche.
I am not sure to follow this argument. Why would it make sense for
anybody to use that in shared mode? We document that the exclusive
mode is required because we retrieve the lock position in shmem that
an extension needs to know about when it initializes its own shmem
state, and that cannot be done safely without LW_EXCLUSIVE. Any
extension I can find in https://codesearch.debian.net/ does that as
well.
--
Michael