Re: [PATCH] Refactoring of LWLock tranches

On Mon, Jan 4, 2016 at 8:28 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:
> > 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.
>
> 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

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