Re: [PATCH] Refactoring of LWLock tranches

On Mon, Jan 4, 2016 at 5:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jan 4, 2016 at 1:20 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > 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.
>
> Well, I agree with you that we should provide the most convenient API
> possible, but I don't agree that yours is more convenient than the one
> I proposed. I think it is less convenient. In most cases, if the
> user asks for a large number of LWLocks, they aren't going to be each
> for a different purpose - they will all be for the same purpose, like
> one per buffer or one per hash partition. The case where the user
> wants to allocate 8 lwlocks from an extension, each for a different
> purpose, and spread those allocations across a bunch of different
> functions probably does not exist. *Maybe* there is somebody
> allocating lwlocks from an extension for unrelated purposes, but I'd
> be willing to bet that, if so, all of those allocations happen one
> right after the other in a single function, because anything else
> would be completely nuts.
>

I'm not sure if we should return LWLockPadded*.

+LWLockPadded *
> +GetLWLockAddinTranche(const char *tranche_name)

It would be nice to isolate extension from knowledge about padding. Could
we one day change it from LWLockPadded * to LWLockMinimallyPadded * or any?

+LWLock **
> +GetLWLockAddinTranche(const char *tranche_name)

Could we use this signature?

> >> > 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?
>
> Allocating a new lock after startup mostly doesn't work, because there
> will be at most 3 available and sometimes less. And even if it does
> work, it often isn't very useful because you probably need some other
> shared memory space as well - otherwise, what is the lock protecting?
> And that space might not be available either.
>
> I'd be interested in knowing whether there are cases where useful
> extensions can be loaded apart from shared_preload_libraries because
> of NUM_USER_DEFINED_LWLOCKS and our tendency to allocate a little bit
> of extra shared memory, but my suspicion is that it rarely works out
> and is too flaky to be useful to anybody.

+1 for dropping old API
I don't think it's useful to have LWLocks without having shared memory.

There is another thing I'd like extensions to be able to do. It would be
nice if extensions could use dynamic shared memory instead of static. Then
extensions could use shared memory without being in
shared_preload_libraries. But if extension register DSM, then there is no
way to tell other backends to use it for that extension. Also DSM would be
deallocated when all backends detached from it. This it out of scope for
this patch though.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company