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

On 11/09/2015 10:32 PM, Ildus Kurbangaliev wrote:
>
>> On Nov 9, 2015, at 7:56 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>>
>> Ildus Kurbangaliev wrote:
>>
>>> Thanks for the review. I've attached a new version of SLRU patch. I've
>>> removed add_postfix and fixed EXEC_BACKEND case.
>>
>> Thanks.
>>
>> Please do not use "committs" in commit_ts.c; people didn't like the
>> abbreviated name without the underscore.  But then, why are we
>> abbreviating here?  We could keep it complete and with a space instead
>> of underscore, so why not use just "commit timestamp", because it's just
>> a string, right?
>>
>> In multixact.c, is there a reason to have underscore in the strings?  We
>> could substitute it with a space and it'd look prettier; but really, we
>> could also keep those names parallel to subdirectory names by using the
>> already existing string parameter as name here, and not add another one.
>
> I do not insist on concrete names or a case here, but I think that identifiers are more
> useful when they don't contain spaces. For example that name will be exposed later
> in other places and can be part of some longer string.
>
>>
>> Why do we have two per-buffer loops in SimpleLruInit?  I mean, why not
>> add the LWLockInitialize call to the second one?
>
> Thanks. I didn't see that.
>
>>
>> I'm up to speed on how the LWLockTranche API works -- does assigning to
>> tranche_name a pstrdup string work okay?  Is the pstrdup really
>> necessary?
>
> I think pstrdup can be removed here.
>
>>
>>>     /* Initialize our shared state struct */
>>> diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
>>> index 90c7cf5..868b35a 100644
>>> --- a/src/backend/access/transam/slru.c
>>> +++ b/src/backend/access/transam/slru.c
>>> @@ -157,6 +157,8 @@ SimpleLruShmemSize(int nslots, int nlsns)
>>>     if (nlsns > 0)
>>>         sz += MAXALIGN(nslots * nlsns * sizeof(XLogRecPtr));    /* group_lsn[] */
>>>
>>> +    sz += MAXALIGN(nslots * sizeof(LWLockPadded)); /* lwlocks[] */
>>> +
>>>     return BUFFERALIGN(sz) + BLCKSZ * nslots;
>>> }
>>
>> What is the "lwlocks[]" comment supposed to mean?  I don't think there's
>> a struct member with that name, is there?
>
> It just means that we are allocating memory for an array of lwlocks,
> i'll change it.
>
>>
>> Uhm, actually, why do we keep buffer_locks[] at all?  This arrangement
>> seems pretty odd, where if I understand correctly we have one array
>> which is the tranche and another array which points to each item in the
>> tranche ...
>
> Actually yes, that is a good idea.

Attached a new version of the patch that moves SLRU tranches and LWLocks
to SLRU control structs.

`buffer_locks` field now contains LWLocks itself, so we have some
economy of the memory here. `pstrdup` removed in SimpleLruInit. I didn't
change names from the previous patch yet, but I don't mind if they'll
be changed.

--
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company