Обсуждение: 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

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