Re: [PATCH] Refactoring of LWLock tranches - Mailing list pgsql-hackers

On Mon, Jan 4, 2016 at 1:20 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Jan 4, 2016 at 4:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Dec 31, 2015 at 3:36 AM, Amit Kapila <amit.kapila16@gmail.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.

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.

>> > 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.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company