Re: Improve LWLock tranche name visibility across backends

Hi Nathan,

> We only ever add new tranche names to the end of the list, so we should be
> able to avoid the lock for the vast majority of tranche name lookups. We
> hold the lock while we increment LWLockCounter and add a name, and also
> whenever we update LocalLWLockCounter. This should prevent any unsafe
> accesses to NamedLWLockTrancheNames.
>
> I've added some additional commentary about this in v21.
>

Thanks for adding the comments.
I think there may be a brief period in the current code where unsafe access
to
LWLockTrancheNames is possible.

Since updates to LocalLWLockCounter and LWLockTrancheNames are performed
while
holding the lock, but reading LocalLWLockCounter and then
LWLockTrancheNames in
GetLWTrancheName can occur without acquiring the same lock in case
trancheID < LocalLWLockCounter,
There is a small window between updating LocalLWLockCounter and adding the
name
to LWLockTrancheNames. During this window, if GetLWTrancheNames is called,
it might attempt
to access a name in LWLockTrancheNames that hasn't been added yet.

I’ll see if I can demonstrate the situation with a test.

>
> > + ereport(ERROR,
> > + (errmsg("maximum number of tranches
> already registered"),
> > + errdetail("No more than %d tranches may
> be registered.",
> > + MAX_NAMED_TRANCHES)));
> > + }
> >
> > Since this patch sets a maximum limit on the number of LW lock tranches
> > that can be registered, would it make sense to offer a configurable
> > option rather than using a hard-coded MAX_NUM_TRANCHES? This will allow
> > users who have reached the maximum limit to register their LW Locks.
>
> Hm. I'm not sure. I think it would be good to offer a way to accommodate
> more tranche names that didn't involve recompiling, but I don't know
> whether that situation is likely enough in practice to add yet another GUC.
> Thus far, we've been operating under the assumption that we'll be able to
> choose a number far beyond any realistic usage. If others have opinions
> about this, please share...
>
>
Thank you for clarifying the rationale behind the decision.

Thank you,
Rahila Syed