Re: [PATCH] Refactoring of LWLock tranches

On Mon, Jan 4, 2016 at 4:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Dec 31, 2015 at 3:36 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > LWLock *LWLockAssignFromTranche(const char *tranche_name) will
> > assign a lock for the specified tranche. This also ensures that no
> > more than requested number of LWLocks can be assigned from a
> > specified tranche.
>
> However, this seems like an awkward API, because if there are many
> LWLocks you're going to need to look up the tranche name many times,
> and then you're going to need to make an array of pointers to them
> somehow, introducing a thoroughly unnecessary layer of indirection.
> Instead, I suggest just having a function LWLockPadded
> *GetLWLockAddinTranche(const char *tranche_name) that returns a
> pointer to the base of the array.
>

If we do that way, then user of API needs to maintain the interlock
guarantee that the requested number of locks is same as allocations
done from that tranche and also if it is not allocating all the
LWLocks in one function, it needs to save the base address of the
array and then allocate from it by maintaining some counter.
I agree that looking up for tranche names multiple times is not good,
if there are many number of lwlocks, but that is done at startup
time and not at operation-level. Also if we look currently at
the extensions in contrib, then just one of them allocactes one
LWLock in this fashion, now there could be extnsions apart from
extensions in contrib, but not sure if they require many number of
LWLocks, so I feel it is better to give an API which is more
convinient for user to use.

> > Also I have retained NUM_USER_DEFINED_LWLOCKS in
> > MainLWLockArray so that if any extensions want to assign a LWLock
> > after startup, it can be used from this pool. The tranche for such
locks
> > will be reported as main.
>
> I don't like this. I think we should get rid of
> NUM_USER_DEFINED_LWLOCKS altogether. It's just an accident waiting to
> happen, and I don't think there are enough people using LWLocks from
> extensions that we'll annoy very many people by breaking backward
> compatibility here. If we are going to care about backward
> compatibility, then I think the old-style lwlocks should go in their
> own tranche, not main. But personally, I think it'd be better to just
> force a hard break and make people switch to using the new APIs.
>

One point to think before removing it altogether, is that the new API's
provide a way to allocate LWLocks at the startup-time and if any user
wants to allocate a new LWLock after start-up, it will not be possible
with the proposed API's. Do you think for such cases, we should ask
user to use it the way we have exposed or do you have something else
in mind?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com