Re: [PATCH] Refactoring of LWLock tranches

On Fri, Jan 29, 2016 at 6:21 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Mon, Jan 4, 2016 at 5:58 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:
>> > 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.
>>
>
> I'm not sure if we should return LWLockPadded*.
>
> +LWLockPadded *
>> +GetLWLockAddinTranche(const char *tranche_name)
>
>
> It would be nice to isolate extension from knowledge about padding. Could
> we one day change it from LWLockPadded * to LWLockMinimallyPadded * or any?
>
>
Valid point, but I think there is an advantage of exposing padded
structure as well which is that if extension owner wants
LWLockPadded for some performance reason or otherwise (like
currently SlruSharedData is using), it can use this API as it is.

> +LWLock **
>> +GetLWLockAddinTranche(const char *tranche_name)
>
>
> Could we use this signature?
>
>

I think we can do this way as well. There is some discussion
upthread [1] about the signature of API where the current API has
been thought of as a better API.

I think we can expose it in many ways like the v1 version of patch or
the way it has been discussed and done in latest patch or in the
way you are suggesting and each has its pros and cons. It seems
to me we should leave this point at committers discretion unless,
there is clear win for any kind of API or more people are in favour of one
kind of signature over other.

[1] -
http://www.postgresql.org/message-id/CA+TgmoZFBRjTo3rnEbPRmMcCBVYSbjkRJNDFqgCkOaenh3V9og@mail.gmail.com

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