Re: [PATCH] Refactoring of LWLock tranches

On 11/09/2015 10:32 PM, Ildus Kurbangaliev wrote:
>
>> On Nov 9, 2015, at 7:56 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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