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

On Mon, Jan 4, 2016 at 8:28 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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.

Probably, but the point is to make user of API do what he or she wants

to accomplish without much knowledge of underlying stuff.  However,

I think it is not too much details for user to know, so I have changed the

API as per your suggestion.

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

I am not aware of such cases, however the reason I have kept it was for

backward-compatability, but now I have removed it in the attached patch.

Apart from that, I have updated the docs to reflect the changes related

to new API's.

Fe things to Note -

Some change is needed in LWLockCounter[1] if this goes after 2

other patches (separate tranche for PGProcs and separate tranche

for ReplicationSlots).  Also, LWLockAssign() can be removed after

those patches