Re: [PATCH] Refactoring of LWLock tranches

On Tue, Jan 5, 2016 at 4:04 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

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

Also couple of minor comments from me.

I think this

+ StrNCpy(LWLockTrancheRequestArray[LWLockTrancheRequestsCount].tranche_name,
> tranche_name, strlen(tranche_name) + 1);

should be

+ StrNCpy(LWLockTrancheRequestArray[LWLockTrancheRequestsCount].tranche_name,
> tranche_name,
> sizeof(LWLockTrancheRequestArray[LWLockTrancheRequestsCount].tranche_name));

And as far as I know english "it's" should be "its" in the sentence below.

+ from <function>_PG_init</>. Tranche repersents an array of LWLocks
> and
> + can be accessed by it's name. First parameter
> <literal>tranche_name</>

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