On Tue, Jan 5, 2016 at 4:04 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Mon, Jan 4, 2016 at 8:28 PM, Robert Haas <robertmh...@gmail.com> wrote: > > > > On Mon, Jan 4, 2016 at 1:20 AM, Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > On Mon, Jan 4, 2016 at 4:49 AM, Robert Haas <robertmh...@gmail.com> > wrote: > > >> On Thu, Dec 31, 2015 at 3:36 AM, Amit Kapila <amit.kapil...@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 > 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
