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.
